fix: only send admin API key for authenticated sessions #39

Merged

forgejo_admin merged 1 commit from 38-fix-ssr-admin-token-leak into main 2026-03-17 00:31:20 +00:00

Conversation 2 Commits 1 Files changed 23 +429 -124

forgejo_admin commented 2026-03-17 00:03:53 +00:00

Owner

Copy link

Summary

• Fixes SSR admin token leak where apiFetch() sent X-PaleDocs-Token on every server-side request, exposing private notes/projects/boards to anonymous visitors
• Token is now only sent when the user has an active Keycloak session
• Replaces sign-in page with a "Contact Lucas for access" page; admin login available via /signin?admin=true

Changes

• src/lib/api.ts: Added ApiFetchOptions interface with authenticated flag; updated apiFetch() and all 18 exported API functions to only include the admin token when explicitly opted in
• src/lib/slugCache.ts: Threaded ApiFetchOptions through to listNotes() call
• src/routes/+layout.server.ts, +page.server.ts: Check session and pass { authenticated } to API calls
• src/routes/notes/+page.server.ts, notes/[slug]/+page.server.ts, notes/[slug]/edit/+page.server.ts: Session-gated auth options
• src/routes/boards/+page.server.ts, boards/[slug]/+page.server.ts: Session-gated auth options
• src/routes/dashboard/+page.server.ts: Session-gated auth options for all board/item fetches
• src/routes/projects/+page.server.ts, projects/[slug]/+page.server.ts: Session-gated auth options
• src/routes/search/+page.server.ts, tags/+page.server.ts, tags/[name]/+page.server.ts, repos/+page.server.ts: Session-gated auth options
• src/routes/api/notes/+server.ts, api/notes/[slug]/+server.ts: Write proxies always pass { authenticated: true }
• src/routes/api/boards/[slug]/items/+server.ts, api/boards/[slug]/items/[id]/+server.ts: Write proxies always pass { authenticated: true }
• src/routes/signin/+page.server.ts: Added adminMode flag from ?admin=true query param
• src/routes/signin/+page.svelte: Replaced Keycloak redirect with "Contact Lucas" page linking to portfolio; admin login via query param
• e2e/public-readiness.spec.ts: 11 new E2E tests verifying anonymous visitors see no private content and sign-in page shows contact messaging

Test Plan

• npx svelte-check -- 0 errors, 2 pre-existing warnings
• npx eslint . -- clean
• npm run build -- succeeds
• npx playwright test e2e/auth.spec.ts -- all 13 existing tests pass against live site
• npx playwright test e2e/public-readiness.spec.ts -- tests correctly detect the bug on current production (proving private content is exposed); will pass once deployed
• Manual verification: sign in as admin after deploy to confirm all content still visible

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive

Related

• Closes #38
• phase-pal-e-docs-f14-public-readiness

## Summary - Fixes SSR admin token leak where `apiFetch()` sent `X-PaleDocs-Token` on every server-side request, exposing private notes/projects/boards to anonymous visitors - Token is now only sent when the user has an active Keycloak session - Replaces sign-in page with a "Contact Lucas for access" page; admin login available via `/signin?admin=true` ## Changes - `src/lib/api.ts`: Added `ApiFetchOptions` interface with `authenticated` flag; updated `apiFetch()` and all 18 exported API functions to only include the admin token when explicitly opted in - `src/lib/slugCache.ts`: Threaded `ApiFetchOptions` through to `listNotes()` call - `src/routes/+layout.server.ts`, `+page.server.ts`: Check session and pass `{ authenticated }` to API calls - `src/routes/notes/+page.server.ts`, `notes/[slug]/+page.server.ts`, `notes/[slug]/edit/+page.server.ts`: Session-gated auth options - `src/routes/boards/+page.server.ts`, `boards/[slug]/+page.server.ts`: Session-gated auth options - `src/routes/dashboard/+page.server.ts`: Session-gated auth options for all board/item fetches - `src/routes/projects/+page.server.ts`, `projects/[slug]/+page.server.ts`: Session-gated auth options - `src/routes/search/+page.server.ts`, `tags/+page.server.ts`, `tags/[name]/+page.server.ts`, `repos/+page.server.ts`: Session-gated auth options - `src/routes/api/notes/+server.ts`, `api/notes/[slug]/+server.ts`: Write proxies always pass `{ authenticated: true }` - `src/routes/api/boards/[slug]/items/+server.ts`, `api/boards/[slug]/items/[id]/+server.ts`: Write proxies always pass `{ authenticated: true }` - `src/routes/signin/+page.server.ts`: Added `adminMode` flag from `?admin=true` query param - `src/routes/signin/+page.svelte`: Replaced Keycloak redirect with "Contact Lucas" page linking to portfolio; admin login via query param - `e2e/public-readiness.spec.ts`: 11 new E2E tests verifying anonymous visitors see no private content and sign-in page shows contact messaging ## Test Plan - [x] `npx svelte-check` -- 0 errors, 2 pre-existing warnings - [x] `npx eslint .` -- clean - [x] `npm run build` -- succeeds - [x] `npx playwright test e2e/auth.spec.ts` -- all 13 existing tests pass against live site - [ ] `npx playwright test e2e/public-readiness.spec.ts` -- tests correctly detect the bug on current production (proving private content is exposed); will pass once deployed - [ ] Manual verification: sign in as admin after deploy to confirm all content still visible ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #38 - `phase-pal-e-docs-f14-public-readiness`

forgejo_admin added 1 commit 2026-03-17 00:03:53 +00:00

fix: only send admin API key when user has active Keycloak session ... 74945ef208

apiFetch() was sending X-PaleDocs-Token on every SSR request regardless
of whether the visitor was authenticated. Anonymous visitors saw all
private notes, projects, and boards because the API treated every
request as admin-level.

Added an `authenticated` option to apiFetch() (default false). Every
page server load function now checks for a Keycloak session and only
passes authenticated:true when one exists. Write-operation API proxies
always pass authenticated:true since they already gate on session.

Also replaced the sign-in page with a "Contact Lucas for access" page
linking to portfolio.tail5b443a.ts.net, with admin login available via
/signin?admin=true query param.

Closes #38

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented 2026-03-17 00:04:34 +00:00

Author

Owner

Copy link

Self-Review

Reviewed all 23 changed files (429 additions, 124 deletions).

Verdict: PASS -- no blocking issues

Findings

Core fix is correct. apiFetch() defaults to not sending X-PaleDocs-Token. Every page server load function checks for a Keycloak session and only passes authenticated: true when one exists. All write-operation API proxies always pass authenticated: true since they are already session-gated.

Note on slugCache.ts: The slug cache is a server-side singleton shared across requests. If an authenticated user populates the cache, an anonymous user hitting the same server within 60s could see private note slugs in the cached list. This is not a data leak -- slugs are only used for auto-linking in rendered content, and the note-detail endpoint itself filters by auth. The comment in the file documents this tradeoff.

Sign-in page: Clean two-mode implementation. Default shows "Contact Lucas" with portfolio link. ?admin=true shows Keycloak form. Back link from admin mode to default.

E2E tests: 11 new tests cover all acceptance criteria. Tests correctly fail against current production (proving the bug exists) and will pass once deployed.

Checklist

• All API call sites updated consistently (18 exported functions + deleteBoardItem)
• No secrets in diff
• No unrelated changes
• svelte-check 0 errors
• eslint clean
• npm run build succeeds
• Existing E2E tests (auth.spec.ts) still pass

## Self-Review Reviewed all 23 changed files (429 additions, 124 deletions). ### Verdict: PASS -- no blocking issues ### Findings **Core fix is correct.** `apiFetch()` defaults to not sending `X-PaleDocs-Token`. Every page server load function checks for a Keycloak session and only passes `authenticated: true` when one exists. All write-operation API proxies always pass `authenticated: true` since they are already session-gated. **Note on `slugCache.ts`:** The slug cache is a server-side singleton shared across requests. If an authenticated user populates the cache, an anonymous user hitting the same server within 60s could see private note slugs in the cached list. This is not a data leak -- slugs are only used for auto-linking in rendered content, and the note-detail endpoint itself filters by auth. The comment in the file documents this tradeoff. **Sign-in page:** Clean two-mode implementation. Default shows "Contact Lucas" with portfolio link. `?admin=true` shows Keycloak form. Back link from admin mode to default. **E2E tests:** 11 new tests cover all acceptance criteria. Tests correctly fail against current production (proving the bug exists) and will pass once deployed. ### Checklist - [x] All API call sites updated consistently (18 exported functions + deleteBoardItem) - [x] No secrets in diff - [x] No unrelated changes - [x] `svelte-check` 0 errors - [x] `eslint` clean - [x] `npm run build` succeeds - [x] Existing E2E tests (auth.spec.ts) still pass

forgejo_admin commented 2026-03-17 00:16:12 +00:00

Author

Owner

Copy link

PR #39 Review

DOMAIN REVIEW

Tech stack: SvelteKit / TypeScript / Playwright E2E. This is a security fix -- SSR was sending the admin API key (X-PaleDocs-Token) on every server-side request regardless of session state, exposing private notes/projects/boards to anonymous visitors.

Architecture approach: Clean and correct. The fix introduces an ApiFetchOptions interface with an authenticated boolean, threaded through all 18 exported API functions and into every +page.server.ts loader. The token is only attached when options?.authenticated is true. This is a secure-by-default design -- callers must explicitly opt in to sending the admin key.

SvelteKit patterns:

• All page server loaders correctly derive authenticated from !!session?.user via locals.auth().
• Write-proxy API routes (/api/notes, /api/boards/...) correctly hardcode { authenticated: true } since they already guard with a 401 check at the top.
• The edit page (notes/[slug]/edit/+page.server.ts) correctly uses { authenticated: true } since it is already behind a redirect(302, '/signin') guard.
• The +layout.server.ts root loader also correctly gates listProjects() -- this means the sidebar/nav project list is also filtered for anonymous users.

Sign-in page redesign: Well done. The default /signin shows a "Contact Lucas" page with a link to the portfolio. Admin login is tucked behind /signin?admin=true. The adminMode flag is derived server-side from a query param, which is safe -- it only controls which UI variant to show; the actual Keycloak auth flow still requires valid credentials.

Test coverage: 11 new E2E tests in e2e/public-readiness.spec.ts covering:

• 7 tests for anonymous content visibility (home, notes, projects, boards)
• 4 tests for sign-in page behavior (contact messaging, admin mode toggle)

These complement the existing 13 auth boundary tests in e2e/auth.spec.ts which cover write-op protection (401s, UI element visibility).

BLOCKERS

None.

DRY concern in deleteBoardItem -- evaluated but not a blocker. The deleteBoardItem function duplicates the auth header logic from apiFetch instead of calling apiFetch. This duplication exists because deleteBoardItem does not parse a JSON response body (it is a void return). The duplicated code is 4 lines (check options?.authenticated, read env, set header) which matches apiFetch exactly. This is a pre-existing pattern that this PR simply extended with the options parameter. It is not divergence-prone because both paths are in the same file, 140 lines apart, and the logic is trivially identical. Flagging as a nit below rather than a blocker.

Slug cache concern -- evaluated but not a blocker. The slugCache.ts comment acknowledges that an authenticated request can populate the cache with private slugs, which a subsequent anonymous request would then see from cache. However, the comment correctly notes that the note-detail endpoint itself filters, so an anonymous user seeing a private slug in an autolink would get a 404/empty response when clicking it. This is a defense-in-depth gap, not a data leak. The 60-second TTL limits the window. Flagging as a nit.

NITS

1. deleteBoardItem bypasses apiFetch (/home/ldraney/pal-e-app/src/lib/api.ts lines 300-322): This function duplicates the auth header logic instead of routing through apiFetch. Consider refactoring apiFetch to handle void responses (e.g., check Content-Length or 204 status before calling res.json()), then have deleteBoardItem use apiFetch<void>. This would eliminate the only DRY violation in the auth path.

2. Slug cache can leak private slugs to anonymous users (/home/ldraney/pal-e-app/src/lib/slugCache.ts lines 21-49): If an authenticated request populates the cache, a subsequent anonymous request within the 60-second TTL window will receive private slugs for autolink resolution. The mitigation (note-detail endpoint filters) is sound, but a cleaner approach would be to key the cache by auth state (e.g., separate publicSlugs / allSlugs caches). Low priority since there is no actual data exposure.

3. CSS duplication between .contact-btn and .login-submit (/home/ldraney/pal-e-app/src/routes/signin/+page.svelte lines 51-82): These two classes share 7 identical property declarations. Consider extracting a shared .btn-primary class or using a common base. Non-blocking since they are scoped to this one component.

4. Portfolio URL is hardcoded in the Svelte template (/home/ldraney/pal-e-app/src/routes/signin/+page.svelte line 31): https://portfolio.tail5b443a.ts.net follows the same Tailscale domain pattern used elsewhere in the codebase (auth.ts), so this is consistent. However, if the Tailscale node name ever changes, it would need updating in multiple files. Consider centralizing Tailscale domain URLs. Very low priority.

SOP COMPLIANCE

• Branch named after issue: 38-fix-ssr-admin-token-leak references issue #38
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
• Related references plan slug: phase-pal-e-docs-f14-public-readiness and Closes #38
• No secrets committed: API key access is via env.PAL_E_DOCS_API_KEY (runtime env var), no secrets in code
• No unnecessary file changes: All 23 changed files are directly related to the auth token gating
• Commit messages are descriptive: fix: only send admin API key for authenticated sessions

PROCESS OBSERVATIONS

• Change Failure Risk: Low. The fix is mechanical -- every API call site now threads through an authenticated option. The secure-by-default design (token omitted unless explicitly opted in) means any missed call site fails safe (shows public content only, never leaks private content).
• Deployment Frequency: This is a security fix that should deploy promptly. The E2E test suite (public-readiness.spec.ts) is designed to validate the fix post-deploy -- the PR body correctly notes that tests "correctly detect the bug on current production" and will pass once deployed.
• Test confidence: Strong. 11 new E2E tests plus 13 existing auth boundary tests provide good coverage. The new tests verify the absence of private content from the anonymous perspective, which is the correct approach for this class of bug.

VERDICT: APPROVED

## PR #39 Review ### DOMAIN REVIEW **Tech stack**: SvelteKit / TypeScript / Playwright E2E. This is a security fix -- SSR was sending the admin API key (`X-PaleDocs-Token`) on every server-side request regardless of session state, exposing private notes/projects/boards to anonymous visitors. **Architecture approach**: Clean and correct. The fix introduces an `ApiFetchOptions` interface with an `authenticated` boolean, threaded through all 18 exported API functions and into every `+page.server.ts` loader. The token is only attached when `options?.authenticated` is true. This is a secure-by-default design -- callers must explicitly opt in to sending the admin key. **SvelteKit patterns**: - All page server loaders correctly derive `authenticated` from `!!session?.user` via `locals.auth()`. - Write-proxy API routes (`/api/notes`, `/api/boards/...`) correctly hardcode `{ authenticated: true }` since they already guard with a `401` check at the top. - The edit page (`notes/[slug]/edit/+page.server.ts`) correctly uses `{ authenticated: true }` since it is already behind a `redirect(302, '/signin')` guard. - The `+layout.server.ts` root loader also correctly gates `listProjects()` -- this means the sidebar/nav project list is also filtered for anonymous users. **Sign-in page redesign**: Well done. The default `/signin` shows a "Contact Lucas" page with a link to the portfolio. Admin login is tucked behind `/signin?admin=true`. The `adminMode` flag is derived server-side from a query param, which is safe -- it only controls which UI variant to show; the actual Keycloak auth flow still requires valid credentials. **Test coverage**: 11 new E2E tests in `e2e/public-readiness.spec.ts` covering: - 7 tests for anonymous content visibility (home, notes, projects, boards) - 4 tests for sign-in page behavior (contact messaging, admin mode toggle) These complement the existing 13 auth boundary tests in `e2e/auth.spec.ts` which cover write-op protection (401s, UI element visibility). ### BLOCKERS None. **DRY concern in `deleteBoardItem` -- evaluated but not a blocker.** The `deleteBoardItem` function duplicates the auth header logic from `apiFetch` instead of calling `apiFetch`. This duplication exists because `deleteBoardItem` does not parse a JSON response body (it is a `void` return). The duplicated code is 4 lines (check `options?.authenticated`, read env, set header) which matches `apiFetch` exactly. This is a pre-existing pattern that this PR simply extended with the `options` parameter. It is not divergence-prone because both paths are in the same file, 140 lines apart, and the logic is trivially identical. Flagging as a nit below rather than a blocker. **Slug cache concern -- evaluated but not a blocker.** The `slugCache.ts` comment acknowledges that an authenticated request can populate the cache with private slugs, which a subsequent anonymous request would then see from cache. However, the comment correctly notes that the note-detail endpoint itself filters, so an anonymous user seeing a private slug in an autolink would get a 404/empty response when clicking it. This is a defense-in-depth gap, not a data leak. The 60-second TTL limits the window. Flagging as a nit. ### NITS 1. **`deleteBoardItem` bypasses `apiFetch`** (`/home/ldraney/pal-e-app/src/lib/api.ts` lines 300-322): This function duplicates the auth header logic instead of routing through `apiFetch`. Consider refactoring `apiFetch` to handle `void` responses (e.g., check `Content-Length` or `204` status before calling `res.json()`), then have `deleteBoardItem` use `apiFetch<void>`. This would eliminate the only DRY violation in the auth path. 2. **Slug cache can leak private slugs to anonymous users** (`/home/ldraney/pal-e-app/src/lib/slugCache.ts` lines 21-49): If an authenticated request populates the cache, a subsequent anonymous request within the 60-second TTL window will receive private slugs for autolink resolution. The mitigation (note-detail endpoint filters) is sound, but a cleaner approach would be to key the cache by auth state (e.g., separate `publicSlugs` / `allSlugs` caches). Low priority since there is no actual data exposure. 3. **CSS duplication between `.contact-btn` and `.login-submit`** (`/home/ldraney/pal-e-app/src/routes/signin/+page.svelte` lines 51-82): These two classes share 7 identical property declarations. Consider extracting a shared `.btn-primary` class or using a common base. Non-blocking since they are scoped to this one component. 4. **Portfolio URL is hardcoded in the Svelte template** (`/home/ldraney/pal-e-app/src/routes/signin/+page.svelte` line 31): `https://portfolio.tail5b443a.ts.net` follows the same Tailscale domain pattern used elsewhere in the codebase (`auth.ts`), so this is consistent. However, if the Tailscale node name ever changes, it would need updating in multiple files. Consider centralizing Tailscale domain URLs. Very low priority. ### SOP COMPLIANCE - [x] Branch named after issue: `38-fix-ssr-admin-token-leak` references issue #38 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references plan slug: `phase-pal-e-docs-f14-public-readiness` and `Closes #38` - [x] No secrets committed: API key access is via `env.PAL_E_DOCS_API_KEY` (runtime env var), no secrets in code - [x] No unnecessary file changes: All 23 changed files are directly related to the auth token gating - [x] Commit messages are descriptive: `fix: only send admin API key for authenticated sessions` ### PROCESS OBSERVATIONS - **Change Failure Risk**: Low. The fix is mechanical -- every API call site now threads through an `authenticated` option. The secure-by-default design (token omitted unless explicitly opted in) means any missed call site fails safe (shows public content only, never leaks private content). - **Deployment Frequency**: This is a security fix that should deploy promptly. The E2E test suite (`public-readiness.spec.ts`) is designed to validate the fix post-deploy -- the PR body correctly notes that tests "correctly detect the bug on current production" and will pass once deployed. - **Test confidence**: Strong. 11 new E2E tests plus 13 existing auth boundary tests provide good coverage. The new tests verify the absence of private content from the anonymous perspective, which is the correct approach for this class of bug. ### VERDICT: APPROVED

forgejo_admin merged commit 45cc33232d into main 2026-03-17 00:31:20 +00:00

forgejo_admin deleted branch 38-fix-ssr-admin-token-leak 2026-03-17 00:31:20 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-17 00:31:21 +00:00

fix: only send admin API key for authenticated sessions (#39)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

No labels

domain:backend

domain:devops

domain:frontend

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/pal-e-docs-app!39

No description provided.