Skip to content

PercentCodec decoder to disallow non-ASCII characters in input - #691

Open
ok2c wants to merge 1 commit into
apache:masterfrom
ok2c:percent_decoder_optimization
Open

PercentCodec decoder to disallow non-ASCII characters in input#691
ok2c wants to merge 1 commit into
apache:masterfrom
ok2c:percent_decoder_optimization

Conversation

@ok2c

@ok2c ok2c commented Aug 27, 2026

Copy link
Copy Markdown
Member

@arturobernalg Could you please double-check?

@ok2c
ok2c requested a review from arturobernalg August 27, 2026 17:16

@arturobernalg arturobernalg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am afraid this implementation changes the existing behavior in two problematic ways.

CharsetDecoder#newDecoder() defaults to REPORT, while flushBin ignores the returned CoderResult. Unconsumed bytes are then retained by compact() and can be combined with a later percent-encoded sequence across a literal character. For example, decoding %C3x%A4 as UTF-8 currently produces \uFFFDx\uFFFD, whereas this change produces xä.

The encode fast path also bypasses charset encoding for the initial safe-character sequence. For example, encoding abc as UTF-16 currently produces %FE%FF%00a%00b%00c, whereas this change produces abc.

I think both cases need to be addressed before this can be merged.

void testDecodeDoesNotCombineEscapedBytesAcrossLiteralCharacters() {
    assertEquals(
            "\uFFFDx\uFFFD",
            PercentCodec.decode("%C3x%A4", StandardCharsets.UTF_8));
}

@Test
void testEncodeHonorsUtf16Charset() {
    assertEquals(
            "%FE%FF%00a%00b%00c",
            PercentCodec.encode("abc", StandardCharsets.UTF_16));
}```

@ok2c

ok2c commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

@arturobernalg Thank you so much for a thorough review. I will re-design the whole thing and will likely take a different, simpler approach.

@ok2c
ok2c force-pushed the percent_decoder_optimization branch from 4ef802c to 467b460 Compare August 28, 2026 13:11
@ok2c ok2c changed the title Percent codec optimization PercentCodec decoder to disallow non-ASCII characters in input Aug 28, 2026
@ok2c
ok2c force-pushed the percent_decoder_optimization branch from 467b460 to 9259715 Compare August 28, 2026 13:28
@ok2c

ok2c commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

@arturobernalg I dropped all attempts at memory optimization and made the decode method treat non-ASCII characters in input as invalid / unrecognized.

Please do another pass.

@arturobernalg arturobernalg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ok2c LGTM

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