Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit warnings when dumping binary strings #643

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Emit warnings when dumping binary strings #643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions ext/json/ext/generator/generator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ static void generate_json_fixnum(FBuffer *buffer, struct generate_json_data *dat
static void generate_json_bignum(FBuffer *buffer, struct generate_json_data *data, JSON_Generator_State *state, VALUE obj);
static void generate_json_float(FBuffer *buffer, struct generate_json_data *data, JSON_Generator_State *state, VALUE obj);

static int usascii_encindex, utf8_encindex, binary_encindex;

/* Converts in_string to a JSON string (without the wrapping '"'
* characters) in FBuffer out_buffer.
*
Expand Down Expand Up @@ -535,7 +537,7 @@ static VALUE mString_to_json_raw_object(VALUE self)
VALUE result = rb_hash_new();
rb_hash_aset(result, rb_funcall(mJSON, i_create_id, 0), rb_class_name(rb_obj_class(self)));
ary = rb_funcall(self, i_unpack, 1, rb_str_new2("C*"));
rb_hash_aset(result, rb_str_new2("raw"), ary);
rb_hash_aset(result, rb_utf8_str_new_lit("raw"), ary);
return result;
}

Expand Down Expand Up @@ -822,8 +824,6 @@ static void generate_json_array(FBuffer *buffer, struct generate_json_data *data
fbuffer_append_char(buffer, ']');
}

static int usascii_encindex, utf8_encindex, binary_encindex;

static inline int enc_utf8_compatible_p(int enc_idx)
{
if (enc_idx == usascii_encindex) return 1;
Expand All @@ -837,13 +837,14 @@ static inline VALUE ensure_valid_encoding(VALUE str)
VALUE utf8_string;
if (RB_UNLIKELY(!enc_utf8_compatible_p(encindex))) {
if (encindex == binary_encindex) {
// For historical reason, we silently reinterpret binary strings as UTF-8 if it would work.
// TODO: Deprecate in 2.8.0
// TODO: Remove in 3.0.0
utf8_string = rb_enc_associate_index(rb_str_dup(str), utf8_encindex);
switch (rb_enc_str_coderange(utf8_string)) {
case ENC_CODERANGE_7BIT:
return utf8_string;
case ENC_CODERANGE_VALID:
// For historical reason, we silently reinterpret binary strings as UTF-8 if it would work.
// TODO: Raise in 3.0.0
rb_warn("JSON.generate: UTF-8 string passed as BINARY, this will raise an encoding error in json 3.0");
return utf8_string;
break;
}
Expand Down
25 changes: 14 additions & 11 deletions ext/json/ext/parser/parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1798,9 +1798,12 @@ static VALUE convert_encoding(VALUE source)

if (encindex == binary_encindex) {
// For historical reason, we silently reinterpret binary strings as UTF-8 if it would work.
// TODO: Deprecate in 2.8.0
// TODO: Remove in 3.0.0
return rb_enc_associate_index(rb_str_dup(source), utf8_encindex);
VALUE utf8_string = rb_enc_associate_index(rb_str_dup(source), utf8_encindex);
switch (rb_enc_str_coderange(utf8_string)) {
case ENC_CODERANGE_7BIT:
case ENC_CODERANGE_VALID:
return utf8_string;
}
}

return rb_str_conv_enc(source, rb_enc_from_index(encindex), rb_utf8_encoding());
Expand Down Expand Up @@ -1955,15 +1958,15 @@ static VALUE cParser_initialize(int argc, VALUE *argv, VALUE self)
}


#line 1959 "parser.c"
#line 1962 "parser.c"
enum {JSON_start = 1};
enum {JSON_first_final = 10};
enum {JSON_error = 0};

enum {JSON_en_main = 1};


#line 867 "parser.rl"
#line 870 "parser.rl"


/*
Expand All @@ -1981,16 +1984,16 @@ static VALUE cParser_parse(VALUE self)
GET_PARSER;


#line 1985 "parser.c"
#line 1988 "parser.c"
{
cs = JSON_start;
}

#line 884 "parser.rl"
#line 887 "parser.rl"
p = json->source;
pe = p + json->len;

#line 1994 "parser.c"
#line 1997 "parser.c"
{
if ( p == pe )
goto _test_eof;
Expand Down Expand Up @@ -2024,7 +2027,7 @@ case 1:
cs = 0;
goto _out;
tr2:
#line 859 "parser.rl"
#line 862 "parser.rl"
{
char *np = JSON_parse_value(json, p, pe, &result, 0);
if (np == NULL) { p--; {p++; cs = 10; goto _out;} } else {p = (( np))-1;}
Expand All @@ -2034,7 +2037,7 @@ cs = 0;
if ( ++p == pe )
goto _test_eof10;
case 10:
#line 2038 "parser.c"
#line 2041 "parser.c"
switch( (*p) ) {
case 13: goto st10;
case 32: goto st10;
Expand Down Expand Up @@ -2123,7 +2126,7 @@ case 9:
_out: {}
}

#line 887 "parser.rl"
#line 890 "parser.rl"

if (cs >= JSON_first_final && p == pe) {
return result;
Expand Down
9 changes: 6 additions & 3 deletions ext/json/ext/parser/parser.rl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,9 +693,12 @@ static VALUE convert_encoding(VALUE source)

if (encindex == binary_encindex) {
// For historical reason, we silently reinterpret binary strings as UTF-8 if it would work.
// TODO: Deprecate in 2.8.0
// TODO: Remove in 3.0.0
return rb_enc_associate_index(rb_str_dup(source), utf8_encindex);
VALUE utf8_string = rb_enc_associate_index(rb_str_dup(source), utf8_encindex);
switch (rb_enc_str_coderange(utf8_string)) {
case ENC_CODERANGE_7BIT:
case ENC_CODERANGE_VALID:
return utf8_string;
}
Comment on lines +696 to +701
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is #138. I'm having second thoughts about deprecating / removing it, as I fear lots of people depend on it, and looking at other parsers they all seem to accept it. So maybe we should just support it in the Java parser?

WDYT @headius @eregon ?

Copy link
Contributor

@headius headius Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I commented too early and thought this was a YAML issue.

After reexamining the code, it does appear that the json implementation for JRuby works directly with bytes via the ragel parser. It would probably be possible to make it accept invalid characters.

My position remains the same, however. I'm not sure what other parsers you tested, but all json specifications I looked at explicitly require content to be in one of the main unicode UTF encodings. That says to me that providing invalid characters is off spec and should produce an error.

The original RFC from 2006 even says:

JSON text SHALL be encoded in Unicode. The default encoding is UTF-8.

https://www.ietf.org/rfc/rfc4627.txt

The character stream cannot be parsed as Unicode, then it is not up to spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what other parsers you tested,

oj and Python's stdlib.

I totally get your argument, and generally agree, the concern is really about breaking existing (misbehaving) code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm crazy, but I would break it in a minor release so it's easy to revert with another minor release, rather than break it in a major release and have to go back. I doubt the impact will be large. If we put out a minor release that starts erroring on bad input and it doesn't get reported for a few months, it's probably fine.

I think it's also important to remember that Ruby has become much more strict about character encodings over the years so the chance of getting bad content has reduced significantly since the original issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common case where this still happens a lot today is JSON POST requests handled by Rack.

The Rack spec clearly state the body should be ASCII_8BIT, so if you do the common JSON.parse(request.body) you hit this case.

That's why I'm worried about breaking this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a decent point.

Just yesterday someone at work told me they had an outage because they have a JSON based logger and were logging binary strings (some avro stuff or whatever).

So perhaps you are right and should just keep this behavior, unless strict: true is set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless strict: true is set.

json/lib/json/pure/generator.rb

Lines 227 to 228 in 6d3b3ac

# Strict mode only allow serializing JSON native types: Hash, Array,
# String, Integer, Float, true, false and nil.

I think even strict: true should accept it given the documented meaning of it.
It could be another option, but not sure there is any value to prohibit broken strings if anyway it works fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be willing to make a PR with a test of what you have in mind? I can make it pass, just want to be 100% sure we're talking about the same thing.

Copy link
Member

@eregon eregon Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm thinking is:

diff --git a/test/json/json_parser_test.rb b/test/json/json_parser_test.rb
index 59cfcfa..a7ec04d 100644
--- a/test/json/json_parser_test.rb
+++ b/test/json/json_parser_test.rb
@@ -196,6 +196,19 @@ class JSONParserTest < Test::Unit::TestCase
     )
   end
 
+  def test_parse_broken_string
+    # https://github.com/ruby/json/issues/138
+    s = parse(%{["\x80"]})[0]
+    assert_equal("\x80", s)
+    assert_equal Encoding::UTF_8, s.encoding
+    assert_equal false, s.valid_encoding?
+
+    s = parse(%{["\x80"]}.b)[0]
+    assert_equal("\x80", s)
+    assert_equal Encoding::UTF_8, s.encoding
+    assert_equal false, s.valid_encoding?
+  end
+
   def test_parse_big_integers
     json1 = JSON(orig = (1 << 31) - 1)
     assert_equal orig, parse(json1)

And that passes on master, because of the rb_str_conv_enc() below which if there is an error during conversion returns the source string as-is (https://github.com/ruby/ruby/blob/22abcce704cf3d82aaa9af5b8ae4e6bf628502ea/spec/ruby/optional/capi/string_spec.rb#L884). And I guess the C extension doesn't really care whether the input String is internally in UTF-8 or BINARY and just works on bytes (although that could lead to subtle bugs as e.g. ENC_CODERANGE_VALID means something different for both encodings).
If I replace the rb_str_conv_enc() with return rb_funcall(source, rb_intern("encode"), 1, rb_const_get(rb_path2class("Encoding"), rb_intern("UTF_8"))); then I get Encoding::UndefinedConversionError.
If on top of that I undo the change in this file then it works again.

BTW, the pure-Ruby parser actually uses the input String as BINARY internally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#665

}

return rb_str_conv_enc(source, rb_enc_from_index(encindex), rb_utf8_encoding());
Expand Down
2 changes: 1 addition & 1 deletion lib/json/add/bigdecimal.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def self.json_create(object)
def as_json(*)
{
JSON.create_id => self.class.name,
'b' => _dump,
'b' => _dump.force_encoding(Encoding::UTF_8),
}
end

Expand Down
9 changes: 7 additions & 2 deletions test/json/json_generator_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,13 @@ def test_valid_utf8_in_different_encoding
wrong_encoding_string = utf8_string.b
# This behavior is historical. Not necessary desirable. We should deprecated it.
# The pure and java version of the gem already don't behave this way.
assert_equal utf8_string.to_json, wrong_encoding_string.to_json
assert_equal JSON.dump(utf8_string), JSON.dump(wrong_encoding_string)
assert_warning(/UTF-8 string passed as BINARY, this will raise an encoding error in json 3.0/) do
assert_equal utf8_string.to_json, wrong_encoding_string.to_json
end

assert_warning(/UTF-8 string passed as BINARY, this will raise an encoding error in json 3.0/) do
assert_equal JSON.dump(utf8_string), JSON.dump(wrong_encoding_string)
end
end

def test_string_ext_included_calls_super
Expand Down
Loading