Self Serve Security Review

Self Serve Security Review

LFX One is the authenticated front door for every persona. It holds the user's OIDC session and brokers each authenticated business request to the microservice mesh with that user's identity and bearer token. It also exposes a deliberately public surface — the data-bearing /meetings/ join pages and /public/api, plus /docs, health, and a few non-data utility routes — reachable without a user session, where handlers use M2M credentials for the upstream calls that need them (optional-auth routes may still carry a user token). auth.middleware.ts is the authoritative inventory for routes that reach the auth middleware (the OIDC login/logout/callback routes mount earlier in server.ts), so verify a route's class there before calling a public route a regression. So the failure modes that matter here are a cross-user or cross-persona authorization bypass, a session or token leak, member PII exposure, and anything that lets an anonymous caller reach more than the public surface intends. Those facts set the stakes for every security judgment.

Methodology

Run a focused, diff-aware review, not a whole-repo audit:

1. Only new risk. Assess what this PR introduces or weakens. Do not relitigate pre-existing issues the diff does not touch.
2. Assume hostile input, report only what is real. Flag only high-confidence, concretely exploitable findings: if you cannot trace a path from an attacker-controlled input (a request param, body, header, cookie, the returnTo URL, user- or project-supplied content) to a sensitive sink, it is not a reportable security finding.
3. Three passes.
   • Context: discover, from the code and the repo docs at review time, the guards this application relies on around the diff (the route's auth class, the effective-identity helpers, validateAndSanitizeUrl / fetchSafeUrl, the logger's redaction, Angular's default sanitizer, constantTimeEquals). Never assume a guard exists; find it.
   • Comparative: does the change deviate from the guard patterns the surrounding code establishes? docs/reviews/knowledge-base/security.md records the patterns this repo has been bitten by — use it as a checklist of known shapes, not a substitute for tracing the actual path.
   • Assessment: trace each input to its sink and confirm a guard sits on the path the data actually takes, not three functions away.
4. Confidence-gate every finding (1-10, report only >= 8, matching the reviewer skill's >=80% gate). A few real findings beat a speculative list.
5. Evidence, not vibes. Each finding names the file and function, what the attacker controls, the boundary crossed, the concrete impact, and the fix.

Per-fact data-exposure pass

When the diff adds or changes a field on a response payload or a rendered view, or adds a new read or write path, run this structured pass on top of the methodology above:

1. Fact inventory. For every field the diff adds or changes on a payload or view, record its grain (per-person, per-object, aggregate), whose data it is, and whether it is PII (name, email, identity, address, financial, or otherwise sensitive).
2. Gate of record, per fact. For each protected fact, find which code path actually enforces access — the route's class in auth.middleware.ts, requireExecutiveDirector, the writer flag, an in-app ownership check, or an access check enforced upstream behind the proxy — not what the PR body claims. If no code enforces it, say so.
3. Sibling-path parity. Find the equivalent read or write path elsewhere in the repo for the same or an analogous entity (the list endpoint vs. the single-get, the live handler vs. a new archived/past-object handler, the authenticated page vs. its public twin) and compare enforcement level path-by-path. A newer, less-traveled path that is weaker than its sibling — same data, lower gate — is the single highest-value finding this pass exists to catch.
4. Verdict per fact. Enforced and matching sibling parity; a gap (no enforcement, or weaker than a sibling path serving the same data); or unverifiable here because enforcement lives in an upstream microservice you cannot read — name the service and report the guard as asserted, not confirmed.

Skip this pass only when the diff adds no field to a payload or view and no new or changed read/write path (a pure refactor, style-only, or build-tooling change).

Durable threat anchors

These are the kinds of boundaries that make a diff security-relevant in this application. They describe its shape, not its current line-level guards; verify the concrete mechanism in the code each time.

• Secrets in the diff. Grep the diff itself for hardcoded credentials — API keys, bearer or M2M tokens, passwords, connection strings, private keys — including in test fixtures, e2e configs, and workflow files. A committed secret is always a finding, even when the code path that reads it is dead.
• User token vs M2M token. The single most important rule here, documented in .claude/rules/development-rules.md. Endpoints must act with the authenticated user's bearer token; an M2M token is the application's identity and erases user identity, per-user authorization, and the audit trail. M2M is legitimate when an upstream call genuinely needs application credentials and the user's token/context is preserved and restored around it: on the public surface where there is no session, or as a scoped sub-call from an authenticated route — including an optional-auth handler that still carries a user token for its user-specific work. What matters is what the upstream call is authorized to do and that the user token is preserved, not the mere absence of a session. Flag: an M2M token on a new or existing /api route to do normal work, M2M used to skip a per-user authorization check, an M2M call whose scope is wider than the one upstream request that needs it, or a privileged call that never restores the user context.
• The route classification (selective auth). auth.middleware.ts maps each route to public / optional / required (and whether a token is required). This mapping is the public-vs-protected boundary. Flag: a new route that lands on the catch-all with the wrong class, a route moved from required to optional/public, a pattern that is unanchored or ordered so a protected path matches a more permissive rule first (fail-open), a new unauthenticated route outside the documented public surface, or a /public/api endpoint from which a private record can reach the anonymous caller — those are top-scale. A new /public/api endpoint that keeps private records out of the anonymous response is deliberate public surface: scrutinize it with the per-fact data-exposure pass rather than flagging it for existing.
• Identity and impersonation. Whenever the code means the acting subject, identity must come from the effective-identity helpers (getEffectiveEmail, getEffectiveUsername, …), not from req.oidc.user.* directly: impersonation lets an admin/ED act as another user, and reading the raw OIDC identity there bypasses that context and mis-attributes the action. A deliberate read of the real session actor is legitimate where the actor is the point — the impersonation machinery recording who the impersonator is (impersonation.service.ts), or an audit of the real caller. Flag a raw read used as the effective subject, or any path that lets the impersonation session be set or widened without the established check.
• Persona-based authorization is server-verified. The persona cookie is unsigned and client-spoofable; it seeds the UI only. A real access decision (ED-only routes, writer/edit permission) must be verified server-side (requireExecutiveDirector via persona detection, the writer flag from upstream FGA). Trusting the cookie or a client-side guard for an actual authorization gate is a bypass; client guards are UX, not security.
• Sessions and tokens. The OIDC session, the refresh and audience-scoped token exchanges (exchangeRefreshTokenForAudience, the API-gateway and crowdfunding tokens), and the M2M token cache. Flag a token minted for the wrong audience, a bearer or refresh token written to a log/response/error, a token forwarded to an endpoint it was not scoped for, or a weakened session/refresh check.
• Errors and information disclosure. Errors reach the client through the custom error classes' controlled toResponse(); stack traces are emitted only in dev/debug (error-serializer.ts). Flag a new error path that returns a stack trace, an internal/upstream detail, or a filesystem path to a caller not entitled to it, or an identity signal (e.g. a differentiated "user exists" response) to any caller not authorized to learn that result — the enumeration risk is not limited to anonymous callers.
• URLs, redirects, and SSRF. These guards are sink-specific — match the finding to the right one. A server-side redirect target (e.g. the returnTo URL) passes validateAndSanitizeUrl; a server-side fetch of a user-controlled URL goes through the SSRF-safe fetch helper (fetchSafeUrl); a user-supplied link normalized for client rendering uses the shared normalizeToUrl — do not demand the server redirect helper on Angular link handling. Confirm each guard's actual behavior at its call site rather than assuming a specific mechanism. Flag a server redirect built from untrusted input without validateAndSanitizeUrl, a fetch/HTTP call to a user-controlled host that bypasses fetchSafeUrl, a non-http(s) scheme reaching a sink, or a missing encodeURIComponent on a value interpolated into a URL.
• Client-side XSS. Angular's default sanitizer strips <script> from [innerHTML], so the real risk is DomSanitizer.bypassSecurityTrustHtml (or bypassSecurityTrustResourceUrl) applied to user- or project-supplied content, or such content flowing into a code path where a bypass already lives. Flag those; flag window.open on an untrusted URL without noopener (it needs the explicit feature), or an anchor that opts back in with rel="opener" — but a plain target="_blank" link is implicitly noopener in modern browsers, so a missing rel on an anchor is not itself a finding. Prefer text interpolation or a sanitizing pipe.
• PII and logging. Recipient/member emails and names are PII. The Pino logger redacts configured paths and LoggerService offers sanitization; flag a new log line or error that emits a raw email/name/token, PII metadata logged without sanitization, or a response that exposes PII to a caller not authorized for that fact — the per-fact pass above decides that; an authorized caller legitimately receiving a member's name, or their own profile, is not a leak. Logging non-PII identifiers and URLs is fine.
• Secrets across the SSR boundary. Server-only secrets and config (API keys, client secrets, M2M credentials) must never cross into the client bundle — through a provider, a TransferState payload, an Angular environment, or runtime config that ships to the browser. Only deliberately public runtime config may reach the client. Flag a server secret newly exposed to the client.
• Public-data visibility. A /public/api endpoint returning meeting/event or project data must keep private records out of the anonymous response. When a public endpoint exposes paginated results to the caller, that means filtering to public visibility before the page bound, so a private record cannot occupy a page slot; an endpoint that fetches all upstream pages and filters before emitting a single response (e.g. the public calendar ICS feeds) satisfies it by filtering before the response. Flag a public read where a private record can actually reach the caller — an unfiltered feed, or a paginated public response filtered only after the page bound — not a fetch-all-then-filter aggregate.

What not to flag

Signal discipline keeps the reviewer trusted. Do not raise:

• Denial of service, resource exhaustion, or "add rate limiting" on their own. (The repo already tiers rate limits; an unauthenticated write with no integrity guard is flagged as authorization/data integrity, not load.)
• Mere lack of hardening or defense-in-depth with no concrete vulnerability.
• Outdated third-party dependencies (managed separately); a new dependency's risk belongs to the architecture lens.
• Theoretical race or timing issues with no practical exploit.
• Test-only files, Markdown, and docs — except a committed secret or credential, which is a finding anywhere (see the secrets anchor above).
• Log spoofing, regex-DoS, and missing audit logs.
• SSRF that only controls a path; it counts when the attacker controls host or protocol.

Unguessability is not authorization, in either direction: an authorization finding rests on a missing server-side check, never on whether an id can be guessed — but identifier format validation against the upstream contract remains a legitimate code-review concern (the knowledge base's regex-too-loose-for-id-format pattern) and is not suppressed by this rule. Some settled precedents: environment variables and runtime config read server-side are trusted inputs; logging URLs and non-PII is fine; an absent client-side guard is only a UX gap when server enforcement is present — a missing server-side check is itself the vulnerability, not a false positive.

Reporting

For each finding give the file and function, what the attacker controls, the boundary crossed, the concrete impact on this application (which persona, whose data, what an anonymous caller gains), and the fix. If the diff does not touch an anchor above, do not invent a finding for it.