Enforce Constant-Time Token Comparison, Input Length Bounds, and Awaited Email Dispatch - #54
malakasaray-del wants to merge 2 commits into
Conversation
…ted Email Dispatch
### Description
This pull request addresses medium-severity API authorization, data validation, and asynchronous error handling defects in `website` (`server/src/app.ts`) identified during the Quantus workspace security audit (**FM-04, FM-05, FM-06**).
Previously, the `/api/send-email` authorization check used a non-constant-time comparison (`!==`), leaking secret contents and length through timing side channels. Request body fields across contact and inquiry endpoints were forwarded to the mailer and database with only truthiness checks, allowing unbounded text payloads. Furthermore, `emailClient.sendMail()` calls were unawaited (fire-and-forget), causing failed sends to report false 200/201 success responses while triggering unhandled promise rejections.
### Key Changes & Remediations
#### Constant-Time Bearer Token Verification (FM-04 - `server/src/app.ts`)
* **Timing-Safe Helper:** Implemented `tokensMatch(provided, expected)` using SHA-256 digests evaluated with `crypto.timingSafeEqual`:
```typescript
const tokensMatch = (provided: string | undefined, expected: string | undefined): boolean => {
if (!provided || !expected) return false;
const a = createHash("sha256").update(provided).digest();
const b = createHash("sha256").update(expected).digest();
return timingSafeEqual(a, b);
};
Fail-Closed Semantics: Safely accepts string | undefined, ensuring that an unset EMAIL_TOKEN configuration fails closed rather than throwing runtime exceptions.
####Strict Input Validation & Length Clamping (FM-05 - server/src/app.ts)
Format & Type Constraints: Added strict email regex validation (EMAIL_RE) with a 320-character ceiling across /api/waitlist, /api/inquiries, /api/send-email, and /api/sponsorships.
Bounded Field Sizes: Enforced upper bounds via clampText() and explicit string length checks:
Name / organization / designation: clamped to 200 characters
Subject: bounded to 500 characters
Free-text messages / additional info: bounded to 5,000 characters (MAX_TEXT_LENGTH)
HTML body: bounded to 200,000 characters
Source / phone / tier: bounded to 100 characters
####Synchronous Mailer Dispatch & Observability (FM-06 - server/src/app.ts)
Awaited Promises: Added await to emailClient.sendMail(...) across all email dispatch endpoints.
Structured Error Logging: Wrapped sends in try/catch blocks logging via logger.error and toSafeLogError(), returning a structured HTTP 400 response on failure to prevent false success reporting.
####How to Review
Inspect tokensMatch in server/src/app.ts to verify the SHA-256 pre-hashing and crypto.timingSafeEqual comparison.
Review /api/waitlist, /api/inquiries, /api/send-email, and /api/sponsorships to verify field length checks and type assertions.
Verify that emailClient.sendMail is preceded by await and properly routes exceptions to logger error handlers.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT 5.6 Sol
Request changes: two compatibility regressions remain in the new validation.
-
[P1]
server/src/app.ts:160— Preserve Nodemailer's supportedfromformats.Mail.Options.fromaccepts both a bare address and the standardSender Name <sender@example.com>form, butEMAIL_RE.test(from)rejects the latter because it contains spaces and angle brackets. Authenticated callers using that valid form will now receive HTTP 400 and no email will be sent. Parse and validate the underlying mailbox (or otherwise retain the supported display-name form) instead of applying the bare-address regex to the complete header value. -
[P2]
server/src/app.ts:71— Preserve the omitted-source newsletter default.clampText(undefined, 100)returns"", whilebuildLoopsContactPayloaddefaults only a nullish source and rejects""withUnknownWaitlistSourceError. Consequently, the documentedPOST /api/waitlistrequest withoutsourcechanges from a newsletter subscription to HTTP 400. Keep an omitted source asundefinedand add route-level regression coverage.
Validation: npm test passed all 10 tests; npm run build:release passed. Focused reproductions confirmed both rejected inputs. git diff --check fails because the rewritten server/src/app.ts uses CRLF on every line. GitHub reports no checks for this head.
|
Thanks for the thorough review and catching both regressions @n13! All points have been resolved in the latest update:
|
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT 5.6 Sol
Request changes: the current head does not compile, and the replacement address validation still permits unchecked header content.
-
[P1]
server/src/app.ts:56— Guard the optional regex capture before calling.trim(). With the repository's existingnoUncheckedIndexedAccesssetting,match[1]isstring | undefined, so bothnpm testandnpm run build:releasestop with TS2532 before any tests or deployable output are produced. The locked TypeScript 5.9.2 compiler reproduces the same failure. Please make the capture handling type-safe and add focused coverage for the helper. -
[P2]
server/src/app.ts:55-64— Validate the complete address header rather than only the first angle-bracket substring. The route forwards the original value to Nodemailer, butextractEmailmakes inputs such asSender <sender@example.com>, second@example.compass after checking onlysender@example.com; a 5,000-character display name also passes despite the stated 320-character ceiling. Parse the entire value, enforce a total length bound, require exactly one sender, and validate every permitted recipient.
Validation: npm test failed at TypeScript compilation with TS2532; npm run build:release failed identically; TypeScript 5.9.2 tsc -p tsconfig.json --noEmit reproduced it. Focused address-helper reproductions confirmed the unchecked suffix and length cases. git diff --check also fails because the current blob still uses CRLF on all 287 lines. GitHub reports no checks for this head.
Description
This pull request addresses medium-severity API authorization, data validation, and asynchronous error handling defects in
website(server/src/app.ts) identified during the Quantus workspace security audit (FM-04, FM-05, FM-06).Previously, the
/api/send-emailauthorization check used a non-constant-time comparison (!==), leaking secret contents and length through timing side channels. Request body fields across contact and inquiry endpoints were forwarded to the mailer and database with only truthiness checks, allowing unbounded text payloads. Furthermore,emailClient.sendMail()calls were unawaited (fire-and-forget), causing failed sends to report false 200/201 success responses while triggering unhandled promise rejections.Key Changes & Remediations
Constant-Time Bearer Token Verification (FM-04 -
server/src/app.ts)tokensMatch(provided, expected)using SHA-256 digests evaluated withcrypto.timingSafeEqual: ```typescript const tokensMatch = (provided: string | undefined, expected: string | undefined): boolean => { if (!provided || !expected) return false; const a = createHash("sha256").update(provided).digest(); const b = createHash("sha256").update(expected).digest(); return timingSafeEqual(a, b); }; Fail-Closed Semantics: Safely accepts string | undefined, ensuring that an unset EMAIL_TOKEN configuration fails closed rather than throwing runtime exceptions.Strict Input Validation & Length Clamping (FM-05 - server/src/app.ts) Format & Type Constraints: Added strict email regex validation (EMAIL_RE) with a 320-character ceiling across /api/waitlist, /api/inquiries, /api/send-email, and /api/sponsorships.
Bounded Field Sizes: Enforced upper bounds via clampText() and explicit string length checks:
Name / organization / designation: clamped to 200 characters
Subject: bounded to 500 characters
Free-text messages / additional info: bounded to 5,000 characters (MAX_TEXT_LENGTH)
HTML body: bounded to 200,000 characters
Source / phone / tier: bounded to 100 characters
Synchronous Mailer Dispatch & Observability (FM-06 - server/src/app.ts) Awaited Promises: Added await to emailClient.sendMail(...) across all email dispatch endpoints.
Structured Error Logging: Wrapped sends in try/catch blocks logging via logger.error and toSafeLogError(), returning a structured HTTP 400 response on failure to prevent false success reporting.
How to Review
Inspect tokensMatch in server/src/app.ts to verify the SHA-256 pre-hashing and crypto.timingSafeEqual comparison.
Review /api/waitlist, /api/inquiries, /api/send-email, and /api/sponsorships to verify field length checks and type assertions.
Verify that emailClient.sendMail is preceded by await and properly routes exceptions to logger error handlers.