Reject malformed emails on trade inquiries (#117) - #121
Merged
Merged
Conversation
POST /api/trade-inquiry checked that email existed and fit the field-length limit, but never validated its format — a direct request could bypass the browser's type="email" check, persist a dead tradeLeads record, and hand a guaranteed-failure replyTo to Resend (trade_inquiry.notification_failed noise in monitoring). The post-parse request pipeline now lives in handleTradeInquiry() in lib/trade-leads-common.ts with injected rate-limit/submit collaborators, matching the module's established DI-for-testability pattern. It validates email format via a new shared isValidEmail() (lib/email.ts) — extracted from the identical EMAIL_REGEX duplicated in admin users route and admin-access.tsx — and returns 400 before the honeypot, rate limit, or any persistence/notification work. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideThe PR adds shared server/client email-format validation and refactors the trade-inquiry endpoint around an injectable validation pipeline, ensuring malformed emails return a 400 before honeypot handling, rate limiting, persistence, or notification while preserving existing behavior and adding focused regression coverage. Sequence diagram for trade inquiry email validation pipelinesequenceDiagram
participant Client
participant Route as trade-inquiry route
participant Pipeline as handleTradeInquiry
participant RateLimiter
participant Submit as submitTradeInquiry
participant Storage as Firestore
participant Resend
Client->>Route: POST /api/trade-inquiry
Route->>Pipeline: handleTradeInquiry(body, context, deps)
Pipeline->>Pipeline: isValidEmail(email)
alt malformed email
Pipeline-->>Route: 400 Please enter a valid email address.
Route-->>Client: 400 response
else valid email
Pipeline->>RateLimiter: isRateLimited(clientIp)
alt rate limit exceeded
Pipeline-->>Route: 429 response
Route-->>Client: 429 response
else allowed
Pipeline->>Submit: submitTradeInquiry(input, requestId)
Submit->>Storage: Persist trade lead
Submit->>Resend: Send notification with replyTo
Pipeline-->>Route: 200 response
Route-->>Client: 200 ok
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /api/trade-inquiryverified thatemailexisted and fit the field-length limit, but never validated its format. A direct request could bypass the browser'stype="email"check and submit something likenot-an-email— which was persisted totradeLeadsand passed to Resend asreplyTo, producing a guaranteedtrade_inquiry.notification_failedand monitoring noise while the customer still saw success.The fix adds a server-side email-format check that returns 400 (
Please enter a valid email address.) before the honeypot reply, rate limit, persistence, or any Resend work.Closes #117
Changes
lib/email.ts(new): canonicalisValidEmail()— the identicalEMAIL_REGEXpreviously duplicated inapp/api/admin/users/route.tsandcomponents/admin-access.tsxis now a single dependency-free module both server routes and client components can import.lib/trade-leads-common.ts: newhandleTradeInquiry()holding the whole post-parse pipeline — trim → required fields → field-length bounds → email format → honeypot → rate limit → submit — with injectedisRateLimited/submitcollaborators, matching the module's established DI-for-testability pattern.app/api/trade-inquiry/route.ts: thin wrapper — JSON parse, thenhandleTradeInquiry, mapping the{status, body}result to the response. Rate-limiter implementation unchanged.app/api/admin/users/route.ts,components/admin-access.tsx: consumeisValidEmail(); behavior identical.tests/lib/email.test.ts(new),tests/lib/trade-leads.test.ts: regression coverage — for each malformed address, asserts 400 and thatsubmit(the single entry point to persist + notify) is never called, and that the rate limiter is never consulted. Valid cases covercustomer@example.com,first.last@example.com,customer+trade@example.com,sales@wholesale.example.com, and trimmed whitespace. Missing/oversized email, honeypot, 429, and the generic-500 mapping stay green. Source-level wiring assertions guard thatroute.tsactually delegates to the tested pipeline (the route module itself can't be imported innode:test— it pulls inserver-onlymodules).docs/TECHNICAL.md: §11 updated for the new pipeline and the email-format check.Verification
npx tsc --noEmit— cleannpm run lint— clean (one pre-existingjsx-ast-utilsTSSatisfiesExpressionnotice, also present onmain)npm test— 350 tests pass (13 new: 3 inemail.test.ts, 10 pipeline/wiring intrade-leads.test.ts)npm run test:rules— 29 pass (Firestore/Storage emulators, Java 21)npm run build— succeedsnpx playwright test— 117 smoke tests passnpm run check:md-links— all links resolvenpm run check:react-versions— react/react-dom match (19.3.0)npm audit --omit=dev— 0 vulnerabilitiesisValidEmailcheck removed (pre-fix behavior),not-an-emailflows through with a 200 and reachessubmit; with the fix it returns 400 andsubmitis never invoked.Risk / deployment notes
ok: true; the rate limiter still only counts requests that pass validation.venueTyperemains effectively free-text server-side (only presence + ≤64 chars enforced) while the UI constrains it to a fixed<select>list — a candidate for a separate, narrowly scoped follow-up if desired.Generated with Devin
Summary by Sourcery
Prevent malformed trade-inquiry email addresses from reaching persistence and notification services.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: