Skip to content

Conversation

@Phillip9587
Copy link
Member

@Phillip9587 Phillip9587 commented Dec 15, 2025

This PR improves the handling and validation of character encodings for the JSON and URL-encoded parsers, ensuring compliance with relevant standards and providing more robust, maintainable code.

The main change is extracting the previously inlined charset validation functions into utility functions. I also included tests for these functions.

isValidJsonCharset

  • Previously only checked if the charset starts with utf-
  • Now checks if the charset is utf-8, utf-16 or utf-32 case-insensitive according to RFC 7159 Section 8.1 (Stricter, Maybe semver major)
  • RFC 7159 was superseded by RFC 8259 which is even stricter and defines:

JSON text exchanged between systems that are not part of a closed
ecosystem MUST be encoded using UTF-8 [RFC3629].
Previous specifications of JSON have not required the use of UTF-8
when transmitting JSON text. However, the vast majority of JSON-
based software implementations have chosen to use the UTF-8 encoding,
to the extent that it is the only encoding that achieves
interoperability.

isValidUrlencodedCharset

  • Previously checked case-senstive for utf-8 and iso-8859-1
  • Now checks case-insensitive

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

This is definitely a breaking change. For example, if I currently provide charset=utf-16be for JSONs, it would be decoded correctly and no error would be thrown, with your current PR this would be considered invalid, which makes me wonder whether the specification takes UTF-16BE or UTF-16LE encoding into account.

Additionally, how does this behave with iconv-lite? Given that iconv-lite correctly supports UTF-16BE and UTF-16LE, it should be able to decode the JSONs correctly.

My initial comments, i’ll continue reviewing these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants