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.
jmanico commentedon Oct 18, 2026
You're right about whitespace. HTML does not remove whitespace between the letters of a tag name, and I have no browser evidence to support that claim. PR #247 now says this explicitly.
Your proposed < escape addresses the HTML delimiter problem when the entire string value is encoded. Quotes, backslashes, and the other required JavaScript string escapes still need handling too.
The remaining distinction is fragment composition, which these methods also support. If the server writes trusted </scr, encodes the input i, then writes trusted pt>, the browser receives </script>. The input contains no < to escape. This is source text assembled before HTML parsing, not JavaScript runtime concatenation.
PR #247 therefore leaves interior letters readable and retains an escape for a final C, I, P, R, S, or T in either case. It produces http://google.com for the URL in your report. The final-letter rule is deliberately conservative: you're also right that it does not distinguish script from csript; both receive a final \x74 escape. That does not mean csript is an HTML tag the browser recognizes.
I checked the specific whitespace and < examples in Chrome 155, including unsafe controls for complete and split closing tags. The tests agree with the HTML specification and this distinction. The follow-up changes to #247 clarify the documentation and writer usage; they do not change the proposed encoding algorithm.
The proposed resolution is to ship #247 in a minor release, 1.6.0, with these output and streaming changes documented. It is not released yet.
Specification: https://html.spec.whatwg.org/multipage/parsing.html#script-data-end-tag-name-state
PR: #247
arpadf78 commentedon Oct 19, 2026
You are basing your assumption that there is server side coordination in the attack? You say server writes "</scr", why would a server write such a thing? That is not even legal html and it would mean the code generation is compromised on the server. If you can inject server code somehow, you don't need XSS.
Do you have a real example (a simple html) where injecting and "i" characters blows the page?
jmanico commentedon Oct 19, 2026
Yes, it's a very rare edge case. I will send you several examples when I have a moment.
But I listened to you carefully. Release 1.6.0 should be more reasonable for your use case. The edge case rules should not be harmful. Can you give is a try and let me know if this release is still causing you harm?
jmanico commentedon Oct 19, 2026
I re-opened the issue for now. I want to make sure your concerns are fully addressed before letting this go.
Don't worry, I got your back here. You are helping improve this library and I appreciate it.
I'm mostly alone working on this with a family to top AI models. I need feedback like this, thank you!