fix: pre-auth path traversal in /static/* handler (GHSA-mc8w-wjhw-45x5)

[JohnMcLear]

Fixes GHSA-mc8w-wjhw-45x5 — unauthenticated path-traversal / arbitrary file read in the /static/* handler (CVSS 9.8). Reported by @gcm-explo1t.

Root cause

sanitizePathname.ts deliberately leaves backslashes literal on POSIX (a \ is a legal filename byte there), so a segment like ..\..\.. contains no ./.. path components and passes every traversal check unchanged — the repo's own test sanitizePathname.ts documents ['posix', '..\\foo'] as accepted-unchanged.

Minify.ts then ran filename.replace(/\\/g, '/') unconditionally, after the sanitizer, with a comment claiming it was safe "because all ..\\ substrings have already been removed by sanitizePathname" — which is false on POSIX. That turned the already-sanitized bytes back into ../ traversal components after the check. Because path.join(pluginPath, libraryPath) produces an absolute path, path.resolve(ROOT_DIR, …) discards ROOT_DIR and surplus .. collapses at /, so the payload is root-depth-agnostic:

curl --path-as-is 'http://TARGET:9001/static/plugins/ep_etherpad-lite/static/..%5C..%5C..%5C..%5C..%5C..%5C..%5Cetc/passwd'

The route is mounted on expressPreSession, before sessionMiddleware/checkAccess, so no credentials are required. %5C is the only working separator (%2F decodes to / and is rejected by the existing .. guard). A reverse proxy does not mitigate it — %5C forwards unchanged. Disclosed settings.json/credentials.json//proc/self/environ escalate to an admin session and, via the plugin installer, in-process RCE.

Fix

Guard the backslash→slash conversion to Windows only (if (path.sep === '\\')), restoring the invariant already documented and enforced in sanitizePathname.ts. On POSIX the backslash bytes stay literal and resolve to a non-existent filename → 404. No behavior change for legitimate requests (the replace was already a no-op for valid POSIX paths). The sibling sink in sanitizePathname.ts is already correctly Windows-guarded; the LIBRARY_WHITELIST branch never did the replace.

Test

Adds src/tests/backend/specs/staticPathTraversal.ts:

• traversal payload → 404, asserts /etc/passwd is not disclosed
• /proc/self/cwd/settings.json payload → 404
• a legitimate plugin static asset still serves 200

Verified locally (Node 24): full backend suite 1614 passing; the 3 new specs pass. Reverting the one-line guard makes the /etc/passwd test fail with status 200. Live end-to-end reproduction against a prod server: fixed → 404, 0 bytes; vulnerable control → 200, 3686 bytes returning /etc/passwd.

Reported by @gcm-explo1t.

[qodo-code-review]

PR Summary by Qodo

Fix pre-auth /static/* path traversal via POSIX backslash handling (GHSA-mc8w-wjhw-45x5)

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Prevent encoded-backslash traversal from escaping plugin static roots on POSIX.
• Limit backslash→slash normalization to Windows, preserving sanitizePathname() invariants.
• Add backend regression tests to ensure traversal payloads 404 while valid assets still serve.

Diagram

graph TD
  A(["Unauth client"]) --> B["/static/* (Minify.ts)"] --> C["sanitizePathname()"] --> D["path.join(pluginPath, libraryPath)"] --> E{"Windows?"}
  E -->|"Yes"| F["replace \\u2192/"] --> G["statFile/getFile"] --> H[("Filesystem")] --> I["200/404"]
  E -->|"No"| G
  subgraph Legend
    direction LR
    _u(["Client"]) ~~~ _p["Handler/step"] ~~~ _d{"Decision"} ~~~ _fs[("Filesystem")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Re-sanitize after any path normalization

• ➕ Reduces risk of future post-sanitize mutations reintroducing traversal
• ➕ Enforces a single invariant: final path string is validated
• ➖ More code and potential platform-specific behavior changes
• ➖ Must be careful which path API (posix/win32) is used for checks

2. Normalize separators before sanitizePathname()

• ➕ Traversal checks apply to the final separator form
• ➕ Avoids post-check string mutations entirely
• ➖ On POSIX, changes meaning of literal backslashes (potentially breaking)
• ➖ Conflicts with current documented behavior that '' is a legal byte on POSIX

3. Use path.posix for URL-derived paths end-to-end

• ➕ Avoids platform-dependent semantics when interpreting URL paths
• ➕ Simplifies reasoning about '/' in URL context
• ➖ Requires careful mapping when running on Windows filesystems
• ➖ Broader refactor than needed for this vulnerability fix

Recommendation: Keep the PR’s approach: guarding the backslash→slash conversion to Windows only is the smallest, least breaking fix that restores the intended sanitizePathname() contract on POSIX and removes the post-sanitize traversal reinterpretation. Consider a follow-up hardening step (helper/utility for separator normalization + lint/guardrails) to prevent future post-sanitize string transforms from reintroducing similar issues.

Files changed (2) +87 / -7

Bug fix (1) +12 / -7

Minify.tsGuard backslash→slash conversion to Windows to prevent traversal +12/-7

Guard backslash→slash conversion to Windows to prevent traversal

• Restricts the backslash-to-forward-slash normalization to Windows (path.sep === '\\'). Updates the inline documentation to explain why unconditional replacement on POSIX can turn harmless '..\\' bytes into '../' traversal components after sanitization, enabling arbitrary file read (GHSA-mc8w-wjhw-45x5).

src/node/utils/Minify.ts

Tests (1) +75 / -0

staticPathTraversal.tsAdd regression tests for encoded-backslash traversal in /static/* +75/-0

Add regression tests for encoded-backslash traversal in /static/*

• Adds backend specs that exercise the historical exploit payloads using encoded backslashes (%5C) and assert they now 404 and do not disclose sensitive files (/etc/passwd, /proc/self/cwd/settings.json). Also verifies a legitimate plugin static asset is still served (200).

src/tests/backend/specs/staticPathTraversal.ts

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[Copilot]

Pull request overview

This PR fixes a pre-auth path traversal / arbitrary file read in Etherpad’s /static/* handler by preventing a post-sanitization backslash→slash normalization on POSIX, which could otherwise turn harmless ..\..\.. byte sequences into real ../ traversal components. It also adds a backend regression test to ensure the traversal payload is blocked while legitimate static assets still serve correctly.

Changes:

• Guard filename.replace(/\\/g, '/') in src/node/utils/Minify.ts so it only runs on Windows (path.sep === '\\').
• Add backend regression coverage for encoded-backslash traversal attempts against the plugin static handler.
• Include a sanity check that a normal plugin static asset is still served.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/node/utils/Minify.ts	Prevents POSIX backslash bytes from being transformed into / separators after sanitization (core vulnerability fix).
src/tests/backend/specs/staticPathTraversal.ts	Adds regression tests that assert traversal payloads 404 and a legitimate plugin asset still returns 200.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/tests/backend/specs/staticPathTraversal.ts:35

• This suite doesn't set a Mocha timeout. Many backend specs that call common.init() set this.timeout(30000) to avoid flakiness on slower CI or developer machines; without it, the default 2s timeout can intermittently fail before the server is ready.

describe(__filename, function () {
  before(async function () { agent = await common.init(); });