Skip to content

Commit 21cf936

Browse files
authored
test(web): pin three server-function invariants that nothing guards (#3111)
* test(web): pin three server-function invariants that nothing guards The origin gate, the redirect mask's status set, and the error header's bound each came from a recent fix, and each can be undone today without a test failing. - The gate has six branches; two were exercised. Sec-Fetch-Site is authoritative when present, so same-site and none are refused outright and never reach the trusted-origin matcher — loosening same-site is the natural-looking repair for a broken subdomain deployment. Without Sec-Fetch-Site, Origin decides and then Referer; a matcher that answered true for a non-matching origin would fail open unnoticed, and the Referer branch was unexercised entirely. - The mask covers the statuses fetch follows, but only 302 was tested. Narrowing the set to {302, 303} leaves a scripted caller a real 307 that fetch chases before the transport can read it. - The error header's bound is applied by re-encoding a shrinking slice of the SOURCE; cutting the encoded form instead severs a percent escape and the value stops decoding. One message cannot tell the two apart — a naive slice survives whichever padding lands on an escape boundary — so the test sweeps six. Each was checked by mutating the built runtime: loosening same-site fails 1, narrowing the redirect set fails 3, slicing the encoding fails 5 of the 6 paddings. Tests only; no runtime change, so no changeset. * test(web): drop a duplicated case, correct a comment, cover two matchers Review of the first commit: - The parameterised redirect test repeated 302, which the file already covers as "a returned redirect envelope: masked for scripted, real for unscripted". Narrowed to the four statuses nothing exercised, and the describe now carries its issue reference like its neighbours. - The padding sweep's comment was wrong. Measured against a naive `encode(message).slice(0, LIMIT)`: padding 0 is the ONE case that still decodes, because the ceiling lands on an escape boundary — every other phase breaks. So the sweep was five copies of the same signal plus the one case that proves nothing. Two paddings state the property: the aligned case and a misaligned one. - The matrix left `matchesOrigin`'s function and array branches unexercised, which is precisely the fail-open the commit message warned about. Both now have a negative case. Each checked by mutating the built runtime: loosening same-site, a function matcher that returns true, an array matcher that returns true, and narrowing the redirect set each fail a test. * test(web): match the file's idiom for the two-case parameterisations `.each` appears once in all of packages/web/test; a plain loop reads the same and drifts less. The four-status redirect case keeps it, where the parameter is the point. Also a missing blank line.
1 parent 5230666 commit 21cf936

3 files changed

Lines changed: 217 additions & 0 deletions

File tree

packages/web/test/server/server-functions-csrf.spec.tsx

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,156 @@ describe("server-function CSRF bridge", () => {
4848
expect(response.status).toBe(200);
4949
});
5050
});
51+
52+
/**
53+
* The gate's branches, pinned. Each one is a decision someone could
54+
* plausibly "fix" in the wrong direction, and today only two of them fail
55+
* a test if reversed:
56+
*
57+
* - `Sec-Fetch-Site` is authoritative WHEN PRESENT. A `same-site` call
58+
* (a sibling subdomain) and a `none` call (address bar, bookmark) are
59+
* refused outright — they never fall through to the trusted-origin
60+
* matcher, so widening `origin` cannot re-admit them. Loosening
61+
* `same-site` is the natural-looking repair for a broken subdomain
62+
* deployment, and it would hand every subdomain a CSRF surface.
63+
* - Without `Sec-Fetch-Site`, `Origin` decides, then `Referer`; a matcher
64+
* that answered `true` for a non-matching origin would fail open, and
65+
* nothing currently notices.
66+
* - With no proof of origin at all, the request is refused unless the
67+
* deployment has explicitly opted out.
68+
*/
69+
describe("the origin gate's decision matrix", () => {
70+
for (const site of ["same-site", "none"]) {
71+
it(`refuses a ${site} call even when the Origin is trusted`, async () => {
72+
const fn = vi.fn(async () => "ok");
73+
registerServerFunction(`csrf-site-${site}`, fn);
74+
75+
const response = await handleServerFunctionRequest(
76+
request(`csrf-site-${site}`, {
77+
"Sec-Fetch-Site": site,
78+
Origin: "https://app.example"
79+
}),
80+
{ csrf: { origin: "https://app.example" }, provideEvent }
81+
);
82+
83+
expect(response.status).toBe(403);
84+
expect(fn).not.toHaveBeenCalled();
85+
});
86+
}
87+
88+
it("refuses an Origin the deployment does not trust", async () => {
89+
const fn = vi.fn(async () => "ok");
90+
registerServerFunction("csrf-origin-mismatch", fn);
91+
92+
const response = await handleServerFunctionRequest(
93+
request("csrf-origin-mismatch", { Origin: "https://evil.example" }),
94+
{ csrf: { origin: "https://trusted.example" }, provideEvent }
95+
);
96+
97+
expect(response.status).toBe(403);
98+
expect(fn).not.toHaveBeenCalled();
99+
});
100+
101+
it("defaults to the request's own origin when none is configured", async () => {
102+
registerServerFunction("csrf-origin-default", async () => "ok");
103+
104+
const same = await handleServerFunctionRequest(
105+
request("csrf-origin-default", { Origin: "https://app.example" }),
106+
{ provideEvent }
107+
);
108+
expect(same.status).toBe(200);
109+
110+
const other = await handleServerFunctionRequest(
111+
request("csrf-origin-default", { Origin: "https://evil.example" }),
112+
{ provideEvent }
113+
);
114+
expect(other.status).toBe(403);
115+
});
116+
117+
it("falls back to Referer when Origin is absent", async () => {
118+
registerServerFunction("csrf-referer", async () => "ok");
119+
120+
const allowed = await handleServerFunctionRequest(
121+
request("csrf-referer", { Referer: "https://app.example/some/page" }),
122+
{ provideEvent }
123+
);
124+
expect(allowed.status).toBe(200);
125+
126+
const refused = await handleServerFunctionRequest(
127+
request("csrf-referer", { Referer: "https://evil.example/some/page" }),
128+
{ provideEvent }
129+
);
130+
expect(refused.status).toBe(403);
131+
});
132+
133+
it("refuses a Referer it cannot parse rather than ignoring it", async () => {
134+
const fn = vi.fn(async () => "ok");
135+
registerServerFunction("csrf-referer-garbage", fn);
136+
137+
const response = await handleServerFunctionRequest(
138+
request("csrf-referer-garbage", { Referer: "not a url" }),
139+
{ provideEvent }
140+
);
141+
142+
expect(response.status).toBe(403);
143+
expect(fn).not.toHaveBeenCalled();
144+
});
145+
146+
it("refuses a request carrying no proof of origin, unless opted out", async () => {
147+
registerServerFunction("csrf-no-proof", async () => "ok");
148+
149+
const refused = await handleServerFunctionRequest(request("csrf-no-proof"), { provideEvent });
150+
expect(refused.status).toBe(403);
151+
152+
const allowed = await handleServerFunctionRequest(request("csrf-no-proof"), {
153+
csrf: { allowRequestsWithoutOriginCheck: true },
154+
provideEvent
155+
});
156+
expect(allowed.status).toBe(200);
157+
});
158+
159+
it("asks a function matcher, and honours its refusal", async () => {
160+
registerServerFunction("csrf-origin-fn", async () => "ok");
161+
const seen: string[] = [];
162+
const csrf: ServerFunctionCSRFOptions = {
163+
origin: async origin => {
164+
seen.push(origin);
165+
return origin === "https://trusted.example";
166+
}
167+
};
168+
169+
const allowed = await handleServerFunctionRequest(
170+
request("csrf-origin-fn", { Origin: "https://trusted.example" }),
171+
{ csrf, provideEvent }
172+
);
173+
expect(allowed.status).toBe(200);
174+
175+
const refused = await handleServerFunctionRequest(
176+
request("csrf-origin-fn", { Origin: "https://evil.example" }),
177+
{ csrf, provideEvent }
178+
);
179+
expect(refused.status).toBe(403);
180+
expect(seen).toEqual(["https://trusted.example", "https://evil.example"]);
181+
});
182+
183+
it("treats a list as the whole allowlist", async () => {
184+
registerServerFunction("csrf-origin-list", async () => "ok");
185+
const csrf: ServerFunctionCSRFOptions = {
186+
origin: ["https://one.example", "https://two.example"]
187+
};
188+
189+
for (const origin of ["https://one.example", "https://two.example"]) {
190+
const allowed = await handleServerFunctionRequest(
191+
request("csrf-origin-list", { Origin: origin }),
192+
{ csrf, provideEvent }
193+
);
194+
expect(allowed.status).toBe(200);
195+
}
196+
197+
const refused = await handleServerFunctionRequest(
198+
request("csrf-origin-list", { Origin: "https://three.example" }),
199+
{ csrf, provideEvent }
200+
);
201+
expect(refused.status).toBe(403);
202+
});
203+
});

packages/web/test/server/server-functions-encode-hygiene.spec.tsx

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest";
2020
import { markSafeError, respond } from "@solidjs/web";
2121
import {
2222
ERROR_HEADER,
23+
decodeErrorHeaderValue,
2324
handleServerFunctionRequest,
2425
registerServerFunction
2526
} from "@solidjs/web/server-functions/server";
@@ -163,3 +164,39 @@ describe("null-body statuses (#3095)", () => {
163164
}
164165
});
165166
});
167+
168+
/**
169+
* The bound is enforced by re-encoding a shrinking slice of the SOURCE,
170+
* never by cutting the encoded form — a percent escape severed in half
171+
* (`%D0` without its second byte) leaves a header that no longer decodes.
172+
* Asserting only the length cannot tell the two implementations apart:
173+
* ASCII encodes to itself, and a truncated Cyrillic value is still short
174+
* enough to pass. Decoding it is what pins the difference.
175+
*/
176+
describe("the bound applies to the source, not to the encoding (#3093)", () => {
177+
// A percent escape is six characters (`%D0%AF`), so where the ceiling
178+
// lands inside the encoded form depends on what precedes the run. With
179+
// no padding it lands on an escape boundary and a naive
180+
// `encode(message).slice(0, LIMIT)` produces a value that still decodes
181+
// — that case cannot tell the two implementations apart. One character
182+
// of padding moves the ceiling into the middle of an escape, and the
183+
// naive form stops decoding. Both are here so the property is stated
184+
// rather than sampled.
185+
for (const padding of [0, 1]) {
186+
it(`a bounded non-latin1 header decodes back to a prefix of the message (padding ${padding})`, async () => {
187+
const message = `${"x".repeat(padding)}${"\u042f".repeat(600)}`;
188+
const id = `bounded-cyrillic-roundtrip-${padding}`;
189+
registerServerFunction(id, async () => {
190+
throw markSafeError(new Error(message));
191+
});
192+
193+
const response = await handleServerFunctionRequest(scriptedPost(id));
194+
const encoded = response.headers.get(ERROR_HEADER)!;
195+
196+
expect(encoded.length).toBeLessThanOrEqual(1024);
197+
const decoded = decodeErrorHeaderValue(encoded);
198+
expect(decoded.length).toBeGreaterThan(0);
199+
expect(message.startsWith(decoded)).toBe(true);
200+
});
201+
}
202+
});

packages/web/test/server/server-functions-redirect-status.spec.tsx

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,3 +278,30 @@ describe("the no-JS form convention honors returned redirects (#3096)", () => {
278278
expect(response.headers.get("Location")).toBe("https://app.example/elsewhere");
279279
});
280280
});
281+
282+
/**
283+
* The mask is justified by ONE fact — that fetch follows these statuses —
284+
* so it has to cover exactly the set fetch follows. Exercising 302 alone
285+
* cannot show that: narrowing the set to {302, 303}, or dropping 307/308,
286+
* leaves a scripted caller a real redirect that fetch chases before the
287+
* transport can read it, and the redirect is silently lost.
288+
*/
289+
describe("every status in the Fetch redirect set masks, and only for scripted callers (#3096)", () => {
290+
// 302 is covered above; these are the four the file never exercised.
291+
it.each([301, 303, 307, 308])(
292+
"%i travels as the redirect carrier for scripted callers, real for the rest",
293+
async status => {
294+
const id = `redirect-set-${status}`;
295+
registerServerFunction(id, async () => respond(undefined, { status, headers: location }));
296+
297+
const masked = await handleServerFunctionRequest(scripted(id));
298+
expect(masked.status).toBe(200);
299+
expect(masked.headers.get(REDIRECT_HEADER)).toBe(`${status} https://app.example/elsewhere`);
300+
expect(masked.headers.get("Location")).toBeNull();
301+
302+
const real = await handleServerFunctionRequest(unscripted(id));
303+
expect(real.status).toBe(status);
304+
expect(real.headers.get("Location")).toBe("/elsewhere");
305+
}
306+
);
307+
});

0 commit comments

Comments
 (0)