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.

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

Remove h from ESCAPED_CHARS in JsonMapObjectReaderWriter #1872

Merged
merged 2 commits into from
May 26, 2024

Conversation

cwperks
Copy link
Contributor

@cwperks cwperks commented May 15, 2024

The JsonMapObjectReaderWriter class maintains a list of ESCAPED_CHARS which includes special characters that need to be escaped like the newline (\n) and tab (\t) characters. This list also includes \h, but I can't find any links to official documentation about this character needing to be escaped.

According to this SO post which details escaped characters in JSON, it also does not include \h in this list.

I'm opening a PR to discuss removing this from the list.

Issue in OpenSearch where this issue is discussed: opensearch-project/security#2531 (comment)

Simple test case which demonstrates the difference:

package org.opensearch.security.auth;

import org.apache.cxf.rs.security.jose.jwk.JsonWebKey;
import org.apache.cxf.rs.security.jose.jwk.KeyType;
import org.apache.cxf.rs.security.jose.jwk.PublicKeyUse;
import org.apache.cxf.rs.security.jose.jws.JwsUtils;
import org.apache.cxf.rs.security.jose.jwt.JoseJwtProducer;
import org.apache.cxf.rs.security.jose.jwt.JwtClaims;
import org.apache.cxf.rs.security.jose.jwt.JwtToken;
import org.apache.cxf.rs.security.jose.jwt.JwtUtils;
import org.junit.Test;

public class CXFTests {
    @Test
    public void testBase64URLUtility() {
        JoseJwtProducer jwtProducer = new JoseJwtProducer();
        JsonWebKey jwk = new JsonWebKey();

        jwk.setKeyType(KeyType.OCTET);
        jwk.setAlgorithm("HS512");
        jwk.setPublicKeyUse(PublicKeyUse.SIGN);
        jwk.setProperty("k", "<exchangeKey>");
        jwtProducer.setSignatureProvider(JwsUtils.getSignatureProvider(jwk));

        JwtClaims jwtClaims1 = new JwtClaims();
        JwtToken jwt1 = new JwtToken(jwtClaims1);
        jwtClaims1.setSubject("user1\\nexample");

        JwtClaims jwtClaims2 = new JwtClaims();
        JwtToken jwt2 = new JwtToken(jwtClaims2);
        jwtClaims2.setSubject("user2\\hexample");


        String encodedJwt1 = jwtProducer.processJwt(jwt1);
        String encodedJwt2 = jwtProducer.processJwt(jwt2);

        System.out.println("encodedJwt1: " + encodedJwt1);
        System.out.println("encodedJwt2: " + encodedJwt2);

        System.out.println("jwt1.toJson: " + JwtUtils.claimsToJson(jwt1.getClaims(), null));
        System.out.println("jwt2.toJson: " + JwtUtils.claimsToJson(jwt2.getClaims(), null));
    }
}

Output:

encodedJwt1: eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiJ1c2VyMVxcbmV4YW1wbGUifQ.iFP82Rq_EsEDkL4X7gaa21q0k4Yt81ybMWnqM2QgS55R1xKE8NARWZEQrjvA5lZwT4368-61BUeix7ufmdOz0A
encodedJwt2: eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiJ1c2VyMlxoZXhhbXBsZSJ9.O-glz6XD7guP4DemL6nyAnEVgGb4XcyLprtcHnpVGoFCe37eEW7UFi8MTmJ8D3JvJoLso1OfKiLkYTHjRecUcw
jwt1.toJson: {"sub":"user1\\nexample"}
jwt2.toJson: {"sub":"user2\hexample"}

jwt2 has an invalid payload.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks
Copy link
Contributor Author

cwperks commented May 15, 2024

@reta Could you take a look? This is related to an issue in the OpenSearch security plugin.

@@ -49,7 +49,6 @@ public class JsonMapObjectReaderWriter {
chars.add('/');
chars.add('b');
chars.add('f');
Copy link
Member

@reta reta May 15, 2024

Choose a reason for hiding this comment

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

chars.add('n');

I believe it was a typo (https://datatracker.ietf.org/doc/html/rfc8259#section-7)L

string = quotation-mark *char quotation-mark

      char = unescaped /
          escape (
              %x22 /          ; "    quotation mark  U+0022
              %x5C /          ; \    reverse solidus U+005C
              %x2F /          ; /    solidus         U+002F
              %x62 /          ; b    backspace       U+0008
              %x66 /          ; f    form feed       U+000C
              %x6E /          ; n    line feed       U+000A
              %x72 /          ; r    carriage return U+000D
              %x74 /          ; t    tab             U+0009
              %x75 4HEXDIG )  ; uXXXX                U+XXXX

      escape = %x5C              ; \

      quotation-mark = %x22      ; "

      unescaped = %x20-21 / %x23-5B / %x5D-10FFFF

Thanks a lot @cwperks !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to n. Good catch!

Copy link
Member

@reta reta May 15, 2024

Choose a reason for hiding this comment

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

@coheigea could you please take a look? very minor change, thank you (I think a typo sneaked in during https://issues.apache.org/jira/browse/CXF-8555)

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@reta reta requested a review from coheigea May 15, 2024 01:12
@cwperks
Copy link
Contributor Author

cwperks commented May 21, 2024

@coheigea Could you review this change?

@reta reta merged commit 7dcafef into apache:main May 26, 2024
4 checks passed
reta pushed a commit that referenced this pull request May 26, 2024
* Remove h from ESCAPED_CHARS in JsonMapObjectReaderWriter

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Change to n

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 7dcafef)
reta pushed a commit that referenced this pull request May 26, 2024
* Remove h from ESCAPED_CHARS in JsonMapObjectReaderWriter

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Change to n

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 7dcafef)
reta pushed a commit that referenced this pull request May 26, 2024
* Remove h from ESCAPED_CHARS in JsonMapObjectReaderWriter

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Change to n

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 7dcafef)
(cherry picked from commit 311ce17)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants