Repository navigation
Bug: forJavaScriptSource() should not encode "script" character in isolation #243
Description
Artifact, version and runtime
org.owasp.encoder:encoder:1.5.0
Expected and actual behavior
Encode.forJavaScriptSource() starting from v1.5.0 encodes the "script" characters, which leads to encoding of the string "http://google.com" as "h\x74\x74\x70:\/\/google.\x63om". Why?
No browser checks the characters in isolation. You cannot create a script with just "s" characters. You need to spell out "<script>"; not "script", not "", not "scr" or any combination there of. Such an aggressive encoding is totally unwarranted.
Minimal non-sensitive reproduction
assertThat(encoderUtil.forJavaScript("http://google.com"), is("http://google.com"));
Activity
arpadf78 commentedon Oct 15, 2026
Funny, github eats my characters (I wrote '<' 's' 'c' 'r' 'i' 'p' '>') as one word it and it gets deleted, but it accepts <script>.
jmanico commentedon Oct 15, 2026
I will get a new release out today and fix this. Sorry this is my mistake, I'm being overly aggressive.
arpadf78 commentedon Oct 15, 2026
Thanks!
jmanico commentedon Oct 15, 2026
I need to correct my earlier reply: I was too quick to promise a fix. Sorry for the confusion.
You’re right that a letter like i is harmless by itself. The problem is what happens when it joins the text around it.
Imagine the server builds a page from these three pieces:
Text before your value: </scr
Your value: i
Text after your value: pt>
The browser receives </script>. That closes the script block—even when those characters appear inside a JavaScript string. The browser sees the finished page; it does not know which characters came from your input.
Encoding that i as \x69 prevents the pieces from forming an HTML closing tag. JavaScript still reads \x69 as the original letter i, so the string’s value stays the same.
That is why checking only for a complete </script> inside the input would miss some cases. Part of the dangerous text can come from outside the input.
There is also an important method distinction: in the released 1.5.0 version, Encode.forJavaScriptSource("http://google.com") already returns the URL unchanged. The reported escaping comes from forJavaScript and forJavaScriptBlock. Your example calls encoderUtil.forJavaScript, so its underlying method needs checking. Source mode is intended for standalone JavaScript files, not inline HTML script blocks.
We checked the released library and tested the split-letter examples in Chrome. Removing the letter escapes really does allow the browser to close the script early.
We therefore won’t remove these escapes from the existing HTML-script methods. They provide a protection the library explicitly promises. The longer output is a fair concern, and we can investigate making it smaller while keeping that protection. But simply removing the escapes would trade away safety, rather than fix an incorrect JavaScript value.
jmanico commentedon Oct 15, 2026
Thanks for raising this. We completed another detailed review, including tests against the released library and an actual browser.
One important clarification:
forJavaScriptSource("http://google.com")already returns the URL unchanged. The additional escaping comes fromforJavaScript()andforJavaScriptBlock().We're keeping that behavior because the browser reads the whole page, including text around the encoded value. For example,
</scr+i+pt>becomes</script>. The browser can close the script even when those characters appear inside a JavaScript string. Escaping the insertediprevents that, while JavaScript still reads its value asi.Your concern about larger output is valid. Our documentation understated that cost. We've opened PR #244 to explain the tradeoff, which method fits each context, and the limits of the protection. The PR also strengthens the regression tests.
We therefore won't remove these escapes as the fix for this issue. A future improvement that produces smaller output while preserving the same protection remains worth considering.
jmanico commentedon Oct 15, 2026
Please re-open if you think any of this is wrong! The update fixes the JavaDocs to explain this.
arpadf78 commentedon Oct 16, 2026
Yes, I made a mistake during initial report - it is forJavaScript() not for forJavaScriptSource(); thanks for correcting.
However, I still think this is too aggressive. If you are concerned about whitespace characters then make a regexp to search for whitespace between the script characters, but treating "script" chars in isolation is the wrong approach to protect against XSS injection.
You cannot argue that "http" and a million of other words not being safe, still the encoder encodes all these combination just to protect against an absolute edge case. The encoder encodes safe combinations!
Is also important to clarify, that even if some browsers would eliminate whitespace, those still look for the "<script>" characters exactly in that sequence.
My concern is not about "larger output"; my concern is that the encoder does something it shouldn't do. It obfuscates valid/safe text.
I don't seem to have permissions to reopen; please reconsider.
jmanico commentedon Oct 16, 2026
I'll review this again soon. Re-opened!
jmanico commentedon Oct 17, 2026
@arpadf78, thank you for pushing back. Your reopening comment was right: the boundary example explains why some escaping is necessary, but it does not justify escaping every interior occurrence of those letters. I also mischaracterized your concern as output size; your objection to unnecessarily obscuring safe text is valid. HTML does not ignore whitespace between the letters of a script tag name.
I've opened draft PR #247 for you to review. It leaves interior letters readable while retaining a terminal-letter escape to protect combinations such as
</scr+ encodedi+pt>.Examples of the proposed
forJavaScript()output:http://google.comhttp:\/\/google.comscriptscrip\x74i\x69scripturescripturePunctuation and whitespace escapes remain. This is still conservative rather than minimal:
catbecomesca\x74. Standalone-source and attribute-only methods are unchanged. The PR also handles pending letters and surrogates safely across explicit writer flushes.The full build passes, along with extensive actual-Chrome checks of HTML parsing, evaluated JavaScript values, and deliberately unsafe controls. The PR describes the tests, remaining validation limits, and the change from a literal-substring guarantee to recognized HTML-token protection.
Could you review this approach and the resulting output against your use case? We'll keep this unmerged and unreleased until you've had a chance to review it. The issue remains open, and the release/version decision comes afterward.
arpadf78 commentedon Oct 17, 2026
I also want to comment on the example you've given earlier about script and whitespace characters; I'm not sure if that was a real example or just theoretical, but according to HTML spec whitespace cannot appear inside a tag name. Do you have any browser with such behavior?
Tokenization — Tag name state (defines that whitespace ends the tag name and moves into attribute parsing):
https://html.spec.whatwg.org/multipage/parsing.html#tag-name-state
So in order to protect against JS injection, I would concentrate on the tag opening character. That is all what is needed for javascript.