security(proxy-path): set Vary on public routes that echo x-proxy-path (cache poisoning)

[JohnMcLear]

Follow-up hardening to GHSA-fjgc-3mj7-8rg8 (x-proxy-path admin XSS/open-redirect, fixed in #7784). Reported privately by meifukun (https://github.com/meifukun).

That advisory added Vary: x-proxy-path to the admin routes but left the public routes uncovered. The home page, pad page and timeslider (plus the legacy timeslider redirect) echo the sanitised x-proxy-path prefix into rendered URLs, social-preview metadata, manifest links and the redirect target — but advertised no Vary. A shared cache/CDN keyed on URL alone could store a prefix injected by one client and serve it to others.

Scope

This is cache-correctness hardening, not XSS/open-redirect — the value is still passed through sanitizeProxyPath (quotes / angle-brackets / protocol-relative // / .. already blocked by #7784). Deliberately does not change x-proxy-path's trust model: it stays honored under trustProxy: false because subpath / Home-Assistant-ingress deployments rely on that default. The fix is purely additive (Vary).

Change

Adds a varyOnProxyPath(res) helper applied at every sanitizeProxyPath() call site in specialpages.ts (both dev-watched and prod route blocks).

Test

New proxyPathVary.ts asserts Vary: x-proxy-path on /, /p/:pad, the timeslider embed page, and the legacy redirect. Existing proxyPathRedirect.ts still passes; tsc --noEmit clean.

🤖 Generated with Claude Code

[qodo-code-review]

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]

PR Summary by Qodo

security(proxy-path): add Vary on public routes that echo x-proxy-path

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Add Vary on public HTML/redirect routes that incorporate sanitized proxy-path prefixes.
• Prevent shared-cache/CDN cross-user prefix contamination (cache poisoning) via header-keyed
 variants.
• Add backend regression tests covering /, /p/:pad, timeslider embed, and legacy redirect.

Diagram

graph TD
  A["Client request"] --> B[("Shared cache/CDN")] --> C["Express public routes"] --> D["sanitizeProxyPath(req)"] --> E["varyOnProxyPath(res)"] --> F["HTML / redirect response"]
  F -- "cached per Vary" --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Global middleware to always Vary on proxy-path headers

• ➕ Eliminates risk of missing future call sites that echo proxy-path into responses
• ➕ Centralizes caching semantics in one place
• ➖ May add Vary to responses that do not actually vary by proxy-path, reducing cache hit rate
• ➖ Harder to scope to only routes that reflect the value into body/redirect targets

2. Set Vary inside shared render helpers (e.g., social meta / template wrapper)

• ➕ Keeps behavior close to the rendering path that consumes proxyPath
• ➕ Reduces duplicated call sites while staying narrower than global middleware
• ➖ Still risks gaps for redirects or other non-template responses that use sanitizeProxyPath
• ➖ Couples rendering utilities to HTTP caching headers

Recommendation: The PR’s explicit varyOnProxyPath(res) helper at each sanitizeProxyPath() usage in public routes is a good balance: it’s narrowly scoped to the known reflection points (HTML + redirect) and mirrors the prior admin hardening without changing trust behavior. If future proxy-path reflection expands, consider middleware or a render-layer hook to make omissions less likely.

Files changed (2) +74 / -0

Bug fix (1) +15 / -0

specialpages.tsEmit 'Vary' on public routes that reflect sanitized proxy-path +15/-0

Emit 'Vary' on public routes that reflect sanitized proxy-path

• Introduces a small helper ('varyOnProxyPath') that sets 'Vary' for proxy-path-related headers. Applies it to the home, pad, timeslider embed, and legacy timeslider redirect handlers wherever 'sanitizeProxyPath(req)' is used.

src/node/hooks/express/specialpages.ts

Tests (1) +59 / -0

proxyPathVary.tsRegression tests for 'Vary: x-proxy-path' on public pages/redirect +59/-0

Regression tests for 'Vary: x-proxy-path' on public pages/redirect

• Adds backend tests asserting that public routes which reflect proxy-path information advertise 'Vary: x-proxy-path'. Covers '/', '/p/:pad', '/p/:pad/timeslider?embed=1', and the '/p/:pad/timeslider' redirect response.

src/tests/backend/specs/proxyPathVary.ts

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

Code Review by Qodo

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

1. Over-broad Vary headers ✓ Resolved 🐞 Bug ➹ Performance

Description

specialpages.ts now varies public responses on x-forwarded-prefix and x-ingress-path even when
sanitizeProxyPath() will ignore those headers unless settings.trustProxy is enabled,
unnecessarily fragmenting shared caches on attacker-supplied headers. This reduces cache hit rate
and, in some shared-cache/CDN setups, can increase cache churn/resource usage without improving
correctness when trustProxy is false.

Code

src/node/hooks/express/specialpages.ts[R33-34]

+const PROXY_PATH_VARY_HEADERS = ['x-proxy-path', 'x-forwarded-prefix', 'x-ingress-path'];
+const varyOnProxyPath = (res: any) => res.vary(PROXY_PATH_VARY_HEADERS);

Evidence

The production code declares all three headers in PROXY_PATH_VARY_HEADERS, but
sanitizeProxyPath() explicitly ignores x-forwarded-prefix and x-ingress-path unless
trustProxy is enabled, so varying on them is unnecessary in the default/untrusted-proxy mode.

src/node/hooks/express/specialpages.ts[28-35]
src/node/utils/sanitizeProxyPath.ts[12-15]
src/node/utils/sanitizeProxyPath.ts[57-61]

Agent prompt

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

## Issue description
`varyOnProxyPath()` unconditionally varies on `x-forwarded-prefix` and `x-ingress-path`, but `sanitizeProxyPath()` only consults those headers when `settings.trustProxy` is true. When `trustProxy` is false, the response representation does not depend on those headers, so advertising them in `Vary` can cause unnecessary cache-key fragmentation.
## Issue Context
- `sanitizeProxyPath()` gates `x-forwarded-prefix` and `x-ingress-path` on `trustProxy`.
- `specialpages.ts` now unconditionally calls `res.vary([...])` for all public routes.
## Fix Focus Areas
- src/node/hooks/express/specialpages.ts[28-35]
## Suggested fix
Make `varyOnProxyPath()` configuration-aware:
- Always `res.vary('x-proxy-path')`.
- Only add `x-forwarded-prefix` and `x-ingress-path` when `settings.trustProxy` is true (or when you explicitly enable those headers in `sanitizeProxyPath()` for the current deployment).
Example:

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

---

2. Test omits extra Vary ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new proxyPathVary.ts test only asserts Vary contains x-proxy-path, so it will not catch
regressions where x-forwarded-prefix or x-ingress-path are accidentally removed from the
production PROXY_PATH_VARY_HEADERS list. This weakens the intended coverage of the newly added
multi-header vary behavior.

Code

src/tests/backend/specs/proxyPathVary.ts[R21-28]

+const assertVariesOnProxyPath = (res: any, where: string) => {
+  const vary = String(res.headers.vary || '').toLowerCase();
+  if (!vary.split(',').map((s: string) => s.trim()).includes('x-proxy-path')) {
+    throw new Error(
+        `${where}: response must advertise "Vary: x-proxy-path" so a shared ` +
+        `cache cannot serve a poisoned proxy-path to another user ` +
+        `(got Vary: "${res.headers.vary || ''}")`);
+  }

Evidence

Production code now varies on three proxy-related headers, but the new test only checks for one of
them, so it cannot detect regressions affecting the other two headers.

src/node/hooks/express/specialpages.ts[33-34]
src/tests/backend/specs/proxyPathVary.ts[21-29]

Agent prompt

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

## Issue description
The test helper `assertVariesOnProxyPath()` only checks for `x-proxy-path`, even though production code varies on three headers. The test suite would still pass if `x-forwarded-prefix` or `x-ingress-path` were removed.
## Issue Context
Production defines:
- `['x-proxy-path', 'x-forwarded-prefix', 'x-ingress-path']`
Test asserts only:
- `'x-proxy-path'`
## Fix Focus Areas
- src/tests/backend/specs/proxyPathVary.ts[21-29]
- src/node/hooks/express/specialpages.ts[33-34]
## Suggested fix
Update the test to assert all intended vary headers are present.
Options:
1) Hardcode the expected list in the test and assert each is included.
2) (Better) If behavior is meant to be configuration-dependent, add a test variant where `trustProxy` is enabled and verify `x-forwarded-prefix` + `x-ingress-path` are included (and/or actually influence output), while still always expecting `x-proxy-path`.
Example assertion:

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

---

[Copilot]

Pull request overview

This PR hardens Etherpad’s public HTML/redirect responses against cache poisoning when a reverse proxy injects a per-request subpath via x-proxy-path, by ensuring caches correctly key responses that reflect the sanitized proxy-path into rendered content and redirects.

Changes:

• Add a varyOnProxyPath(res) helper and apply it at each sanitizeProxyPath() call site in specialpages.ts (dev-watch and production route blocks).
• Add a new backend spec to assert Vary: x-proxy-path is present on key public routes (/, /p/:pad, timeslider embed, and the legacy timeslider redirect).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/node/hooks/express/specialpages.ts	Adds Vary behavior on public routes that render/redirect using the sanitized proxy-path prefix.
src/tests/backend/specs/proxyPathVary.ts	Adds regression tests asserting Vary: x-proxy-path on relevant public endpoints.

---

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

[JohnMcLear]

Thanks @qodo-free-for-open-source-projects — agreed. sanitizeProxyPath() ignores x-forwarded-prefix/x-ingress-path unless trustProxy is on, so varying on them otherwise just fragments caches. Fixed in the latest commit: always Vary: x-proxy-path, add the two standard headers only when trustProxy is enabled; test covers both states.

[qodo-code-review]

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]

Thanks @qodo-free-for-open-source-projects — agreed. sanitizeProxyPath() ignores x-forwarded-prefix/x-ingress-path unless trustProxy is on, so varying on them otherwise just fragments caches. Fixed in the latest commit: always Vary: x-proxy-path, add the two standard headers only when trustProxy is enabled; test covers both states.

Thanks for the update. The implementation and coverage now correctly keep x-proxy-path in Vary always and add the trust-gated headers only when trustProxy is enabled.

[Copilot]

Pull request overview

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