Skip to content

test(server): structural assertion that dispatch path holds no Map<RequestId,> state #2075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
felixweinberger wants to merge 1 commit into
base: fweinberger/r3-handlehttp
Choose a base branch
from
+49 −0
Draft

test(server): structural assertion that dispatch path holds no Map<RequestId,> state #2075

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions packages/server/test/ghsa345pStructural.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { readFileSync } from 'node:fs';
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';

import { describe, expect, test } from 'vitest';

/**
* Structural guard for GHSA-345p (cross-tenant routing via id-keyed shared state).
*
* The original vulnerability shape was a `Map<RequestId, ...>` (or equivalent) on a
* long-lived object that routes per-request data. The `dispatch(req, env)` path yields
* its outputs into a per-call iterator with no id-keyed routing map; this test asserts
* that property structurally so a future change cannot quietly reintroduce the shape.
*
* Intentionally NOT covered: `StreamDriver._responseHandlers` (outbound correlation for
* requests this side initiates over a persistent channel) is a `Map<RequestId, ...>` but
* is not in the inbound dispatch path and is not the GHSA shape. `shttpHandler` keys its
* abort map on `(sessionId, requestId)`, not bare `RequestId`.
Comment on lines +15 to +18
Copy link
Copy Markdown

@claude claude Bot May 12, 2026

Choose a reason for hiding this comment

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

🟡 The "Intentionally NOT covered" comment is factually wrong against current streamDriver.ts: _responseHandlers is Map<number, ...> (wouldn't match this regex anyway), while the field that would match — _requestHandlerAbortControllers: Map<RequestId, AbortController> — is populated in _onrequest(), so "not in the inbound dispatch path" doesn't hold. The actual reason StreamDriver is safe to exclude is that it's instantiated per-connection, so its id-keyed maps can't cross tenants — worth stating that instead, since for a GHSA regression guard the exclusion rationale is what future maintainers will lean on.

Extended reasoning...

What's wrong. The doc-comment block at lines 15–18 explains why StreamDriver is excluded from the structural scan, but both the field it names and the rationale it gives don't match packages/core/src/shared/streamDriver.ts:

  1. It says StreamDriver._responseHandlers "is a Map<RequestId, ...>". It isn't — line 85 declares it as Map<number, (response: ...) => void>. The test regex /(?::\s*|new\s+)Map<\s*(?:RequestId|string\s*\|\s*number)\b/ only matches RequestId or the literal union string | number, so a bare Map<number, would never trip the assertion even if streamDriver.ts were added to the files array. Citing _responseHandlers as the reason for exclusion is moot.
  2. It says the excluded map "is not in the inbound dispatch path". But the only StreamDriver field that would match the regex is _requestHandlerAbortControllers: Map<RequestId, AbortController> (line 88), and that one is populated inside _onrequest() (line 259) and consumed by inbound notifications/cancelled handling (line 297) — i.e. it is squarely in the inbound request-handling path.

Step-by-step proof. Suppose a maintainer reads the comment, then greps StreamDriver to verify the exclusion is still valid:

  • grep _responseHandlers → finds Map<number, ...> at L85. Type doesn't match what the comment claims; regex wouldn't catch it regardless. Comment claim (1) is false.
  • grep Map<RequestId in streamDriver.ts → finds _requestHandlerAbortControllers at L88. This does match /(?::\s*|new\s+)Map<\s*RequestId\b/.
  • Trace where it's set → _onrequest(request) at L259 does this._requestHandlerAbortControllers.set(request.id, abort). _onrequest is the inbound request handler. Comment claim (2) ("not in the inbound dispatch path") is false for the field that actually matches.

So the comment names the wrong field, states the wrong type, and gives a rationale that contradicts the one matching declaration.

Why nothing else fixes it. The test body never reads streamDriver.ts, so the assertions are unaffected — this is purely a prose error in the explanatory header. But for a GHSA regression-guard test, the exclusion rationale is the load-bearing documentation: it's what someone will read before deciding whether it's safe to add or drop a file from the scan. As written it will send them on a goose chase and force them to re-derive the real argument.

Correct rationale. StreamDriver is safe not because its id-keyed map is outbound-only (it isn't), but because a StreamDriver is instantiated per connection/transport — its maps are scoped to a single peer, so id collisions cannot cross tenants. That's a different (and stronger) argument than the one stated, and it's also the argument the PR description itself makes ("Per-connection correlation maps belong in StreamDriver (one instance per connection)").

Suggested fix. Replace lines 15–18 with something like:

 * Intentionally NOT covered: `StreamDriver` holds `_requestHandlerAbortControllers: Map<RequestId, ...>`,
 * but a StreamDriver is instantiated per connection so its id-keyed maps cannot cross tenants —
 * not the GHSA shape. `shttpHandler` keys its abort map on `(sessionId, requestId)`, not bare `RequestId`.

Severity. Nit — comment-only, no runtime or test-assertion impact.

*/
describe('GHSA-345p structural guard: no id-keyed maps in inbound dispatch path', () => {
const here = dirname(fileURLToPath(import.meta.url));
const files = [
join(here, '../../core/src/shared/dispatcher.ts'),
join(here, '../src/server/mcp.ts'),
join(here, '../src/server/shttpHandler.ts')
];

/**
* Matches the field-declaration shapes that produced GHSA-345p:
* `: Map<RequestId, ...>`
* `new Map<RequestId, ...>`
* `: Map<string | number, ...>` (RequestId's underlying union)
* `Record<RequestId, ...>`
*/
const idKeyedMapPattern = /(?::\s*|new\s+)Map<\s*(?:RequestId|string\s*\|\s*number)\b|Record<\s*RequestId\b/;

for (const file of files) {
test(`${file.split('/packages/')[1]} has no Map<RequestId, ...> field declarations`, () => {
Copy link
Copy Markdown

@claude claude Bot May 12, 2026

Choose a reason for hiding this comment

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

🟡 nit: file.split('/packages/')[1] won't match on Windows because path.join() produces backslash-separated paths there, so all three tests end up titled undefined has no Map<RequestId, ...> field declarations. Purely cosmetic (CI is ubuntu-only and the assertions still run correctly), but you could store the relative paths in the array and join(here, rel) only at readFileSync time, or use path.relative.

Extended reasoning...

What happens. Line 38 derives the test title with file.split('/packages/')[1]. The file value is built from dirname(fileURLToPath(import.meta.url)) plus join(here, '../../...'), both of which produce platform-native separators. On Windows that means the string looks like C:\\...\\packages\\core\\src\\shared\\dispatcher.ts, which does not contain the literal substring /packages/, so .split('/packages/') returns a one-element array and [1] is undefined.

Step-by-step on Windows:

  1. fileURLToPath(import.meta.url) → C:\\repo\\packages\\server\\test\\ghsa345pStructural.test.ts.
  2. dirname(...) → C:\\repo\\packages\\server\\test.
  3. join(here, '../../core/src/shared/dispatcher.ts') normalizes to C:\\repo\\packages\\core\\src\\shared\\dispatcher.ts.
  4. 'C:\\repo\\packages\\core\\...'.split('/packages/') → ['C:\\repo\\packages\\core\\...'] (no match).
  5. [1] → undefined, template literal renders undefined has no Map<RequestId, ...> field declarations for all three iterations.

Why nothing else prevents it. node:path is platform-aware by design; there's no normalization step between join() and the string split. The only consumer of file besides the title is readFileSync(file, 'utf8'), which is happy with the native path, so the test body still executes and asserts correctly.

Impact. Cosmetic only. Every workflow in .github/workflows/ uses runs-on: ubuntu-latest, so CI never sees this. Vitest does not error on duplicate test names within a describe, so the suite still passes; a Windows contributor running locally just sees three identically-named tests and, on failure, can't tell which file tripped the regex from the title alone (though the assertion message still prints the offending lines).

Fix. Easiest is to keep the relative strings as the source of truth:

const files = [
    'core/src/shared/dispatcher.ts',
    'server/src/server/mcp.ts',
    'server/src/server/shttpHandler.ts'
];
for (const rel of files) {
    test(`${rel} has no Map<RequestId, ...> field declarations`, () => {
        const src = readFileSync(join(here, '../..', rel), 'utf8');
        ...
    });
}

Alternatively split on a separator-agnostic regex (/[/\\\\]packages[/\\\\]/) or use path.relative(join(here, '../..'), file).

const src = readFileSync(file, 'utf8');
const matches: string[] = [];
for (const [i, line] of src.split('\n').entries()) {
if (idKeyedMapPattern.test(line)) {
matches.push(` ${i + 1}: ${line.trim()}`);
}
}
expect(matches, `id-keyed map declarations found:\n${matches.join('\n')}`).toEqual([]);
});
}
});
Loading