fix(security): cap POST compatibility arrays and harden safeHtml links#3607
fix(security): cap POST compatibility arrays and harden safeHtml links#3607lspassos1 wants to merge 1 commit into
Conversation
|
@lspassos1 is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds two targeted hardening guards: a per-key array cap (200 values) in the POST-to-GET compatibility path in
Confidence Score: 4/5Both guards are narrowly scoped and add strictly more restrictive behavior; the gateway change only affects the legacy POST compatibility fallback, and the dom-utils change only adds tokens to blank-target anchor rel attributes. The emitRequest call and early return for oversized arrays sit inside the try block that silently swallows exceptions, so any future synchronous throw from the analytics call would suppress the 400 response. The safeHtml integration path for target=_blank also has no end-to-end test, leaving a gap where attribute-ordering regressions would go undetected. server/gateway.ts (emitRequest placement inside try block) and tests/dom-utils.test.mts (missing safeHtml integration test) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming POST request] --> B{Route match?}
B -- Yes --> Z[Handle normally]
B -- No --> C{Content-Length < 1 MiB?}
C -- No --> D[Skip POST→GET conversion]
C -- Yes --> E[Clone & parse JSON body]
E -- parse error --> D
E -- parsed --> F{For each key: is Array?}
F -- Scalar --> G[searchParams.set]
F -- Array --> H{length > 200?}
H -- Yes --> I[Return 400 with parameter name]
H -- No --> J[searchParams.append each scalar item]
G --> K[Retry route match as GET]
J --> K
K -- matched --> Z
K -- no match --> L[404 / 405]
Reviews (1): Last reviewed commit: "fix(security): harden post compatibility..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import assert from 'node:assert/strict'; | ||
| import { describe, it } from 'node:test'; | ||
|
|
||
| import { ensureNoopenerRel } from '../src/utils/dom-utils.ts'; | ||
|
|
||
| describe('dom-utils safe link helpers', () => { | ||
| it('adds noopener and noreferrer for blank-target links (#3550)', () => { | ||
| assert.equal(ensureNoopenerRel(null), 'noopener noreferrer'); | ||
| }); | ||
|
|
||
| it('preserves safe rel tokens while removing opener (#3550)', () => { | ||
| assert.equal(ensureNoopenerRel('nofollow OPENER'), 'nofollow noopener noreferrer'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
safeHtml blank-target path untested
The new tests exercise ensureNoopenerRel in isolation, but the safeHtml integration — where the function is actually wired to strip opener and inject noopener noreferrer on <a target="_blank"> nodes — has no coverage. A regression in the safeHtml attribute-sanitization ordering (e.g., rel stripped before the blank-target guard runs) would go undetected because no test exercises the full safeHtml('<a href="https://x.com" target="_blank" rel="opener">…</a>') path.
| for (const [k, v] of Object.entries(body as Record<string, unknown>)) { | ||
| if (Array.isArray(v)) v.forEach((item) => { if (isScalar(item)) url.searchParams.append(k, String(item)); }); | ||
| else if (isScalar(v)) url.searchParams.set(k, String(v)); | ||
| if (Array.isArray(v)) { | ||
| if (v.length > POST_TO_GET_MAX_ARRAY_VALUES_PER_KEY) { | ||
| emitRequest(400, 'ok', null); | ||
| return new Response(JSON.stringify({ | ||
| error: 'Too many values for POST compatibility parameter', | ||
| parameter: k, | ||
| maxValues: POST_TO_GET_MAX_ARRAY_VALUES_PER_KEY, | ||
| }), { | ||
| status: 400, | ||
| headers: { 'Content-Type': 'application/json', ...corsHeaders }, | ||
| }); | ||
| } | ||
| v.forEach((item) => { if (isScalar(item)) url.searchParams.append(k, String(item)); }); | ||
| } else if (isScalar(v)) url.searchParams.set(k, String(v)); | ||
| } | ||
| } catch { /* non-JSON body — skip POST→GET conversion */ } |
There was a problem hiding this comment.
emitRequest called inside try block before early return
emitRequest is invoked from within the try body. If the function ever begins to throw synchronously (e.g., after a future refactor of the usage module), the exception would be swallowed by the surrounding catch { /* non-JSON body */ } block, silently skipping the 400 response and letting execution fall through to attempt a GET route match against the partially-built URL. Moving the analytics call and return outside the try block (similar to how every other emitRequest+return pair in this function is structured) avoids that coupling.
| for (const [k, v] of Object.entries(body as Record<string, unknown>)) { | |
| if (Array.isArray(v)) v.forEach((item) => { if (isScalar(item)) url.searchParams.append(k, String(item)); }); | |
| else if (isScalar(v)) url.searchParams.set(k, String(v)); | |
| if (Array.isArray(v)) { | |
| if (v.length > POST_TO_GET_MAX_ARRAY_VALUES_PER_KEY) { | |
| emitRequest(400, 'ok', null); | |
| return new Response(JSON.stringify({ | |
| error: 'Too many values for POST compatibility parameter', | |
| parameter: k, | |
| maxValues: POST_TO_GET_MAX_ARRAY_VALUES_PER_KEY, | |
| }), { | |
| status: 400, | |
| headers: { 'Content-Type': 'application/json', ...corsHeaders }, | |
| }); | |
| } | |
| v.forEach((item) => { if (isScalar(item)) url.searchParams.append(k, String(item)); }); | |
| } else if (isScalar(v)) url.searchParams.set(k, String(v)); | |
| } | |
| } catch { /* non-JSON body — skip POST→GET conversion */ } | |
| let oversizedKey: string | null = null; | |
| for (const [k, v] of Object.entries(body as Record<string, unknown>)) { | |
| if (Array.isArray(v)) { | |
| if (v.length > POST_TO_GET_MAX_ARRAY_VALUES_PER_KEY) { | |
| oversizedKey = k; | |
| break; | |
| } | |
| v.forEach((item) => { if (isScalar(item)) url.searchParams.append(k, String(item)); }); | |
| } else if (isScalar(v)) url.searchParams.set(k, String(v)); | |
| } | |
| if (oversizedKey !== null) { | |
| emitRequest(400, 'ok', null); | |
| return new Response(JSON.stringify({ | |
| error: 'Too many values for POST compatibility parameter', | |
| parameter: oversizedKey, | |
| maxValues: POST_TO_GET_MAX_ARRAY_VALUES_PER_KEY, | |
| }), { | |
| status: 400, | |
| headers: { 'Content-Type': 'application/json', ...corsHeaders }, | |
| }); | |
| } | |
| } catch { /* non-JSON body — skip POST→GET conversion */ } |
Cap POST-to-GET array expansion per body key before compatibility routing builds large query strings. Also normalize safeHtml blank-target anchor rel values so sanitized HTML cannot preserve opener access.
lspassos1
force-pushed
the
fix/post-get-array-noopener
branch
from
128661b to
cd71792
Compare
|
Addressed the two review items in cd71792:
Re-ran the focused tests, typecheck/typecheck:api, targeted Biome lint, diff check, and the full data test suite ( |
1 participant
Summary
Closes #3550 by hardening the gateway POST-to-GET compatibility path and external links opened from sanitized HTML. The gateway now rejects excessive repeated array parameters before route matching, and sanitized
_blanklinks always getnoopener noreferrerwhile dropping unsafeopenertokens.Root cause
The POST compatibility shim expanded JSON arrays into repeated GET query params without a per-key value cap, so a large array could create an oversized route/search-param workload. Separately,
safeHtml()stripped event handlers and invalid URLs but did not normalizerelfor_blankanchors, leaving reverse-tabnabbing protection dependent on caller-provided markup.Changes
server/gateway.ts.ensureNoopenerRel()and apply it to sanitized_blankanchors.openerfrom existing rel tokens while preserving safe rel tokens.safeHtml()integration path.Validation
npx tsx --test tests/premium-stock-gateway.test.mts tests/dom-utils.test.mts- 14/14 passnpm run typechecknpm run typecheck:apinpx biome lint server/gateway.ts src/utils/dom-utils.ts tests/premium-stock-gateway.test.mts tests/dom-utils.test.mts- exits 0; existing complexity/optional-chain warnings remain inserver/gateway.tsgit diff --check -- server/gateway.ts src/utils/dom-utils.ts tests/premium-stock-gateway.test.mts tests/dom-utils.test.mtsnpm run test:data- 7876/7876 passRisk
Low. The gateway change only affects the legacy POST compatibility fallback for GET handlers and fails closed when a single JSON array is unusually large. The link sanitizer change only strengthens
_blankanchor output by adding standard reverse-tabnabbing protections.