P2 — web SPA data layer + same-origin auth #1144

Merged

shaggy merged 5 commits from feat/webui-p2-data-auth into next 2026-08-10 01:52:02 +00:00

Conversation 2 Commits 5 Files Changed 23

+1094 -143

shaggy commented 2026-08-10 01:51:06 +00:00

Contributor

Copy Link

Copy Source

P2 — Web SPA data layer + same-origin auth (Phase P RFC increment)

Ports the web auth surface to the same-origin Vite/React-Router SPA architecture from the approved Phase P RFC. Second increment after P1 (Vite skeleton, merged).

What changed (apps/web)

• Relative-path data layer — lib/api.ts uses relative /api/...; lib/auth-client.ts omits absolute baseURL; lib/socket.ts uses io('/chat'). No gateway origin anywhere in src (§2.4 same-origin hard invariant).
• Runtime SSO discovery — deleted the static lib/sso-providers.ts (build-time NEXT_PUBLIC list); both SPA and Next baseline now discover providers at runtime via /api/sso/providers (served by apps/gateway/src/auth/sso.controller.ts).
• React Router pages — real spa/pages/{login,register,sso-callback}.tsx behind session guards (spa/guards.tsx: GuestGuard → /chat, AuthGuard → /login).
• Same-origin callback hardening — lib/auth-redirect.ts::resolveAuthCallbackURL constrains post-login redirects to same-origin relative paths.
• Contract tests — api, auth-client, socket, auth-redirect, guards, routes, sso-callback specs. Vitest env aligned to Node-native AbortController/AbortSignal (test/setup.ts) so react-router client navigation tests run under node ≥24 undici.

Verification evidence (integrator independently re-ran — not author self-report)

• pnpm test → Test Files 10 passed (10) / Tests 33 passed (33), 0 failed / 0 errors (node v22; author-confirmed identical under node v26 — brackets CI's node 24).
• pnpm typecheck → 0 · pnpm lint → 0 · pnpm build:vite → ✓ · Next pnpm build → ✓ · root pnpm format:check → clean.

Independent reviews (author ≠ reviewer; DO-178B/C-inspired)

• Code review: FINDINGS — 5 × minor, all dispositioned (deferred/accepted); no blockers. See CODE-REVIEW DISPOSITION comment.
• Security review: found a [blocker] open redirect (resolveAuthCallbackURL returned protocol-relative //evil.com for /..//evil.com) → remediated (commit 90cf286a, structural reject of leading // + 5 regression cases) → independent re-review: BLOCKER CLOSED (47-vector adversarial re-probe, zero off-origin returns). §2.4 same-origin invariant holds; auth/session/secrets clean. See SECURITY-REVIEW DISPOSITION comment.

Same-origin invariant preserved. No absolute origin in apps/web/src. No secrets. Scope: apps/web/** (+ mission scratchpad doc).

🤖 Generated with Claude Code

## P2 — Web SPA data layer + same-origin auth (Phase P RFC increment) Ports the web auth surface to the same-origin Vite/React-Router SPA architecture from the approved Phase P RFC. Second increment after P1 (Vite skeleton, merged). ### What changed (`apps/web`) - **Relative-path data layer** — `lib/api.ts` uses relative `/api/...`; `lib/auth-client.ts` omits absolute `baseURL`; `lib/socket.ts` uses `io('/chat')`. No gateway origin anywhere in `src` (§2.4 same-origin hard invariant). - **Runtime SSO discovery** — deleted the static `lib/sso-providers.ts` (build-time `NEXT_PUBLIC` list); both SPA and Next baseline now discover providers at runtime via `/api/sso/providers` (served by `apps/gateway/src/auth/sso.controller.ts`). - **React Router pages** — real `spa/pages/{login,register,sso-callback}.tsx` behind session guards (`spa/guards.tsx`: GuestGuard → `/chat`, AuthGuard → `/login`). - **Same-origin callback hardening** — `lib/auth-redirect.ts::resolveAuthCallbackURL` constrains post-login redirects to same-origin relative paths. - **Contract tests** — `api`, `auth-client`, `socket`, `auth-redirect`, `guards`, `routes`, `sso-callback` specs. Vitest env aligned to Node-native `AbortController`/`AbortSignal` (`test/setup.ts`) so react-router client navigation tests run under node ≥24 undici. ### Verification evidence (integrator independently re-ran — not author self-report) - `pnpm test` → **Test Files 10 passed (10) / Tests 33 passed (33)**, 0 failed / 0 errors (node v22; author-confirmed identical under node v26 — brackets CI's node 24). - `pnpm typecheck` → 0 · `pnpm lint` → 0 · `pnpm build:vite` → ✓ · Next `pnpm build` → ✓ · root `pnpm format:check` → clean. ### Independent reviews (author ≠ reviewer; DO-178B/C-inspired) - **Code review:** FINDINGS — 5 × minor, all dispositioned (deferred/accepted); no blockers. See CODE-REVIEW DISPOSITION comment. - **Security review:** found a **[blocker] open redirect** (`resolveAuthCallbackURL` returned protocol-relative `//evil.com` for `/..//evil.com`) → remediated (commit `90cf286a`, structural reject of leading `//` + 5 regression cases) → **independent re-review: BLOCKER CLOSED** (47-vector adversarial re-probe, zero off-origin returns). §2.4 same-origin invariant holds; auth/session/secrets clean. See SECURITY-REVIEW DISPOSITION comment. Same-origin invariant preserved. No absolute origin in `apps/web/src`. No secrets. Scope: `apps/web/**` (+ mission scratchpad doc). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

shaggy added 5 commits 2026-08-10 01:51:06 +00:00

feat(web): add same-origin SPA authentication 46d68e1ff4

docs(scratchpad): record WebUI P2 verification a4861c221f

test(web): align jsdom abort signals with Node e16c08aa9f

docs(scratchpad): record P2 remediation evidence 0aef432052

fix(web): reject protocol-relative auth callbacks 90cf286a09

shaggy commented 2026-08-10 01:51:15 +00:00

Author

Contributor

Copy Link

Copy Source

CODE-REVIEW DISPOSITION (P2 — data layer + auth) — independent reviewer (author ≠ reviewer), verdict FINDINGS (5 × minor, none blocking), all dispositioned. Gates independently re-run by the integrator (not author self-report).

Independent gate re-run (integrator, box worktree stack-p1/apps/web):

• pnpm test → Test Files 10 passed (10) / Tests 28 passed (28) under node v22.23.2; author-confirmed identical 28/28 under node v26.4.0. These bracket CI's canonical node 24 (.woodpecker/ci.yml = node:24-alpine). The env fix (test/setup.ts) aligns AbortController/AbortSignal to the Node-native realm via builtin node:util — version-agnostic, no dependency added.
• pnpm typecheck → exit 0 · pnpm lint → exit 0 · pnpm build:vite → exit 0 · Next pnpm build → exit 0 · root pnpm format:check → clean.
• Diff scope: apps/web/** (+ one scratchpad doc), merge-base 068d0f9b (merged P1). No scope creep.

Integrity verified (the prior false-PASS risk): guards.spec.tsx redirect tests assert the target by BOTH rendered content and router.state.location.pathname (authed→/chat, unauthed→/login); the e16c08aa env fix is test-setup-only and weakened/skipped no assertion. Deleted sso-providers.ts has zero dangling source references (only stale .vite/.turbo caches); runtime /api/sso/providers discovery is wired to the real backend controller and shared uniformly by SPA + Next baseline.

#	Finding (all [minor])	Disposition
1	login.tsx/register.tsx submit handlers have no component tests	DEFERRED (tracked follow-up) — guards, routes, and the lib layer are well covered; the untested logic is submit/error UX, not a security or data-integrity path.
2	sso-callback.spec.tsx covers only the control-char security path (unknown-provider / !configured / non-oidc / happy-path unexercised)	DEFERRED (tracked follow-up) — the security-critical rejection path IS tested; remaining branches are non-security.
3	GuestGuard has no isPending gate → brief flash-of-login for already-authed users	ACCEPTED (rationale) — deliberate and documented by test guards.spec.tsx:616-623; UX nit, not a bug.
4	redundant configured double-filter (login.tsx + sso-provider-buttons.tsx)	ACCEPTED (rationale) — harmless duplication; cosmetic.
5	api.ts:38 throws Error with empty message when a non-OK JSON body lacks message	DEFERRED (tracked follow-up) — rare edge (only when server returns valid JSON error without a message field); statusCode is still attached. One-line fix (errorBody.message ?? res.statusText) queued for the next apps/web increment.

No blockers. Merge-eligible on the code lens. (Deferred minors #1/#2/#5 to be picked up in a follow-up apps/web increment; none affect alpha testability.)

**CODE-REVIEW DISPOSITION (P2 — data layer + auth)** — independent reviewer (author ≠ reviewer), verdict **FINDINGS (5 × minor, none blocking), all dispositioned**. Gates independently re-run by the integrator (not author self-report). **Independent gate re-run (integrator, box worktree `stack-p1/apps/web`):** - `pnpm test` → `Test Files 10 passed (10)` / `Tests 28 passed (28)` under **node v22.23.2**; author-confirmed identical 28/28 under **node v26.4.0**. These bracket CI's canonical **node 24** (`.woodpecker/ci.yml` = `node:24-alpine`). The env fix (`test/setup.ts`) aligns AbortController/AbortSignal to the Node-native realm via builtin `node:util` — version-agnostic, no dependency added. - `pnpm typecheck` → exit 0 · `pnpm lint` → exit 0 · `pnpm build:vite` → exit 0 · Next `pnpm build` → exit 0 · root `pnpm format:check` → clean. - Diff scope: `apps/web/**` (+ one scratchpad doc), merge-base `068d0f9b` (merged P1). No scope creep. **Integrity verified (the prior false-PASS risk):** `guards.spec.tsx` redirect tests assert the target by BOTH rendered content and `router.state.location.pathname` (authed→/chat, unauthed→/login); the `e16c08aa` env fix is test-setup-only and weakened/skipped no assertion. Deleted `sso-providers.ts` has zero dangling source references (only stale `.vite`/`.turbo` caches); runtime `/api/sso/providers` discovery is wired to the real backend controller and shared uniformly by SPA + Next baseline. | # | Finding (all [minor]) | Disposition | |---|---|---| | 1 | `login.tsx`/`register.tsx` submit handlers have no component tests | **DEFERRED (tracked follow-up)** — guards, routes, and the lib layer are well covered; the untested logic is submit/error UX, not a security or data-integrity path. | | 2 | `sso-callback.spec.tsx` covers only the control-char security path (unknown-provider / !configured / non-oidc / happy-path unexercised) | **DEFERRED (tracked follow-up)** — the security-critical rejection path IS tested; remaining branches are non-security. | | 3 | `GuestGuard` has no `isPending` gate → brief flash-of-login for already-authed users | **ACCEPTED (rationale)** — deliberate and documented by test `guards.spec.tsx:616-623`; UX nit, not a bug. | | 4 | redundant `configured` double-filter (`login.tsx` + `sso-provider-buttons.tsx`) | **ACCEPTED (rationale)** — harmless duplication; cosmetic. | | 5 | `api.ts:38` throws `Error` with empty message when a non-OK JSON body lacks `message` | **DEFERRED (tracked follow-up)** — rare edge (only when server returns valid JSON error without a `message` field); `statusCode` is still attached. One-line fix (`errorBody.message ?? res.statusText`) queued for the next apps/web increment. | No blockers. Merge-eligible on the code lens. (Deferred minors #1/#2/#5 to be picked up in a follow-up apps/web increment; none affect alpha testability.)

shaggy commented 2026-08-10 01:51:15 +00:00

Author

Contributor

Copy Link

Copy Source

SECURITY-REVIEW DISPOSITION (P2 — data layer + auth) — independent reviewer (author ≠ reviewer), verdict APPROVE (blocker found → remediated → re-reviewed CLOSED). The remediation was independently gate-verified AND empirically bypass-probed by the integrator, not accepted on author self-report.

[BLOCKER] open redirect in resolveAuthCallbackURL (apps/web/src/lib/auth-redirect.ts) — FIXED & re-review CLOSED.

• Finding: the function validated resolved.origin === expectedOrigin but returned resolved.pathname; input /..//evil.com keeps origin same-origin yet yields pathname //evil.com (protocol-relative) → a browser resolves it to https://evil.com/, and it flowed into signIn.oauth2({ callbackURL }) on both SSO callback routes. The author's original test table missed the /..// path-traversal class.
• Integrator confirmed the bypass empirically before routing remediation: new URL('/..//evil.com','https://app.local').pathname === '//evil.com'.
• Fix (commit 90cf286a): added || resolved.pathname.startsWith('//') to the reject condition → structural closure of the whole class, plus 5 additive it.each regression cases (/..//evil.com, /..//evil.com/x, /./..//evil.com, /../..//evil.com, /foo/..//evil.com).
• Integrator verification: 12-vector empirical probe against the exact patched logic → no off-origin output (traversal, encoded %2e%2e, backslash forms all return /chat; legit paths pass). Independent security re-review (the reviewer that raised the finding): 47-vector adversarial re-probe → zero off-origin returns, structurally complete (pathname is the only leading component, WHATWG normalizes \→/ for special schemes, //-prefixed pathnames rejected), scope clean (only auth-redirect.ts + spec), non-weakening (additive tests only). SECURITY RE-REVIEW: BLOCKER CLOSED.

Priority 1 — §2.4 SAME-ORIGIN HARD INVARIANT: HOLDS. No gateway origin anywhere under apps/web/src/**: api.ts uses relative /api/..., auth-client.ts omits baseURL, socket.ts uses io('/chat'), SSO discovery hits runtime /api/sso/providers. The only :14242 literal is the dev-only Vite proxy target in vite.config.ts (build config, permitted). All other grep hits are test fixtures / font CDN / SVG namespace / placeholder.

Priority 3 — auth/session/secrets: PASSES. Cookie-based same-origin sessions, no secrets/credentials added, no console.* leakage, no sensitive error exposure, guards behave correctly.

Two pre-existing [minor] notes, both OUT of the P2 range and benign, no action here: Google Fonts CDN preconnect in layout.tsx; SAML fallback off-origin link in sso-provider-buttons.tsx.

No open blockers. Merge-eligible on the security lens.

**SECURITY-REVIEW DISPOSITION (P2 — data layer + auth)** — independent reviewer (author ≠ reviewer), verdict **APPROVE (blocker found → remediated → re-reviewed CLOSED)**. The remediation was independently gate-verified AND empirically bypass-probed by the integrator, not accepted on author self-report. **[BLOCKER] open redirect in `resolveAuthCallbackURL` (`apps/web/src/lib/auth-redirect.ts`) — FIXED & re-review CLOSED.** - Finding: the function validated `resolved.origin === expectedOrigin` but returned `resolved.pathname`; input `/..//evil.com` keeps origin same-origin yet yields pathname `//evil.com` (protocol-relative) → a browser resolves it to `https://evil.com/`, and it flowed into `signIn.oauth2({ callbackURL })` on both SSO callback routes. The author's original test table missed the `/..//` path-traversal class. - Integrator confirmed the bypass empirically before routing remediation: `new URL('/..//evil.com','https://app.local').pathname === '//evil.com'`. - Fix (commit `90cf286a`): added `|| resolved.pathname.startsWith('//')` to the reject condition → structural closure of the whole class, plus 5 additive `it.each` regression cases (`/..//evil.com`, `/..//evil.com/x`, `/./..//evil.com`, `/../..//evil.com`, `/foo/..//evil.com`). - Integrator verification: 12-vector empirical probe against the exact patched logic → no off-origin output (traversal, encoded `%2e%2e`, backslash forms all return `/chat`; legit paths pass). Independent security **re-review** (the reviewer that raised the finding): 47-vector adversarial re-probe → **zero off-origin returns**, structurally complete (pathname is the only leading component, WHATWG normalizes `\`→`/` for special schemes, `//`-prefixed pathnames rejected), scope clean (only `auth-redirect.ts` + spec), non-weakening (additive tests only). `SECURITY RE-REVIEW: BLOCKER CLOSED`. **Priority 1 — §2.4 SAME-ORIGIN HARD INVARIANT: HOLDS.** No gateway origin anywhere under `apps/web/src/**`: `api.ts` uses relative `/api/...`, `auth-client.ts` omits `baseURL`, `socket.ts` uses `io('/chat')`, SSO discovery hits runtime `/api/sso/providers`. The only `:14242` literal is the dev-only Vite proxy target in `vite.config.ts` (build config, permitted). All other grep hits are test fixtures / font CDN / SVG namespace / placeholder. **Priority 3 — auth/session/secrets: PASSES.** Cookie-based same-origin sessions, no secrets/credentials added, no `console.*` leakage, no sensitive error exposure, guards behave correctly. Two pre-existing **[minor]** notes, both OUT of the P2 range and benign, no action here: Google Fonts CDN preconnect in `layout.tsx`; SAML fallback off-origin link in `sso-provider-buttons.tsx`. No open blockers. Merge-eligible on the security lens.

shaggy merged commit 87daa12976 into next 2026-08-10 01:52:02 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Defect in framework tooling or delivered code

fleet-enhancement

Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

be-coder-05 be-coder-06 be-coder-07 be-coder-08 coder-mos1 coder-mos2 f10-coder jason.woltje (Jason Woltje) merge-gate mos-dt-0 pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1)) rev-security-02 scooby (Scooby) velma

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mosaicstack/stack#1144