k1nq/TREK
1
0
Fork 0
mirror of https://github.com/mauriceboe/TREK.git synced 2026-06-21 06:11:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

security: internal audit — batch 1

Browse Source 
Fixes the critical + high + medium findings from our internal security
review. Bundled into one PR because the changes overlap heavily (JWT
verification unifies across three call sites; backup-code hashing and
demo-email handling cross-cut several services); splitting them out
would mean redundant reviews of the same files.

Critical
- CI-C1 — .github/workflows/test.yml: restore actions/{checkout,setup-
  node,upload-artifact} to @v4. The @v6 refs don't exist, so the test
  workflow was errorring before a single test ran.
- SEC-C1 — mfaPolicy now extracts the token via extractToken() (cookie-
  first, Bearer fallback). Previously it only read Authorization, so
  every cookie-authenticated SPA session bypassed require_mfa entirely.
- SEC-C2/C4/C6 — all JWT verification paths (MCP bearer, file download,
  photo route) now go through the shared verifyJwtAndLoadUser that
  checks password_version. resetPassword additionally deletes every
  mcp_tokens row and marks outstanding oauth_tokens revoked, so a
  password reset invalidates ALL credential classes — not just the
  cookie JWT.

High
- SEC-H2 — reset email URL is built from server-side APP_URL /
  ALLOWED_ORIGINS (via existing getAppUrl()), not request headers.
  Closes the host-header-injection vector into reset links.
- SEC-H3 — OIDC findOrCreateUser wraps the invite-redemption UPDATE +
  user INSERT in a transaction. The UPDATE is the capacity check; if
  a concurrent callback takes the last slot, the whole transaction
  aborts with registration_disabled instead of double-creating users.
- SEC-H4 — new verifyIdToken() performs full JWT signature
  verification via the provider's JWKS (Node's crypto.createPublicKey
  accepts JWK directly — no extra dependency), plus iss/aud/exp
  checks. The callback also rejects the login when userinfo.sub does
  not match id_token.sub.
- SEC-H5 — OAuth DCR now validates redirect_uris against an allowlist
  of schemes: https, http-loopback, or a private custom scheme. Plain
  http://non-loopback is rejected.
- SEC-H6 — oauthService audience defaults to mcpResource when the
  `resource` parameter is missing, so tokens are always audience-bound
  to /mcp instead of being issued with audience=null.
- SEC-H7 — HSTS is enabled any time NODE_ENV=production (previously
  required FORCE_HTTPS=true), includeSubDomains defaults on and can
  be disabled with HSTS_INCLUDE_SUBDOMAINS=false.
- SEC-H8 — trek_session cookie Secure flag is also driven by
  req.secure (which Express resolves from X-Forwarded-Proto once
  trust proxy is set), so instances behind a TLS-terminating proxy
  get Secure cookies without needing FORCE_HTTPS.

Medium
- SEC-M1 — permanentDeleteFile / emptyTrash / avatar unlink now use
  fs.promises.rm with { force: true } (one async op vs the previous
  existsSync + unlinkSync pair per file).
- SEC-M2 — invalidatePermissionsCache() is called inside restoreFromZip
  so a restored DB with different permission rows is honoured
  immediately.
- SEC-M3 + C1 — idempotency store bounds the key at 128 chars, caches
  only responses ≤ 256 KiB, and scopes the lookup by (key, user_id,
  method, path) rather than (key, user_id). Same key replayed against
  a different endpoint no longer returns a stale unrelated body.
- SEC-M4 — share_tokens gets an expires_at column; new tokens default
  to 90-day TTL, expired tokens are denied at lookup. Existing tokens
  stay NULL = no expiry so already-published links don't break.
- SEC-M5 — /uploads/photos/:filename now resolves the photo to its
  trip_id and requires the share token to cover THAT trip. Previously
  any share token for any trip would unlock any photo filename.
- SEC-M6 — BLOCKED_EXTENSIONS is the single source of truth shared
  between fileService and collab uploads. The '*' allowed_file_types
  wildcard now still rejects executables/scripts.
- SEC-M7 — single DEMO_EMAILS constant (services/demo.ts) used by
  demoUploadBlock, mfaPolicy, and every demo-mode guard in
  authService. The old demoUploadBlock only matched 'demo@nomad.app'
  so the seed 'demo@trek.app' could in fact upload in demo mode.
- SEC-M8 — MFA backup codes are now bcrypt-hashed at rest
  (hashBackupCodeBcrypt). matchBackupCode accepts both bcrypt and
  legacy SHA-256 hex hashes, so existing installs keep working until
  the user regenerates codes via enableMfa.
- SEC-M9 — document the "security via UUID v4 filename" model for
  /uploads/avatars|covers|journey. Requires no code change but
  captures the decision so future reviewers don't re-flag it.
- SEC-M10 — already covered by the resetPassword revocation logic
  above: mcp_tokens DELETE + oauth_tokens UPDATE … SET revoked_at.

Performance
- PERF-H1 — new migration adds the indexes flagged in the audit:
  trips(user_id), trips(created_at DESC), photos(day_id),
  photos(place_id), reservations(day_id), share_tokens(token), plus
  conditional day_accommodations and notifications indexes depending
  on which columns are present.

Tests
- tests/integration/oidc.test.ts now mocks verifyIdToken and passes
  an id_token in the exchangeCodeForToken stub for the three flows
  that exercise a successful callback. The three remaining failures
  tests pointed out were all pre-existing (file-upload flakes +
  notificationPreferences event_types count drift), none introduced
  by this PR.
This commit is contained in:
Maurice
2026-04-20 20:36:52 +02:00
parent e612de9143
commit 2d0414b4a3
20 changed files with 539 additions and 127 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/services/oidcService.ts
+118 -13
View File
@@ -14,6 +14,8 @@ export interface OidcDiscoveryDoc {
authorization_endpoint: string;
token_endpoint: string;
userinfo_endpoint: string;
jwks_uri?: string;
issuer?: string;
_issuer?: string;
}
@@ -221,6 +223,96 @@ export async function getUserInfo(userinfoEndpoint: string, accessToken: string)
return (await res.json()) as OidcUserInfo;
}
// ---------------------------------------------------------------------------
// id_token verification (signature + iss + aud + exp)
// ---------------------------------------------------------------------------
// 5 minute JWKS cache — short enough to pick up key rotation within a
// reasonable window, long enough that normal login flow doesn't fetch
// JWKS on every callback.
const JWKS_TTL_MS = 5 * 60 * 1000;
type JwksEntry = { keys: Array<Record<string, unknown>>; fetchedAt: number };
const jwksCache = new Map<string, JwksEntry>();
async function fetchJwks(jwksUri: string): Promise<Array<Record<string, unknown>>> {
const cached = jwksCache.get(jwksUri);
if (cached && Date.now() - cached.fetchedAt < JWKS_TTL_MS) return cached.keys;
const res = await fetch(jwksUri);
if (!res.ok) throw new Error(`JWKS fetch failed: HTTP ${res.status}`);
const json = (await res.json()) as { keys?: Array<Record<string, unknown>> };
const keys = json.keys ?? [];
jwksCache.set(jwksUri, { keys, fetchedAt: Date.now() });
return keys;
}
function base64UrlDecode(input: string): Buffer {
const padded = input.replace(/-/g, '+').replace(/_/g, '/') + '='.repeat((4 - (input.length % 4)) % 4);
return Buffer.from(padded, 'base64');
}
/**
* Verify an OIDC id_token end-to-end: signature against the provider's
* JWKS, issuer match, audience match, and exp/nbf. Does NOT verify a
* nonce — the server doesn't currently send one in the auth request;
* when that's added, pass the expected nonce here and check `claims.nonce`.
*
* Returning the claims lets callers cross-check `sub` / `email` against
* the userinfo response. A mismatch would mean the provider's userinfo
* endpoint is speaking for a different subject than the id_token — a
* classic IdP-side compromise signal worth refusing login over.
*/
export async function verifyIdToken(
idToken: string,
doc: OidcDiscoveryDoc,
clientId: string,
expectedIssuer: string,
): Promise<{ ok: true; claims: Record<string, unknown> } | { ok: false; error: string }> {
if (!doc.jwks_uri) return { ok: false, error: 'no_jwks_uri' };
const parts = idToken.split('.');
if (parts.length !== 3) return { ok: false, error: 'malformed_token' };
let header: { kid?: string; alg?: string };
try { header = JSON.parse(base64UrlDecode(parts[0]!).toString('utf8')); }
catch { return { ok: false, error: 'bad_header' }; }
const alg = header.alg;
if (!alg || !/^(RS256|RS384|RS512|ES256|ES384|ES512|PS256|PS384|PS512)$/.test(alg)) {
return { ok: false, error: 'unsupported_alg' };
}
let keys: Array<Record<string, unknown>>;
try { keys = await fetchJwks(doc.jwks_uri); }
catch (e) { return { ok: false, error: 'jwks_fetch_failed' }; }
const jwk = keys.find(k => !header.kid || k['kid'] === header.kid) ?? keys[0];
if (!jwk) return { ok: false, error: 'no_matching_key' };
let publicKey;
try {
// Node 16+ understands JWK directly; no PEM conversion library needed.
// Node's crypto accepts a JWK object directly as `{ key, format: 'jwk' }`.
// The type signature isn't strict on our TS config so we cast through any.
publicKey = crypto.createPublicKey({ key: jwk as any, format: 'jwk' });
} catch {
return { ok: false, error: 'key_import_failed' };
}
let claims: Record<string, unknown>;
try {
const verified = jwt.verify(idToken, publicKey, {
algorithms: [alg as jwt.Algorithm],
issuer: expectedIssuer,
audience: clientId,
});
claims = typeof verified === 'string' ? {} : (verified as Record<string, unknown>);
} catch (err) {
const msg = err instanceof Error ? err.message : 'verify_failed';
return { ok: false, error: `signature_or_claim_mismatch: ${msg}` };
}
return { ok: true, claims };
}
// ---------------------------------------------------------------------------
// Find or create user by OIDC sub / email
// ---------------------------------------------------------------------------
@@ -286,21 +378,34 @@ export function findOrCreateUser(
const existing = db.prepare('SELECT id FROM users WHERE LOWER(username) = LOWER(?)').get(username);
if (existing) username = `${username}_${Date.now() % 10000}`;
const result = db.prepare(
'INSERT INTO users (username, email, password_hash, role, oidc_sub, oidc_issuer, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, ?, ?, 0)',
).run(username, email, hash, role, sub, config.issuer, process.env.APP_VERSION || '0.0.0');
if (validInvite) {
const updated = db.prepare(
'UPDATE invite_tokens SET used_count = used_count + 1 WHERE id = ? AND (max_uses = 0 OR used_count < max_uses)',
).run(validInvite.id);
if (updated.changes === 0) {
console.warn(`[OIDC] Invite token ${inviteToken?.slice(0, 8)}... exceeded max_uses (race condition)`);
// Atomic registration: if an invite was presented, the increment IS
// the capacity check — UPDATE matches zero rows the moment another
// concurrent callback wins the last slot, and the transaction aborts
// the user INSERT. Without this, two parallel OIDC callbacks could
// both pass the earlier SELECT-based check and each create a user.
const inviteRaceError = new Error('invite_exhausted');
try {
const createUser = db.transaction(() => {
if (validInvite) {
const updated = db.prepare(
'UPDATE invite_tokens SET used_count = used_count + 1 WHERE id = ? AND (max_uses = 0 OR used_count < max_uses)',
).run(validInvite.id);
if (updated.changes === 0) throw inviteRaceError;
}
return db.prepare(
'INSERT INTO users (username, email, password_hash, role, oidc_sub, oidc_issuer, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, ?, ?, 0)',
).run(username, email, hash, role, sub, config.issuer, process.env.APP_VERSION || '0.0.0');
});
const result = createUser() as { lastInsertRowid: number | bigint };
user = { id: Number(result.lastInsertRowid), username, email, role } as User;
return { user };
} catch (err) {
if (err === inviteRaceError) {
console.warn(`[OIDC] Invite token ${inviteToken?.slice(0, 8)}... exhausted — concurrent callback won the last slot`);
return { error: 'registration_disabled' };
}
throw err;
}
user = { id: Number(result.lastInsertRowid), username, email, role } as User;
return { user };
}
// ---------------------------------------------------------------------------