Skip to content

chore(deps): vendor the unmaintained security escaper into core#7993

Merged
JohnMcLear merged 4 commits into
ether:developfrom
JohnMcLear:chore/vendor-security-escaper
Jun 22, 2026
Merged

chore(deps): vendor the unmaintained security escaper into core#7993
JohnMcLear merged 4 commits into
ether:developfrom
JohnMcLear:chore/vendor-security-escaper

Conversation

@JohnMcLear

Copy link
Copy Markdown
Member

What

The security npm package — escapeHTML, escapeHTMLAttribute, and the JS/CSS encoders — has had no release since 2012 (v1.0.0), yet it sits directly in Etherpad's XSS-defense path:

  • client-side: pad_utils.ts (linkify / escapeHtml), domline.ts
  • server-side: ExportHtml.ts (HTML export)

Keeping a 14-year-old, single-maintainer dependency guarding output encoding is a risk we don't need to carry. This PR vendors its implementation into core instead of swapping in a different escaper, so the escaping output is byte-for-byte identical and nothing downstream changes.

Second slice of the dependency-staleness audit (follows #7992).

Changes

  • static/js/security.ts now contains the escaping logic directly — reproduced verbatim from security@1.0.0 (MIT, Chad Weider, an original Etherpad author), with attribution. It no longer does require('security'). The full public API is preserved (all 7 functions), so plugins that require('ep_etherpad-lite/static/js/security') keep working unchanged.
  • pad_utils.ts requires the local ./security module instead of the bare 'security' specifier. (domline.ts and ExportHtml.ts already used the local module path.)
  • Dropped security from src/package.json dependencies and from Minify.ts's LIBRARY_WHITELIST — no bare security specifier is served to the browser anymore.

After this change there are zero bare require('security') call sites in the tree; every consumer routes through the vendored first-party module.

Why vendor instead of replace

The only functions actually used in core are escapeHTML (×5) and escapeHTMLAttribute (×10). Swapping to a different escaper (e.g. he) would change encoding output (named vs numeric entities, casing) and risk breaking plugins and snapshot/round-trip expectations. Vendoring keeps behaviour provably identical while removing the dead dependency.

Tests

  • Added tests/backend/specs/security.ts locking the byte-for-byte escaping output (named entities for & < > " ' /, lowercase hex &#xNN; for other ASCII, JS/CSS \uXXXX / \XXXXXX encoders, falsy-passthrough, full API surface).
  • security spec: 10 passing.
  • export_list + import specs (exercise the server-side ExportHtml → Security path with a full server boot): pass.
  • tsc --noEmit: clean.

🤖 Generated with Claude Code

The `security` npm package (escapeHTML / escapeHTMLAttribute and the JS/CSS
encoders) has had no release since 2012, yet it sits directly in Etherpad's
client-side XSS-defense path (pad_utils, domline) and the server-side HTML
export. Rather than keep a 14-year-old, single-maintainer dependency guarding
output encoding, vendor its implementation into core.

- static/js/security.ts now contains the escaping logic directly (reproduced
  verbatim from security@1.0.0, MIT, Chad Weider — byte-identical output) and
  no longer does `require('security')`. The full public API is preserved, so
  plugins that `require('ep_etherpad-lite/static/js/security')` keep working
  unchanged.
- pad_utils.ts requires the local './security' module instead of the bare
  'security' specifier (domline.ts and ExportHtml.ts already did).
- Drop `security` from src/package.json dependencies and from Minify's
  LIBRARY_WHITELIST (no bare specifier is served to the browser anymore).

Added tests/backend/specs/security.ts locking the byte-for-byte escaping
output so the vendored copy can never silently drift.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 21, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Vendor security HTML/CSS/JS escapers into core and remove npm dependency
✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

Description

• Vendor security@1.0.0 escaping helpers into core to remove an unmaintained dependency.
• Route client callers through the local ./security module; stop serving bare security.
• Add backend specs to lock byte-for-byte escaping output and preserved public API.
Diagram

graph TD
PU["pad_utils.ts"] --> SEC["static/js/security.ts"] --> OUT["Escaped output"]
DL["domline.ts"] --> SEC["static/js/security.ts"]
EX["ExportHtml.ts"] --> SEC["static/js/security.ts"]
TEST["security spec"] --> SEC["static/js/security.ts"]
PKG["src/package.json"] -->|"remove"| DEP{{"security npm"}}
MIN["Minify.ts whitelist"] -->|"remove"| DEP{{"security npm"}}
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Replace with a maintained escaper library (e.g., `he`)
  • ➕ Eliminates custom/vendor code ownership
  • ➕ Potentially broader coverage and ongoing security maintenance
  • ➖ High risk of output differences (named vs numeric entities, casing) breaking plugins/snapshots
  • ➖ Requires a compatibility audit across client + export paths
2. Keep the dependency but pin + vendor patches (fork)
  • ➕ Keeps consumption model similar to today
  • ➕ Allows applying fixes while retaining history
  • ➖ Still carries external supply-chain and maintenance overhead
  • ➖ Does not solve the “dead dependency in XSS path” concern as cleanly as first-party code
3. Implement only the two functions used in core
  • ➕ Smaller surface area to maintain
  • ➕ Simpler to reason about than 7-function API
  • ➖ Could break plugins that import static/js/security expecting the full API
  • ➖ Harder to guarantee byte-for-byte compatibility across all existing call sites

Recommendation: Vendoring the existing implementation into static/js/security.ts (with attribution) is the best trade-off here because it removes an unmaintained, security-critical dependency while keeping behavior byte-for-byte identical. The added spec that locks both the full API surface and exact escaping output is key to making this approach low-regression and should be kept as the long-term guardrail.

Files changed (6) +130 / -28

Refactor (2) +61 / -13
pad_utils.tsSwitch from bare 'security' require to local module import +1/-1

Switch from bare 'security' require to local module import

• Replaces 'require('security')' with 'import * as Security from './security'' so the client utility code uses the vendored first-party module path.

src/static/js/pad_utils.ts

security.tsVendor OWASP-style escapers with preserved public API +60/-12

Vendor OWASP-style escapers with preserved public API

• Replaces the previous re-export of the npm 'security' package with a verbatim implementation of the seven helper functions, retaining falsy passthrough and exact entity/escape formats. Uses ESM named exports to improve module resolution under modern tooling (e.g., vitest/vite).

src/static/js/security.ts

Tests (1) +64 / -0
security.tsAdd regression tests for escaping output and API surface +64/-0

Add regression tests for escaping output and API surface

• Introduces backend specs that directly exercise the pure escaping helpers to lock byte-for-byte output for HTML, HTML attributes, and JS/CSS encoders. Verifies the full set of exported helper functions remains available for plugin compatibility.

src/tests/backend/specs/security.ts

Other (3) +5 / -15
pnpm-lock.yamlRemove 'security@1.0.0' from lockfile snapshots +5/-13

Remove 'security@1.0.0' from lockfile snapshots

• Drops the 'security' package entries from the importer and package sections after removing the dependency. Includes incidental lockfile snapshot normalization for eslint resolver peer dependency metadata.

pnpm-lock.yaml

Minify.tsStop whitelisting bare 'security' for browser minification +0/-1

Stop whitelisting bare 'security' for browser minification

• Removes 'security' from 'LIBRARY_WHITELIST', ensuring the server no longer treats 'require('security')' as a browser-served library.

src/node/utils/Minify.ts

package.jsonDrop 'security' from runtime dependencies +0/-1

Drop 'security' from runtime dependencies

• Removes the unmaintained 'security' npm dependency from 'dependencies' now that the implementation is vendored into core.

src/package.json

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. No default export 🐞 Bug ⚙ Maintainability
Description
src/static/js/security.ts now exposes only named exports, so downstream code that previously
relied on a default/CommonJS-style import of ep_etherpad-lite/static/js/security can receive
undefined and crash when calling Security.escapeHTML. This is an avoidable
backwards-compatibility break for plugin ecosystems even though core itself uses require() /
import * as patterns that still work.
Code

src/static/js/security.ts[R42-68]

+export const escapeHTML = (text: string) => text && text.replace(HTML_CHARACTERS_EXPRESSION,
+    (c: string) => HTML_ENTITY_MAP[c] || c);
+
+// OWASP Guidelines: escape all non alphanumeric characters in ASCII space.
+const HTML_ATTRIBUTE_CHARACTERS_EXPRESSION = /[\x00-\x2F\x3A-\x40\x5B-\x60\x7B-\xFF]/gm;
+export const escapeHTMLAttribute = (text: string) => text && text.replace(HTML_ATTRIBUTE_CHARACTERS_EXPRESSION,
+    (c: string) => HTML_ENTITY_MAP[c] || `&#x${(`00${c.charCodeAt(0).toString(16)}`).slice(-2)};`);
+
+// OWASP Guidelines: escape all non alphanumeric characters in ASCII space.
+// Also include line breaks (for literal).
+const JAVASCRIPT_CHARACTERS_EXPRESSION = /[\x00-\x2F\x3A-\x40\x5B-\x60\x7B-\xFF\u2028\u2029]/gm;
+export const encodeJavaScriptIdentifier = (text: string) => text && text.replace(JAVASCRIPT_CHARACTERS_EXPRESSION,
+    (c: string) => `\\u${(`0000${c.charCodeAt(0).toString(16)}`).slice(-4)}`);
+
+export const encodeJavaScriptString = (text: string) => text && `"${encodeJavaScriptIdentifier(text)}"`;
+
+// This is not great, but it is useful.
+const JSON_STRING_LITERAL_EXPRESSION = /"(?:\\.|[^"])*"/gm;
+export const encodeJavaScriptData = (object: any) => JSON.stringify(object).replace(JSON_STRING_LITERAL_EXPRESSION,
+    (string: string) => encodeJavaScriptString(JSON.parse(string)));
+
+// OWASP Guidelines: escape all non alphanumeric characters in ASCII space.
+const CSS_CHARACTERS_EXPRESSION = /[\x00-\x2F\x3A-\x40\x5B-\x60\x7B-\xFF]/gm;
+export const encodeCSSIdentifier = (text: string) => text && text.replace(CSS_CHARACTERS_EXPRESSION,
+    (c: string) => `\\${(`000000${c.charCodeAt(0).toString(16)}`).slice(-6)}`);
+
+export const encodeCSSString = (text: string) => text && `"${encodeCSSIdentifier(text)}"`;
Evidence
The vendored module now defines only named exports (no export default), so
default-import/CommonJS-default consumers have no stable entry point. The repo’s tsconfig.json is
configured for module: CommonJS with esModuleInterop: true, a configuration where default-import
patterns are commonly used and where the absence of a default export is a frequent compatibility
pitfall for downstream TS consumers.

src/static/js/security.ts[31-68]
src/tsconfig.json[8-11]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/static/js/security.ts` used to be a CommonJS re-export (`module.exports = require('security')`). After vendoring, it became a TS module with only named exports (`export const ...`). Downstream consumers (notably plugins) that import it via a default import (or otherwise expect default/CommonJS semantics) can now break.
### Issue Context
- Repo TS config uses CommonJS modules and enables `esModuleInterop`, which makes default-import patterns common/expected.
- Core consumers were updated to `require('./security')` or `import * as Security from './security'`, so core likely won’t notice this regression, but plugins might.
### Fix Focus Areas
- src/static/js/security.ts[31-68]
- src/tsconfig.json[8-11]
### Suggested fix
Add a default export that aliases the existing API surface, without changing the behavior of the named exports. For example, at the bottom of `security.ts`:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Bare security require breaks 🐞 Bug ≡ Correctness
Description
Removing security from LIBRARY_WHITELIST and from core dependencies prevents
/static/plugins/security/... from resolving to node_modules/security, so any plugin/client code
that still does require('security') will fail at runtime. This is a backwards-compatibility break
for external consumers that relied on core providing the security module by its bare specifier.
Code

src/node/utils/Minify.ts[R39-44]

const LIBRARY_WHITELIST = [
'async',
'js-cookie',
-  'security',
'split-grid',
'tinycon',
'underscore',
Evidence
Minify.ts only serves requests under /static/plugins//... from ../node_modules//... if `` is
in LIBRARY_WHITELIST; with security removed, the browser can no longer fetch it that way.
Additionally, src/package.json no longer declares the security dependency, so core no longer
provides it for Node resolution either.

src/node/utils/Minify.ts[39-46]
src/node/utils/Minify.ts[167-193]
src/package.json[79-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Core no longer serves the `security` library under the bare module name (`security`) because it was removed from the dependency set and from the static `/static/plugins/<library>/...` serving whitelist. Any third-party plugin/client code that still uses `require('security')` will now 404 / throw at runtime.
## Issue Context
Internal core code now routes through `ep_etherpad-lite/static/js/security` (vendored), but external plugins might still depend on the old bare module name being available.
## Fix Focus Areas
Choose one (or both):
1) **Compatibility shim**: Provide a backwards-compatible way for browser-side `require('security')` to resolve to the vendored implementation (for example, by serving a shim at the expected `/static/plugins/security/...` location or by adding an alias in the client module loader if one exists).
2) **Explicit migration / release notes**: If intentionally breaking, document that `require('security')` is no longer available and that plugins must switch to `require('ep_etherpad-lite/static/js/security')`.
### Code pointers
- src/node/utils/Minify.ts[39-46]
- src/node/utils/Minify.ts[167-193]
- src/package.json[79-83]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread src/static/js/security.ts Fixed
CI "Run the new vitest tests" failed with `Cannot find module './security'`
from pad_utils.ts. vitest/vite's CJS require() shim doesn't add a `.ts`
extension when resolving a relative specifier, so `require('./security')`
couldn't locate security.ts. (The old bare `require('security')` resolved to
a real .js in node_modules, which is why this only surfaced after vendoring.)

- security.ts now uses ESM `export const` for the seven helpers instead of a
  `module.exports = {...}` block.
- pad_utils.ts imports it as `import * as Security from './security'`, which
  goes through vite's resolver (knows .ts) and is also properly typed.

CJS consumers (domline.ts, ExportHtml.ts, the backend spec) keep working via
tsx/esbuild ESM->CJS interop. Verified: tsc clean, full vitest suite 721
passing, and the mocha security/export/import specs 27 passing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@JohnMcLear JohnMcLear closed this Jun 21, 2026
@JohnMcLear JohnMcLear reopened this Jun 21, 2026
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit dd99bd7

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/static/js/security.ts Fixed
CodeQL flagged a high-severity exponential-backtracking alert on the
JSON-string-literal regex vendored from the `security` package:
`/"(?:\\.|[^"])*"/`. The `[^"]` class also matches a backslash, so it overlaps
with the `\\.` alternative and backtracks exponentially on adversarial input
like `"\!\!\!...` (no closing quote). The original lived inside node_modules so
it was never scanned; vendoring it surfaced the alert.

Fix to the canonical linear form `/"(?:[^"\\]|\\.)*"/`, where the backslash is
excluded from the character class so the two alternatives are mutually
exclusive. It matches exactly the same well-formed JSON string literals (and
encodeJavaScriptData only ever runs it over JSON.stringify output), so behaviour
is unchanged for valid input.

Added tests: encodeJavaScriptData output + a ReDoS guard that runs the regex
over 50k adversarial chars and asserts it returns in well under a second.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@JohnMcLear JohnMcLear merged commit 01500ca into ether:develop Jun 22, 2026
24 checks passed
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