Bug: forJavaScriptSource() should not encode "script" character in isolation

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"));

Funny, github eats my characters (I wrote '<' 's' 'c' 'r' 'i' 'p' '>') as one word it and it gets deleted, but it accepts <script>.

I will get a new release out today and fix this. Sorry this is my mistake, I'm being overly aggressive.

Thanks!

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.

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 from forJavaScript() and forJavaScriptBlock().

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 inserted i prevents that, while JavaScript still reads its value as i.

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.

Please re-open if you think any of this is wrong! The update fixes the JavaDocs to explain this.

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.

I'll review this again soon. Re-opened!

@arpadf78

@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 + encoded i + pt>.

Examples of the proposed forJavaScript() output:

Input	Output
`http://google.com`	`http:\/\/google.com`
`script`	`scrip\x74`
`i`	`\x69`
`scripture`	`scripture`

Punctuation and whitespace escapes remain. This is still conservative rather than minimal: cat becomes ca\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.

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.

Input	Encoded output
<script	\x3Cscript
</script	\x3C/script

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

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?

You are basing your assumption that there is server side coordination in the attack?

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?

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!

I checked version 1.6.0, the impact is much smaller, thus better. IMHO still encodes characters it shouldn't.

last character of the value is always encoded, needlessly
the minus "-" is encoded, not sure why
space char is encoded, not sure why

Let me run this by Mythos, Daybreak Blue and Opus 5.5 Cyber. I almost certain you're right. I'll likely push a new version with this tuning.

I've put a huge amount of work into modernizing this library. I'm extremely grateful for your continued feedback to help make it better.

🤙

@arpadf78

@arpadf78, thank you for checking 1.6.0 and explaining what still bothers you. I agree that readable output matters, and showing that an escape is necessary in one situation does not establish that every occurrence needs escaping. For example, the current cat → ca\x74 behavior is deliberately conservative; it is reasonable to ask whether we can leave that word readable.

I also owe you two corrections. My latest “almost certain you're right” comment was premature, and I should not have suggested another release before checking the proposed changes. My earlier “yes” about server-side coordination was misleading: these examples do not assume an attacker can modify server code. Sorry for making the distinction harder to follow.

After checking the implementation, the HTML specification, and actual browser parsing, my conclusion is: we should investigate narrower escaping, but we cannot safely remove these three escape rules outright from the existing forJavaScript() / forJavaScriptBlock() methods. Here is the reasoning for each concern.

The final character

The additional terminal rule applies only to c, i, p, r, s, or t, in either case. It does not encode every final character. For example:

Input Current forJavaScript() output

cat ca\x74

dog dog

i \x69

http://google.com http:\/\/google.com

The existing methods support inserting encoded input next to trusted literal text within the same JavaScript string. For example, a tag-preview tool or code-example page can legitimately construct closing-tag text like this:
```
out.write("<script>const closeTag = \"</");
Encode.forJavaScriptBlock(out, tagName);
out.write(">\";</script><p id=\"after\">after</p>");
```
With tagName equal to script, current output contains </scrip\x74> inside the string. JavaScript evaluates that to the intended text, and HTML does not treat it as a closing tag. If the terminal escape is removed, the page instead contains a literal </script> there, and the HTML parser ends the script early—even though the developer intended it to be string data.

The server is executing ordinary template code. No injected server code is involved. The earlier </scr + input i + pt> example is the same mechanism with a smaller variable piece. These are pieces joined before HTML parsing, not JavaScript strings concatenated at runtime.

This explains why an isolated i sometimes needs protection. It does not prove that the final t in cat always needs it; that remains a possible optimization.
The space

You were right that HTML does not remove spaces between letters in a tag name. The relevant case here is a space after the name:
```
trusted text: </script
input:        one ASCII space
trusted text: >
```
Passing the space through produces </script >, which is a closing tag. Current encoding produces </script\x20>, which is not. The HTML script end-tag-name rules explicitly give space this delimiter role.

This can also happen when the input supplies some letters and then a space: </scr + input ipt + >. The letters are interior to the input in 1.6.0, so the space escape matters independently of the terminal-letter rule.

There is an important limit: if the template has already written </script , including the space, before calling the encoder, encoding later input cannot repair it. That case is already excluded by the documented insertion requirements.
The hyphen

This is about HTML's script tokenizer, rather than the JavaScript minus operator. Trusted <!- followed by an input - can form  across an insertion boundary.

Those sequences change the tokenizer's escaped/double-escaped states. For example, after a newly formed <!--, a later trusted <script> can put the parser into a state where the intended closing tag is consumed as text and the rest of the page can be swallowed into the script. A pre-existing legacy comment wrapper is not required for that opener to form. The relevant rules are the script escape-start and double-escape-start states.

Again, that does not make every interior hyphen necessary to escape. An ordinary value such as a-b is a reasonable candidate for more selective treatment.

For verification, this review used the implementation at 57692db370cb070609baaf20399a82cf6a53b56a; the relevant encoder/writer implementation matches 1.6.0. The two focused Java test suites passed 576 tests. Separate checks in Chrome 155.0.8059.85 used actual encoder output, then individually restored the raw letter, space, or hyphen. The current output kept the intended script boundaries in all three examples; each removal broke containment. Ordinary-value controls passed, and separate JavaScript evaluation checks preserved the intended string values.

Those are parser-containment results, not a claim that every example demonstrates attacker-controlled JavaScript execution. Early termination or swallowed page content can break a page; proving XSS requires a concrete surrounding application. These targeted checks also do not constitute a full release test run or cross-browser certification.

The promised model-assisted reviews completed, including Mythos, Daybreak Blue and Opus. Their agreement is not the basis for declaring something safe: one proposed optimization from that review was rejected because it failed the split-i example. The specification and reproducible parser behavior are the evidence that matters.

Where I think your feedback can still lead to an improvement is distinguishing ordinary safe text from the particular fragments that need a barrier. Encoding a complete string value, including its trusted text, can allow simpler reasoning than encoding a fragment between unencoded neighbors. However, the existing general/block APIs explicitly support those neighbors, so we should preserve that promise. Any refinement also needs to preserve Writer/chunk/flush behavior, not just the String-returning examples. The narrower source and attribute methods have different placement requirements; they are not replacements for inline script-block encoding merely to obtain shorter output.

I am keeping the issue open. I am not promising a release or an algorithm yet, but I am also not treating the current conservative output as necessarily optimal. A small example of the actual template or output placement you need would help focus that work on your readability concern. Your feedback has already helped separate “some protection is necessary” from “this exact amount of escaping is necessary,” and that distinction is worth preserving.

Thanks for the explanation. While I understand the examples I don't think they are valid examples. The code from the server side is broken in pct1. The structure of the DOM is also broken in pct2. Pct3 it looks like injecting values inside comments/CDATA/DOCTYPE... What I'm saying those are not real examples, those are not real javascript injections points., because we are talking about javascript here. Btw is "]" char encoded?

And as I said previously these examples exist only with server side code coordination. You control both the html structure + the injected values to come up with these examples. If this would happen in real life, I would say ok, but I argue nobody uses the encoder like this and nobody structures dynamic pages like this. Don't forget, we are talking about javascript here.

I'm with you. I'll do another sweep of this later today or tomorrow.

The 3rd example is valid, but not for javascript; maybe you could add a Encode.forXmlComment() for that.

And please, if you have any other details to share, give me a few examples of how you use the library. The more context the better, it will help my agent swarm get it right!

Navigation Menu

Repository navigation

Bug: forJavaScriptSource() should not encode "script" character in isolation #243

Artifact, version and runtime

Expected and actual behavior

Minimal non-sensitive reproduction

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Input	Current `forJavaScript()` output
`cat`	`ca\x74`
`dog`	`dog`
`i`	`\x69`
`http://google.com`	`http:\/\/google.com`

Bug: forJavaScriptSource() should not encode "script" character in isolation #243

Description

Artifact, version and runtime

Expected and actual behavior

Minimal non-sensitive reproduction

Activity

arpadf78 commented on Oct 15, 2026

jmanico commented on Oct 15, 2026

arpadf78 commented on Oct 15, 2026

jmanico commented on Oct 15, 2026

jmanico commented on Oct 15, 2026

jmanico commented on Oct 15, 2026

arpadf78 commented on Oct 16, 2026

jmanico commented on Oct 16, 2026

jmanico commented on Oct 17, 2026

arpadf78 commented on Oct 17, 2026

jmanico commented on Oct 18, 2026

arpadf78 commented on Oct 19, 2026

jmanico commented on Oct 19, 2026

jmanico commented on Oct 19, 2026

arpadf78 commented on Oct 20, 2026

jmanico commented on Oct 20, 2026

jmanico commented on Oct 20, 2026

arpadf78 commented on Oct 20, 2026

jmanico commented on Oct 20, 2026

arpadf78 commented on Oct 20, 2026

jmanico commented on Oct 20, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions