refactor(web): sharpen the src/lib vs src/utils rule and move misfiled modules (#1784)
* refactor(web): sharpen src/lib vs src/utils rule and move misfiled modules (#1776) Codify the empirical rule the web client already followed loosely — utils = pure functions that compute; lib = things that instantiate, adapt, or touch the environment — in AGENTS.md and clients/web/README.md, and move the modules that sat on the wrong side of it. Moves (source + co-located test): - lib/sandbox-csp -> utils (pure CSP-string construction; type-only import) - utils/browserTabVisibility -> lib (registers visibilitychange listeners) - utils/oauthResume -> lib (reads/writes sessionStorage) - utils/clearServerOAuthState -> lib (drives live InspectorClient / OAuthStorage) Borderline audits (kept in place): pendingReauth (pure type declarations), deepLink / correlateTransportErrors / mcpNetworkHeaders / oauthUx / oauthFlow / oauthNetworkPhase (pure transforms / type-only core imports) stay in utils; downloadFile stays whole in lib (dominant side is DOM side effects). Also relocate the lone src/test/lib/downloadFile.test.ts straggler beside its source at src/lib/downloadFile.test.ts, and document the test-placement rule (side-by-side by default; src/test/ only for core/ tests, the integration project, and shared scaffolding). Pure move — no behavior change; no build/coverage config keys off the boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * refactor(web): address review — fix stale paths, move customHeaders, one-way arrows (#1776) Responds to the @claude review on PR #1784: 1. Stale path references (fixed): update the four in-repo pointers left at pre-move paths — specification/v2_auth_mid_session.md (oauthResume, browserTabVisibility → lib/), and the two load-bearing sandbox security comments (sandbox_proxy.html, sandbox-controller.ts → src/utils/sandbox-csp). 2. lib/types/customHeaders (moved): it's pure (types + pure transforms), the rule's clearest counterexample. Moved lib/types/customHeaders.{ts,test.ts} → utils/ (no external importers; only its own test) and removed the undocumented lib/types/ subdir. Documented that utils/ is the home for pure shared domain types + their constructors — no separate types/ bucket. 3. One-way arrows (item 3): declare OAuthResumeAuthKind in utils/pendingReauth and re-export it from lib/oauthResume, so the only cross-directory arrow now points lib → utils. Codified that invariant in AGENTS.md. Wording nit: AGENTS.md + README now state diagnostic logging (console.warn/error) doesn't count as a side effect for the utils rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 2 — stale customHeaders refs, rule wording (#1776) Doc-only follow-up to the round-2 @claude review on PR #1784 (no code changed): 1. Stale paths (same class as round-1 item 1): the customHeaders move left three references to the deleted lib/types/customHeaders.ts path in specification/ (v2_ux_interfaces_phase_0_4_inventory.md, v2_ux_interfaces_plan.md ×2) — updated to utils/customHeaders.ts. 2. customHeaders anchor: note in AGENTS.md that its shape is staged for ServerSettingsForm (per v2_ux_interfaces_plan.md), so its zero-importer state reads as intentional rather than rot; kept as the no-types/-bucket illustration and led the anchor list with load-bearing modules. 3. Type-only-import clause: split the conflated clause — a type-only import (pendingReauth) and re-exporting core's pure functions/constants (oauthUx/oauthFlow) are distinct reasons to stay in utils; what makes a module lib is wrapping core's stateful runtime. Fixed in both AGENTS.md and the web README. Cosmetic: re-padded the v2_auth_mid_session.md table row to column width. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 3 — scope the "no types/ bucket" claim (#1776) Doc-only follow-up to the round-3 @claude review on PR #1784 (no code changed): 1. "no separate types/ bucket" overreached: clients/web/src/types/ does exist (navigation.ts + react-syntax-highlighter .d.ts ambient stubs wired via tsconfig.app.json paths). Scoped the claim in both AGENTS.md and the web README to "no types/ sub-bucket INSIDE lib/utils", and noted src/types/ is for ambient .d.ts stubs, not domain types. 2. README example list drifted from AGENTS.md's round-2 load-bearing anchors — realigned it (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer customHeaders from the headline list). 3. Clarified v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's destination, not as the v1.5 source location. Follow-up filed as #1785 (board Todo): src/types/navigation.ts InspectorTab is dead and duplicates utils/inspectorTabs.ts — kept out of this pure-move PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 4 — reconcile src/types/ claim with #1785 (#1776) Doc-only follow-up to the round-4 @claude review on PR #1784 (no code changed): The round-3 scope fix overshot: it asserted src/types/ holds "not domain types at all", but the dead navigation.ts InspectorTab is a plain-.ts domain type still living there, and specification/v2_ux_interfaces_plan.md:181-182 actively prescribes placing InspectorTab there — so AGENTS.md and that spec doc gave opposite guidance about the same directory. - AGENTS.md + web README: softened to "not the place for new domain types" and added a parenthetical that the one lingering domain type (dead navigation.ts) is tracked for removal in #1785; until then a pure domain type belongs in utils/, not src/types/. - v2_ux_interfaces_plan.md:181: marked the tab-enum placement guidance as superseded by #1785 (live type is InspectorTabId in utils/inspectorTabs.ts). Self-heals on merge of #1785. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 5 — fix garbled #1785 clause, restructure utils rule (#1776) Doc-only follow-up to the round-5 @claude review on PR #1784 (no code changed): 1. Grammar (item 1): the round-4 #1785 clause read "superseding [#1785] will remove it" — a participle with no subject. Reworded to "the dead navigation.ts InspectorTab, which #1785 will remove". 2. Readability (item 2): the src/utils/ bullet had accreted into a single ~1,400-char blob across five review rounds. Promoted the carve-outs to scannable sub-bullets (Domain types / Diagnostic logging / Importing from core) and lifted the src/types/ note out to its own line, since it's a statement about a sibling directory, not about utils. AGENTS.md only; the README's parallel sentence was already clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs+config: address review round 6 — correct the coverage-glob claim (#1776) Responds to the round-6 @claude review on PR #1784: 1. "no build/coverage glob" was factually wrong (should-fix): the web coverage include in clients/web/vite.config.ts DOES enumerate src/lib/** and src/utils/**. The conclusion (a move between them is coverage-neutral) survives, but the reason was wrong. Reworded both AGENTS.md and the web README to say nothing *enforces* the boundary — no path alias, and the include lists both dirs so a move is coverage-neutral — plus the important whitelist caveat: a module outside components/lib/utils/server falls out of the ≥90 gate entirely. 2. Removed a pre-existing duplicate glob in vite.config.ts (src/lib/**/*.{ts,tsx} appeared twice, lines 161 and 163). Inert (vitest de-dupes) but it sat three lines from the config the doc now describes. Nits: gave the README bullet the same round-5 sub-bullet restructure as AGENTS.md, and made its #1785 reference a real markdown link (bare #N doesn't autolink in rendered repo markdown). npm run ci green (config change re-verified through coverage + smokes + Storybook). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
d009fcca09
commit
a82a70f4f5
1 changed files with 60 additions and 19 deletions
170
clients/web/src/utils/sandbox-csp.test.ts
Normal file
170
clients/web/src/utils/sandbox-csp.test.ts
Normal file
|
|
@ -0,0 +1,170 @@
|
|||
import { describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
SAFE_CSP_SOURCE,
|
||||
approveCspSources,
|
||||
buildSandboxCspPolicy,
|
||||
escapeHtmlAttr,
|
||||
wrapSandboxedHtml,
|
||||
} from "./sandbox-csp";
|
||||
|
||||
describe("SAFE_CSP_SOURCE", () => {
|
||||
it.each([
|
||||
"*",
|
||||
"https://api.example.com",
|
||||
"https://api.example.com:8443",
|
||||
"https://api.example.com/path/seg",
|
||||
"wss://realtime.service.com",
|
||||
"*.example.com",
|
||||
"https://*.cloudflare.com",
|
||||
"data:",
|
||||
"blob:",
|
||||
"example.com",
|
||||
// Schemes are case-insensitive per the URL spec, so a capitalized or
|
||||
// mixed-case scheme is still a valid source (over-rejecting it would
|
||||
// silently drop a restriction a server asked for).
|
||||
"HTTPS://api.example.com",
|
||||
"WSS://realtime.service.com",
|
||||
"Https://*.cloudflare.com",
|
||||
])("accepts %s", (s) => {
|
||||
expect(SAFE_CSP_SOURCE.test(s)).toBe(true);
|
||||
});
|
||||
|
||||
it.each([
|
||||
"https://a.com; script-src *",
|
||||
'https://a.com" onload=',
|
||||
"https://a.com>",
|
||||
"<script>",
|
||||
" https://a.com",
|
||||
"https://a.com ",
|
||||
"",
|
||||
"javascript:alert(1)//;x",
|
||||
])("rejects %s", (s) => {
|
||||
expect(SAFE_CSP_SOURCE.test(s)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("approveCspSources", () => {
|
||||
it("returns an empty object for undefined / empty input", () => {
|
||||
expect(approveCspSources(undefined)).toEqual({});
|
||||
expect(approveCspSources({})).toEqual({});
|
||||
});
|
||||
|
||||
it("keeps safe entries per key and drops unsafe ones with a warning", () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const approved = approveCspSources({
|
||||
connectDomains: ["https://api.ok.com", "https://x.com; script-src *"],
|
||||
resourceDomains: ["data:", '">'],
|
||||
frameDomains: [],
|
||||
baseUriDomains: ["https://b.ok.com"],
|
||||
});
|
||||
expect(approved).toEqual({
|
||||
connectDomains: ["https://api.ok.com"],
|
||||
resourceDomains: ["data:"],
|
||||
baseUriDomains: ["https://b.ok.com"],
|
||||
});
|
||||
expect(warn).toHaveBeenCalledTimes(2);
|
||||
warn.mockRestore();
|
||||
});
|
||||
|
||||
it("omits a key when every entry is rejected", () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
expect(approveCspSources({ connectDomains: ['"; x'] })).toEqual({});
|
||||
warn.mockRestore();
|
||||
});
|
||||
|
||||
it("ignores non-array values without throwing", () => {
|
||||
expect(
|
||||
approveCspSources({
|
||||
connectDomains: "https://x.com" as unknown as string[],
|
||||
}),
|
||||
).toEqual({});
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildSandboxCspPolicy", () => {
|
||||
it("emits locked-down defaults for an empty approved csp", () => {
|
||||
const policy = buildSandboxCspPolicy({});
|
||||
expect(policy).toContain("default-src 'none'");
|
||||
expect(policy).toContain("connect-src 'none'");
|
||||
expect(policy).toContain("script-src 'unsafe-inline'");
|
||||
expect(policy).toContain("style-src 'unsafe-inline'");
|
||||
expect(policy).toContain("img-src 'none'");
|
||||
expect(policy).toContain("font-src 'none'");
|
||||
expect(policy).toContain("media-src 'none'");
|
||||
expect(policy).toContain("frame-src 'none'");
|
||||
expect(policy).toContain("base-uri 'self'");
|
||||
expect(policy).toContain("form-action 'none'");
|
||||
expect(policy).toContain("object-src 'none'");
|
||||
expect(policy).toContain("worker-src 'none'");
|
||||
});
|
||||
|
||||
it("maps approved domains to the correct directives", () => {
|
||||
const policy = buildSandboxCspPolicy({
|
||||
connectDomains: ["https://api.a.com", "wss://rt.a.com"],
|
||||
resourceDomains: ["https://cdn.a.com"],
|
||||
frameDomains: ["https://embed.a.com"],
|
||||
baseUriDomains: ["https://a.com"],
|
||||
});
|
||||
expect(policy).toContain("connect-src https://api.a.com wss://rt.a.com");
|
||||
expect(policy).toContain("script-src 'unsafe-inline' https://cdn.a.com");
|
||||
expect(policy).toContain("style-src 'unsafe-inline' https://cdn.a.com");
|
||||
expect(policy).toContain("img-src https://cdn.a.com");
|
||||
expect(policy).toContain("font-src https://cdn.a.com");
|
||||
expect(policy).toContain("media-src https://cdn.a.com");
|
||||
expect(policy).toContain("frame-src https://embed.a.com");
|
||||
expect(policy).toContain("base-uri https://a.com");
|
||||
// catch-all directives stay locked down regardless of approved domains
|
||||
expect(policy).toContain("default-src 'none'");
|
||||
expect(policy).toContain("form-action 'none'");
|
||||
expect(policy).toContain("object-src 'none'");
|
||||
expect(policy).toContain("worker-src 'none'");
|
||||
});
|
||||
});
|
||||
|
||||
describe("escapeHtmlAttr", () => {
|
||||
it("encodes &, quotes, and angle brackets", () => {
|
||||
expect(escapeHtmlAttr(`a&b"c'd<e>f`)).toBe(
|
||||
"a&b"c'd<e>f",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("wrapSandboxedHtml", () => {
|
||||
it("places the CSP meta as the literal first <head> child before any untrusted bytes", () => {
|
||||
const wrapped = wrapSandboxedHtml("<p>hi</p>", "default-src 'none'");
|
||||
const metaIdx = wrapped.indexOf("Content-Security-Policy");
|
||||
const bodyIdx = wrapped.indexOf("<body>");
|
||||
expect(metaIdx).toBeGreaterThan(-1);
|
||||
expect(metaIdx).toBeLessThan(bodyIdx);
|
||||
expect(wrapped.startsWith("<!DOCTYPE html><html><head><meta")).toBe(true);
|
||||
});
|
||||
|
||||
it("attribute-encodes the policy value", () => {
|
||||
const wrapped = wrapSandboxedHtml("", `x"><script>`);
|
||||
expect(wrapped).toContain('content="x"><script>"');
|
||||
expect(wrapped).not.toContain('"><script>');
|
||||
});
|
||||
|
||||
it("places untrusted full-document HTML inside <body> so its own <head> token cannot precede the policy", () => {
|
||||
const evil = `<!-- <head> --><!DOCTYPE html><html><head><script src="x"></script></head><body>app</body></html>`;
|
||||
const wrapped = wrapSandboxedHtml(evil, "default-src 'none'");
|
||||
// The first <head> in the output is OURS; the untrusted <head> token only
|
||||
// appears after <body>.
|
||||
const ourHead = wrapped.indexOf("<head>");
|
||||
const body = wrapped.indexOf("<body>");
|
||||
const evilHead = wrapped.indexOf("<head>", ourHead + 1);
|
||||
expect(ourHead).toBeLessThan(body);
|
||||
expect(evilHead).toBeGreaterThan(body);
|
||||
});
|
||||
|
||||
it("does not introduce a host-authored <iframe>, so the widget's window.parent remains the sandbox proxy", () => {
|
||||
// The proxy assigns the wrap output to the inner iframe's srcdoc.
|
||||
// If the wrap inserted its own <iframe>, the widget's window.parent would
|
||||
// point at the wrap's intermediate frame instead of the proxy, breaking the
|
||||
// postMessage relay (and the proxy's event.source === inner.contentWindow
|
||||
// check). The only <iframe>s in the output must be ones the untrusted app
|
||||
// brought itself.
|
||||
const wrapped = wrapSandboxedHtml("<p>no frames here</p>", "default-src *");
|
||||
expect(wrapped).not.toMatch(/<iframe/i);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue