Module 15 Security Review — Follow-up (tasks #356, #357, #366)
0008-m15-security-hardening-review.md (task #344) was a dedicated pass
over Billing, Marketplace Security, Public API v1/keys, Webhooks, and
Integrations, and found/fixed two HIGH SSRF issues. This is a narrower
follow-up scoped to what was added after that review closed: the
Stripe webhook receiver (task #366), release engineering (task #356),
and the feature flags system (task #357). Same static-review posture as
every prior review in this directory — no running instance, no load/pen
testing tools available in this sandbox.
1. StripeWebhookController — signature verification
Reviewed for the standard webhook-forgery class of bug: can a request
without knowledge of STRIPE_WEBHOOK_SECRET cause a state change?
- Signature check happens before the raw body is parsed as JSON and
before any database access — an attacker with an invalid/missing
signature is rejected (401) at essentially zero cost (one HMAC compute),
never reaching
SubscriptionService/InvoiceService. - Comparison uses
timingSafeEqual, with the same length-mismatch-safe pattern (timingSafeEqualagainst a same-length zero buffer before reporting failure)BillingWebhookController'ssecretsMatch()established — response timing doesn't leak how many leading bytes of a forged signature matched. - A
t=timestamp more than 5 minutes from server time is rejected (replay-window bound), matching Stripe's own documented tolerance. - No new dependency introduces a parsing/deserialization attack surface
—
JSON.parseon the raw body only, noeval/Functionconstruction anywhere in the new code. - If
STRIPE_WEBHOOK_SECRETis unset, the endpoint fails closed (503 STRIPE_WEBHOOK_NOT_CONFIGURED) rather than silently accepting unsigned events. - The global
ThrottlerModuledefault (100 req/60s,app.module.ts) applies to this route like every other — no per-route throttle was added, but none is needed: rejected requests are already cheap, and Stripe's own retry behavior for a real deployment is low-volume.
No finding. One gap already disclosed in
docs/architecture/stripe-webhook.md and not re-litigated here: no
event-id idempotency ledger, so a redelivered event re-applies the same
write (harmless because the write is idempotent-by-value, not
idempotent-by-request, but worth a future pass if strict duplicate-
delivery auditing is ever needed).
2. FeatureFlagsController — authorization model
POST /feature-flags and POST /feature-flags/overrides require only
"authenticated," not a specific role — deliberately, and disclosed both
in the controller's own doc comment and in
docs/architecture/feature-flags.md, following the exact precedent
ClusterOverviewService (Module 14) and the Support ticket queue (task
#355) already established for "no deployment-wide platform-admin role
exists yet." This means any authenticated user of any role can create
flags and set overrides for any scope, including other users' USER-scoped
overrides and other organizations' ORGANIZATION-scoped overrides —
worth stating plainly rather than leaving implicit. Since feature flags
gate feature visibility, not data access or financial transactions, the
blast radius of abuse is "an authenticated user turns a feature on/off
for themselves or, if they guess/enumerate IDs, someone else" — annoying,
not a data breach. Flagged here as the actual concrete risk (not just
"no RBAC" in the abstract) so a future platform-admin-role pass knows
exactly what to gate first.
Recommendation, not implemented this pass (would require the
platform-admin role infrastructure itself, out of scope for a security
review): gate create/setOverride behind that role once it exists; in
the meantime, operators who care should restrict /feature-flags (except
/feature-flags/evaluate) at the reverse-proxy layer.
GET /feature-flags/evaluate was checked for cross-tenant leakage: it
only ever resolves flags for req.user.id plus whatever
workspaceId/organizationId the caller supplies as query params —
there's no server-side lookup of "the caller's" workspace, so a caller
can technically pass an arbitrary organizationId they don't belong to.
The consequence is limited to seeing that organization's
ORGANIZATION-scoped flag overrides (booleans only, no other data) —
still worth tightening in a future pass by validating membership before
including the organization-scoped override in the response, but not
elevated to a fix-now finding given the low sensitivity of the leaked
data (a true/false per flag key, nothing else).
3. Release engineering (RELEASE_CHANNEL) — no new surface
Purely a read of a ConfigService value set at deployment time, surfaced
on an already-@Public() endpoint (GET /observability/health). No
user input involved. No finding.
4. Support ticket email construction — reviewed, not a finding
CreateSupportTicketHandler builds email subject/body by string-
concatenating user-supplied data.subject/data.category/data.message.
Checked for header injection (CRLF in the Subject: header): delivery
goes through nodemailer's transporter.sendMail() with subject as a
distinct object field, not raw header-string concatenation —
nodemailer encodes/validates header values internally and does not
pass through unescaped CRLF sequences. data.email (used as to on the
confirmation mail) is validated with @IsEmail(), which rejects the
malformed multi-address/CRLF strings a header-injection attempt would
need. No finding, but worth re-checking if SmtpMailService is ever
swapped for a provider that constructs raw header strings itself.
Summary
No new HIGH/MEDIUM findings from this pass's own code. One concrete,
plainly-stated authorization gap on feature-flag management routes
(already disclosed, tracked as future work pending a platform-admin
role) and one minor cross-tenant information leak on evaluate (boolean
flag values only, not elevated to a fix-now finding). Everything else —
Stripe webhook signature verification, release-channel surfacing, support
email construction — reviewed clean.