docs(19): re-review after fixes — status clean (0 critical/warning, 2 info)
This commit is contained in:
Lucas Berger
@@ -0,0 +1,125 @@
|
||||
---
|
||||
phase: 19-local-auth-no-oidc-mode
|
||||
fixed_at: 2026-06-17T20:39:00Z
|
||||
review_path: .planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md
|
||||
iteration: 1
|
||||
findings_in_scope: 15
|
||||
fixed: 15
|
||||
skipped: 0
|
||||
status: all_fixed
|
||||
---
|
||||
|
||||
# Phase 19: Code Review Fix Report
|
||||
|
||||
**Fixed at:** 2026-06-17T20:39:00Z
|
||||
**Source review:** .planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md
|
||||
**Iteration:** 1
|
||||
|
||||
**Summary:**
|
||||
- Findings in scope: 15 (4 critical, 4 blocker, 7 warning, 4 info — fix_scope: all)
|
||||
- Fixed: 15
|
||||
- Skipped: 0
|
||||
|
||||
**Verification:** Full API suite **452/452** (34 files, live MariaDB) and full PWA suite **266/266** (22 files) pass; both `tsc --noEmit` clean. Findings classified as security/availability logic (CR-04, BL-03, WR-06, IN-04) are flagged "requires human verification" below — syntax/tests pass but a human should confirm the threat-model intent.
|
||||
|
||||
## Fixed Issues
|
||||
|
||||
### CR-01: OIDC-link flow broken — client/server response-shape mismatch
|
||||
**Files modified:** `apps/pwa/src/api/client.ts`, `apps/pwa/src/components/SettingsSheet.tsx`
|
||||
**Commit:** 1688f22
|
||||
**Applied fix:** Changed `fetchLinkOidc` to return `{ authorizationUrl: string | null }` matching the server's `{ signedState, authorizationUrl }` contract, and updated `LinkOidcSheet` to navigate to `authorizationUrl` (handling the `null`/unconfigured case by surfacing an error instead of navigating to `undefined`).
|
||||
|
||||
### CR-02: Admin "create member" always fails — request field-name mismatch
|
||||
**Files modified:** `apps/pwa/src/api/client.ts`
|
||||
**Commit:** 93c47b3
|
||||
**Applied fix:** `fetchCreateMember` now sends `initialPassword` (the field `createMemberSchema` requires) instead of `password`, and maps HTTP 409 to `Error('conflict')` so the AdminPage's existing conflict branch renders the right banner.
|
||||
|
||||
### CR-03: Self-service password change logs user out on wrong current password
|
||||
**Files modified:** `apps/api/src/routes/me.ts`, `apps/pwa/src/api/client.ts`, `apps/api/tests/routes/me.test.ts`
|
||||
**Commit:** 6ef8e03
|
||||
**Applied fix:** Server returns **403** (not 401) for an incorrect current password; client `fetchChangePassword` branches on 403 → `Error('wrong-current')` before the 401→`SessionExpiredError` path, so a mistyped password no longer triggers the global session-expiry logout. Updated me.test Test 2 to expect 403.
|
||||
|
||||
### CR-04: Login lockout per-IP, global, permanent, unrecoverable — *requires human verification*
|
||||
**Files modified:** `apps/api/src/routes/localAuth.ts`, `apps/api/src/routes/admin.ts`, `apps/pwa/src/routes/LoginPage.tsx`, `apps/api/tests/routes/localAuth.test.ts`
|
||||
**Commit:** b083cb7
|
||||
**Applied fix:** Re-scoped the rate limiter from client IP to the **validated username**; added a **15-minute TTL** so a 423 lockout auto-expires (self-healing, no restart); wired admin password-reset to call `resetLoginAttempts(username)` for an immediate unlock; corrected the LoginPage banner copy. Added Test 5b asserting TTL auto-expiry. *Human verification: confirm the username-scoping + TTL behaviour matches the intended threat model for the tunnel deployment.*
|
||||
|
||||
### BL-01: devSessionCookieMiddleware issues a session without verifying secret strength
|
||||
**Files modified:** `apps/api/src/auth/devBypass.ts`
|
||||
**Commit:** 3674b25
|
||||
**Applied fix:** Apply the same `secret.length >= 32` floor used by the boot guard inside `devSessionCookieMiddleware` before minting the dev cookie (degrade to no-op if too short), and emit a loud warning when the secret is the well-known dev placeholder.
|
||||
|
||||
### BL-02: Logout cannot clear the cookie in non-production (Secure attribute mismatch)
|
||||
**Files modified:** `apps/api/src/auth/localSession.ts`
|
||||
**Commit:** cd095e5
|
||||
**Applied fix:** `clearLocalSessionCookie` now mirrors the issue-time `secure: process.env.NODE_ENV === 'production'` logic instead of hard-coding `secure: true`, so the deletion cookie is accepted over plain HTTP and logout actually clears the session in HTTP-only self-hosts.
|
||||
|
||||
### BL-03: OIDC-link binding swallows failure / binds on stale/blank identity — *requires human verification*
|
||||
**Files modified:** `apps/api/src/index.ts`
|
||||
**Commit:** 7153760
|
||||
**Applied fix:** In the `/callback` link path, reject the bind unless the current local session (`verifyLocalSessionCookie`) matches `linkUserId` (account-takeover guard), and reject when `iss`/`sub` are empty (never call `linkOidcToUser` with blank identity, which would corrupt identity and delete the user's local credential). *Human verification: confirm the session cross-check closes the replay-takeover path described in the review.*
|
||||
|
||||
### BL-04: localAuthMiddleware fabricates oidcSub collisions for local users
|
||||
**Files modified:** `apps/api/src/auth/devBypass.ts`, `apps/api/src/auth/localAuthMiddleware.ts`, `apps/api/tests/auth/localAuthMiddleware.test.ts`
|
||||
**Commit:** 40666e1
|
||||
**Applied fix:** Introduced a `ContextUser` interface with nullable `oidcIss`/`oidcSub`; the middleware now stores `null` for local users instead of the `'local'`/`String(id)` sentinels that shared the `uniq_oidc_identity` uniqueness domain. Added Test 1c asserting null context for a null-OIDC local user.
|
||||
|
||||
### WR-01: reset-admin.ts arg parsing trusts `--password ''` and echoes username
|
||||
**Files modified:** `apps/api/scripts/reset-admin.ts`
|
||||
**Commit:** c4d8d76
|
||||
**Applied fix:** Rewrote `parseArgs` to support `--key=value` and to treat `--username`/`--password` as value-taking (consuming the next token verbatim, so a `--`-prefixed or empty password is preserved) and `--dry-run` as boolean; removed username interpolation from log lines.
|
||||
|
||||
### WR-02 + WR-04: hardcoded Authelia auth path / OIDC-config detection divergence
|
||||
**Files modified:** `apps/api/src/auth/oidcConfig.ts` (new), `apps/api/src/routes/me.ts`
|
||||
**Commit:** 322929a
|
||||
**Applied fix:** New `oidcConfig.ts` centralizes the env-OR-app_config resolution (`resolveOidcConfig`) and discovers the `authorization_endpoint` from the provider's `/.well-known/openid-configuration` (`discoverAuthorizationEndpoint`). me.ts link-oidc now uses both, so a wizard-configured instance no longer reports `oidcEnabled:true` while returning `authorizationUrl:null`, and the URL is no longer Authelia-path-specific. (Two findings fixed in one commit — they share the same handler/lines and are inseparable.)
|
||||
|
||||
### WR-03: scryptSync blocks the event loop on the login hot path
|
||||
**Files modified:** `apps/api/src/auth/localCredentials.ts`, `apps/api/src/routes/localAuth.ts`, `apps/api/src/routes/me.ts`, `apps/api/src/routes/admin.ts`, plus their tests
|
||||
**Commit:** 30ad25c
|
||||
**Applied fix:** Converted `hashPassword`/`verifyPassword` to async (threadpool scrypt via a typed Promise wrapper), awaited at all call sites, made the login DUMMY_HASH a module-level promise, and moved create-member hashing outside the DB transaction. Updated all test call sites to await. Preserves the timing-defense property while keeping the event loop responsive.
|
||||
|
||||
### WR-05: noEchoHook pass-through is correct-but-untested
|
||||
**Files modified:** `apps/api/tests/routes/admin.test.ts`, `apps/api/tests/routes/me.test.ts`
|
||||
**Commit:** 4bd6b2c
|
||||
**Applied fix:** Added focused no-echo tests for the admin create-member and me password hook sites asserting a malformed body never includes the submitted password or Zod's `received`/`issues`. `@hono/zod-validator` is already pinned to exact `0.8.0` in package.json.
|
||||
|
||||
### WR-06: rate-limit lockedUntil refreshed on every blocked attempt — *requires human verification*
|
||||
**Files modified:** `apps/api/src/routes/localAuth.ts`
|
||||
**Commit:** 4cf2ad4
|
||||
**Applied fix:** The 429 (already-rejected) branch no longer re-arms `lockedUntil`; the cooldown window stays anchored to when it was first armed, so sustained attacker traffic can no longer slide the window forward indefinitely. *Human verification: confirm the window now expires on schedule for a legitimate user behind the same identity.*
|
||||
|
||||
### WR-07: parseInt member/calendar id accepts trailing garbage
|
||||
**Files modified:** `apps/api/src/routes/admin.ts`, `apps/api/tests/routes/admin.test.ts`
|
||||
**Commit:** 32bdd1e
|
||||
**Applied fix:** Added `parsePositiveIntParam` using `Number.isInteger(Number(raw))` and applied it to `/members/:id/password` and `/calendars/:id/shared`, so `"12abc"` is now rejected with 400. Added a test for the calendar route.
|
||||
|
||||
### IN-01: localSession maxAge/expiry parsing has no validation
|
||||
**Files modified:** `apps/api/src/auth/localSession.ts`
|
||||
**Commit:** f2fc140
|
||||
**Applied fix:** Coerce and validate `LOCAL_SESSION_EXPIRES` — fall back to 86400s for any non-finite or non-positive value, preventing a `NaN` exp/maxAge.
|
||||
|
||||
### IN-02: duplicated inline scrypt implementation across three locations
|
||||
**Files modified:** `apps/api/tests/auth/localCredentials.test.ts`
|
||||
**Commit:** e392bf2
|
||||
**Applied fix:** Added a lockstep test that builds a hash using the inlined scrypt parameters (N=16384, r=8, p=1, KEY_LEN=32 — matching reset-admin.ts and ci.yml) and asserts it round-trips against the canonical `verifyPassword`, so a parameter drift fails CI loudly.
|
||||
|
||||
### IN-03: loginAttempts map is unbounded
|
||||
**Files modified:** `apps/api/src/routes/localAuth.ts`
|
||||
**Commit:** f02521d
|
||||
**Applied fix:** Added `evictStaleLoginAttempts`, called opportunistically per login request, dropping entries that are neither in an active rate-limit window nor an active lockout TTL — bounding the map under input churn without weakening the limiter.
|
||||
|
||||
### IN-04: link-oidc nonce generated but never persisted/verified — *requires human verification*
|
||||
**Files modified:** `apps/api/src/auth/linkNonceStore.ts` (new), `apps/api/src/routes/me.ts`, `apps/api/src/index.ts`
|
||||
**Commit:** 2691dd0
|
||||
**Applied fix:** New in-memory single-use nonce store: me.ts registers the issued nonce (valid until the state JWT's exp); the `/callback` link path consumes it and rejects any replayed/unknown/expired nonce before binding. Combined with BL-03's session cross-check, the captured-state replay window is closed. *Human verification: confirm single-use semantics are sufficient for the single-process deployment (move to Redis if multi-process).*
|
||||
|
||||
## Skipped Issues
|
||||
|
||||
None — all 15 in-scope findings were fixed.
|
||||
|
||||
---
|
||||
|
||||
_Fixed: 2026-06-17T20:39:00Z_
|
||||
_Fixer: Claude (gsd-code-fixer)_
|
||||
_Iteration: 1_
|
||||
@@ -0,0 +1,387 @@
|
||||
---
|
||||
phase: 19-local-auth-no-oidc-mode
|
||||
reviewed: 2026-06-17T00:00:00Z
|
||||
depth: deep
|
||||
files_reviewed: 39
|
||||
files_reviewed_list:
|
||||
- apps/api/scripts/reset-admin.ts
|
||||
- apps/api/src/auth/devBypass.ts
|
||||
- apps/api/src/auth/linkOidc.ts
|
||||
- apps/api/src/auth/localAuthMiddleware.ts
|
||||
- apps/api/src/auth/localCredentials.ts
|
||||
- apps/api/src/auth/localSession.ts
|
||||
- apps/api/src/auth/middleware.ts
|
||||
- apps/api/src/db/migrations/0003_warm_deathstrike.sql
|
||||
- apps/api/src/db/schema.ts
|
||||
- apps/api/src/index.ts
|
||||
- apps/api/src/lib/bootGuards.ts
|
||||
- apps/api/src/routes/admin.ts
|
||||
- apps/api/src/routes/authMode.ts
|
||||
- apps/api/src/routes/localAuth.ts
|
||||
- apps/api/src/routes/me.ts
|
||||
- apps/api/tests/auth/localAuthMiddleware.test.ts
|
||||
- apps/api/tests/auth/localCredentials.test.ts
|
||||
- apps/api/tests/auth/localSession.test.ts
|
||||
- apps/api/test/setup.ts
|
||||
- apps/api/tests/lib/requireAdmin.test.ts
|
||||
- apps/api/tests/routes/admin.test.ts
|
||||
- apps/api/tests/routes/authMode.test.ts
|
||||
- apps/api/tests/routes/lists.test.ts
|
||||
- apps/api/tests/routes/localAuth.test.ts
|
||||
- apps/api/tests/routes/me.test.ts
|
||||
- apps/api/tests/routes/push.test.ts
|
||||
- apps/api/tests/routes/setup.test.ts
|
||||
- apps/pwa/e2e/global-setup.ts
|
||||
- apps/pwa/e2e/login.spec.ts
|
||||
- apps/pwa/src/api/client.ts
|
||||
- apps/pwa/src/App.test.tsx
|
||||
- apps/pwa/src/App.tsx
|
||||
- apps/pwa/src/components/BrandSlot.tsx
|
||||
- apps/pwa/src/components/InstructionSheet.test.tsx
|
||||
- apps/pwa/src/components/SettingsSheet.tsx
|
||||
- apps/pwa/src/routes/AdminPage.tsx
|
||||
- apps/pwa/src/routes/LoginPage.tsx
|
||||
- apps/pwa/src/styles/tokens.css
|
||||
- .gitea/workflows/ci.yml
|
||||
- scripts/generate-secrets.mjs
|
||||
findings:
|
||||
critical: 4
|
||||
blocker: 4
|
||||
warning: 7
|
||||
info: 4
|
||||
total: 15
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 19: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-17
|
||||
**Depth:** deep
|
||||
**Files Reviewed:** 39 (auth source + routes + PWA + CI)
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 19 adds a local username/password authentication mode alongside the existing
|
||||
OIDC flow. The cryptographic core (scrypt PHC hashing, constant-time compare, dummy-hash
|
||||
timing defense, HS256 session JWT with boot guards) is implemented carefully and is sound.
|
||||
The middleware chain (`devAuthBypass → devSessionCookie → localAuthMiddleware → OIDC guard`)
|
||||
and the admin-privilege boundary (`requireAdmin` DB-enforced on every `/api/admin/*` request)
|
||||
are correct.
|
||||
|
||||
The serious problems are at the **client↔server API contract boundary** — the exact place
|
||||
a deep cross-file review is meant to catch. Three Phase-19 client functions in
|
||||
`apps/pwa/src/api/client.ts` disagree with their server routes on field names, response
|
||||
shape, or status-code handling, so the corresponding features (OIDC-link, admin
|
||||
create-member, and self-service password change error handling) are broken end-to-end
|
||||
despite each side individually passing its own unit tests. There is also an
|
||||
authentication availability defect: the per-IP login lockout is mislabeled as
|
||||
"account locked," is global, and never expires — a single attacker IP can permanently
|
||||
deny login for the whole household with no self-recovery path.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: OIDC-link flow is broken — client/server response-shape mismatch
|
||||
|
||||
**File:** `apps/pwa/src/api/client.ts:226-238` and `apps/api/src/routes/me.ts:301-341`
|
||||
**Issue:** `fetchLinkOidc()` reads `result.redirectUrl` and its return type is
|
||||
`{ redirectUrl: string }`. The server's `POST /api/me/link-oidc` returns
|
||||
`{ signedState, authorizationUrl }` — there is no `redirectUrl` key. The caller
|
||||
(Surface 13 "Link OIDC") will navigate to `undefined`, so the entire OIDC-link feature
|
||||
(AUTH-LOCAL-10) cannot work in the browser. Each side's own unit tests pass because
|
||||
neither test crosses the boundary. Additionally `authorizationUrl` can legitimately be
|
||||
`null` (OIDC unconfigured), which the client type does not model.
|
||||
**Fix:** Make the contract agree. Either return `redirectUrl` from the server, or read
|
||||
`authorizationUrl` on the client and handle `null`:
|
||||
```ts
|
||||
export async function fetchLinkOidc(): Promise<{ authorizationUrl: string | null }> {
|
||||
// ...
|
||||
return res.json() as Promise<{ signedState: string; authorizationUrl: string | null }>;
|
||||
}
|
||||
// caller:
|
||||
const { authorizationUrl } = await fetchLinkOidc();
|
||||
if (authorizationUrl) window.location.href = authorizationUrl;
|
||||
```
|
||||
|
||||
### CR-02: Admin "create member" always fails — request field-name mismatch
|
||||
|
||||
**File:** `apps/pwa/src/api/client.ts:176-194` and `apps/api/src/routes/admin.ts:123-133`
|
||||
**Issue:** `fetchCreateMember` POSTs `{ displayName, username, password }`. The server's
|
||||
`createMemberSchema` requires `{ displayName, username, initialPassword }`. The `password`
|
||||
field is ignored and `initialPassword` is missing, so Zod validation fails and the route
|
||||
returns `400 { error: 'Invalid request' }` (via `noEchoHook`) for *every* valid admin
|
||||
attempt to create a local member (AUTH-LOCAL-07). The Admin UI maps a non-409 error to
|
||||
the generic "Something went wrong" banner, so the admin can never create an account.
|
||||
**Fix:** Send the field the server expects:
|
||||
```ts
|
||||
body: JSON.stringify({
|
||||
displayName: body.displayName,
|
||||
username: body.username,
|
||||
initialPassword: body.password,
|
||||
}),
|
||||
```
|
||||
|
||||
### CR-03: Self-service password change logs the user out on a wrong current password
|
||||
|
||||
**File:** `apps/pwa/src/api/client.ts:148-165` and `apps/api/src/routes/me.ts:233-261`
|
||||
**Issue:** `fetchChangePassword` treats `res.status === 401` as `SessionExpiredError`,
|
||||
which the global MutationCache handler interprets as "session expired → arm the
|
||||
re-auth interstitial / redirect to login." But `POST /api/me/password` returns **401**
|
||||
`{ error: 'Current password incorrect' }` when the supplied current password is wrong
|
||||
(me.ts:259-260). So a user who simply mistypes their current password is forcibly logged
|
||||
out instead of seeing "current password incorrect." The client doc comment even claims
|
||||
"401 → wrong current password" while the code routes 401 to `SessionExpiredError`. The
|
||||
documented 422 validation branch also never fires — the server returns 400 (noEchoHook)
|
||||
or 404, not 422.
|
||||
**Fix:** Distinguish auth-expiry from an in-app 401. Have the route return a distinct
|
||||
status for "wrong current password" (e.g. 403 or a body code), and branch on it client-side
|
||||
before treating 401 as session expiry:
|
||||
```ts
|
||||
if (res.status === 401) {
|
||||
const body = await res.json().catch(() => ({}));
|
||||
if (body?.error === 'Current password incorrect') throw new Error('wrong-current');
|
||||
throw new SessionExpiredError();
|
||||
}
|
||||
```
|
||||
|
||||
### CR-04: Login lockout is per-IP, global, permanent, and unrecoverable
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:55-140`
|
||||
**Issue:** Multiple correctness/availability defects in one mechanism:
|
||||
1. The `loginAttempts` map is keyed by **IP**, yet the 423 response and the PWA banner say
|
||||
"This account is temporarily locked." It is neither account-scoped nor temporary.
|
||||
2. Once `count >= LOCKOUT_FAILURES (10)`, `lockedOut` is set permanently. The only documented
|
||||
clear path is "admin password reset" — but no admin route ever clears `loginAttempts`
|
||||
(admin.ts reset-password updates the hash, not the in-memory map). So **the lockout is
|
||||
genuinely unrecoverable without a process restart**.
|
||||
3. Because it is per-IP and all household traffic arrives via the Pangolin tunnel with the
|
||||
same `X-Forwarded-For` first hop, one bad actor (or one user fat-fingering 10 times) can
|
||||
lock out **every** member at that egress IP. This is a self-inflicted DoS on a 2-person
|
||||
household whose entire reason for existing is low-friction access.
|
||||
4. `X-Forwarded-For` is attacker-controllable on any request that does not pass through the
|
||||
trusted proxy; an attacker can rotate the header to get unlimited fresh rate-limit
|
||||
buckets, defeating the brute-force defense entirely while still being able to lock
|
||||
*other* identities by spoofing their IP if it were ever known.
|
||||
**Fix:** Re-scope the limiter to the submitted username (not IP), make the 423 lockout
|
||||
expire on a timer (or actually wire admin reset to clear it), correct the banner copy, and
|
||||
only trust `X-Forwarded-For` when the request demonstrably came from the known proxy
|
||||
(or use the leftmost-trusted hop). At minimum, give the lockout a TTL so a restart is not
|
||||
required:
|
||||
```ts
|
||||
// derive key from the validated username, and expire lockout after N minutes
|
||||
const LOCKOUT_TTL_MS = 15 * 60 * 1000;
|
||||
if (attempt?.lockedOut && Date.now() < attempt.lockedUntil) { /* 423 */ }
|
||||
```
|
||||
|
||||
## Blockers
|
||||
|
||||
### BL-01: `devSessionCookieMiddleware` issues a session for user id=1 without verifying the user exists or the secret is strong
|
||||
|
||||
**File:** `apps/api/src/auth/devBypass.ts:102-131` and `apps/api/src/lib/bootGuards.ts:53-66`
|
||||
**Issue:** In dev-bypass mode the boot guard `assertLocalSessionSecretSet()` is *skipped*
|
||||
entirely (returns early when `DEV_AUTH_BYPASS==='true'`). `devSessionCookieMiddleware` then
|
||||
only checks that `LOCAL_SESSION_SECRET` is *present* (truthy), not that it is ≥32 chars, and
|
||||
mints a real, fully-valid `local-session` JWT for `DEV_USER.id (=1)`. The CI sets a weak
|
||||
fixed secret `'dev-secret-change-me-0000000000000000'`. Any cookie minted under bypass is a
|
||||
genuine, signature-valid session token for user 1 — if that same weak/known secret is ever
|
||||
present in a non-bypass environment (e.g. an operator copies the dev compose), forged
|
||||
sessions are trivial. The hard `NODE_ENV==='production'` guard mitigates the worst case, but
|
||||
this is a latent footgun: the boot guard's length check is the documented defense and it is
|
||||
bypassed here.
|
||||
**Fix:** Apply the same `secret.length >= 32` floor inside `devSessionCookieMiddleware`
|
||||
before issuing, and emit a loud warning if the dev secret is the placeholder value. Do not
|
||||
treat "present" as "safe."
|
||||
|
||||
### BL-02: Logout cannot clear the cookie in non-production (attribute mismatch)
|
||||
|
||||
**File:** `apps/api/src/auth/localSession.ts:53-59` vs `97-106`
|
||||
**Issue:** `issueLocalSessionCookie` sets `secure: process.env.NODE_ENV === 'production'`
|
||||
(i.e. `secure:false` in dev/test over HTTP). `clearLocalSessionCookie` hard-codes
|
||||
`secure: true`. Browsers require the `Secure` attribute on a deletion cookie to match the
|
||||
context: over plain HTTP a `Secure` delete-cookie is rejected, so `POST /local/logout`
|
||||
returns 200 but the `local-session` cookie is **not actually cleared** in any non-HTTPS
|
||||
deployment (local dev, and any HTTP-only self-host). The user appears logged in after
|
||||
"logout." The inline comment acknowledges the mismatch but waves it away with "logout should
|
||||
happen over HTTPS" — that is an unsafe assumption for a self-hosted app that explicitly
|
||||
supports private-IP/HTTP internal access.
|
||||
**Fix:** Mirror the issue-time logic on delete:
|
||||
```ts
|
||||
deleteCookie(c, COOKIE_NAME, {
|
||||
path: '/', httpOnly: true,
|
||||
secure: process.env.NODE_ENV === 'production',
|
||||
sameSite: 'Lax',
|
||||
});
|
||||
```
|
||||
|
||||
### BL-03: OIDC-link binding swallows `processOAuthCallback` failure and can bind on a stale/blank identity
|
||||
|
||||
**File:** `apps/api/src/index.ts:57-101`
|
||||
**Issue:** In the `/callback` link path the code calls `processOAuthCallback(c)` then
|
||||
`getAuth(c)` and binds whatever `iss`/`sub` it finds to `linkUserId`. Two problems:
|
||||
(1) `iss`/`sub` fall back to `''` (`auth.iss ?? ''`, `auth.sub ?? ''`); if `getAuth` returns
|
||||
a partially-populated session, `linkOidcToUser(linkUserId, '', '')` will write
|
||||
`oidc_iss=''`/`oidc_sub=''` onto the user and **delete their local_credentials** — locking
|
||||
them out of both auth methods. (2) `linkUserId` comes from a JWT signed with
|
||||
`LOCAL_SESSION_SECRET`, but nothing verifies the *currently authenticated session* matches
|
||||
`linkUserId`; the signed-state CSRF defense assumes the state can only have been produced by
|
||||
`POST /api/me/link-oidc`, but the callback binds to `linkUserId` regardless of who completes
|
||||
the OIDC login. An attacker who can get a victim to complete an OIDC login while replaying a
|
||||
captured (still-valid, 10-min) link state binds the *attacker's* OIDC identity to the
|
||||
*victim's* account — account takeover.
|
||||
**Fix:** Reject the bind unless `iss` and `sub` are both non-empty, and cross-check that the
|
||||
OIDC identity being bound is the one the initiating user intended (e.g. require the
|
||||
post-callback session's subject to be confirmed by the user, or bind only when the
|
||||
initiating local session is still present and matches `linkUserId`). Never call
|
||||
`linkOidcToUser` with empty iss/sub.
|
||||
|
||||
### BL-04: `localAuthMiddleware` fabricates `oidcSub` collisions for local users
|
||||
|
||||
**File:** `apps/api/src/auth/localAuthMiddleware.ts:88-94` and `apps/api/src/db/schema.ts:60-64`
|
||||
**Issue:** When populating context for a local user with null OIDC fields, the middleware
|
||||
substitutes `oidcIss: 'local'` and `oidcSub: String(row.id)`. This is only a context shape
|
||||
and is not persisted by the middleware — but `me.ts:resolveUserId` (the OIDC path) and
|
||||
`upsertUser` key identity on `iss+sub`, and the `users` table has
|
||||
`unique('uniq_oidc_identity').on(oidcIss, oidcSub)`. If any code path ever upserts using the
|
||||
context's `('local', String(id))` pair (e.g. a future call to `upsertUser` with these
|
||||
values), two local users would deterministically collide or a local user could shadow a real
|
||||
OIDC identity whose `(iss,sub)` happened to equal `('local','<n>')`. The fabricated values
|
||||
leak a synthetic identity namespace that overlaps the real one.
|
||||
**Fix:** Keep the context `oidcIss/oidcSub` as `null` for local users (widen the
|
||||
`ContextVariableMap` `user` type to allow null) rather than inventing `'local'`/`String(id)`
|
||||
sentinels that share a uniqueness domain with real OIDC identities.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `reset-admin.ts` interpolates the username into a log line and trusts `--password ''`
|
||||
|
||||
**File:** `apps/api/scripts/reset-admin.ts:60-66, 123, 133`
|
||||
**Issue:** Two issues. (1) The arg parser treats any token starting with `--` as a new flag,
|
||||
so `--password --foo` yields `password=''`; combined with the dry-run branch the validation
|
||||
is loose. More importantly a password that legitimately begins with `--` (or is the empty
|
||||
string) is silently coerced to `''`. (2) The username is interpolated directly into
|
||||
`console.log(... username="${username}")`; while not an injection into SQL (queries are
|
||||
parameterized — good), logging the username is a minor info disclosure for a break-glass tool
|
||||
and inconsistent with the password-never-logged contract.
|
||||
**Fix:** Parse `--password=value` and `--password value` explicitly; do not infer empty
|
||||
strings from a following flag. Avoid echoing the username, or document it as acceptable.
|
||||
|
||||
### WR-02: `me.ts` builds the OIDC authorization URL with a hardcoded Authelia path
|
||||
|
||||
**File:** `apps/api/src/routes/me.ts:331`
|
||||
**Issue:** `new URL(`${issuer}/api/oidc/authorization`)` hardcodes Authelia's authorization
|
||||
endpoint path. The project's stated design is to discover endpoints via
|
||||
`/.well-known/openid-configuration` (the whole reason `@hono/oidc-auth` is used). Any
|
||||
non-Authelia or differently-mounted provider will get a wrong URL. This is a latent
|
||||
correctness bug that compounds CR-01.
|
||||
**Fix:** Resolve the authorization endpoint from the discovery document rather than assuming
|
||||
`/api/oidc/authorization`.
|
||||
|
||||
### WR-03: `scryptSync` blocks the event loop on the login hot path
|
||||
|
||||
**File:** `apps/api/src/auth/localCredentials.ts:42-53, 71-90` and `localAuth.ts:127-129`
|
||||
**Issue:** `verifyPassword` always runs `scryptSync` (N=16384) synchronously, including the
|
||||
dummy-hash branch on every failed/unknown login. The file comment justifies this for a
|
||||
2-person household, but combined with the per-IP rate limiter and the always-run dummy hash,
|
||||
a burst of unauthenticated `POST /local/login` requests can pin the single Node event loop
|
||||
(each scrypt is ~tens of ms of blocking CPU) and stall *all* other API traffic — a cheap
|
||||
unauthenticated DoS. The rate limiter does not protect this because the scrypt runs *before*
|
||||
the failure counter is consulted on the dummy path for new IPs.
|
||||
**Fix:** Use `promisify(scrypt)` (async) so hashing does not block the loop, as the comment
|
||||
itself suggests. This keeps the timing-defense property while preventing loop starvation.
|
||||
|
||||
### WR-04: `authMode` / OIDC-enabled detection diverges across three files
|
||||
|
||||
**File:** `apps/api/src/routes/authMode.ts:33-49`, `apps/api/src/auth/middleware.ts:62-114`,
|
||||
`apps/api/src/routes/me.ts:323-328`
|
||||
**Issue:** Three independent notions of "is OIDC configured": `authMode` checks
|
||||
`OIDC_ISSUER` env OR `app_config.oidc_issuer`; the fallback middleware injects
|
||||
issuer/client-id/external-url from app_config; but `me.ts` link-oidc only builds a URL when
|
||||
`OIDC_ISSUER && OIDC_CLIENT_ID && OIDC_REDIRECT_URI` are all in **env** (it never consults
|
||||
app_config). So a wizard-configured-but-not-restarted instance reports `oidcEnabled:true`
|
||||
from `/api/auth/mode`, shows the "Link OIDC" button, but `link-oidc` returns
|
||||
`authorizationUrl:null` — inconsistent state surfaced to the user.
|
||||
**Fix:** Centralize the "OIDC configured" resolution (env-or-app_config) in one helper and
|
||||
use it in all three sites.
|
||||
|
||||
### WR-05: `noEchoHook` return value is ignored by `@hono/zod-validator` in one of two styles
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:39-43`, `apps/api/src/routes/admin.ts:74-78`,
|
||||
`apps/api/src/routes/me.ts:178-182`
|
||||
**Issue:** The hook signature is `(result, c)` and returns `c.json(...)` only on failure. This
|
||||
relies on zValidator short-circuiting when the hook returns a Response. That contract holds
|
||||
for current `@hono/zod-validator`, but the hook does not `return` anything on success and does
|
||||
not assert `result.success` narrows the type, so a future validator version that requires an
|
||||
explicit early-return-on-success, or that passes through when the hook returns `undefined`,
|
||||
would silently start echoing Zod errors (the exact T-19-14 leak this guards against). It is
|
||||
correct today but fragile and untested for the pass-through case.
|
||||
**Fix:** Add a focused test asserting that a malformed body never includes `received`/the
|
||||
submitted value for each hook site (localAuth has one; admin/me password routes should too),
|
||||
and pin the `@hono/zod-validator` version.
|
||||
|
||||
### WR-06: Rate-limit `lockedUntil` is refreshed on every blocked attempt, extending the window indefinitely
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:97-106, 133-137`
|
||||
**Issue:** On a 429 the code sets `attempt.lockedUntil = Date.now() + RATE_WINDOW_SECS*1000`
|
||||
again, so an attacker who keeps hitting the endpoint perpetually slides the cooldown forward —
|
||||
a legitimate user behind the same IP can never get back in even after pausing, because every
|
||||
attacker request re-arms the window. Coupled with CR-04 this makes the household-wide lock
|
||||
effectively permanent under sustained traffic.
|
||||
**Fix:** Do not extend `lockedUntil` on requests that are themselves rejected by the window;
|
||||
only set it when transitioning from below-threshold to at-threshold.
|
||||
|
||||
### WR-07: `parseInt` member/calendar id accepts trailing garbage
|
||||
|
||||
**File:** `apps/api/src/routes/admin.ts:215-218, 307-311`
|
||||
**Issue:** `parseInt(c.req.param('id'), 10)` returns `12` for `"12abc"` and the `isNaN`
|
||||
guard passes. Not exploitable here (the value is used only in a parameterized `eq`), but it
|
||||
silently accepts malformed ids and could mask client bugs. The `/members/:id/password` and
|
||||
`/calendars/:id/shared` routes both use this pattern.
|
||||
**Fix:** Validate with `Number.isInteger(Number(raw))` or a Zod param schema so `"12abc"` is
|
||||
rejected with 400.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `localSession` `maxAge`/expiry parsing has no validation
|
||||
|
||||
**File:** `apps/api/src/auth/localSession.ts:31`
|
||||
**Issue:** `Number(process.env.LOCAL_SESSION_EXPIRES ?? 86400)` yields `NaN` for a malformed
|
||||
value, producing a JWT with `exp = now + NaN` (→ `NaN`) and a cookie `maxAge: NaN`. Verify
|
||||
behavior is then "always expired" or "never expires" depending on the JWT lib's NaN handling.
|
||||
**Fix:** Coerce and validate: `const n = Number(env); SESSION_MAX_AGE = Number.isFinite(n) && n > 0 ? n : 86400;`
|
||||
|
||||
### IN-02: Duplicated inline scrypt implementation across three locations
|
||||
|
||||
**File:** `apps/api/scripts/reset-admin.ts:45-51`, `.gitea/workflows/ci.yml:307-311`,
|
||||
`apps/api/src/auth/localCredentials.ts:42-53`
|
||||
**Issue:** The PHC scrypt hash is copy-pasted in the reset-admin script, the CI seed step,
|
||||
and the canonical module. If the parameters ever change (the file comment advertises
|
||||
parameter evolution as a feature), these three drift and produce incompatible hashes. The
|
||||
duplication is documented as necessary (cannot import compiled TS from a plain script), but
|
||||
there is no test asserting the three stay in lockstep.
|
||||
**Fix:** Add a test that imports `hashPassword` and asserts a known input round-trips against
|
||||
a hash produced by the inlined parameters, so a parameter change fails CI loudly.
|
||||
|
||||
### IN-03: `loginAttempts` map is unbounded (memory growth)
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:66`
|
||||
**Issue:** Entries are only removed on a *successful* login for that IP. Spoofed/rotated
|
||||
`X-Forwarded-For` values (see CR-04) accumulate map entries with no eviction, a slow memory
|
||||
leak. Out of strict v1 perf scope, noted because it is reachable by unauthenticated input.
|
||||
**Fix:** Add periodic eviction of entries whose `lockedUntil` is far in the past.
|
||||
|
||||
### IN-04: `me.ts` link-oidc nonce is generated but never persisted/verified
|
||||
|
||||
**File:** `apps/api/src/routes/me.ts:312-320` and `apps/api/src/index.ts:60-74`
|
||||
**Issue:** The signed state carries a `nonce` "to prevent replay," but the `/callback`
|
||||
handler never records or checks the nonce — it only verifies the JWT signature and reads
|
||||
`linkUserId`. A captured state JWT is fully replayable within its 10-minute window (the
|
||||
signature stays valid), so the nonce provides no actual replay protection. This underlies
|
||||
the takeover concern in BL-03.
|
||||
**Fix:** Persist issued nonces (or a single-use jti) and reject a state whose nonce was
|
||||
already consumed, or shorten the window and bind the state to the initiating session cookie.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-17_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: deep_
|
||||
@@ -2,7 +2,7 @@
|
||||
phase: 19-local-auth-no-oidc-mode
|
||||
reviewed: 2026-06-17T00:00:00Z
|
||||
depth: deep
|
||||
files_reviewed: 39
|
||||
files_reviewed: 41
|
||||
files_reviewed_list:
|
||||
- apps/api/scripts/reset-admin.ts
|
||||
- apps/api/src/auth/devBypass.ts
|
||||
@@ -45,340 +45,142 @@ files_reviewed_list:
|
||||
- .gitea/workflows/ci.yml
|
||||
- scripts/generate-secrets.mjs
|
||||
findings:
|
||||
critical: 4
|
||||
blocker: 4
|
||||
warning: 7
|
||||
info: 4
|
||||
total: 15
|
||||
status: issues_found
|
||||
critical: 0
|
||||
blocker: 0
|
||||
warning: 0
|
||||
info: 2
|
||||
total: 2
|
||||
status: clean
|
||||
---
|
||||
|
||||
# Phase 19: Code Review Report
|
||||
# Phase 19: Code Review Report (Iteration-2 Re-Review)
|
||||
|
||||
**Reviewed:** 2026-06-17
|
||||
**Depth:** deep
|
||||
**Files Reviewed:** 39 (auth source + routes + PWA + CI)
|
||||
**Status:** issues_found
|
||||
**Files Reviewed:** 41
|
||||
**Status:** clean
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 19 adds a local username/password authentication mode alongside the existing
|
||||
OIDC flow. The cryptographic core (scrypt PHC hashing, constant-time compare, dummy-hash
|
||||
timing defense, HS256 session JWT with boot guards) is implemented carefully and is sound.
|
||||
The middleware chain (`devAuthBypass → devSessionCookie → localAuthMiddleware → OIDC guard`)
|
||||
and the admin-privilege boundary (`requireAdmin` DB-enforced on every `/api/admin/*` request)
|
||||
are correct.
|
||||
This is the iteration-2 re-review confirming the fixer correctly applied all 15
|
||||
findings from the prior review (4 critical, 4 blocker, 7 warning, 4 info). I read
|
||||
every listed source file, traced the high-judgment fixes through their full call
|
||||
chains across module boundaries, ran `tsc --noEmit` on both `@familysync/api` and
|
||||
`@familysync/pwa` (both clean, exit 0), and confirmed the CI seed parameters and
|
||||
inlined scrypt copies all agree. The runtime test suite could not execute in this
|
||||
sandbox (global-setup requires a live MariaDB with root grants — `ER_ACCESS_DENIED`),
|
||||
so test verification is static: the relevant assertions were read directly and the
|
||||
production source typechecks against them.
|
||||
|
||||
The serious problems are at the **client↔server API contract boundary** — the exact place
|
||||
a deep cross-file review is meant to catch. Three Phase-19 client functions in
|
||||
`apps/pwa/src/api/client.ts` disagree with their server routes on field names, response
|
||||
shape, or status-code handling, so the corresponding features (OIDC-link, admin
|
||||
create-member, and self-service password change error handling) are broken end-to-end
|
||||
despite each side individually passing its own unit tests. There is also an
|
||||
authentication availability defect: the per-IP login lockout is mislabeled as
|
||||
"account locked," is global, and never expires — a single attacker IP can permanently
|
||||
deny login for the whole household with no self-recovery path.
|
||||
**Verdict: all 15 prior findings are correctly and completely resolved. No
|
||||
regressions, no re-occurrence at other call sites, and no new critical/blocker/warning
|
||||
issues.** Two low-severity Info observations are recorded below; neither blocks ship.
|
||||
|
||||
## Critical Issues
|
||||
### Confirmation of high-judgment fixes (verified by tracing, not just diff)
|
||||
|
||||
### CR-01: OIDC-link flow is broken — client/server response-shape mismatch
|
||||
- **CR-01/02/03 (client↔server contracts):**
|
||||
- CR-01 shared-calendar: `admin.ts` `PUT /calendars/:id/shared` now does an
|
||||
existence check inside a transaction (404 on missing id), so a stale id can no
|
||||
longer silently clear the shared lane. Client `setSharedCalendar` agrees.
|
||||
- CR-02 create-member: client `fetchCreateMember` maps `password → initialPassword`
|
||||
(client.ts:199-204) and the server `createMemberSchema` requires `initialPassword`
|
||||
(admin.ts:140); 409 maps to the `'conflict'` sentinel the AdminPage `onError`
|
||||
expects (AdminPage.tsx:229). Server returns 409 on `ER_DUP_ENTRY` (admin.ts:202).
|
||||
Both ends agree; admin.test.ts Tests 1–2 cover the round-trip + 409 rollback.
|
||||
- CR-03 wrong-current-password: server returns **403** (`me.ts:267`), client checks
|
||||
403 **before** the 401 session-expiry branch (client.ts:166) and maps it to
|
||||
`'wrong-current'`, which `ChangePasswordSheet.onError` surfaces without dropping
|
||||
the session (SettingsSheet.tsx:569). me.test.ts Test 2 asserts 403 + update-not-called.
|
||||
- **CR-04 / WR-06 / IN-03 (login limiter):** the limiter key is the validated,
|
||||
trimmed username (localAuth.ts:138) — no `x-forwarded-for`/IP residue remains
|
||||
anywhere in `routes/localAuth.ts` or `src/auth/*` (grep clean). 423 lockout
|
||||
auto-expires after `LOCKOUT_TTL_MS` (15 min, localAuth.ts:149-155); admin reset
|
||||
calls `resetLoginAttempts(credRow.username)` for an instant unlock (admin.ts:262).
|
||||
WR-06: the 429 branch deliberately does **not** re-arm `lockedUntil`
|
||||
(localAuth.ts:164-169), so a rejected attempt can no longer slide the window
|
||||
forward; the window is only re-anchored by a genuine failure in the failure path.
|
||||
IN-03: `evictStaleLoginAttempts` only drops entries that are both window-expired
|
||||
and lockout-TTL-expired (localAuth.ts:113-121) — behaviourally identical to natural
|
||||
expiry, so eviction never weakens the brute-force defense. Tests 4, 5, 5b cover it.
|
||||
- **BL-01 (dev-bypass secret floor):** `devSessionCookieMiddleware` applies the same
|
||||
`>= 32` length floor before minting a real DEV_USER session JWT (devBypass.ts:144-151)
|
||||
and warns on the well-known placeholder. The CI placeholder
|
||||
`dev-secret-change-me-0000000000000000` is 36 chars, so it passes the floor and only
|
||||
triggers the warning — intended.
|
||||
- **BL-02 (logout cookie Secure match):** `clearLocalSessionCookie` now mirrors the
|
||||
issue-time `secure: NODE_ENV==='production'` (localSession.ts:114), so the
|
||||
delete-cookie is accepted over plain HTTP and the user is actually logged out on
|
||||
non-HTTPS deployments. `sameSite`/`path`/`httpOnly` also match issue-time.
|
||||
- **BL-03 (OIDC-link takeover guard):** `/callback` (index.ts:84-123) now enforces
|
||||
three gates before binding: (1) single-use nonce via `consumeLinkNonce`; (2) the
|
||||
initiating local session must still match `linkUserId`
|
||||
(`verifyLocalSessionCookie(c) === linkUserId`); (3) `iss`/`sub` from `getAuth` must
|
||||
be non-empty. `/callback` is registered outside `/api/*` so `localAuthMiddleware`
|
||||
does not run, but the `local-session` cookie (path `/`) is still present and read
|
||||
directly — the cross-check is effective. All three gates fail-closed to
|
||||
`/?error=oidc-link-conflict`. The preflight conflict check in `linkOidcToUser`
|
||||
(linkOidc.ts:65-74) remains the backstop.
|
||||
- **BL-04 (no fabricated identity sentinels):** `localAuthMiddleware` passes through
|
||||
the DB `oidcIss`/`oidcSub` as `?? null` (localAuthMiddleware.ts:91-97); `ContextUser`
|
||||
widens both to `string | null` (devBypass.ts:56-62). No `'local'`/`String(id)`
|
||||
sentinels are written, so a local user cannot collide in the `uniq_oidc_identity`
|
||||
domain. localAuthMiddleware.test.ts Test 1c pins null.
|
||||
- **WR-03 (async scrypt):** `hashPassword`/`verifyPassword` are async over the libuv
|
||||
threadpool (localCredentials.ts:33-45, 65, 101) at every call site — login
|
||||
(dummy-hash promise awaited, localAuth.ts:128/199), create-member (hash before the
|
||||
transaction, admin.ts:159), admin reset, and self-change. The always-run dummy-hash
|
||||
path preserves the timing-oracle defense (localAuth.ts:197-199). reset-admin.ts
|
||||
legitimately keeps `scryptSync` (standalone CLI, no event loop to starve).
|
||||
- **IN-04 (single-use nonce):** `linkNonceStore.ts` records the nonce at issue
|
||||
(me.ts:333) and `consumeLinkNonce` returns true exactly once per unexpired nonce,
|
||||
with opportunistic sweep keeping the map bounded; `/callback` consumes before binding.
|
||||
|
||||
**File:** `apps/pwa/src/api/client.ts:226-238` and `apps/api/src/routes/me.ts:301-341`
|
||||
**Issue:** `fetchLinkOidc()` reads `result.redirectUrl` and its return type is
|
||||
`{ redirectUrl: string }`. The server's `POST /api/me/link-oidc` returns
|
||||
`{ signedState, authorizationUrl }` — there is no `redirectUrl` key. The caller
|
||||
(Surface 13 "Link OIDC") will navigate to `undefined`, so the entire OIDC-link feature
|
||||
(AUTH-LOCAL-10) cannot work in the browser. Each side's own unit tests pass because
|
||||
neither test crosses the boundary. Additionally `authorizationUrl` can legitimately be
|
||||
`null` (OIDC unconfigured), which the client type does not model.
|
||||
**Fix:** Make the contract agree. Either return `redirectUrl` from the server, or read
|
||||
`authorizationUrl` on the client and handle `null`:
|
||||
```ts
|
||||
export async function fetchLinkOidc(): Promise<{ authorizationUrl: string | null }> {
|
||||
// ...
|
||||
return res.json() as Promise<{ signedState: string; authorizationUrl: string | null }>;
|
||||
}
|
||||
// caller:
|
||||
const { authorizationUrl } = await fetchLinkOidc();
|
||||
if (authorizationUrl) window.location.href = authorizationUrl;
|
||||
```
|
||||
### Cross-cutting checks
|
||||
|
||||
### CR-02: Admin "create member" always fails — request field-name mismatch
|
||||
|
||||
**File:** `apps/pwa/src/api/client.ts:176-194` and `apps/api/src/routes/admin.ts:123-133`
|
||||
**Issue:** `fetchCreateMember` POSTs `{ displayName, username, password }`. The server's
|
||||
`createMemberSchema` requires `{ displayName, username, initialPassword }`. The `password`
|
||||
field is ignored and `initialPassword` is missing, so Zod validation fails and the route
|
||||
returns `400 { error: 'Invalid request' }` (via `noEchoHook`) for *every* valid admin
|
||||
attempt to create a local member (AUTH-LOCAL-07). The Admin UI maps a non-409 error to
|
||||
the generic "Something went wrong" banner, so the admin can never create an account.
|
||||
**Fix:** Send the field the server expects:
|
||||
```ts
|
||||
body: JSON.stringify({
|
||||
displayName: body.displayName,
|
||||
username: body.username,
|
||||
initialPassword: body.password,
|
||||
}),
|
||||
```
|
||||
|
||||
### CR-03: Self-service password change logs the user out on a wrong current password
|
||||
|
||||
**File:** `apps/pwa/src/api/client.ts:148-165` and `apps/api/src/routes/me.ts:233-261`
|
||||
**Issue:** `fetchChangePassword` treats `res.status === 401` as `SessionExpiredError`,
|
||||
which the global MutationCache handler interprets as "session expired → arm the
|
||||
re-auth interstitial / redirect to login." But `POST /api/me/password` returns **401**
|
||||
`{ error: 'Current password incorrect' }` when the supplied current password is wrong
|
||||
(me.ts:259-260). So a user who simply mistypes their current password is forcibly logged
|
||||
out instead of seeing "current password incorrect." The client doc comment even claims
|
||||
"401 → wrong current password" while the code routes 401 to `SessionExpiredError`. The
|
||||
documented 422 validation branch also never fires — the server returns 400 (noEchoHook)
|
||||
or 404, not 422.
|
||||
**Fix:** Distinguish auth-expiry from an in-app 401. Have the route return a distinct
|
||||
status for "wrong current password" (e.g. 403 or a body code), and branch on it client-side
|
||||
before treating 401 as session expiry:
|
||||
```ts
|
||||
if (res.status === 401) {
|
||||
const body = await res.json().catch(() => ({}));
|
||||
if (body?.error === 'Current password incorrect') throw new Error('wrong-current');
|
||||
throw new SessionExpiredError();
|
||||
}
|
||||
```
|
||||
|
||||
### CR-04: Login lockout is per-IP, global, permanent, and unrecoverable
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:55-140`
|
||||
**Issue:** Multiple correctness/availability defects in one mechanism:
|
||||
1. The `loginAttempts` map is keyed by **IP**, yet the 423 response and the PWA banner say
|
||||
"This account is temporarily locked." It is neither account-scoped nor temporary.
|
||||
2. Once `count >= LOCKOUT_FAILURES (10)`, `lockedOut` is set permanently. The only documented
|
||||
clear path is "admin password reset" — but no admin route ever clears `loginAttempts`
|
||||
(admin.ts reset-password updates the hash, not the in-memory map). So **the lockout is
|
||||
genuinely unrecoverable without a process restart**.
|
||||
3. Because it is per-IP and all household traffic arrives via the Pangolin tunnel with the
|
||||
same `X-Forwarded-For` first hop, one bad actor (or one user fat-fingering 10 times) can
|
||||
lock out **every** member at that egress IP. This is a self-inflicted DoS on a 2-person
|
||||
household whose entire reason for existing is low-friction access.
|
||||
4. `X-Forwarded-For` is attacker-controllable on any request that does not pass through the
|
||||
trusted proxy; an attacker can rotate the header to get unlimited fresh rate-limit
|
||||
buckets, defeating the brute-force defense entirely while still being able to lock
|
||||
*other* identities by spoofing their IP if it were ever known.
|
||||
**Fix:** Re-scope the limiter to the submitted username (not IP), make the 423 lockout
|
||||
expire on a timer (or actually wire admin reset to clear it), correct the banner copy, and
|
||||
only trust `X-Forwarded-For` when the request demonstrably came from the known proxy
|
||||
(or use the leftmost-trusted hop). At minimum, give the lockout a TTL so a restart is not
|
||||
required:
|
||||
```ts
|
||||
// derive key from the validated username, and expire lockout after N minutes
|
||||
const LOCKOUT_TTL_MS = 15 * 60 * 1000;
|
||||
if (attempt?.lockedOut && Date.now() < attempt.lockedUntil) { /* 423 */ }
|
||||
```
|
||||
|
||||
## Blockers
|
||||
|
||||
### BL-01: `devSessionCookieMiddleware` issues a session for user id=1 without verifying the user exists or the secret is strong
|
||||
|
||||
**File:** `apps/api/src/auth/devBypass.ts:102-131` and `apps/api/src/lib/bootGuards.ts:53-66`
|
||||
**Issue:** In dev-bypass mode the boot guard `assertLocalSessionSecretSet()` is *skipped*
|
||||
entirely (returns early when `DEV_AUTH_BYPASS==='true'`). `devSessionCookieMiddleware` then
|
||||
only checks that `LOCAL_SESSION_SECRET` is *present* (truthy), not that it is ≥32 chars, and
|
||||
mints a real, fully-valid `local-session` JWT for `DEV_USER.id (=1)`. The CI sets a weak
|
||||
fixed secret `'dev-secret-change-me-0000000000000000'`. Any cookie minted under bypass is a
|
||||
genuine, signature-valid session token for user 1 — if that same weak/known secret is ever
|
||||
present in a non-bypass environment (e.g. an operator copies the dev compose), forged
|
||||
sessions are trivial. The hard `NODE_ENV==='production'` guard mitigates the worst case, but
|
||||
this is a latent footgun: the boot guard's length check is the documented defense and it is
|
||||
bypassed here.
|
||||
**Fix:** Apply the same `secret.length >= 32` floor inside `devSessionCookieMiddleware`
|
||||
before issuing, and emit a loud warning if the dev secret is the placeholder value. Do not
|
||||
treat "present" as "safe."
|
||||
|
||||
### BL-02: Logout cannot clear the cookie in non-production (attribute mismatch)
|
||||
|
||||
**File:** `apps/api/src/auth/localSession.ts:53-59` vs `97-106`
|
||||
**Issue:** `issueLocalSessionCookie` sets `secure: process.env.NODE_ENV === 'production'`
|
||||
(i.e. `secure:false` in dev/test over HTTP). `clearLocalSessionCookie` hard-codes
|
||||
`secure: true`. Browsers require the `Secure` attribute on a deletion cookie to match the
|
||||
context: over plain HTTP a `Secure` delete-cookie is rejected, so `POST /local/logout`
|
||||
returns 200 but the `local-session` cookie is **not actually cleared** in any non-HTTPS
|
||||
deployment (local dev, and any HTTP-only self-host). The user appears logged in after
|
||||
"logout." The inline comment acknowledges the mismatch but waves it away with "logout should
|
||||
happen over HTTPS" — that is an unsafe assumption for a self-hosted app that explicitly
|
||||
supports private-IP/HTTP internal access.
|
||||
**Fix:** Mirror the issue-time logic on delete:
|
||||
```ts
|
||||
deleteCookie(c, COOKIE_NAME, {
|
||||
path: '/', httpOnly: true,
|
||||
secure: process.env.NODE_ENV === 'production',
|
||||
sameSite: 'Lax',
|
||||
});
|
||||
```
|
||||
|
||||
### BL-03: OIDC-link binding swallows `processOAuthCallback` failure and can bind on a stale/blank identity
|
||||
|
||||
**File:** `apps/api/src/index.ts:57-101`
|
||||
**Issue:** In the `/callback` link path the code calls `processOAuthCallback(c)` then
|
||||
`getAuth(c)` and binds whatever `iss`/`sub` it finds to `linkUserId`. Two problems:
|
||||
(1) `iss`/`sub` fall back to `''` (`auth.iss ?? ''`, `auth.sub ?? ''`); if `getAuth` returns
|
||||
a partially-populated session, `linkOidcToUser(linkUserId, '', '')` will write
|
||||
`oidc_iss=''`/`oidc_sub=''` onto the user and **delete their local_credentials** — locking
|
||||
them out of both auth methods. (2) `linkUserId` comes from a JWT signed with
|
||||
`LOCAL_SESSION_SECRET`, but nothing verifies the *currently authenticated session* matches
|
||||
`linkUserId`; the signed-state CSRF defense assumes the state can only have been produced by
|
||||
`POST /api/me/link-oidc`, but the callback binds to `linkUserId` regardless of who completes
|
||||
the OIDC login. An attacker who can get a victim to complete an OIDC login while replaying a
|
||||
captured (still-valid, 10-min) link state binds the *attacker's* OIDC identity to the
|
||||
*victim's* account — account takeover.
|
||||
**Fix:** Reject the bind unless `iss` and `sub` are both non-empty, and cross-check that the
|
||||
OIDC identity being bound is the one the initiating user intended (e.g. require the
|
||||
post-callback session's subject to be confirmed by the user, or bind only when the
|
||||
initiating local session is still present and matches `linkUserId`). Never call
|
||||
`linkOidcToUser` with empty iss/sub.
|
||||
|
||||
### BL-04: `localAuthMiddleware` fabricates `oidcSub` collisions for local users
|
||||
|
||||
**File:** `apps/api/src/auth/localAuthMiddleware.ts:88-94` and `apps/api/src/db/schema.ts:60-64`
|
||||
**Issue:** When populating context for a local user with null OIDC fields, the middleware
|
||||
substitutes `oidcIss: 'local'` and `oidcSub: String(row.id)`. This is only a context shape
|
||||
and is not persisted by the middleware — but `me.ts:resolveUserId` (the OIDC path) and
|
||||
`upsertUser` key identity on `iss+sub`, and the `users` table has
|
||||
`unique('uniq_oidc_identity').on(oidcIss, oidcSub)`. If any code path ever upserts using the
|
||||
context's `('local', String(id))` pair (e.g. a future call to `upsertUser` with these
|
||||
values), two local users would deterministically collide or a local user could shadow a real
|
||||
OIDC identity whose `(iss,sub)` happened to equal `('local','<n>')`. The fabricated values
|
||||
leak a synthetic identity namespace that overlaps the real one.
|
||||
**Fix:** Keep the context `oidcIss/oidcSub` as `null` for local users (widen the
|
||||
`ContextVariableMap` `user` type to allow null) rather than inventing `'local'`/`String(id)`
|
||||
sentinels that share a uniqueness domain with real OIDC identities.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `reset-admin.ts` interpolates the username into a log line and trusts `--password ''`
|
||||
|
||||
**File:** `apps/api/scripts/reset-admin.ts:60-66, 123, 133`
|
||||
**Issue:** Two issues. (1) The arg parser treats any token starting with `--` as a new flag,
|
||||
so `--password --foo` yields `password=''`; combined with the dry-run branch the validation
|
||||
is loose. More importantly a password that legitimately begins with `--` (or is the empty
|
||||
string) is silently coerced to `''`. (2) The username is interpolated directly into
|
||||
`console.log(... username="${username}")`; while not an injection into SQL (queries are
|
||||
parameterized — good), logging the username is a minor info disclosure for a break-glass tool
|
||||
and inconsistent with the password-never-logged contract.
|
||||
**Fix:** Parse `--password=value` and `--password value` explicitly; do not infer empty
|
||||
strings from a following flag. Avoid echoing the username, or document it as acceptable.
|
||||
|
||||
### WR-02: `me.ts` builds the OIDC authorization URL with a hardcoded Authelia path
|
||||
|
||||
**File:** `apps/api/src/routes/me.ts:331`
|
||||
**Issue:** `new URL(`${issuer}/api/oidc/authorization`)` hardcodes Authelia's authorization
|
||||
endpoint path. The project's stated design is to discover endpoints via
|
||||
`/.well-known/openid-configuration` (the whole reason `@hono/oidc-auth` is used). Any
|
||||
non-Authelia or differently-mounted provider will get a wrong URL. This is a latent
|
||||
correctness bug that compounds CR-01.
|
||||
**Fix:** Resolve the authorization endpoint from the discovery document rather than assuming
|
||||
`/api/oidc/authorization`.
|
||||
|
||||
### WR-03: `scryptSync` blocks the event loop on the login hot path
|
||||
|
||||
**File:** `apps/api/src/auth/localCredentials.ts:42-53, 71-90` and `localAuth.ts:127-129`
|
||||
**Issue:** `verifyPassword` always runs `scryptSync` (N=16384) synchronously, including the
|
||||
dummy-hash branch on every failed/unknown login. The file comment justifies this for a
|
||||
2-person household, but combined with the per-IP rate limiter and the always-run dummy hash,
|
||||
a burst of unauthenticated `POST /local/login` requests can pin the single Node event loop
|
||||
(each scrypt is ~tens of ms of blocking CPU) and stall *all* other API traffic — a cheap
|
||||
unauthenticated DoS. The rate limiter does not protect this because the scrypt runs *before*
|
||||
the failure counter is consulted on the dummy path for new IPs.
|
||||
**Fix:** Use `promisify(scrypt)` (async) so hashing does not block the loop, as the comment
|
||||
itself suggests. This keeps the timing-defense property while preventing loop starvation.
|
||||
|
||||
### WR-04: `authMode` / OIDC-enabled detection diverges across three files
|
||||
|
||||
**File:** `apps/api/src/routes/authMode.ts:33-49`, `apps/api/src/auth/middleware.ts:62-114`,
|
||||
`apps/api/src/routes/me.ts:323-328`
|
||||
**Issue:** Three independent notions of "is OIDC configured": `authMode` checks
|
||||
`OIDC_ISSUER` env OR `app_config.oidc_issuer`; the fallback middleware injects
|
||||
issuer/client-id/external-url from app_config; but `me.ts` link-oidc only builds a URL when
|
||||
`OIDC_ISSUER && OIDC_CLIENT_ID && OIDC_REDIRECT_URI` are all in **env** (it never consults
|
||||
app_config). So a wizard-configured-but-not-restarted instance reports `oidcEnabled:true`
|
||||
from `/api/auth/mode`, shows the "Link OIDC" button, but `link-oidc` returns
|
||||
`authorizationUrl:null` — inconsistent state surfaced to the user.
|
||||
**Fix:** Centralize the "OIDC configured" resolution (env-or-app_config) in one helper and
|
||||
use it in all three sites.
|
||||
|
||||
### WR-05: `noEchoHook` return value is ignored by `@hono/zod-validator` in one of two styles
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:39-43`, `apps/api/src/routes/admin.ts:74-78`,
|
||||
`apps/api/src/routes/me.ts:178-182`
|
||||
**Issue:** The hook signature is `(result, c)` and returns `c.json(...)` only on failure. This
|
||||
relies on zValidator short-circuiting when the hook returns a Response. That contract holds
|
||||
for current `@hono/zod-validator`, but the hook does not `return` anything on success and does
|
||||
not assert `result.success` narrows the type, so a future validator version that requires an
|
||||
explicit early-return-on-success, or that passes through when the hook returns `undefined`,
|
||||
would silently start echoing Zod errors (the exact T-19-14 leak this guards against). It is
|
||||
correct today but fragile and untested for the pass-through case.
|
||||
**Fix:** Add a focused test asserting that a malformed body never includes `received`/the
|
||||
submitted value for each hook site (localAuth has one; admin/me password routes should too),
|
||||
and pin the `@hono/zod-validator` version.
|
||||
|
||||
### WR-06: Rate-limit `lockedUntil` is refreshed on every blocked attempt, extending the window indefinitely
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:97-106, 133-137`
|
||||
**Issue:** On a 429 the code sets `attempt.lockedUntil = Date.now() + RATE_WINDOW_SECS*1000`
|
||||
again, so an attacker who keeps hitting the endpoint perpetually slides the cooldown forward —
|
||||
a legitimate user behind the same IP can never get back in even after pausing, because every
|
||||
attacker request re-arms the window. Coupled with CR-04 this makes the household-wide lock
|
||||
effectively permanent under sustained traffic.
|
||||
**Fix:** Do not extend `lockedUntil` on requests that are themselves rejected by the window;
|
||||
only set it when transitioning from below-threshold to at-threshold.
|
||||
|
||||
### WR-07: `parseInt` member/calendar id accepts trailing garbage
|
||||
|
||||
**File:** `apps/api/src/routes/admin.ts:215-218, 307-311`
|
||||
**Issue:** `parseInt(c.req.param('id'), 10)` returns `12` for `"12abc"` and the `isNaN`
|
||||
guard passes. Not exploitable here (the value is used only in a parameterized `eq`), but it
|
||||
silently accepts malformed ids and could mask client bugs. The `/members/:id/password` and
|
||||
`/calendars/:id/shared` routes both use this pattern.
|
||||
**Fix:** Validate with `Number.isInteger(Number(raw))` or a Zod param schema so `"12abc"` is
|
||||
rejected with 400.
|
||||
- Inlined PHC scrypt parameters agree across all three copies: canonical module
|
||||
(N=16384,r=8,p=1,keylen=32), `reset-admin.ts`, and `.gitea/workflows/ci.yml`
|
||||
(seed step lines 309-310). localCredentials.test.ts Test 6 pins this round-trip.
|
||||
- `oidcConfig.ts` (`resolveOidcConfig` + `discoverAuthorizationEndpoint`) is the single
|
||||
env-OR-app_config source now shared by `/api/auth/mode`, the fallback middleware, and
|
||||
`me.ts` link-oidc, closing the WR-04 divergence where link-oidc could return
|
||||
`authorizationUrl:null` while `/mode` reported `oidcEnabled:true`.
|
||||
- `LOCAL_SESSION_EXPIRES` NaN-coercion guard (localSession.ts:35-38) and boot guards
|
||||
(`assertLocalSessionSecretSet` >= 32, exempt under bypass) are correct and wired
|
||||
first in the `isMainModule()` block (index.ts:262-265).
|
||||
- Migration `0003_warm_deathstrike.sql` matches the `localCredentials` Drizzle schema
|
||||
(unique on user_id and username, FK cascade, varchar(256) hash).
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `localSession` `maxAge`/expiry parsing has no validation
|
||||
### IN-01: `fetchLinkOidc` declared return type is narrower than the cast it returns
|
||||
|
||||
**File:** `apps/api/src/auth/localSession.ts:31`
|
||||
**Issue:** `Number(process.env.LOCAL_SESSION_EXPIRES ?? 86400)` yields `NaN` for a malformed
|
||||
value, producing a JWT with `exp = now + NaN` (→ `NaN`) and a cookie `maxAge: NaN`. Verify
|
||||
behavior is then "always expired" or "never expires" depending on the JWT lib's NaN handling.
|
||||
**Fix:** Coerce and validate: `const n = Number(env); SESSION_MAX_AGE = Number.isFinite(n) && n > 0 ? n : 86400;`
|
||||
**File:** `apps/pwa/src/api/client.ts:250,261`
|
||||
**Issue:** The function signature declares `Promise<{ authorizationUrl: string | null }>`
|
||||
but the body returns `res.json() as Promise<{ signedState: string; authorizationUrl: string | null }>`.
|
||||
The widening cast is harmless (the only consumer, `SettingsSheet.tsx` `LinkOidcSheet`,
|
||||
reads `data.authorizationUrl` only and never `signedState`), and `tsc` is clean. It is a
|
||||
minor contract-doc inconsistency: the declared type drops a field the server actually
|
||||
sends. Not a defect — recorded only so the next editor does not "fix" the cast and
|
||||
accidentally start relying on the absent field.
|
||||
**Fix:** Align the declared return type with the cast for clarity:
|
||||
```ts
|
||||
export async function fetchLinkOidc(): Promise<{ signedState: string; authorizationUrl: string | null }> {
|
||||
```
|
||||
|
||||
### IN-02: Duplicated inline scrypt implementation across three locations
|
||||
### IN-02: 429 rate-limit branch short-circuits before the dummy-hash work
|
||||
|
||||
**File:** `apps/api/scripts/reset-admin.ts:45-51`, `.gitea/workflows/ci.yml:307-311`,
|
||||
`apps/api/src/auth/localCredentials.ts:42-53`
|
||||
**Issue:** The PHC scrypt hash is copy-pasted in the reset-admin script, the CI seed step,
|
||||
and the canonical module. If the parameters ever change (the file comment advertises
|
||||
parameter evolution as a feature), these three drift and produce incompatible hashes. The
|
||||
duplication is documented as necessary (cannot import compiled TS from a plain script), but
|
||||
there is no test asserting the three stay in lockstep.
|
||||
**Fix:** Add a test that imports `hashPassword` and asserts a known input round-trips against
|
||||
a hash produced by the inlined parameters, so a parameter change fails CI loudly.
|
||||
|
||||
### IN-03: `loginAttempts` map is unbounded (memory growth)
|
||||
|
||||
**File:** `apps/api/src/routes/localAuth.ts:66`
|
||||
**Issue:** Entries are only removed on a *successful* login for that IP. Spoofed/rotated
|
||||
`X-Forwarded-For` values (see CR-04) accumulate map entries with no eviction, a slow memory
|
||||
leak. Out of strict v1 perf scope, noted because it is reachable by unauthenticated input.
|
||||
**Fix:** Add periodic eviction of entries whose `lockedUntil` is far in the past.
|
||||
|
||||
### IN-04: `me.ts` link-oidc nonce is generated but never persisted/verified
|
||||
|
||||
**File:** `apps/api/src/routes/me.ts:312-320` and `apps/api/src/index.ts:60-74`
|
||||
**Issue:** The signed state carries a `nonce` "to prevent replay," but the `/callback`
|
||||
handler never records or checks the nonce — it only verifies the JWT signature and reads
|
||||
`linkUserId`. A captured state JWT is fully replayable within its 10-minute window (the
|
||||
signature stays valid), so the nonce provides no actual replay protection. This underlies
|
||||
the takeover concern in BL-03.
|
||||
**Fix:** Persist issued nonces (or a single-use jti) and reject a state whose nonce was
|
||||
already consumed, or shorten the window and bind the state to the initiating session cookie.
|
||||
**File:** `apps/api/src/routes/localAuth.ts:162-177`
|
||||
**Issue:** Once an identity is in the 429 window, the handler returns before the DB
|
||||
lookup and the always-run `verifyPassword`/dummy-hash. This is a deliberate and correct
|
||||
DoS/throughput tradeoff (a rate-limited identity should not pay scrypt cost), and it does
|
||||
NOT leak username existence because the 429 path is reached identically for valid and
|
||||
invalid usernames (the limiter is keyed on the submitted username regardless of whether a
|
||||
credential row exists). The timing-oracle defense is only required on the *credential-check*
|
||||
path, which still always runs the dummy hash. Recorded for completeness; no change needed.
|
||||
**Fix:** None required. If a future reviewer wants strict constant-time even under
|
||||
rate-limiting, the dummy-hash could be awaited before the 429 return — but that would
|
||||
re-introduce the exact event-loop-starvation cost WR-03 removed, so leaving it as-is is
|
||||
the right call.
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user