feat(admin): Scaffold Vite/Svelte admin SPA with oAuth - #240
Conversation
Site previewPreview: https://0d9c90f3-site.fullsend-ai.workers.dev Commit: |
ifireball
changed the title
7a333b4 to
c0242ff
Compare
c0242ff to
faf10b7
Compare
faf10b7 to
561c200
Compare
ifireball
changed the title
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review: 3 High-Severity Issues
Reviewed the full diff across all 33 changed files (excluding package-lock.json). The BFF pattern, PKCE implementation, and overall scaffold are architecturally sound — nice work. However, there are three high-severity issues that should be addressed before merge.
High 1: Worker OAuth BFF reachable in production with client_secret in env
File: cloudflare_site/worker/src/index.ts
The /api/oauth/token and /api/github/user routes are always active — there is no env flag to disable them. The origin check (isLocalhostDevOrigin) only allows localhost, which means:
- Legitimate production browser requests will get 403 (the admin SPA won't work on deployed previews or production).
- Non-browser clients (
curl) can bypass the Origin check entirely. Worse,getEffectiveLoopbackOrigin()falls back to theRefererheader whenOriginis absent —Refereris trivially forgeable:This passes the origin check and reachescurl -H "Referer: http://localhost:5173/" \ https://prod-worker.example/api/oauth/token -d '...'
handleOAuthToken, which hasGITHUB_APP_CLIENT_SECRETavailable in env.
Recommendation: Either:
- Gate API routes behind an env var (
ADMIN_OAUTH_ENABLED=1) so they're explicitly dead in production until configured, or - Expand the origin allowlist to include real production/preview origins and remove the
Refererfallback —Referershould never be used for origin validation in production. - Add rate limiting on the token exchange endpoint.
High 2: Token expiry stored but never checked — dead code creating false confidence
Files: web/admin/src/lib/auth/tokenStore.ts, session.ts, oauth.ts
expiresAt is saved in localStorage via saveToken() but loadToken() returns the token regardless of whether it's expired. No consumer checks expiresAt anywhere in the codebase — the field is dead code.
Additionally, expiresAt: 0 is used as a sentinel for "GitHub didn't tell us when it expires" (oauth.ts), but 0 is a valid epoch timestamp. This creates ambiguity if an expiry check is added later.
Recommendation:
- Add expiry check in
loadToken():if (t.expiresAt && t.expiresAt > 0 && Date.now() > t.expiresAt) { clearSession(); return null; }
- Change type to
expiresAt: number | nulland usenullinstead of0for unknown expiry.
High 3: Minimal test coverage for security-critical OAuth code paths
Files: Only web/admin/src/lib/auth/pkce.test.ts exists (2 test cases)
The following security-critical paths have zero test coverage:
| Module | Untested functions |
|---|---|
oauth.ts |
consumeOAuthParamsFromDocumentUrl(), completeGithubOAuthFromHandoff(), startGithubSignIn() — contains the state validation logic where a bypass exists when sessionStorage is cleared |
tokenStore.ts |
saveToken/loadToken/clearSession round-trip (the plan document has these tests written out but they weren't implemented) |
session.ts |
refreshSession(), signOut() |
user.ts |
fetchGitHubUser() |
Worker index.ts |
Origin validation, CORS, token exchange handler — no tests at all |
The existing pkce.test.ts also doesn't validate against the RFC 7636 Appendix B known test vector, which would prove correctness rather than just stability.
Recommendation: At minimum, add tests for oauth.ts state validation and tokenStore.ts round-trip before merge. Worker tests can follow up but should be tracked.
…High 3 Add RFC 7636 Appendix B PKCE vector, completeGithubOAuthFromHandoff state and token-exchange scenarios with mocked Turnstile, session, and fetch, session refresh/sign-out tests with mocked GitHub user fetch, fetchGitHubUser BFF tests, and invalid JSON handling for tokenStore. Made-with: Cursor
|
@waynesun09 can you please re-review or drop the req0changes flag? |
Declare the runtime versions used for local admin SPA and Go CLI work without adding JavaScript packaging yet. Made-with: Cursor
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review: #240
Head SHA: fc70542bc15ec5390847eaffcc5c264826f6bee4
Method: 4 independent review agents (security, quality, cursor, gemini) — consolidated
Outcome: comment-only
Previous High-Severity Issues: All 3 Resolved
| Issue | Status | How |
|---|---|---|
| Worker OAuth BFF / Referer bypass | Resolved | getBrowserOrigin() reads only Origin; fetchTabBindingOk requires it for token exchange; Turnstile env gate returns 503 if keys missing; rate limiting added |
| Token expiry stored but never checked | Resolved | loadToken() validates expiry + auto-clears; parseExpiresAt normalizes 0 → null; tested |
| Minimal test coverage for OAuth paths | Resolved | 12 test files added covering Worker endpoints, CORS, OAuth flow, token store, session, PKCE (RFC 7636 vectors), preview handoff |
New Findings
| Severity | Count | Summary |
|---|---|---|
| High | 1 | Dead Octokit client module + unused event listener (see inline) |
| Medium | 2 | Broad HTTPS redirect_uri allowlist (4/4 agents flagged); no Turnstile timeout |
| Low | 1 | No-op test for dead module |
Overall
Strong security hardening since the prior review. OAuth BFF has proper layered defenses: Origin-only validation, Turnstile, rate limiting, PKCE, tab-binding, and field stripping on /user proxy. The high finding is dead code cleanup, not a security issue. Medium findings are documented design trade-offs worth tracking.
There was a problem hiding this comment.
High: Dead code — createUserOctokit is never imported or used
Nothing in the SPA imports createUserOctokit. The actual GitHub user fetch goes through fetchGitHubUser() in user.ts (plain fetch via same-origin BFF), called by session.ts:refreshSession().
The 401 auto-signout this module enables via window.dispatchEvent(new CustomEvent("fullsend:github-unauthorized")) is also dead — App.svelte listens for this event, but 401 handling actually works through session.ts catching GitHubUserRequestError with status 401 directly.
The test in client.test.ts only asserts expect(o).toBeDefined() — it doesn't verify the auth header or the 401 hook, so it provides no value.
Recommendation: Either remove client.ts, client.test.ts, and the event listener in App.svelte:28, or if this is intentional scaffolding for future endpoints, add a code comment explaining that and exclude it from the bundle until needed.
| window.addEventListener("fullsend:github-unauthorized", onGithub401); | ||
|
|
||
| void (async () => { | ||
| try { |
There was a problem hiding this comment.
High: Dead event listener — fullsend:github-unauthorized is never dispatched
This listener calls signOut() on the fullsend:github-unauthorized event, but the only code that dispatches this event is createUserOctokit in client.ts, which is never imported or instantiated anywhere in the SPA.
The 401→signOut path actually works through session.ts:refreshSession() catching GitHubUserRequestError with status === 401 — this listener is redundant dead code.
Consider removing this listener along with the cleanup of client.ts.
| u.hostname === "localhost" || | ||
| u.hostname === "127.0.0.1" || | ||
| u.hostname === "[::1]" | ||
| ); |
There was a problem hiding this comment.
Medium: isAllowedOAuthRedirectUri accepts any HTTPS host at /admin* paths
This allows https://evil.com/admin/ to pass validation. The defense-in-depth is documented and sound (GitHub App callback registration + PKCE + tab-binding + Turnstile + rate limits), but it means the Worker will process OAuth flows for any HTTPS host.
All 4 independent reviewers flagged this as the most notable design trade-off.
Suggestion: Consider adding an env-driven ALLOWED_REDIRECT_HOSTS allowlist for production deployments, so this broad acceptance is limited to dev/preview environments where preview URLs are not enumerable.
There was a problem hiding this comment.
Medium: No timeout on Turnstile token acquisition
obtainTurnstileToken() creates a promise that resolves on callback or rejects on error-callback, but if the Turnstile widget hangs (network issue, CDN latency, script loaded but no callback), the user sees an indefinite loading state with no error. The hidden DOM element also leaks if the promise never settles.
Suggestion: Add a setTimeout that rejects with a user-friendly error after ~15 seconds (e.g., "Turnstile verification timed out — please try again").
There was a problem hiding this comment.
Low: No-op test — asserts only that a constructor returns a value
The test name says "sets auth header from token" but only checks expect(o).toBeDefined(). It doesn't verify the auth header is set or that the 401 hook dispatches the custom event. Since the module under test (client.ts) appears to be dead code (see file-level comment there), this test provides false confidence.
If client.ts is kept, this test should verify the auth header and 401 event dispatch. If client.ts is removed, delete this test too.
- turnstile: align with feat/admin-spa-org-list (AbortSignal, teardown on abort) plus 120s acquisition deadline (future branch had signal only, no wall-clock cap) - oauth: optional AbortSignal + readJsonBodyWithSignal; SIGNING_IN_CANCELLED_MESSAGE; clearSession if aborted after token save (org-list parity) - App: AbortController for OAuth boot; suppress error UI on cancel message - github/client: Octokit ignores request.hook in constructor — use hook.wrap so fullsend:github-unauthorized fires on 401 (response or thrown RequestError) - Tests: turnstile timeout + abort; oauth abort during JSON read; client 401 dispatch Made-with: Cursor
|
Pushed `ef495068` addressing the latest consolidated review: Turnstile timeout — `feat/admin-spa-org-list` already had `AbortSignal` plumbing for `obtainTurnstileToken` + OAuth handoff; there was still no wall-clock cap. This branch merges that signal behavior and adds a 120s deadline so a stuck widget cannot block OAuth boot forever (`turnstile.ts` + `turnstile.test.ts`). OAuth boot — `App.svelte` now uses an `AbortController` (same pattern as org-list) and passes `signal` into `completeGithubOAuthFromHandoff`; `oauth.ts` gains `readJsonBodyWithSignal`, cancel handling, and `SIGNING_IN_CANCELLED_MESSAGE` for unmount. Octokit / “dead module” — `createUserOctokit` is kept: `feat/admin-spa-org-list` uses it from `fetchOrgs.ts`. The real bug was that `new Octokit({ request: { hook } })` never ran our hook (`@octokit/core` overwrites `request.hook`). The hook is now registered with `octokit.hook.wrap("request", …)`, so 401 dispatches `fullsend:github-unauthorized` (response or thrown error). `client.test.ts` asserts that with a mocked `fetch`. |
Review: #240Head SHA: 3c95c5b SummaryThis PR scaffolds a Vite/Svelte admin SPA with GitHub OAuth via a Cloudflare Worker BFF. The implementation is well-structured: PKCE S256 is correctly implemented per RFC 7636, the OAuth state parameter provides CSRF protection, Turnstile bot protection is wired in, and the Worker strips GitHub FindingsMedium
Low
Info
FooterOutcome: comment-only Previous runReview: #240Head SHA: ef49506 SummaryThis PR scaffolds a Vite + Svelte admin SPA with a Cloudflare Worker OAuth BFF (Backend-for-Frontend), including PKCE, Turnstile verification, CORS hardening, and rate limiting. The architecture is sound: FindingsMedium
Low
Info
FooterOutcome: approve
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Verified review: 2 Medium + 1 Low confirmed out of 8 High/Medium findings from 4-agent review
Ran 4 independent review agents (security, architecture, code quality, CI/infra) which surfaced 3 High and 5 Medium findings. Verified each against source code — most were either edge cases, documented design decisions, or not applicable to the current scope.
Confirmed:
- Medium:
refreshSessionsilently swallows non-401 errors — user can't distinguish "not authenticated" from "network down" - Medium (design): Token in
localStoragegives persistent XSS exposure —sessionStoragewould limit blast radius for an admin SPA - Low: Unreachable
response.status === 401check on Octokit success path (dead code)
Dismissed after verification:
Empty OAuth code—takeDocHandoff()catches it, GitHub never sends empty codemergeAbortSignalslistener leakisAllowedOAuthRedirectUriany HTTPS hostNo proactive token refresh— no ongoing API calls in current scopeTurnstile site key in OAuth state— site keys are public by design
| if (e instanceof GitHubUserRequestError && e.status === 401) { | ||
| signOut(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Medium: refreshSession silently swallows non-401 errors
When fetchGitHubUser throws a non-401 error (network failure, 500, timeout), this catch block sets githubUser to null without surfacing the error. The user sees a logged-out state with no indication that a transient network issue occurred — indistinguishable from "not authenticated."
Suggestion: Expose an error store (e.g. export const sessionError = writable<string | null>(null)) or return a result type so the UI can distinguish auth failures from network errors and show an appropriate message like "Could not reach server — retrying."
| expiresAt: number | null; | ||
| }; | ||
|
|
||
| const KEY = "fullsend_admin_github_token"; |
There was a problem hiding this comment.
Medium (design): GitHub token stored in localStorage — persistent XSS exposure
localStorage persists across tabs and browser restarts, so any XSS on this origin gives an attacker persistent access to the GitHub token. Since this is an admin SPA (elevated privilege surface), consider using sessionStorage instead — it limits exposure to the current tab and is cleared when the tab closes. The trade-off is that users would need to re-authenticate in each new tab.
Alternatively, if cross-tab auth is needed later, an HttpOnly cookie set by the Worker BFF would prevent client-side JS access entirely.
| octokit.hook.wrap("request", async (request, options) => { | ||
| try { | ||
| const response = await request(options as never); | ||
| if (response.status === 401) { |
There was a problem hiding this comment.
Low: Unreachable 401 check on success path
@octokit/request throws RequestError for non-2xx responses, so request(options) never resolves with status === 401 — this branch is dead code. The 401 dispatch is correctly handled in the catch block below (and verified by the test).
Consider removing this check to avoid misleading future readers into thinking Octokit can resolve with 4xx status codes.
| if (response.status === 401) { | |
| const response = await request(options as never); | |
| return response as OctokitResponse<unknown>; |
Octokit rejects non-2xx before returning; 401 is handled in the catch path. Confirmed in waynesun09 verified review (2026-04-29). Made-with: Cursor
|
Round-up for waynesun09’s latest verified review (2026-04-29) and follow-ups: Confirmed items from that review
|
Follow-up for reviewers: loose ends and tracked issuesThis comment ties review threads to tracked follow-ups under the delivery umbrella #509 (sub-issues). Use it when reply-all on review comments so each thread gets a pointer to the right issue. Sub-issues of #509 (this PR + follow-ups)
All of the above are now sub-issues of #509 for Story #124 indexing. Loose ends not filed as issues (by design)
Suggested one-liners when replying on reviews
If anything here should be in-scope for this PR instead of a follow-up, say which issue to close and we will fold the change into #240. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Re-review at 3c95c5bd — all previous issues resolved, LGTM
Ran 4 independent review agents (security, quality, cursor, gemini). All 7 previously-flagged issues are confirmed resolved with no regressions:
- Original 3 highs (Referer bypass, token expiry dead code, minimal tests)
- Dead Octokit client → fixed via
hook.wrap, test verifies 401 dispatch - No Turnstile timeout → 120s deadline via AbortSignal
- No-op test → now mocks fetch + asserts event
- Unreachable 401 check → removed
One new finding: missing CSP headers (see inline comment on index.ts). Not blocking for this scaffold PR — recommend tracking under #509 for production hardening.
Existing tracked follow-ups (#505, #506, #541, #542, #543-546) are confirmed still present and properly tracked with issues. No action needed in this PR.
There was a problem hiding this comment.
Medium: No Content-Security-Policy headers on any response
4 independent review agents flagged this as the strongest new finding at this SHA. No CSP is set on Worker API responses or static asset serving. For an admin SPA that stores OAuth tokens in localStorage and dynamically loads Turnstile from challenges.cloudflare.com, the absence of CSP means any XSS on the origin can exfiltrate the token without restriction.
Recommended minimum CSP (via Worker response headers or Cloudflare _headers file):
default-src 'self';
script-src 'self' https://challenges.cloudflare.com;
connect-src 'self';
style-src 'self' 'unsafe-inline';
img-src 'self' https://avatars.githubusercontent.com;
frame-src https://challenges.cloudflare.com;
frame-ancestors 'none';
base-uri 'self';
form-action 'self'
This is the primary mitigation for the localStorage token storage trade-off (#541) and should be addressed before production deployment. Not blocking for this scaffold PR, but worth tracking alongside the existing follow-ups under #509.
CSP follow-up (review thread)@waynesun09 raised Medium — no Tracked under the Story #124 / #509 umbrella as #585 — Site / admin SPA: Content-Security-Policy headers for Worker and static responses. That issue links #541 (token-at-rest strategy); CSP is the main companion hardening called out in review if we keep tokens in the browser. No CSP change on #240 itself—implementation belongs on #585 once we lock real |
… merge) Made-with: Cursor
Made-with: Cursor
3 participants
Summary
scaffolds a minimal admin front-end (Vite + OAuth callback wiring for local dev only). There is no real admin functionality yet—no production-ready screens, workflows, or server-backed admin features beyond what is needed to exercise OAuth locally.
Note: PR is huge because of the packages.lock file which nobody really needs to read...
What changed
How to verify
Follow web/admin/README.md for local OAuth; run npm ci at repo root and then npm run dev to launch the site.