Skip to content

Reduce unnecessary JavaScript letter escaping while preserving boundary safety - #247

Draft
jmanico wants to merge 1 commit into
mainfrom
fix/issue-243-script-letter-escaping
Draft

jmanico wants to merge 1 commit into
mainfrom
fix/issue-243-script-letter-escaping

Conversation

@jmanico

@jmanico jmanico commented Oct 17, 2026

Copy link
Copy Markdown
Member

Change

Addresses #243, especially the reopening comment. forJavaScript and forJavaScriptBlock currently escape every occurrence of a script-name letter, unnecessarily obscuring ordinary text. This change leaves interior letters readable and escapes a script-name letter only at the end of the encoded value. Existing punctuation and whitespace escapes remain.

Examples of complete-value output:

Input Proposed output
http://google.com http:\/\/google.com
script scrip\x74
i \x69
scripture scripture

The terminal escape retains protection when a tag name is split across trusted text and encoded input. The rule is conservative, not minimal: cat still becomes ca\x74. Attribute-only and standalone-source modes are unchanged.

Streaming retains an undecidable trailing letter. EncodedWriter.flush() safely emits a pending script letter or high surrogate before flushing the wrapped writer, preserving barriers before subsequent trusted output. Explicit flushes can add escapes while preserving JavaScript UTF-16 values; ordinary write chunking remains byte-identical to whole-value encoding.

Draft for @arpadf78 to review. Please keep this unmerged and unreleased pending the reporter's feedback. This PR does not close the issue automatically or authorize a release.

Compatibility and validation

No public API or dependency changes. Encoded bytes change, and the guarantee is narrowed from prohibiting every literal </script substring to preventing recognized HTML tokens. Longer names such as </scripture> remain harmless. Existing insertion-boundary restrictions still apply. Documentation, Javadocs, TLDs and the changelog describe these changes. A future minor release is recommended; the development version is unchanged.

Low-level Encoder.encode callers must preserve unread lookahead and supply more input or mark end-of-input. Retrying unchanged nonfinal input can spin; custom streaming adapters must finish pending input before writing trusted suffixes.

Validation on the proposed tree:

  • JDK 17: ./mvnw -B -ntp clean verify passed, including 1,866 core tests, 218 tests per JSP adapter, coverage/style/Javadoc/API checks, OSGi/JPMS checks and both packaged JSP engine fixtures.
  • Actual Chrome: ./mvnw -B -ntp -PtestJakarta -Dtest=JavaScriptTemplateTest -Dsurefire.failIfNoSpecifiedTests=false -Dencoder.browser.local=true test passed all 12 tests. Added matrices cover 355,372 fixtures, including 75,376 unsafe controls and 324 live iframe pages with 24 executed-injection controls.
  • JDK 11/21/25: 39 boundary/Unicode tests passed on each runtime.
  • Supplemental local mutation validation rejected all ten targeted regressions, including removal of terminal escaping and either flush protection; the unchanged baseline passed.

Java 8 runtime execution and the full Docker/WAR release gate were not run locally. Browser execution was tested in Chrome, not every engine. Those release checks remain separate from this review.

  • I kept the change scoped and preserved relevant notices.
  • I checked documentation/changelog and tests for any behavior change.
  • This PR contains no credentials or private vulnerability evidence.

/**
* Whether the ASCII character can continue a case-insensitive script name.
*/
private static boolean isScriptNameLetter(char ch) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because it looks chars in isolation, this probably cannot distinguish "script" from "csript".

@arpadf78 arpadf78 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just one comment

This branch has not been deployed

No deployments
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