mirror of
https://github.com/mvschwarz/openrig-studio.git
synced 2026-11-07 05:56:21 -10:00
ctx.onClose returns nothing native, and its control fires at the real site
sdk-qa re-round 5 — MF1 request side (response side + ownCallback proven sound,
untouched). Two defects on ctx.onClose:
1. RETURN-VALUE CROSSING. ctx.onClose was `(cb) => req.on("close",
ownCallback(cb))`. EventEmitter.on returns the emitter — the native request —
so an ordinary handler got req straight back as the return value. ownCallback
guarded the callback's this/args but not the RETURN direction.
2. CONTROL-CANNOT-FIRE. The prior request-side control tested ownCallback in
isolation, so removing it from the real wiring left the suite green — it
satisfied "its own control" and missed the point (the control-must-fire
lesson, landing on exactly that ruling).
Both fixed. ctx-building is extracted to app/handler-context.mjs (makeCtx), so
the actual wiring is unit-testable; onClose now wraps the callback with
ownCallback AND returns undefined (never the emitter). test/handler-context.test.mjs
drives the ACTUAL makeCtx across the ctx crossing matrix — no native this, args,
or return — and carries negative controls proving each assertion can fail;
demonstrated by stripping both guards from the real makeCtx (suite RED, test 249
of the run fails) and restoring (green). The design note is corrected: the
request side now states the return crossing and the real-site control, not an
isolated-wrapper claim. The adapter's sanctioned `req` count drops 6->3 as
method/headers/body/close move into makeCtx; the confinement guard is updated
deliberately.
MF2 verified closed, unchanged. 374 -> 378 tests, 378/378 green.
This commit is contained in:
1 parent
0fa8d86607
commit
ba33aa1efa
6 files changed
+147
-24
No files matched your search
@@ -0,0 +1,35 @@
|
||||
import { ownCallback } from "./response-facade.mjs";
|
||||
|
||||
// The `ctx` a route handler receives — the request SIDE of the same rule the
|
||||
// response facade enforces: no native object (the request, its socket, an
|
||||
// emitter) crosses into handler-controlled code in any direction. The request
|
||||
// itself never reaches a handler; `ctx` carries only what a handler needs.
|
||||
//
|
||||
// The crossings that had to be closed here, matching the response side:
|
||||
// - `method`/`headers`/`searchParams`/`verb`/`rest` are plain data.
|
||||
// - `readBody()` resolves to a string — a primitive, no native object.
|
||||
// - `onClose(cb)` registers a close listener, and closing it needed BOTH:
|
||||
// * the callback is wrapped with `ownCallback`, because
|
||||
// `req.on("close", cb)` invokes `cb` with `this === req`; and
|
||||
// * the call returns UNDEFINED, because `EventEmitter.on` returns the
|
||||
// emitter — the native request — which an expression-bodied arrow handed
|
||||
// straight back to the handler as the return value (RETURN crossing).
|
||||
//
|
||||
// `readBody` is injected rather than imported so this module has no side effects
|
||||
// and is unit-testable against a fake request.
|
||||
export function makeCtx(req, routed, readBody) {
|
||||
return {
|
||||
method: req.method,
|
||||
headers: req.headers,
|
||||
searchParams: routed.searchParams,
|
||||
verb: routed.verb,
|
||||
rest: routed.rest,
|
||||
readBody: () => readBody(req),
|
||||
onClose(cb) {
|
||||
req.on("close", ownCallback(cb));
|
||||
// Return nothing: `req.on` returns the native emitter, which must not cross
|
||||
// back to the handler.
|
||||
return undefined;
|
||||
},
|
||||
};
|
||||
}
|
||||
+5
-14
@@ -21,7 +21,8 @@ import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { RUNTIME_OWNED_VERBS, SUBSTITUTABLE_VERBS, openVocabMap, lookup, verbMatches, routeRequest } from "./verbs.mjs";
|
||||
import { makeResFacade, ownCallback } from "./response-facade.mjs";
|
||||
import { makeResFacade } from "./response-facade.mjs";
|
||||
import { makeCtx } from "./handler-context.mjs";
|
||||
|
||||
const HERE = path.dirname(fileURLToPath(import.meta.url));
|
||||
const ROOT = path.resolve(HERE, "..");
|
||||
@@ -1306,19 +1307,9 @@ const handleRequest = async (req, res) => {
|
||||
// status the miss branch can have: the resolver returns an index and a
|
||||
// body object — data — never a closure and never a status.
|
||||
if (row) {
|
||||
const ctx = {
|
||||
method: req.method,
|
||||
headers: req.headers,
|
||||
searchParams: m.searchParams,
|
||||
verb: row.verb,
|
||||
rest: m.rest,
|
||||
readBody: () => readBody(req),
|
||||
// SAME CLASS as the response callbacks: `req.on("close", cb)` would
|
||||
// invoke a handler's close callback with `this === req`. The callback
|
||||
// is wrapped so the facade owns its invocation — called with no `req`
|
||||
// as `this` and no native args.
|
||||
onClose: (cb) => req.on("close", ownCallback(cb)),
|
||||
};
|
||||
// `ctx` is the request-side surface (app/handler-context.mjs): only data
|
||||
// and callbacks the runtime owns, never the native request or an emitter.
|
||||
const ctx = makeCtx(req, { searchParams: m.searchParams, verb: row.verb, rest: m.rest }, readBody);
|
||||
// A RESPONSE FACADE, not the raw `res` (MF1). Node's ServerResponse
|
||||
// back-references the request (`res.req`, the socket), so handing
|
||||
// handlers `res` let an ordinary handler read the url. The facade gives
|
||||
|
||||
@@ -39,11 +39,14 @@ never holds a pathname at all.
|
||||
(`ownCallback`) that the facade invokes — with no `this` a handler can use and
|
||||
no forwarded arguments — because Node invokes a response/emitter callback with
|
||||
the native object as its `this` (`res.end(chunk, cb)` runs `cb` with
|
||||
`this === res`; `req.on("close", cb)` runs it with `this === req`). The same
|
||||
wrapper guards the request side: `ctx.onClose`. **The boundary is that neither
|
||||
the request nor anything reaching it is in handler scope, passes through a
|
||||
handler's hands, OR arrives as the receiver of handler code the facade let a
|
||||
native object call.**
|
||||
`this === res`; `req.on("close", cb)` runs it with `this === req`). The request
|
||||
side (`ctx.onClose`, in `app/handler-context.mjs`) uses the same wrapper AND
|
||||
returns nothing: `EventEmitter.on` returns the emitter — the native request —
|
||||
so an expression-bodied arrow handed it straight back as the RETURN value, a
|
||||
crossing the callback wrapper did not cover. **The boundary is that neither the
|
||||
request nor anything reaching it is in handler scope, passes through a handler's
|
||||
hands, arrives as the receiver of handler code a native object invokes, OR is
|
||||
returned to the handler from a call it made.**
|
||||
|
||||
MF1's history is FIVE narrower fixes, each of which stated a rule too narrow
|
||||
and leaked through the next crossing: a spelling guard, then `res.req`, then a
|
||||
@@ -113,9 +116,14 @@ exist to catch a change that reintroduces it.
|
||||
no method returns/yields `res`, none read `this`, `pipeFrom` iterates a source
|
||||
without handing it the response, and (measured against a REAL `ServerResponse`)
|
||||
a handler callback passed to any method runs with no native `this` and no
|
||||
native argument. `ownCallback` is tested directly for the request side
|
||||
(`ctx.onClose`). sdk-qa's `pipe(res)` and `end(cb)` trojans are the negative
|
||||
native argument. sdk-qa's `pipe(res)` and `end(cb)` trojans are the negative
|
||||
controls.
|
||||
- **ctx crossing matrix** (`test/handler-context.test.mjs`): drives the ACTUAL
|
||||
`makeCtx` and asserts `ctx.onClose` leaks nothing native as `this`, as an
|
||||
argument, or as the return value — with negative controls proven to go RED when
|
||||
the wrapper or the return-handling is stripped. (An earlier request-side control
|
||||
tested `ownCallback` in isolation and could not fail when the real wiring
|
||||
broke.)
|
||||
- **Unwritability control** (`test/reserved-verbs.test.mjs`): plants sdk-qa's
|
||||
round-4 literal arm and round-5 derived-prefix arm at the top of the API flow
|
||||
and asserts each detonates as a `ReferenceError` naming the absent binding —
|
||||
|
||||
@@ -0,0 +1,87 @@
|
||||
// The request-side crossing matrix for `ctx`, driven against the ACTUAL makeCtx
|
||||
// the runtime uses — not an isolated test of the wrapper. sdk-qa re-round-5:
|
||||
// ctx.onClose leaked TWO ways — the callback ran with `this === req`, AND the
|
||||
// call returned the native emitter (req.on returns the emitter). And the prior
|
||||
// control could not fire: it tested ownCallback in isolation, so breaking the
|
||||
// real wiring stayed green. This exercises makeCtx directly and the negative
|
||||
// controls below prove it goes RED when either guard is stripped.
|
||||
//
|
||||
// node --test 'test/*.test.mjs'
|
||||
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { EventEmitter } from "node:events";
|
||||
import { makeCtx } from "../app/handler-context.mjs";
|
||||
|
||||
// A request stand-in: a real EventEmitter (so `.on`/`.emit` behave like Node's,
|
||||
// invoking listeners with `this === req` and forwarding args) carrying a url the
|
||||
// leak would recover.
|
||||
function fakeReq() {
|
||||
return Object.assign(new EventEmitter(), {
|
||||
url: "/secret?token=leak-me", method: "GET", headers: { host: "x" },
|
||||
});
|
||||
}
|
||||
const routed = { searchParams: new URLSearchParams("a=b"), verb: "/api/x", rest: "" };
|
||||
const noBody = () => Promise.resolve("");
|
||||
|
||||
test("ctx carries only data — no native request reachable through its fields", async () => {
|
||||
const req = fakeReq();
|
||||
const ctx = makeCtx(req, routed, noBody);
|
||||
assert.deepEqual(Object.keys(ctx).sort(), ["headers", "method", "onClose", "readBody", "rest", "searchParams", "verb"]);
|
||||
assert.equal(ctx.verb, "/api/x");
|
||||
assert.equal(ctx.searchParams.get("a"), "b");
|
||||
assert.equal(await ctx.readBody(), "", "readBody must resolve to a primitive body, not a native object");
|
||||
// No field is (or exposes) the request or an emitter.
|
||||
for (const v of Object.values(ctx)) assert.notEqual(v, req, "a ctx field IS the native request");
|
||||
});
|
||||
|
||||
test("ctx.onClose CROSSING MATRIX against the real wiring: no native this, args, or return", () => {
|
||||
const req = fakeReq();
|
||||
const ctx = makeCtx(req, routed, noBody);
|
||||
|
||||
// RETURN crossing: req.on returns the emitter (the native request). onClose
|
||||
// must not hand it back.
|
||||
const ret = ctx.onClose(() => {});
|
||||
assert.notEqual(ret, req, "ctx.onClose returned the native request emitter");
|
||||
assert.equal(ret, undefined, "ctx.onClose returned a value — it must return nothing native");
|
||||
|
||||
// CALLBACK this/args crossing: a native emitter invokes listeners with
|
||||
// `this === emitter` and forwards the emit args. The wrapped listener must see
|
||||
// neither.
|
||||
let seenThis = "unset", seenArgs = null;
|
||||
ctx.onClose(function (...a) { seenThis = this; seenArgs = a; });
|
||||
req.emit("close", req, "native-arg"); // emitter passes its own this + args
|
||||
assert.notEqual(seenThis, req, "the close callback ran with the native request as `this`");
|
||||
assert.equal(seenThis, undefined, "the close callback saw a `this`");
|
||||
assert.deepEqual(seenArgs, [], "the close callback received native arguments");
|
||||
});
|
||||
|
||||
// NEGATIVE CONTROLS — prove this file can FAIL at the real site. Each rebuilds a
|
||||
// ctx with one guard stripped (the shapes an accidental regression would take)
|
||||
// and asserts the matrix above WOULD have caught it. Without these, the tests
|
||||
// above could pass against broken wiring and nobody would know (the exact
|
||||
// control-cannot-fire defect sdk-qa found in the prior round).
|
||||
test("negative control: dropping the return-guard is caught", () => {
|
||||
const req = fakeReq();
|
||||
// Regressed onClose: expression-bodied, returns the emitter (the original leak).
|
||||
const brokenReturn = { onClose: (cb) => req.on("close", cb) };
|
||||
const ret = brokenReturn.onClose(() => {});
|
||||
assert.equal(ret, req, "precondition: the regressed onClose returns the emitter");
|
||||
// The matrix's return assertion would fail on this — demonstrated:
|
||||
assert.throws(() => assert.equal(ret, undefined),
|
||||
"the return-crossing assertion cannot fail — it would not catch the leak");
|
||||
});
|
||||
|
||||
test("negative control: dropping the callback-wrapper is caught", () => {
|
||||
const req = fakeReq();
|
||||
const brokenCb = { onClose: (cb) => { req.on("close", cb); } }; // no ownCallback
|
||||
let seenThis = "unset", seenArgs = null;
|
||||
brokenCb.onClose(function (...a) { seenThis = this; seenArgs = a; });
|
||||
req.emit("close", req, "native-arg");
|
||||
assert.equal(seenThis, req, "precondition: the unwrapped callback runs with this === req");
|
||||
// The matrix's this/args assertions would fail on this — demonstrated:
|
||||
assert.throws(() => assert.notEqual(seenThis, req),
|
||||
"the callback-this assertion cannot fail — it would not catch the leak");
|
||||
assert.throws(() => assert.deepEqual(seenArgs, []),
|
||||
"the callback-args assertion cannot fail — it would not catch the leak");
|
||||
});
|
||||
@@ -534,9 +534,10 @@ test("MF1: the raw request is CONFINED to the routing adapter — handlers never
|
||||
assert.equal(reqRefs(staticFlow), 0,
|
||||
"the static-file flow references `req` — it must work from the decision's url, not the raw request");
|
||||
// The adapter's sanctioned references: the (req, res) param, routeRequest(req.url),
|
||||
// req.method, req.headers, readBody(req), req.on. Any pre-lookup arm reading req
|
||||
// in ANY spelling raises this above the sanctioned count.
|
||||
assert.equal(reqRefs(adapter), 6,
|
||||
// and makeCtx(req, …). Method/headers/body/close moved into makeCtx
|
||||
// (app/handler-context.mjs), which is why this is 3 and not the earlier 6. Any
|
||||
// pre-lookup arm reading req in ANY spelling raises this above the sanctioned count.
|
||||
assert.equal(reqRefs(adapter), 3,
|
||||
"the adapter's `req` references changed — a new one is a pre-lookup channel into the raw request; " +
|
||||
"if this is a legitimate adapter change, update the count deliberately");
|
||||
});
|
||||
|
||||
@@ -368,6 +368,7 @@ test("starts when the manifest's whole directory is missing at boot", async (t)
|
||||
// than by reading.
|
||||
fs.cpSync(path.join(REPO, "app", "verbs.mjs"), path.join(dir, "app", "verbs.mjs"));
|
||||
fs.cpSync(path.join(REPO, "app", "response-facade.mjs"), path.join(dir, "app", "response-facade.mjs"));
|
||||
fs.cpSync(path.join(REPO, "app", "handler-context.mjs"), path.join(dir, "app", "handler-context.mjs"));
|
||||
|
||||
const srv = await start(dir);
|
||||
t.after(() => srv.stop());
|
||||
|
||||
Reference in new issue
Block a user