JSON string escaping support

[noloader]

This commit adds JSON string escaping support according to RFC 8259, Section 7. See GitHub Issue closes #754. [added by @kwwall]

[jeremiahjstacey]

@noloader, I scanned the existing bugs and I don't see a task to associate this PR to. (I may have missed it, sorry)

If something does not exist already, would you please create an issue in the project that encapsulate the intent and value of this change? Once identified or created, please associate it with this PR for project traceability.

[kwwall]

I believe there is an old, closed GitHub issue for this that was closed because he thought lost the code when he did a rebase, but he was able to recover it. We should find that old GitHub issue and just reopen it.

-kevin

On Wed, Oct 26, 2022, 6:34 AM jeremiahjstacey @.***> wrote:

@noloader https://github.com/noloader, I scanned the existing bugs and I don't see a task to associate this PR to. (I may have missed it, sorry)

If something does not exist already, would you please create an issue in the project that encapsulate the intent and value of this change? Once identified or created, please associate it with this PR for project traceability.

— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/pull/753#issuecomment-1291830532, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG7ZS5YYM6HZPNOIGI3WFECJ5ANCNFSM6AAAAAARORONOE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[noloader]

@jeremiahjstacey

I scanned the existing bugs and I don't see a task to associate this PR to. (I may have missed it, sorry)

If something does not exist already, would you please create an issue in the project that encapsulate the intent and value of this change? Once identified or created, please associate it with this PR for project traceability.

Its kind of a winding road to get here.

This started with Kevin and I talking offline because we need a JSONEncoder at $dayjob. We did not want to use a JavaScript encoder because the grammars are slightly different. Then, the discussion moved to the mailing list at JSON codec discussion on esapi-github, https://groups.google.com/a/owasp.org/g/esapi-project-users/c/Q0LTNGxbTQc/m/xoyjMNwCAgAJ .

The mailing list message points to the dead PR at https://github.com/ESAPI/esapi-java-legacy/pull/722 . The original PR is dead because I accidentally blew away the code changes on a rebase. I was able to recover the deleted changes and files through Git's reflog, which eventually lead to this [new] PR.

[kwwall]

I looked at the issue history and didn't find one. No worries. I will create one for you tonight, and reference this PR so they will get linked together.

-kevin

On Thu, Oct 27, 2022, 3:02 PM Jeffrey Walton @.***> wrote:

@jeremiahjstacey https://github.com/jeremiahjstacey

I scanned the existing bugs and I don't see a task to associate this PR to. (I may have missed it, sorry)

If something does not exist already, would you please create an issue in the project that encapsulate the intent and value of this change? Once identified or created, please associate it with this PR for project traceability.

Its kind of a winding road to get here.

This started with Kevin and I talking offline because we need a JSONEncoder at $dayjob. We did not want to use a JavaScript encoder because the grammars are slightly different. Then, the discussion moved to the mailing list at JSON codec discussion on esapi-github, https://groups.google.com/a/owasp.org/g/esapi-project-users/c/Q0LTNGxbTQc/m/xoyjMNwCAgAJ .

The mailing list message points to the dead PR at #722 https://github.com/ESAPI/esapi-java-legacy/pull/722 . The original PR is dead because I accidentally blew away the code changes on a rebase. I was able to recover the deleted changes and files through Git's reflog, which eventually lead to this [new] PR.

— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/pull/753#issuecomment-1293944200, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG3GHNDHPT5W7LY3OPTWFLGTTANCNFSM6AAAAAARORONOE . You are receiving this because you commented.Message ID: @.***>

[kwwall]

@noloader, @jeremiahjstacey, & @xeno6696 - Call for final comments and/ commits on this PR. I'd like to get this and @jeremiahjstacey's PR #756 merged soon so we can do another release. I know Dave Wichers is planning on a AntiSamy point release to fix a CVE in a dependency so we should release when AntiSamy does to prevent people from claiming ESAPI is vulnerable.

[noloader]

On Wed, Nov 16, 2022 at 6:07 AM jeremiahjstacey @.***> wrote:

@.**** commented on this pull request.

In src/main/java/org/owasp/esapi/codecs/JSONCodec.java https://github.com/ESAPI/esapi-java-legacy/pull/753#discussion_r1023853508 :

• // Do nothing. Fall into throw below.
• }
• catch (IllegalArgumentException e)
• {
• // Do nothing. Fall into throw below.
• }
• // Catch all. The escaped character sequence was invalid.
• input.reset();
• throw new IllegalArgumentException( errorMessage );
• }
• protected int charToCodepoint( Character ch ) {
• final String s = Character.toString(ch);
• assert (s.length() == 1) : "Ooops";

@kwwall https://github.com/kwwall per your comment on the assert statement I've unresolved that conversation. Suggest waiting to merge pending updated resolution.

The code does not depend on behavior from the assert. The idea behind the assert is to alert the developer when the code is being debugged. That's all I wanted to do there: "Hey! Something looks sideways. Take a look at this!".

In C/C++/ObjC apps, an assert should snap the debugger. In release builds the assert should be removed. If my understanding is mistaken or a web app does not alert the developer during a debug session, then would someone correct it, please.

Now the behavior when asserts are not in effect may be wrong. The behavior is to return the first character in the pair. I feel like that's not quite correct. I think the docs say an empty string will be returned on encoding and decoding errors. Maybe that code path should return "" instead.

Message ID: @.*** com>

[kwwall]

In Java, assertions are disabled by default. We enable them via the Surefire plugin for our JUnit tests, but for others development where ESAPI is used, they would be disabled. So, just consider that as a possibility.

-kevin

On Wed, Nov 16, 2022, 10:51 AM Jeffrey Walton @.***> wrote:

On Wed, Nov 16, 2022 at 6:07 AM jeremiahjstacey @.***> wrote:

@.**** commented on this pull request.

In src/main/java/org/owasp/esapi/codecs/JSONCodec.java < https://github.com/ESAPI/esapi-java-legacy/pull/753#discussion_r1023853508

:

• // Do nothing. Fall into throw below.
• }
• catch (IllegalArgumentException e)
• {
• // Do nothing. Fall into throw below.
• }
• // Catch all. The escaped character sequence was invalid.
• input.reset();
• throw new IllegalArgumentException( errorMessage );
• }
• protected int charToCodepoint( Character ch ) {
• final String s = Character.toString(ch);
• assert (s.length() == 1) : "Ooops";

@kwwall https://github.com/kwwall per your comment on the assert statement I've unresolved that conversation. Suggest waiting to merge pending updated resolution.

The code does not depend on behavior from the assert. The idea behind the assert is to alert the developer when the code is being debugged. That's all I wanted to do there: "Hey! Something looks sideways. Take a look at this!".

In C/C++/ObjC apps, an assert should snap the debugger. In release builds the assert should be removed. If my understanding is mistaken or a web app does not alert the developer during a debug session, then would someone correct it, please.

Now the behavior when asserts are not in effect may be wrong. The behavior is to return the first character in the pair. I feel like that's not quite correct. I think the docs say an empty string will be returned on encoding and decoding errors. Maybe that code path should return "" instead.

Message ID: @.*** com>

— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/pull/753#issuecomment-1317235237, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG2FKDVUTMQZZRJCWR3WIT7ILANCNFSM6AAAAAARORONOE . You are receiving this because you were mentioned.Message ID: @.***>

[xeno6696]

@kwwall i know I need to review this, I’m on travel for work (again) and I can’t sit down with this until this weekend.

If we need to push faster than that I’ll trust the group judgment here.

[xeno6696]

Given that this was recovered code from #722 I feel like I beat that up pretty well already. @kwwall I'm okay with moving forward. I would normally just merge here but it sounded like you had some changes and I don't want to step on your toes here.

[xeno6696]

I did want to call myself out to building some unit tests that handled the full Unicode range. If we stayed with the AbstractCharacterCodec I have a minor frowny-face just because it was only supposed to be a bridge until all the codecs were converted to Integer-based codecs... however this is my fault as I should have marked that abstract class as deprecated. (It could be that there are some codecs where for some reason, it makes zero sense at all to expand to handle the full UTF-8 range, but I figured we'd handle those case by case.)