fix(security): send upgrade-insecure-requests in production only - #1
Merged
Conversation
Covers removal of 641 lines of unreachable code (including the unused src/services scaffolding), a Vitest harness for the pure networking and routing logic, and six correctness fixes led by the routing lab's tied-cost route mislabelling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
Fixes three internal contradictions: the verification gate ran pnpm test before Phase 2 creates it, the approach section claimed one commit per phase against a seven-commit list, and the env.ts fix (3.6) had no commit assigned. Corrects an overstated claim: the falsy-guard and negative-delta changes in 3.1 are defensive only. The cost slider is min=1 and endpoints cannot be equal, so neither case is reachable today. Adds a live bug found while reviewing: z.number() accepts 1.5, and octetToBinary(1.5) returns "000001.1", so the converter silently renders a fractional binary string with no validation error. clampOctet moves to binary-utils.ts so the pure-logic spec can cover it. Also removes src/styles/ (a lone README, nothing imports it) for the same reason src/utils/ goes, defines clampOctet rather than leaving it dangling, narrows with typeof instead of a non-null assertion, and notes that .env is gitignored so that edit stays local. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
Seven tasks matching the spec's seven commits, each with exact file paths, complete code, and verified expected values. All test expectations were confirmed against the real implementations before being written down, and the routing fix was prototyped to confirm the post-fix output the tests assert. Corrects two spec errors found while writing the plan: the PageHeader collapse needs no page edits because all 11 pages already import from the wrapper path, and the documentation list omitted the tsconfig path aliases, docs/deployment.md, and four README locations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
Deletes lines with no importers: the src/services placeholder contracts, five unused visualization components, tool-card.tsx, types/platform.ts, src/data/routing.ts, metric-card, format.ts, and routing-utils.ts. Trims src/data/platform.ts from 156 to 8 lines, keeping only roadmapItems, which app/page.tsx still renders. Collapses the PageHeader pass-through into a single component, drops the now-empty src/utils and src/styles directories along with their tsconfig path aliases, and removes the four env slots that only existed for the deleted services. Also reads NEXT_PUBLIC_APP_URL as a literal process.env expression so the value survives Next's static replacement if env.ts is ever imported from a client component. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
Adds Vitest with a node environment and no jsdom, since the functions under test are pure. Characterization tests pin the current behaviour of findShortestPath and calculateSubnet so the fixes that follow cannot disturb them unnoticed. Wires pnpm test into CI and the pre-commit hook. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
The candidate list sorted on cost alone, so a route tying with the one Dijkstra chose sorted above it, and the reason string had no zero case. In the lab's default state the panel headed "Why this route wins" led with an unselected route reading "Costs 0 more than selected." Adds a secondary sort key so the selected route wins its cost tier, and replaces the bare subtraction with a helper covering the equal and cheaper cases. Also hoists selectedCost out of the map, where it was recomputed once per candidate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
The decimal and binary readouts were derived independently, so they could disagree. Clearing the field showed "NaN = 00000000", because ?? 0 does not catch NaN. Typing 1.5 showed "1.5 = 000001.1" with no validation error, because z.number() accepts floats and padStart pads "1.1" rather than rejecting it. Adds clampOctet to binary-utils, which truncates and clamps to 0-255, and derives both readouts from it. Placing it in the lib keeps it covered by the pure-logic spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
The learned-cache cutover was a bare `stepIndex >= 5`, coupling behaviour to an array position, and it duplicated the target IP and MAC that arpHosts already carries. Reordering the steps would have moved the cache update silently. Exports cacheLearnedStepIndex from the step data and derives both cache states from the host record. A test asserts the index resolves to the cache-update step, so removing that step fails loudly rather than making the cache appear populated from the first step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
parseIpv4Address accepted "192.168.001.1" and silently read it as 192.168.1.1. Real stacks reject leading zeros or interpret them as octal, and this app teaches IPv4 addressing, so quietly normalising them misleads. Both callers already surface a validation message on parse failure, so the stricter pattern produces a visible error rather than a blank panel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
The policy shipped as Content-Security-Policy-Report-Only with no report-uri or report-to directive, so it neither blocked anything nor collected reports. Enforcing the existing directives is low risk because script-src already permits 'unsafe-inline' and 'unsafe-eval', so Next's bootstrap is unaffected. It makes frame-ancestors, object-src, base-uri, form-action and connect-src real on a site that loads no external resources. Verified against a production build across all 12 routes with no console violations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24
The Turbopack dev server on 16.2.6 forked one `.next/dev/build/postcss.js` child process per PostCSS evaluation without bound. Compiling a single page spawned 30 processes within 1.8s and 552 within 20s, saturating every core and hard-freezing the machine. `turbopackPluginRuntimeStrategy: "workerThreads"` was not a way out either: it throws ERR_SOCKET_BAD_PORT in createIpc and the compile then hangs indefinitely. 16.3.0 fixes the spawn loop, and the same page now serves in 456ms with the server idling at 0% CPU across 4 processes. Separately, Next resolved its workspace root by taking the highest lockfile found while walking up, which reached a stray ~/package-lock.json outside the repo and made it watch and file-trace the entire home directory (400k+ files). 16.3.0 stops that walk at a workspace marker, but only because pnpm-workspace.yaml happens to sit here; deleting that file would let the walk escape again. Pinning turbopack.root and outputFileTracingRoot makes the boundary explicit rather than inferred. next-env.d.ts is regenerated by 16.3.0 typegen. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiVyuXHG9ySrihSvyMa6Db
`next dev` on 16.3.0 writes AGENTS.md and CLAUDE.md into the repo root and re-creates them on every run, so leaving them untracked means a permanently dirty `git status`. Committing them is what the generated file itself advises, and the content is a short pointer telling agents to read node_modules/next/dist/docs before assuming Next 16 matches their training data. Set `agentRules: false` in next.config.ts to stop generating them instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiVyuXHG9ySrihSvyMa6Db
Enforcing the CSP in 073baab made `upgrade-insecure-requests` live for the first time — it had been inert under Content-Security-Policy-Report-Only. Chrome exempts localhost from the upgrade, so `next dev` on http://localhost:3000 is unaffected and the regression is easy to miss. A bare LAN IP is not exempt: on the `Network:` URL that `next dev` prints, every subresource is rewritten to https, the HTTP-only dev server cannot answer, and the page renders with no CSS or JS at all. Verified by loading the dev server over the LAN IP in Chrome: the stylesheet request went to https://…:3117/…css and returned 503 before the fix, and http://…:3122/…css returning 200 with the page fully styled after it. Production builds still send the directive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BaSzXHiVdswD9rKoVh9Ln4
Confirmed in Safari against the fixed dev server: the page renders styled once the directive is dropped. The earlier note framed this as a LAN-IP problem, which understated it — Safari applies upgrade-insecure-requests on localhost as well, so the original breakage hit the plain `next dev` URL and not just the `Network:` one. Chrome on localhost is the only combination that masks the bug, which is how it survived review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BaSzXHiVdswD9rKoVh9Ln4
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR tightens the app’s security posture by enforcing an actual CSP while avoiding the Safari/HTTP-dev-server breakage caused by upgrade-insecure-requests, and it also folds in a broader “repo cleanup” effort (dead-code deletions + a new Vitest unit-test harness for pure logic).
Changes:
- Switch CSP header from report-only to enforced, and attempt to make
upgrade-insecure-requestsproduction-only. - Add Vitest (
pnpm test), wire it into CI and precommit, and add unit tests for networking/routing/subnet/binary/ARP logic. - Remove unused placeholder services/types/components/data/docs and trim platform data/documentation accordingly.
Reviewed changes
Copilot reviewed 52 out of 55 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds Vitest configuration (node env, @ alias). |
| tsconfig.json | Removes path aliases for deleted directories. |
| src/utils/format.ts | Removes unused formatter utility. |
| src/types/platform.ts | Removes unused platform card types. |
| src/styles/README.md | Removes unused styles directory documentation stub. |
| src/services/security/session-service.ts | Removes unused placeholder service contract. |
| src/services/security/rate-limit-service.ts | Removes unused placeholder service contract. |
| src/services/rbac/rbac-service.ts | Removes unused placeholder service contract. |
| src/services/quiz/quiz-service.ts | Removes unused placeholder service contract. |
| src/services/progress/progress-service.ts | Removes unused placeholder service contract. |
| src/services/payments/payment-service.ts | Removes unused placeholder service contract. |
| src/services/database/database-service.ts | Removes unused placeholder service contract. |
| src/services/contracts.ts | Removes unused placeholder contracts/types. |
| src/services/auth/auth-service.ts | Removes unused placeholder service contract. |
| src/services/audit/audit-service.ts | Removes unused placeholder service contract. |
| src/services/api/api-client.ts | Removes unused placeholder API client. |
| src/services/analytics/analytics-service.ts | Removes unused placeholder analytics service. |
| src/lib/networking/ipv4.ts | Tightens IPv4 parsing to reject leading-zero octets. |
| src/lib/networking/ipv4.test.ts | Adds unit tests for IPv4 parsing and int conversions. |
| src/features/subnet/lib/subnet-utils.test.ts | Adds characterization tests for subnet calculations. |
| src/features/routing/lib/shortest-path.test.ts | Adds characterization tests for shortest-path logic. |
| src/features/routing/lib/routing-utils.ts | Removes unused routing re-export shim. |
| src/features/routing/lib/path-candidates.ts | Improves tie-break sorting + reason text; factors delta description. |
| src/features/routing/lib/path-candidates.test.ts | Adds unit tests covering candidate ordering/reasons and edge cases. |
| src/features/binary/lib/binary-utils.ts | Adds clampOctet helper to normalize octet inputs. |
| src/features/binary/lib/binary-utils.test.ts | Adds tests for binary conversion + octet clamping. |
| src/features/binary/components/binary-converter.tsx | Uses clampOctet to keep decimal/binary display consistent. |
| src/features/arp/hooks/use-arp-simulation.ts | Removes hardcoded step index/addresses; derives from ARP source data. |
| src/features/arp/data/arp-steps.ts | Exports derived cacheLearnedStepIndex. |
| src/features/arp/data/arp-steps.test.ts | Adds tests ensuring derived ARP step index remains valid. |
| src/data/routing.ts | Removes unused routing data adapter. |
| src/data/platform.ts | Removes unused card data; retains only roadmapItems. |
| src/config/env.ts | Removes unused env placeholders; parses literal env keys for Next inlining. |
| src/components/shared/page-header.tsx | Inlines header markup and removes dependency on deleted component. |
| src/components/visualizations/protocol-state-panel.tsx | Removes unused visualization component. |
| src/components/visualizations/packet-inspector.tsx | Removes unused visualization component. |
| src/components/visualizations/network-background.tsx | Removes unused visualization component. |
| src/components/visualizations/event-timeline.tsx | Removes unused visualization component. |
| src/components/visualizations/bit-grid.tsx | Removes unused visualization component. |
| src/components/shared/tool-card.tsx | Removes unused shared UI component. |
| src/components/shared/metric-card.tsx | Removes unused shared UI component. |
| src/components/shared/learning-page-header.tsx | Removes unused shared header component (replaced by PageHeader impl). |
| README.md | Updates scripts/CI/security notes and documents CSP/dev behavior. |
| docs/deployment.md | Removes references to deleted placeholder env vars. |
| docs/architecture.md | Removes “future services” section now that services are deleted. |
| docs/superpowers/specs/2026-08-04-repo-cleanup-design.md | Adds repo-cleanup design/spec document. |
| docs/superpowers/plans/2026-08-04-repo-cleanup.md | Adds detailed execution plan for the cleanup/fixes. |
| package.json | Adds Vitest + pnpm test, updates precommit, bumps Next + eslint-config-next. |
| next.config.ts | Pins workspace roots, enforces CSP, and conditionally includes upgrade-insecure-requests. |
| next-env.d.ts | Adds reference to generated root-params types. |
| AGENTS.md | Adds Next agent rules stub file. |
| CLAUDE.md | Adds pointer to @AGENTS.md. |
| .github/workflows/ci.yml | Adds pnpm test step in CI. |
| .env.example | Removes placeholder private env vars (now unused). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+24
| const isDev = process.env.NODE_ENV === "development"; | ||
|
|
Comment on lines
14
to
21
| "typecheck": "next typegen && tsc --noEmit", | ||
| "test": "vitest run", | ||
| "docker:build": "docker build -t acklab:local .", | ||
| "docker:run": "docker run --rm -p 3000:3000 acklab:local", | ||
| "docker:compose": "docker compose up --build", | ||
| "clean": "rm -rf .next out coverage tsconfig.tsbuildinfo", | ||
| "precommit": "lint-staged && pnpm typecheck", | ||
| "precommit": "lint-staged && pnpm typecheck && pnpm test", | ||
| "prepare": "husky" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The app rendered with no CSS or JS at all in Safari.
073baabflipped the CSP header fromContent-Security-Policy-Report-OnlytoContent-Security-Policy. That was a one-word change, but it madeupgrade-insecure-requestslive for the first time. The dev server speaks plain HTTP, so the browser rewrote every subresource tohttps://, nothing loaded, and pages fell back to raw unstyled HTML.Browser behaviour differs in kind here, which is why it slipped through:
localhostNetwork:URL)Chrome on
localhostis the only combination that masks the bug completely.073baabwas verified as "across all 12 routes with no console violations" — that pass was Chrome-only.Fix
Send
upgrade-insecure-requestsin production builds only. Every other directive is unchanged in both modes, soframe-ancestors,object-src,base-uri,form-actionandconnect-srcstay enforced in dev — the security posture073baabwas after is intact.Verification
https://…:3117/…css→ 503 before;http://…:3122/…css→ 200 after, page fully styled.localhost: confirmed rendering correctly against the fixed dev server.Fast-forward onto
main, no conflicts.🤖 Generated with Claude Code
https://claude.ai/code/session_01BaSzXHiVdswD9rKoVh9Ln4