diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md new file mode 100644 index 0000000..fa3d8f8 --- /dev/null +++ b/SECURITY-AUDIT.md @@ -0,0 +1,370 @@ +# Indiko Security Audit — Findings + +Adversarial review of indiko (https://indiko.dunkirk.sh), conducted 2026-07-28. +Code audit + live instance probing. Ordered by severity. + +--- + +## Critical + +### C1. Login challenge lookup uses "latest row wins" — cross-session challenge confusion / login DoS +**File:** `src/routes/auth.ts:576-598` + +```ts +const challenge = db.query( + "SELECT challenge, expires_at FROM challenges WHERE username = ? AND type = 'authentication' ORDER BY created_at DESC LIMIT 1" +).get(challengeUsername) +``` + +The server never checks that the challenge inside the signed WebAuthn assertion corresponds to the challenge it issued *for this specific login attempt*. It just grabs the most recent challenge for the username (or empty username for conditional UI) and hands it to `verifyAuthenticationResponse` as `expectedChallenge`. + +**Attack:** +- **DoS against any user:** Attacker calls `POST /auth/login/options` with `{"username":"victim"}` in a loop. Each call inserts a new challenge row. When the victim completes their real passkey ceremony, `loginVerify` reads the attacker's newer challenge, not the victim's. Verification fails. Repeat indefinitely → victim can't log in. +- **DoS against all conditional-UI logins:** Conditional UI challenges are stored under `username = ""`. Any anonymous caller can request new conditional challenges, racing every legitimate conditional login on the site. + +**Verified live:** Two back-to-back conditional `loginOptions` calls returned different challenges; both are stored, only the newest is ever used. + +**Fix:** Generate a random `challenge_id`, return it to the client, require the client to submit it in `loginVerify`, look up by `(challenge_id, username)`. Burn the challenge on *any* verify attempt (success or failure), not just success. + +--- + +### C2. Invite consumption race — loser keeps account + session +**File:** `src/routes/auth.ts:339-351` + +```ts +const result = db.query( + "UPDATE invites SET current_uses = current_uses + 1 WHERE id = ? AND current_uses < max_uses" +).run(inviteId); + +if (result.changes === 0) { + return Response.json({ error: "Invite code fully used" }, { status: 403 }); +} +``` + +The atomic increment is good. But the user row, credential row, and session row were **already inserted** at lines 307-332, 374-379. The loser of the race walks away with: +- A registered account (username taken) +- A passkey credential +- A valid 24h session cookie + Bearer token + +For a single-use LDAP-locked invite, that's two provisioned accounts from one invite. + +**Attack:** Attacker intercepts an invite link (referer leak, shared chat log), races the legitimate invitee's registration with their own WebAuthn ceremony on the same code. Whoever loses the atomic race still has a working session. + +**Fix:** Wrap the whole registration (user insert, credential insert, session insert, invite increment) in a SQLite transaction. Roll back everything if the invite UPDATE returns 0 changes. + +--- + +### C3. Consent POST does not re-validate `redirect_uri` against registered list — attacker-minted authorization codes +**File:** `src/routes/oauth/authorize.ts:281-413` + +The GET path carefully validates `redirectUri ∈ app.redirect_uris` at line 96. The POST handler (consent form submission) re-canonicalizes the POSTed `redirect_uri` at lines 333-334 but **never checks it against `allowedRedirects`** and never re-runs `ensureApp`. + +**Attack:** +1. Attacker registers a malicious app with `redirect_uri=https://attacker.com/cb` (any URL-based client can publish any same-host redirect). +2. Victim goes through the OAuth flow with the attacker's client_id. +3. The consent page renders hidden inputs including `redirect_uri=https://attacker.com/cb` and `code_challenge=attacker-controlled`. +4. Attacker tricks the victim into submitting the form (or uses a MITM on an http:// client). +5. Server inserts an authcode bound to `client_id` + `redirect_uri=https://attacker.com/cb` and 302s the victim there with a valid authorization code. +6. Attacker exchanges the code for access + refresh tokens in the victim's name. + +PKCE doesn't help because the attacker chose the `code_challenge` and knows the verifier. + +**Mitigating factor:** The consent form has no CSRF token, and the session cookie uses `SameSite=Lax`, which blocks cross-site POST cookies in modern browsers. So pure CSRF is blunted. But a malicious client can still do this by rewriting the POST body in transit (http:// clients are allowed per `validateClientURL`), or via social engineering. + +**Fix:** In `authorizePost`, after canonicalizing `redirectUri`, look up the app and enforce `allowedRedirects.includes(redirectUri)` exactly like the GET path. Also consider binding the consent state (client_id + redirect_uri + code_challenge) with a server-side nonce to prevent tampering. + +--- + +## High + +### H1. SSRF via unauthenticated client metadata fetch — no DNS resolution validation +**File:** `src/lib/ssrf-safe-fetch.ts:207-253`, used by `src/lib/oauth/client-metadata.ts:102` + +`validateExternalURL` blocks literal private IPs and local hostnames, but a hostname like `attacker.com` resolving to `10.0.0.5` or `169.254.169.254` passes every check. There is **no DNS resolution + IP validation step**, and no post-connect validation. + +The fetch is triggered **unauthenticated**: +- `GET /auth/authorize?client_id=https://rebinding.attacker.com&redirect_uri=...` → `ensureApp` → `fetchClientMetadata` → `safeFetch` + +**Verified live:** The authorize endpoint runs `ensureApp` *before* the login check. An unauthenticated attacker can force the server to fetch any URL. + +**DNS rebinding:** Trivial. First DNS resolution (for validation) returns a public IP. Second resolution (for the actual fetch, done by Bun's HTTP client) returns the target private IP. Since validation never resolves at all, you don't even need rebinding — one A record pointing at the target works. + +**Redirect chain bypass:** The redirect re-validation at line 243-251 checks `response.url` *after* the redirect was already followed by `fetch(..., redirect: "follow")`. The target has already been fetched; only the response body usage is blocked. For cloud metadata endpoints (AWS IMDSv1), credentials are already in the TCP stream. + +**IPv6 bypass:** `isPrivateIP` handles `[::ffff:10.1.1.1]` (dotted-quad after `::ffff:`) but not hex-form mapped addresses like `[::ffff:a01:101]` — the regex at line 73 only matches dotted-quad. + +**Port blocklist:** Only blocks 22, 23, 25, 53, 110, 143, 445, 3306, 5432, 6379, 11211, 27017. Any internal service on 8080, 3000, 9090, 2375 (Docker), 8500 (Consul), etc. is reachable. + +**Attack scenario:** Unauthenticated attacker hits `/auth/authorize?client_id=http://attacker-controlled.example&redirect_uri=...` where the A record points to an internal IP reachable from the server (Proxmox, TrueNAS, LAN services). Response body isn't shown directly, but `ensureApp` error messages include fetch failure details — an oracle for host/port scanning of the internal network. + +**Fix:** +1. Resolve the hostname with `dns.resolve`, validate every returned A/AAAA against `isPrivateIP`, then connect by IP with the `Host`/TLS SNI of the original hostname (or pin resolution via a custom fetch dispatcher). +2. Use `redirect: "manual"` and validate each hop before following. +3. Fix the IPv6 parser to canonicalize hex-mapped forms before prefix checks. +4. Consider blocking all non-standard ports for metadata fetches (only allow 80/443). + +--- + +### H2. Device-flow user code brute force — no rate limiting on verification endpoint +**File:** `src/routes/oauth/device-verify.ts:172-318` + +The user code is 8 chars from a 20-char alphabet (~34.6 bits, ~2.5×10¹⁰). RFC 8628 §5.2 explicitly warns about this and demands rate limiting. There is none. + +**Attack:** +- Attacker initiates their own device flow to see the code format. +- Attacker polls `/device?code=XXXX-XXXX` in a loop. No lockout, no CAPTCHA. +- With N pending codes, expected guesses ≈ 2.5×10¹⁰ / N. Against a busy server with 1000 pending codes, ~2.5×10⁷ guesses — feasible in hours. +- Once guessed, the attacker *approves the victim's pending device code as themselves* (`devicePost` sets `user_id` to the session user). The victim's device (TV/CLI) then receives a token for the **attacker's** account. + +The phishing direction is worse: attacker initiates a device code, sends the victim `verification_uri_complete` (provided at `device.ts:90`), victim clicks and approves → attacker's polling client gets the **victim's** token. + +**Fix:** Rate-limit `/device` verification attempts per session/IP (e.g., 10 failures → 15-min lockout). Increase code entropy (or shorten TTL below 600s). Log/alert on failed lookups. + +--- + +### H3. Device-code polling `client_id` check is optional — cross-client device-code confusion +**File:** `src/routes/oauth/token.ts:186-222` + +```ts +clientId = rawClientId ? canonicalizeURL(rawClientId) : undefined; +... +if (clientId && deviceCode.client_id !== clientId) { ... } +``` + +If the caller omits `client_id` entirely, the check is skipped. Anyone holding a `device_code` can poll for it. + +**Verified live:** `POST /auth/token` with `grant_type=urn:ietf:params:oauth:grant-type:device_code&device_code=...` (no `client_id`) returns `authorization_pending`. With a mismatched `client_id`, it returns `client_id mismatch`. The oracle confirms the check is skipped when `client_id` is absent. + +RFC 8628 §3.4 requires `client_id` for public clients. The `device_code` itself is 32 random bytes so this isn't directly guessable, but it breaks the binding between the device authorization and its client. A leaked `device_code` (logs, screenshots) can be redeemed by any client. + +**Also:** The device grant path never calls `verifyClientCredentials`, so a **confidential client's** device code can be redeemed without its secret at all. + +**Fix:** Require `client_id` on the device_code grant and match it. For pre-registered (confidential) clients, also require the client secret via `verifyClientCredentials`. + +--- + +### H4. Token endpoint has zero rate limiting — brute force oracle +**File:** `src/routes/oauth/token.ts:21-57` + +No throttling anywhere on `/auth/token`. Practical consequences: + +1. `verifyClientCredentials` (lines 340-382) is an online oracle for `iks_...` secrets. nanoid(43) is ~254 bits, so brute force is infeasible, but there's also no lockout/alerting. A leaked-prefix secret can be ground against the endpoint indefinitely. The 401 vs 400 responses distinguish "unknown client" from "wrong secret" from "missing secret," aiding enumeration. +2. Authorization codes are 32 bytes — safe from guessing, but each *failed* PKCE verify costs a SHA-256 + DB hit. Unauthenticated CPU-DoS amplifier. +3. `tokenIntrospect` (lines 623-686) is unauthenticated and returns token validity + scopes + username for any presented token. Fine against 32-byte tokens, but it's an oracle that never locks out. + +**Mitigating factor:** Cloudflare in front provides some rate limiting (observed 429s during testing). But the app should not rely solely on the edge. + +**Fix:** Per-IP + per-client sliding-window rate limit on `/auth/token` (e.g., 20 req/min burst for public clients). Exponential backoff after repeated `invalid_client`. Uniform error responses (don't distinguish unknown client from wrong secret). + +--- + +### H5. Refresh-token rotation race — concurrent refresh yields two valid tokens +**File:** `src/routes/oauth/token.ts:59-170` + +The flow: SELECT token row → check `rotated === 0` → `UPDATE ... SET rotated = 1 WHERE id = ?` → INSERT new row. There is **no transaction** and the UPDATE isn't conditional (`WHERE id = ? AND rotated = 0`). + +Two concurrent requests with the same refresh token both pass the `rotated === 1` check, both issue new access+refresh tokens in the same family. This defeats the reuse-detection model (RFC 9700 §4.14.2 assumes exactly one winner) and doubles token lifetime for a racing client. An attacker holding a stolen refresh token races the legitimate client and both win, without ever tripping family revocation. + +**Fix:** Wrap in `db.transaction`, or make rotation atomic: `UPDATE tokens SET rotated = 1 WHERE id = ? AND rotated = 0` and bail with `invalid_grant` when `changes === 0` (then revoke family per your reuse policy). + +--- + +### H6. OIDC: ID token `sub` is mutable user website URL — identity instability + userinfo mismatch +**File:** `src/oidc.ts:120-138`, `src/routes/oauth/token.ts:580-617` + +- `sub = meValue` (token.ts:582) where `meValue` is `user.url` if set — the user can change `user.url` anytime via `/api/profile`, changing their `sub` at every RP. +- OIDC Core §8: `sub` MUST be locally unique and **never reassigned**. Stability matters. +- `userinfo` (userinfo.ts:55) uses the stable `/u/username` — so `sub` differs between the ID token and userinfo for the same user, violating OIDC Core §5.7 ("the sub value in ID token and userinfo MUST match"). +- An RP keying accounts on ID-token `sub` lets a user hop identities by changing their URL (including, after verification, to a URL previously owned by someone else's account). + +**Fix:** `sub = origin/u/username` always (put the `me` delegation in a custom claim or `website`). Add `at_hash` (SHA-256 left-half, base64url) since access tokens are always issued alongside. + +--- + +## Medium + +### M1. XSS via attacker-controlled `client_name` in error-page `hint` (unescaped HTML) +**File:** `src/lib/oauth/pages.ts:160`, `src/routes/oauth/authorize.ts:116` + +```ts +${opts.hint ? `

${opts.hint}

` : ""} +``` + +`opts.hint` is **not escaped** (intentional, since callers embed ``). Callers in `authorize.ts:116` build the hint with `` `${appName}` `` where `appName = app.name || clientId`. `app.name` comes from attacker-controlled client metadata (`client_name` in the fetched JSON document). + +**Attack:** Attacker registers a client with `client_name = ""` or ``, gets any user to start an authorize flow with a *mismatched* `redirect_uri` (easy — the attacker's own site initiates it), and the error page renders the stored `client_name` as raw HTML in the user's authenticated origin. + +Session cookie is `HttpOnly` so no direct theft, but the injected JS can: +- Fetch `/api/*` with Bearer tokens (if any are stored in localStorage) +- Approve devices (`/device` POST) +- Complete OAuth flows as the victim +- Modify the user's profile + +**Fix:** Escape `appName` before interpolating into `hint`, or make `hint` accept structured data and escape by default. + +--- + +### M2. No CSRF token on consent POST and device approve POST +**File:** `src/routes/oauth/authorize.ts:281`, `src/routes/oauth/device-verify.ts:258` + +Both rely solely on `SameSite=Lax`. Lax blocks cross-site POST cookies in modern browsers, but: +- Top-level GET navigation from attacker site *carries* the cookie, and `deviceGet` with `?code=` shows the confirmation. Combined with auto-`verification_uri_complete`, an attacker can frame the whole phish in one link. +- Clickjacking is blocked by `frame-ancestors 'none'` (good), but a user following an emailed `verification_uri_complete` + clicking "allow" is one click from authorizing the attacker's device. + +**Fix:** Add per-page CSRF nonces on both POSTs. Check `Sec-Fetch-Site` headers. + +--- + +### M3. `authorizeGet` auto-approve path builds redirect with raw string interpolation of `state` +**File:** `src/routes/oauth/authorize.ts:209`, `:344`, `:411` + +```ts +Response.redirect(`${redirectUri}?code=${code}&state=${state}&iss=${encodeURIComponent(origin)}`) +``` + +`state` is attacker-controlled and inserted unencoded. A state of `x&code=INJECTED` or `x#` lets the attacker append/override query params of the redirect URL. + +**Attack:** `state = legit&code=AAA` produces `...?code=REAL&state=legit&code=AAA&iss=...` — many client-side parsers take the *last* `code`, so the attacker controls which code value the client sees. With `error` appended: `state=x&error=access_denied` — spoofed error responses. + +**Fix:** Use `URLSearchParams` to build the redirect URL, or at minimum `encodeURIComponent(state)`. + +--- + +### M4. Suspended users keep passkey management — divergent session parsing +**File:** `src/routes/passkeys.ts:12-331` + +Every handler duplicates a raw `sessions` lookup (`expires_at > strftime('%s','now')`) instead of `getSessionUser`/`validateSession`. Unlike `session.ts:34`, these never check `users.status === 'active'`. + +A **suspended user can still list, add, rename, and delete passkeys** as long as an unexpired session token exists. `disableUser` deletes sessions, so the window is small. But LDAP-orphan suspension (`LDAP_ORPHAN_ACTION=suspend`) and `loginOptions`-triggered suspension do **not** delete sessions — a suspended LDAP user keeps full passkey management until token expiry, and `addPasskeyVerify` lets them plant a *new* credential persisting beyond suspension. + +Also `listPasskeys` regex-parses the cookie (`match(/indiko_session=([^;]+)/)`) differently from `getUserFromCookie` — divergent parsing is asking for edge-case auth confusion. + +**Fix:** Consolidate all session checks on `session.ts`. Add `status === 'active'` check to passkey routes. Delete sessions on suspension. + +--- + +### M5. LDAP group-vs-credentials error split → LDAP user enumeration +**File:** `src/routes/auth.ts:719-733` + +`ldapVerify` returns 401 "Invalid credentials" vs 403 "not a member of the required group". That distinction confirms *valid LDAP usernames* to unauthenticated callers. + +**Fix:** Return a single uniform error for both cases. + +--- + +### M6. Unauthenticated unbounded dynamic registration — DB DoS + phishing clients +**File:** `src/routes/oauth/register.ts:41-112` + +`POST /oauth/register` is unauthenticated with no rate limit. Comment says "rate limiting is out of scope here." + +Risks: +- Mass registration = one-line loop DoS (each call inserts a row) +- Cheap CPU exhaustion +- Attacker registers a client whose `client_name` contains HTML hoping some admin page renders it raw +- `redirect_uris` like `https://victim.com` let the attacker create a confusingly-named confidential client for phishing consent screens ("Acme Corp — sign in"), since they control name/logo freely + +**Fix:** Rate-limit by IP. Cap `redirect_uris.length`. Require https for redirect_uris (or explicitly allow http localhost only). Consider an `INITIAL_ACCESS_TOKEN` env-gate for the endpoint. + +--- + +### M7. Passkey counter regression not enforced — cloned authenticator detection gap +**File:** `src/routes/auth.ts:596-623` + +`verifyAuthenticationResponse` is called with `credential.counter` from DB, and `authenticationInfo.newCounter` is stored back (lines 617-623). But SimpleWebAuthn only *returns* the values; enforcement of counter progression is on the caller. + +If `newCounter <= storedCounter` (and storedCounter > 0), that's a cloned authenticator. The code doesn't reject it. + +**Fix:** Add `if (newCounter <= credential.counter && credential.counter !== 0) reject`. + +--- + +### M8. OIDC key IDs are millisecond timestamps — guessable/enumerable +**File:** `src/oidc.ts:31` + +```ts +const kid = `indiko-oidc-key-${Date.now()}`; +``` + +Millisecond timestamp kids are guessable/enumerable. Low impact since the JWKS endpoint is public anyway, but use a random UUID instead. + +--- + +## Low + +### L1. Duplicated `if (refreshToken)` block (dead code) +**File:** `src/routes/oauth/token.ts:571-573` + +```ts +if (refreshToken) { response.refresh_token = refreshToken; } +if (refreshToken) { response.refresh_token = refreshToken; } // duplicate +``` + +Sloppy, no vuln. + +--- + +### L2. `verifyPKCE` uses `===` string comparison, not constant-time +**File:** `src/lib/oauth/urls.ts:182-185` + +The challenge is a hash of a high-entropy verifier, so timing gives the attacker nothing useful. Low. + +--- + +### L3. `canonicalizeURL` passes non-http(s) strings through unchanged +**File:** `src/lib/oauth/urls.ts:2-15` + +`ikc_xxx` IDs are fine, but `javascript:...` survives canonicalization and only later gets rejected by `validateClientURL`. A footgun for future callers. + +--- + +### L4. `hasDotSegments` doesn't decode percent-encoded dots +**File:** `src/lib/oauth/urls.ts:83-92` + +`%2e%2e` isn't decoded, so `https://example.com/%2e%2e/x` passes. `new URL` keeps `%2e` encoded in pathname. Impact limited since downstream comparisons use canonical strings consistently. + +--- + +### L5. Introspection returns `username` — non-standard claim +**File:** `src/routes/oauth/token.ts:680` + +Leaks the local login name to any caller holding any valid token. Information disclosure, low. + +--- + +### L6. `errorPage` returns HTTP 400 for all error pages +**File:** `src/lib/oauth/pages.ts:127-129` + +Semantics only. Some errors should be 401, 403, or 500. + +--- + +## Live Instance Observations + +- **Cloudflare in front** provides rate limiting (observed 429s), DDoS protection, and some WAF coverage. The app should not rely solely on this. +- **Security headers present:** `X-Frame-Options: DENY`, `Content-Security-Policy: frame-ancestors 'none'`, `Referrer-Policy` (implicit via Caddy). Good. +- **No `Strict-Transport-Security` header observed.** Add `max-age=31536000; includeSubDomains; preload`. +- **No `X-Content-Type-Options: nosniff`.** Add it. +- **JWKS endpoint returns empty keys array** until first OIDC token is issued. This is fine functionally but may confuse RPs that pre-fetch keys. + +--- + +## Priority Fix Order + +1. **C3** — One `allowedRedirects.includes(redirectUri)` check in `authorizePost`. Small, surgical, closes a real code-issuance bypass. +2. **C1** — Key login challenges by client-returned ID. Burn on any verify attempt. Small change, kills login DoS. +3. **H1** — DNS-resolve-and-pin in `safeFetch`. Manual redirect handling. The meatiest fix but the highest impact. +4. **C2 / H5** — Transactions around invite consumption and refresh rotation. +5. **H2 / H4** — A tiny in-memory rate limiter on `/device` verify and `/auth/token`. +6. **H6** — Stable `sub` + `at_hash`. Spec compliance + identity stability. +7. **M1** — Escape `appName` in error-page hints. One-line fix. +8. **M3** — Encode `state` in success redirects. One-line fix. + +--- + +## Defense-in-Depth Recommendations + +- Add `Strict-Transport-Security` and `X-Content-Type-Options` headers globally. +- Add structured logging for security events (failed logins, invite races, token reuse detection, SSRF blocks). +- Consider a `Content-Security-Policy` beyond `frame-ancestors` for the consent/error pages (restrict script-src to self). +- Add a `Referrer-Policy: strict-origin-when-cross-origin` header. +- Consider requiring `Sec-Fetch-Site: same-origin` on sensitive POSTs (defense in depth alongside SameSite=Lax). diff --git a/src/index.ts b/src/index.ts index 33f9d04..4ab116f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -393,28 +393,57 @@ const ldapCleanupJob = ); // 7 days default const now = Math.floor(Date.now() / 1000); + // Don't take any destructive action if there were LDAP errors — + // an infrastructure failure must not trigger account deletion. + if (result.errors > 0) { + console.warn( + `[LDAP Cleanup] ${result.errors} LDAP errors encountered — skipping orphan actions this run`, + ); + return; + } + + // Mark newly orphaned users (set orphaned_since on first detection) + for (const orphan of result.orphanedUsers) { + db.query( + "UPDATE users SET orphaned_since = ? WHERE id = ? AND orphaned_since IS NULL", + ).run(now, orphan.id); + } + + // Clear orphaned_since for users found back in LDAP + for (const activeUser of result.activeUsers) { + db.query( + "UPDATE users SET orphaned_since = NULL WHERE id = ? AND orphaned_since IS NOT NULL", + ).run(activeUser.id); + } + // Only take action on accounts orphaned longer than grace period + // (measured from first orphan detection, not account creation) if (result.orphaned > 0) { - const expiredOrphans = result.orphanedUsers.filter( - (user) => now - user.createdAt > gracePeriod, - ); + const expiredOrphans = db + .query( + "SELECT id, username FROM users WHERE provisioned_via_ldap = 1 AND orphaned_since IS NOT NULL AND (? - orphaned_since) > ?", + ) + .all(now, gracePeriod) as Array<{ + id: number; + username: string; + }>; if (expiredOrphans.length > 0) { + const expiredResult = { + ...result, + orphanedUsers: expiredOrphans.map((u) => ({ + username: u.username, + id: u.id, + status: "unknown", + createdAt: 0, + })), + }; if (action === "suspend") { - await updateOrphanedAccounts( - { ...result, orphanedUsers: expiredOrphans }, - "suspend", - ); + await updateOrphanedAccounts(expiredResult, "suspend"); } else if (action === "deactivate") { - await updateOrphanedAccounts( - { ...result, orphanedUsers: expiredOrphans }, - "deactivate", - ); + await updateOrphanedAccounts(expiredResult, "deactivate"); } else if (action === "remove") { - await updateOrphanedAccounts( - { ...result, orphanedUsers: expiredOrphans }, - "remove", - ); + await updateOrphanedAccounts(expiredResult, "remove"); } console.log( `[LDAP Cleanup] ${action === "remove" ? "Removed" : action === "suspend" ? "Suspended" : "Deactivated"} ${expiredOrphans.length} LDAP orphan accounts (grace period: ${gracePeriod}s)`, diff --git a/src/ldap-cleanup.ts b/src/ldap-cleanup.ts index f6b2598..07be09c 100644 --- a/src/ldap-cleanup.ts +++ b/src/ldap-cleanup.ts @@ -19,9 +19,17 @@ interface AuditResult { status: string; createdAt: number; }>; + activeUsers: Array<{ + username: string; + id: number; + }>; } -export async function checkLdapUser(username: string): Promise { +export type LdapCheckResult = "exists" | "not_found" | "error"; + +export async function checkLdapUser( + username: string, +): Promise { try { const user = await authenticate({ ldapOpts: { @@ -34,10 +42,22 @@ export async function checkLdapUser(username: string): Promise { username: username, verifyUserExists: true, }); - return !!user; - } catch (_error) { - // User not found or invalid credentials (expected for non-existence check) - return false; + return user ? "exists" : "not_found"; + } catch (error) { + // Distinguish "user not found" from infrastructure errors (network, + // bind failure, timeout). Only an authoritative not-found should + // count as orphaned — an LDAP outage must not trigger account + // suspension/deletion. + const message = error instanceof Error ? error.message : String(error); + if ( + message.includes("not found") || + message.includes("No such object") || + message.includes("Invalid Credentials") + ) { + return "not_found"; + } + console.error(`[ldap] Error checking user ${username}:`, error); + return "error"; } } @@ -93,6 +113,7 @@ export async function getLdapAccounts(): Promise { orphaned: 0, errors: 0, orphanedUsers: [], + activeUsers: [], }; console.log(`Found ${result.total} LDAP-provisioned accounts\n`); @@ -101,28 +122,27 @@ export async function getLdapAccounts(): Promise { for (const user of ldapUsers) { process.stdout.write(`Checking ${user.username}... `); - try { - const existsInLdap = await checkLdapUser(user.username); - - if (existsInLdap) { - console.log("✅ Found in LDAP"); - result.active++; - } else { - console.log("❌ NOT FOUND in LDAP"); - result.orphaned++; - result.orphanedUsers.push({ - username: user.username, - id: user.id, - status: user.status, - createdAt: user.created_at, - }); - } - } catch (error) { - console.log("⚠️ Error checking LDAP"); + const checkResult = await checkLdapUser(user.username); + + if (checkResult === "exists") { + console.log("✅ Found in LDAP"); + result.active++; + result.activeUsers.push({ + username: user.username, + id: user.id, + }); + } else if (checkResult === "not_found") { + console.log("❌ NOT FOUND in LDAP"); + result.orphaned++; + result.orphanedUsers.push({ + username: user.username, + id: user.id, + status: user.status, + createdAt: user.created_at, + }); + } else { + console.log("⚠️ Error checking LDAP (skipping)"); result.errors++; - console.error( - ` Error: ${error instanceof Error ? error.message : String(error)}`, - ); } } diff --git a/src/lib/session.ts b/src/lib/session.ts index 7ebe5e9..edf8536 100644 --- a/src/lib/session.ts +++ b/src/lib/session.ts @@ -96,3 +96,19 @@ export function getUserFromCookie(req: Request): SessionUser | null { return validateSession(lookupSession(sessionToken)); } + +/** + * Authenticate via Bearer token or indiko_session cookie, whichever is + * present. Returns a SessionUser or a 401/403 Response. Use this for + * endpoints that serve both API clients and browser pages. + */ +export function getSessionUserFlexible(req: Request): SessionUser | Response { + const bearerResult = getSessionUser(req); + if (!(bearerResult instanceof Response)) return bearerResult; + + // Bearer auth failed — try cookie auth + const cookieUser = getUserFromCookie(req); + if (cookieUser) return cookieUser; + + return bearerResult; // return the original 401 +} diff --git a/src/migrations/011_add_orphaned_since.sql b/src/migrations/011_add_orphaned_since.sql new file mode 100644 index 0000000..182ad37 --- /dev/null +++ b/src/migrations/011_add_orphaned_since.sql @@ -0,0 +1,5 @@ +-- Track when a user was first detected as orphaned (missing from LDAP). +-- NULL means the user is not currently considered orphaned. +-- Used by the hourly cleanup job to implement a real grace period: +-- action is only taken after orphaned_since + grace_period has elapsed. +ALTER TABLE users ADD COLUMN orphaned_since INTEGER DEFAULT NULL; diff --git a/src/routes/api.ts b/src/routes/api.ts index 1647767..034c0ba 100644 --- a/src/routes/api.ts +++ b/src/routes/api.ts @@ -433,6 +433,9 @@ export function disableUser(req: Request, userId: string): Response { db.query("DELETE FROM sessions WHERE user_id = ?").run(targetUserId); + // Revoke all OAuth tokens so suspension actually cuts off API access + db.query("UPDATE tokens SET revoked = 1 WHERE user_id = ?").run(targetUserId); + return Response.json({ success: true }); } diff --git a/src/routes/auth.ts b/src/routes/auth.ts index b234453..3408c52 100644 --- a/src/routes/auth.ts +++ b/src/routes/auth.ts @@ -482,9 +482,9 @@ export async function loginOptions(req: Request): Promise { now - user.last_ldap_verified_at > checkInterval; if (shouldCheck) { - const existsInLdap = await checkLdapUser(username); - if (!existsInLdap) { - // User no longer exists in LDAP - suspend the account + const ldapResult = await checkLdapUser(username); + if (ldapResult === "not_found") { + // User authoritatively no longer exists in LDAP — suspend db.query("UPDATE users SET status = 'suspended' WHERE id = ?").run( user.id, ); @@ -493,12 +493,18 @@ export async function loginOptions(req: Request): Promise { { status: 401 }, ); } - - // Update last verification timestamp - db.query("UPDATE users SET last_ldap_verified_at = ? WHERE id = ?").run( - Math.floor(now / 1000), - user.id, - ); + if (ldapResult === "error") { + // LDAP infrastructure error — don't suspend, but also don't + // update the verification timestamp so we retry next login + console.warn( + `[auth] LDAP check failed for ${username}, skipping verification`, + ); + } else { + // User exists — update last verification timestamp + db.query( + "UPDATE users SET last_ldap_verified_at = ? WHERE id = ?", + ).run(Math.floor(now / 1000), user.id); + } } } @@ -768,14 +774,13 @@ export async function ldapVerify(req: Request): Promise { return Response.json({ error: "Invalid credentials" }, { status: 401 }); } - // Check group membership if configured + // Check group membership if configured. + // Return the same error as invalid credentials to avoid leaking + // whether the username exists in LDAP. if (userDn) { const isInGroup = await checkLdapGroupMembership(username, userDn); if (!isInGroup) { - return Response.json( - { error: "User is not a member of the required group" }, - { status: 403 }, - ); + return Response.json({ error: "Invalid credentials" }, { status: 401 }); } } diff --git a/src/routes/oauth/device-verify.ts b/src/routes/oauth/device-verify.ts index 4d156e6..8403ec1 100644 --- a/src/routes/oauth/device-verify.ts +++ b/src/routes/oauth/device-verify.ts @@ -145,6 +145,9 @@ const WINDOW_MS = 15 * 60 * 1000; // 15 minutes const failedAttempts = new Map(); function isRateLimited(userId: number): boolean { + // Skip rate limiting in tests + if (process.env.NODE_ENV === "test") return false; + const entry = failedAttempts.get(userId); if (!entry) return false; if (Date.now() - entry.firstAt > WINDOW_MS) { diff --git a/src/routes/oauth/register.ts b/src/routes/oauth/register.ts index 43c2509..52eb144 100644 --- a/src/routes/oauth/register.ts +++ b/src/routes/oauth/register.ts @@ -11,6 +11,26 @@ function generateClientSecret(): string { return `iks_${nanoid(43)}`; // indiko secret } +// Rate limiting for dynamic registration — unauthenticated endpoint +// that inserts DB rows, so cap per-IP to prevent flooding. +const REGISTER_WINDOW_MS = 60 * 1000; // 1 minute +const REGISTER_MAX = 5; // max registrations per window +const registerAttempts = new Map(); + +function isRegisterRateLimited(ip: string): boolean { + // Skip rate limiting in tests + if (process.env.NODE_ENV === "test") return false; + + const now = Date.now(); + const entry = registerAttempts.get(ip); + if (!entry || now > entry.resetAt) { + registerAttempts.set(ip, { count: 1, resetAt: now + REGISTER_WINDOW_MS }); + return false; + } + entry.count++; + return entry.count > REGISTER_MAX; +} + interface RegisterBody { redirect_uris?: unknown; client_name?: unknown; @@ -32,7 +52,15 @@ function asStringArray(value: unknown): string[] | null { function isValidRedirectUri(uri: string): boolean { try { const url = new URL(uri); - return url.protocol === "https:" || url.protocol === "http:"; + // Allow http only for loopback (localhost dev) + if (url.protocol === "http:") { + return ( + url.hostname === "localhost" || + url.hostname === "127.0.0.1" || + url.hostname === "[::1]" + ); + } + return url.protocol === "https:"; } catch { return false; } @@ -40,8 +68,21 @@ function isValidRedirectUri(uri: string): boolean { // POST /oauth/register — RFC 7591 Dynamic Client Registration. // Registers a confidential client (opaque client_id + client_secret). -// No auth required per RFC 7591; rate limiting is out of scope here. +// Rate-limited per IP to prevent DB flooding. export async function registerClient(req: Request): Promise { + const clientIp = + req.headers.get("cf-connecting-ip") || + req.headers.get("x-forwarded-for")?.split(",")[0].trim() || + "unknown"; + + if (isRegisterRateLimited(clientIp)) { + return oauthError( + 429, + "invalid_client_metadata", + "Too many registration requests. Please try again later.", + ); + } + let body: RegisterBody; try { body = (await req.json()) as RegisterBody; @@ -58,6 +99,14 @@ export async function registerClient(req: Request): Promise { ); } + if (redirectUris.length > 10) { + return oauthError( + 400, + "invalid_redirect_uri", + "redirect_uris must contain at most 10 URIs", + ); + } + for (const uri of redirectUris) { if (!isValidRedirectUri(uri)) { return oauthError( diff --git a/src/routes/oauth/token.ts b/src/routes/oauth/token.ts index 640f6e1..100944f 100644 --- a/src/routes/oauth/token.ts +++ b/src/routes/oauth/token.ts @@ -13,12 +13,44 @@ import { signIDToken } from "../../oidc"; const ACCESS_TOKEN_TTL = 3600; // 1 hour const REFRESH_TOKEN_TTL = 2592000; // 30 days +// Rate limiting for the token endpoint — per-IP sliding window. +const TOKEN_WINDOW_MS = 60 * 1000; // 1 minute +const TOKEN_MAX = 30; // max requests per window per IP +const tokenAttempts = new Map(); + +function isTokenRateLimited(ip: string): boolean { + // Skip rate limiting in tests + if (process.env.NODE_ENV === "test") return false; + + const now = Date.now(); + const entry = tokenAttempts.get(ip); + if (!entry || now > entry.resetAt) { + tokenAttempts.set(ip, { count: 1, resetAt: now + TOKEN_WINDOW_MS }); + return false; + } + entry.count++; + return entry.count > TOKEN_MAX; +} + function generateToken(): string { return crypto.randomBytes(32).toString("base64url"); } // POST /auth/token - Exchange authorization code or refresh token export async function token(req: Request): Promise { + const clientIp = + req.headers.get("cf-connecting-ip") || + req.headers.get("x-forwarded-for")?.split(",")[0].trim() || + "unknown"; + + if (isTokenRateLimited(clientIp)) { + return oauthError( + 429, + "invalid_request", + "Too many requests. Please try again later.", + ); + } + try { const body = await parseBody(req); if (!body) { diff --git a/src/routes/oauth/userinfo.ts b/src/routes/oauth/userinfo.ts index 7659832..b79e279 100644 --- a/src/routes/oauth/userinfo.ts +++ b/src/routes/oauth/userinfo.ts @@ -18,7 +18,7 @@ export function userinfo(req: Request): Response { const tokenData = db .query( - "SELECT t.user_id, t.scope, t.expires_at, t.revoked, u.name, u.email, u.photo, u.url, u.username FROM tokens t JOIN users u ON t.user_id = u.id WHERE t.token = ?", + "SELECT t.user_id, t.scope, t.expires_at, t.revoked, u.name, u.email, u.photo, u.url, u.username, u.status FROM tokens t JOIN users u ON t.user_id = u.id WHERE t.token = ?", ) .get(tokenValue) as | { @@ -31,6 +31,7 @@ export function userinfo(req: Request): Response { photo: string | null; url: string | null; username: string; + status: string; } | undefined; @@ -41,6 +42,15 @@ export function userinfo(req: Request): Response { ); } + // Defense in depth: reject tokens for suspended users even if the + // token row itself wasn't revoked + if (tokenData.status !== "active") { + return unauthorizedResponse( + "invalid_token", + "Invalid or revoked access token", + ); + } + const now = Math.floor(Date.now() / 1000); if (tokenData.expires_at < now) { return unauthorizedResponse("invalid_token", "Access token expired"); diff --git a/src/routes/passkeys.ts b/src/routes/passkeys.ts index e43d3b1..1cb19d4 100644 --- a/src/routes/passkeys.ts +++ b/src/routes/passkeys.ts @@ -5,34 +5,20 @@ import { verifyRegistrationResponse, } from "@simplewebauthn/server"; import { db } from "../db"; +import { getSessionUserFlexible } from "../lib/session"; const RP_NAME = "Indiko"; // Get all passkeys for current user export function listPasskeys(req: Request): Response { - const sessionToken = - req.headers.get("Authorization")?.replace("Bearer ", "") || - req.headers.get("Cookie")?.match(/indiko_session=([^;]+)/)?.[1]; - - if (!sessionToken) { - return Response.json({ error: "Unauthorized" }, { status: 401 }); - } - - const session = db - .query( - "SELECT user_id, expires_at FROM sessions WHERE token = ? AND expires_at > strftime('%s', 'now')", - ) - .get(sessionToken) as { user_id: number; expires_at: number } | undefined; - - if (!session) { - return Response.json({ error: "Invalid session" }, { status: 401 }); - } + const user = getSessionUserFlexible(req); + if (user instanceof Response) return user; const passkeys = db .query( "SELECT id, name, created_at FROM credentials WHERE user_id = ? ORDER BY created_at DESC", ) - .all(session.user_id) as Array<{ + .all(user.userId) as Array<{ id: number; name: string; created_at: number; @@ -43,36 +29,13 @@ export function listPasskeys(req: Request): Response { // Generate options for adding a new passkey export async function addPasskeyOptions(req: Request): Promise { - const sessionToken = - req.headers.get("Authorization")?.replace("Bearer ", "") || - req.headers.get("Cookie")?.match(/indiko_session=([^;]+)/)?.[1]; - - if (!sessionToken) { - return Response.json({ error: "Unauthorized" }, { status: 401 }); - } - - const session = db - .query( - "SELECT user_id, expires_at FROM sessions WHERE token = ? AND expires_at > strftime('%s', 'now')", - ) - .get(sessionToken) as { user_id: number; expires_at: number } | undefined; - - if (!session) { - return Response.json({ error: "Invalid session" }, { status: 401 }); - } - - const user = db - .query("SELECT username FROM users WHERE id = ?") - .get(session.user_id) as { username: string } | undefined; - - if (!user) { - return Response.json({ error: "User not found" }, { status: 404 }); - } + const user = getSessionUserFlexible(req); + if (user instanceof Response) return user; // Get existing credentials to exclude them const existingCredentials = db .query("SELECT credential_id FROM credentials WHERE user_id = ?") - .all(session.user_id) as Array<{ credential_id: Buffer }>; + .all(user.userId) as Array<{ credential_id: Buffer }>; const excludeCredentials = existingCredentials.map((cred) => ({ id: Buffer.from(cred.credential_id).toString("base64url"), @@ -106,31 +69,8 @@ export async function addPasskeyOptions(req: Request): Promise { // Verify and add new passkey export async function addPasskeyVerify(req: Request): Promise { try { - const sessionToken = - req.headers.get("Authorization")?.replace("Bearer ", "") || - req.headers.get("Cookie")?.match(/indiko_session=([^;]+)/)?.[1]; - - if (!sessionToken) { - return Response.json({ error: "Unauthorized" }, { status: 401 }); - } - - const session = db - .query( - "SELECT user_id, expires_at FROM sessions WHERE token = ? AND expires_at > strftime('%s', 'now')", - ) - .get(sessionToken) as { user_id: number; expires_at: number } | undefined; - - if (!session) { - return Response.json({ error: "Invalid session" }, { status: 401 }); - } - - const user = db - .query("SELECT username FROM users WHERE id = ?") - .get(session.user_id) as { username: string } | undefined; - - if (!user) { - return Response.json({ error: "User not found" }, { status: 404 }); - } + const user = getSessionUserFlexible(req); + if (user instanceof Response) return user; const body = await req.json(); const { @@ -165,6 +105,11 @@ export async function addPasskeyVerify(req: Request): Promise { return Response.json({ error: "Challenge expired" }, { status: 400 }); } + // Burn the challenge immediately — single use regardless of outcome + db.query("DELETE FROM challenges WHERE challenge = ?").run( + challenge.challenge, + ); + // Verify WebAuthn response let verification: VerifiedRegistrationResponse; try { @@ -188,7 +133,7 @@ export async function addPasskeyVerify(req: Request): Promise { // Generate default name if not provided const passkeyCount = db .query("SELECT COUNT(*) as count FROM credentials WHERE user_id = ?") - .get(session.user_id) as { count: number }; + .get(user.userId) as { count: number }; const passkeyName = name || `Passkey ${passkeyCount.count + 1}`; @@ -198,18 +143,13 @@ export async function addPasskeyVerify(req: Request): Promise { "INSERT INTO credentials (user_id, credential_id, public_key, counter, name) VALUES (?, ?, ?, ?, ?) RETURNING id", ) .get( - session.user_id, + user.userId, Buffer.from(credential.id), Buffer.from(credential.publicKey), credential.counter, passkeyName, ) as { id: number }; - // Delete challenge - db.query("DELETE FROM challenges WHERE challenge = ?").run( - challenge.challenge, - ); - return Response.json({ success: true, passkey: { @@ -226,23 +166,8 @@ export async function addPasskeyVerify(req: Request): Promise { // Delete a passkey export function deletePasskey(req: Request): Response { - const sessionToken = - req.headers.get("Authorization")?.replace("Bearer ", "") || - req.headers.get("Cookie")?.match(/indiko_session=([^;]+)/)?.[1]; - - if (!sessionToken) { - return Response.json({ error: "Unauthorized" }, { status: 401 }); - } - - const session = db - .query( - "SELECT user_id, expires_at FROM sessions WHERE token = ? AND expires_at > strftime('%s', 'now')", - ) - .get(sessionToken) as { user_id: number; expires_at: number } | undefined; - - if (!session) { - return Response.json({ error: "Invalid session" }, { status: 401 }); - } + const user = getSessionUserFlexible(req); + if (user instanceof Response) return user; const url = new URL(req.url); const passkeyId = url.pathname.split("/").pop(); @@ -256,14 +181,14 @@ export function deletePasskey(req: Request): Response { .query("SELECT user_id FROM credentials WHERE id = ?") .get(Number(passkeyId)) as { user_id: number } | undefined; - if (!passkey || passkey.user_id !== session.user_id) { + if (!passkey || passkey.user_id !== user.userId) { return Response.json({ error: "Passkey not found" }, { status: 404 }); } // Check if this is the last passkey const passkeyCount = db .query("SELECT COUNT(*) as count FROM credentials WHERE user_id = ?") - .get(session.user_id) as { count: number }; + .get(user.userId) as { count: number }; if (passkeyCount.count <= 1) { return Response.json( @@ -280,23 +205,8 @@ export function deletePasskey(req: Request): Response { // Rename a passkey export async function renamePasskey(req: Request): Promise { - const sessionToken = - req.headers.get("Authorization")?.replace("Bearer ", "") || - req.headers.get("Cookie")?.match(/indiko_session=([^;]+)/)?.[1]; - - if (!sessionToken) { - return Response.json({ error: "Unauthorized" }, { status: 401 }); - } - - const session = db - .query( - "SELECT user_id, expires_at FROM sessions WHERE token = ? AND expires_at > strftime('%s', 'now')", - ) - .get(sessionToken) as { user_id: number; expires_at: number } | undefined; - - if (!session) { - return Response.json({ error: "Invalid session" }, { status: 401 }); - } + const user = getSessionUserFlexible(req); + if (user instanceof Response) return user; const url = new URL(req.url); const passkeyId = url.pathname.split("/").pop(); @@ -317,7 +227,7 @@ export async function renamePasskey(req: Request): Promise { .query("SELECT user_id FROM credentials WHERE id = ?") .get(Number(passkeyId)) as { user_id: number } | undefined; - if (!passkey || passkey.user_id !== session.user_id) { + if (!passkey || passkey.user_id !== user.userId) { return Response.json({ error: "Passkey not found" }, { status: 404 }); } diff --git a/test/preload.ts b/test/preload.ts index 768d461..86b80cb 100644 --- a/test/preload.ts +++ b/test/preload.ts @@ -6,3 +6,4 @@ // have opened the real data/indiko.db. A preload runs first, guaranteeing // src/db sees :memory: on its very first evaluation. process.env.DATABASE_URL = ":memory:"; +process.env.NODE_ENV = "test";