feat(blog): notify google chat on newsletter form submit

[dhananjay6561]

Forwards every blog newsletter/lead form submission to a Google Chat space, so the team gets pinged in real time when a new lead comes in. Same approach we already use for the request-a-trial form on the landing repo.

Where the form data goes today

The newsletter form (under the ad banner on every blog post) already sends data to two places, both MongoDB. This PR does not change either of them:

• Newsletter subscription → api-server, via the GraphQL subscription(guestInput) mutation, lands in the subscriptions collection (upsert by email, so repeat emails get merged). This is the actual subscriber list.
• Lead / MQL copy → telemetry POST /blog-mql, verified server-side via reCAPTCHA Enterprise, lands in the keploy-telemetry.blog_mql collection (one row per submit, for lead tracking).

So the data is stored, but nobody gets notified when a new lead arrives. That's the gap this PR fills.

What changed

• New server-side route pages/api/blog-lead-notify.ts that holds the webhook secret and posts a formatted message to the space.
• subscribe-newsletter.tsx calls it fire-and-forget, alongside the existing subscription + blog-mql capture (does not touch or gate either).
• Documented the env var in .env.local.example.

Action needed to turn it on

• Add a server-only env var GOOGLE_CHAT_WEBHOOK_URL in the blog-website Vercel project (not NEXT_PUBLIC_*, so it never reaches the browser).
• Create the webhook in the Chat space: Apps & integrations → Webhooks → add → copy URL.
• Until this env is set, everything keeps working exactly as before, just no Chat message is delivered.

Notes

• Fail-open: if the var is unset or the POST fails, the user's submit and the newsletter subscription are unaffected.
• Delivery-safe & consistent with repo conventions: aborts the Chat fetch at 15s and caps field lengths (name 120 / email 254 / company 160 / page 500), mirroring blog-mql.ts; logs gated on NODE_ENV.
• Basic bot filter: a hidden honeypot field drops autofill bots; server-side email validation.
• Not gated with auth (deliberate follow-up): this mirrors landing's /trial/submit, which is also a public unauthenticated POST. Stronger gating via reCAPTCHA Enterprise (forwarding + verifying the token the client already mints) needs the GCP project-id + api-key secrets that live in telemetry, not this repo — tracked as a follow-up rather than half-gated here.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhananjay6561]

reviewed with the code-review-skill (typescript + security guides). nice clean change overall, the fail-open design and keeping the webhook as a server only secret (not NEXT_PUBLIC) is exactly right, and re-validating on the server instead of trusting the client is good.

the one thing i'd want sorted before merge is that /api/blog-lead-notify is a fully open unauthenticated POST that pushes messages straight into the team chat space, with no recaptcha or rate limit. the sibling blog-mql path verifies a recaptcha enterprise token and this one doesn't, even though the client already mints a token. left inline notes on that plus a couple of smaller things (the honeypot never actually fires, and the chat text isn't escaped). details inline.

severity key: 🔴 blocking, 🟡 important, 🟢 nit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhananjay6561]

second pass, this time reading it against the patterns already in this repo (blog-mql.ts, proxy-image.ts, revalidate.ts) rather than in isolation.

the good stuff holds up: webhook stays a server only secret (not NEXT_PUBLIC), fail-open so a chat hiccup never breaks a subscribe, server side re-validation instead of trusting the client, and routing through router.basePath is correct since the app is served under /blog. all good.

the theme of my comments is that this endpoint doesn't follow the conventions the neighbouring code already established: blog-mql caps field lengths + uses an abort timeout + gates logging on NODE_ENV, proxy-image validates outbound urls against an allowlist, revalidate gates its POST. this one does none of those, and it's a public write that fans out to the team chat space. inline below, ordered by severity.

severity key: 🔴 blocking, 🟡 important, 🟢 nit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhananjay6561]

third pass, focused on test coverage and cross-path consistency (the earlier passes covered security + the handler internals).

the big one: there's already an e2e spec for this exact form, tests/e2e/LeadCapture.spec.ts, and this PR adds a new user-facing network call without touching it. it even has a fail-open case that's the perfect template for the fail-open behaviour you're adding. details inline. also confirmed from that spec's own comment that the sidebar (so this form) renders twice per page, desktop + mobile fallback, which is fine for submit but good to keep in mind.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhananjay6561]

addressed iteration 4 in 4a59efd. took the two that actually improve things, skipped the ones that dont change the outcome:

t3 (double-submit): done, this one matters since the whole point is one chat ping per click. moved the guard onto a useRef and check/set it synchronously. reading the submitting state or the disabled button sees a stale false on a fast double-click because both handlers fire before react re-renders, so two POSTs could slip through. the state still drives the disabled/opacity ui, the ref decides who runs.

t4 (~strikethrough): kept it, didnt strip, but extended the comment. ~ is valid in an email local part (rfc atext, and EMAIL_RE accepts it), so stripping it corrupts real lead data the exact same way stripping _ would. its cosmetic only and cant forge a field or link once < > | * and newlines are gone, so its the same call as _.

t5 (bare url autolink): left as is. the deceptive-label vector (<url|label>) is already dead since < > | are stripped, so a bare url just autolinks to where it visibly points. not a phishing surface.

t6 (honeypot before rate limit): left as is. reordering doesnt save the invocation, the function has already been invoked and runs to return either the 200 or the 429 regardless, so it doesnt address the burned-invocation cost thats the stated harm.

s1 (per-instance rate limit): agreed, its a stopgap. already flagged in the code comment and the PR body that notifying from telemetry's /blog-mql (which already verifies the recaptcha token) is the real fix and deletes this endpoint. tracked as a follow-up.

tsc clean, test:unit 46/46, next lint clean.

[amaan-bhati]

Claude Review Skill: Iteration 5

At 4a59efd (1 new commit). Reviewed from the diff. tsc / test:unit not run this round (no disk space on the review machine), so treat CI as the source of truth for those.

🚦 Verdict: 💬 COMMENT / 0 blocking

🔴 0	🟡 0	🟢 1	💡 1	🎉 1

✅ Iteration 4 verified

T3 fixed, correctly. submittingRef is checked and set synchronously before any await, the state is left to drive the disabled/opacity UI, and both are reset in the same .finally. The comment names the exact failure (two handlers run before React re-renders, so the state and the disabled attribute both still read false). That is the right split of responsibilities between ref and state.

T4: you were right, I was wrong. I suggested stripping ~ for completeness. ~ is RFC 5322 atext, so it is legal in an email local part, and stripping it would have corrupted real addresses in exactly the way T1 did. Declining it and documenting why is the better call.

T5 and T6 remain open as optional nits, unchanged.

🟢 Nit

F1. The stated rationale does not match the strip set. The comment now argues _ and ~ are kept because they are "legal in email local parts … so stripping them corrupts real lead data". By that test |, * and backtick should also be kept, since all three are atext too, and the code does strip them. The real distinction is narrower and worth saying instead: keep the atext characters that cannot inject on their own, strip the ones that can (| builds <url|label>, * bolds, backtick opens code). Otherwise the next reader applies the rule as written and removes a guard.

💡 Suggestion

S1 (carried). The per-instance rate limit still scales the wrong way, since Vercel adds instances under load. Fine as a stopgap. Notifying from telemetry's /blog-mql, which already verifies the reCAPTCHA token, remains the real fix and deletes this endpoint.

🎉 Praise

• Pushing back on T4 with the RFC rather than just applying it. A review suggestion that would have reintroduced the bug the previous round fixed is exactly the kind that should be argued with, and the argument was the correct one.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhananjay6561]

thanks for the iteration 5 pass @amaan-bhati. fixed f1 in df949a1.

you nailed the flaw in the old comment, the "atext so keep" line proved too much since |, * and backtick are atext too and we strip all three. rewrote it on the real split: | * and backtick each inject on their own (link, bold, code span) so they go, while _ and ~ can only do cosmetic matched pair styling once < > | and newlines are already gone, and they actually turn up in real lead data (email local parts, utm page urls like utm_source) so stripping them would corrupt leads.

s1 is fair, the per instance limit does scale the wrong way as vercel adds instances. agreed the real fix is notifying from telemetry /blog-mql where the recaptcha token is already verified, which deletes this endpoint entirely. keeping it as the stopgap for this pr and it stays flagged as a follow up in the header.

and thanks for the note on t4, glad pushing back with the rfc was the right move rather than reintroducing the exact bug the round before had just fixed.

[amaan-bhati]

Claude Review Skill: Iteration 6

Head: df949a1 (1 new commit). The delta was a comment fix, so this pass went at the delivery path's failure modes instead of re-reading the diff.

🚦 Verdict: 💬 COMMENT / 0 blocking

🔴	🟡	🟢	💡
0	2	2	1

✅ Iteration 5 verified

F1 fixed, and the rewrite is better than what I asked for. The comment now leads with the real test rather than the atext one, and splits it into risk and cost per character. Stating why |, * and ` fail that test is what stops the next reader from "simplifying" the strip set.

S1 acknowledged and kept flagged in the header as a follow-up. Agreed on the disposition.

---

🟡 N1. The catch branch goes quiet in production, but the !chatRes.ok branch beside it does not

pages/api/blog-lead-notify.ts:200-213. The non-OK branch logs unconditionally, with an explicit rationale: "Logged in production too (unlike the missing-webhook warning): it's the only signal that a configured webhook has broken." That reasoning is right. The adjacent catch is gated on NODE_ENV !== "production", so in production it swallows:

• DNS failure / host unreachable for chat.googleapis.com
• connection refused, TLS failure
• the 15s abort (see N2)

Those are as silent and as permanent as a 4xx. A revoked webhook returning 404 is visible; the space becoming unreachable is not, which is the same "leads quietly stop arriving and nobody notices" outcome the non-OK branch was written to prevent.

I checked the obvious objection, that logging err in production might leak the webhook URL, since the header promises it "never reaches the client, git, or logs" and the URL carries key/token in its query string. It does not leak. Node's undici gives:

name: TypeError | message: fetch failed
cause: getaddrinfo ENOTFOUND <host>
contains the URL or its query params? no

So a PII-free production line is safe here. Matching the non-OK branch's shape would do it.

🟡 N2. The 15s abort is at or above the platform's own ceiling, so it cannot reliably fire

blog-lead-notify.ts:177-178. The route sets no maxDuration, and vercel.json has only redirects and headers with no functions block, so it runs on Vercel's default function duration, which is below or equal to 15s depending on plan (10s on Hobby, 15s on Pro). A hung webhook is therefore killed by the platform at or before the abort, so the catch never runs and nothing is logged, even in dev where N1's gate would allow it.

The 15s value only does its job if the function is guaranteed to outlive it. Either drop the abort to ~5s, which is far more than a Chat webhook needs and leaves headroom for the response, or set maxDuration on the route explicitly so the two numbers are chosen together rather than one of them inherited.

🟢 Nits (2)

N3. EMAIL_RE runs before sanitize, so the delivered *Email:* line can hold something that is not an email. blog-lead-notify.ts:143,152. EMAIL_RE is /^[^@\s]+@[^@\s]+\.[^@\s]+$/, which accepts *, | and ` in a local part; sanitize then turns each into a space, so a*b@x.com validates and is delivered as a b@x.com. This is a separate point from whether those characters should be stripped, which F1 settled correctly. It's about order: validating the sanitized value instead would return a 400 rather than posting a broken address. Rare in practice, one line to make impossible.

N4. A rate-limited request still records a hit. blog-lead-notify.ts:65-68. hits.push(now) happens before the > RATE_LIMIT_MAX return, so a sustained flooder keeps extending their own window and growing their array (bounded by the window, so not a leak). Punishing the flooder is a reasonable design, but right now it reads as incidental. One clause saying it's deliberate would settle it, or return before the push if it isn't.

💡 N5. The map cap can be exceeded by the case it exists for

blog-lead-notify.ts:58-63. Eviction runs only when size > RATE_MAP_MAX_KEYS and deletes only keys whose last hit is already outside the window, so a caller rotating more than 10k IPs inside one window is exactly the case that cannot be pruned. Consistent with the documented best-effort framing, and S1 (notify from telemetry /blog-mql, deleting this endpoint) subsumes it, so noting rather than asking.

---

✅ Verified this round

Check	Result
test:unit	46/46 pass
tsc --noEmit	Clean
Rate limit boundary	5 allowed, 6th is 429; matches the test and the RATE_LIMIT_MAX = 5 comment
Chat 4096-char limit	Worst case is ~1.1 KB across the caps (120+254+160+500 + labels), so the cap test's premise holds
finally { clearTimeout(timer) }	Reached on every path including the early !chatRes.ok return; no leaked timer
Sanitizer claim, "_/~ can only do cosmetic matched-pair styling"	Holds. Field forgery needs a newline and link forgery needs <, > or `
Fetch error contents	Carries no URL or query params, so N1's fix does not risk the webhook secret
Branch state	0 behind origin/main, 12 ahead

Nothing here blocks. N1 is the one I would take, since it protects the same property the non-OK branch already argues for.

[amaan-bhati]

Iteration 6: local end-to-end verification

Ran the endpoint for real rather than reading it, since the last two rounds were static analysis. df949a1, isolated worktree, next dev on :3111.

GOOGLE_CHAT_WEBHOOK_URL pointed at a local sink on :4599 that echoes each payload, so no traffic left the machine and no real Chat space was touched. Endpoint is /blog/api/blog-lead-notify (the basePath: '/blog', which subscribe-newsletter.tsx:137 gets right via router.basePath).

Results

#	Request	Response	Delivered?
1	GET	405 + Allow: POST	no
2	POST {}	400 {"ok":false,"error":"validation"}	no
3	POST invalid email	400 {"ok":false,"error":"validation"}	no
4	POST + company_website (honeypot)	200 {"ok":true}	no
5	valid lead	200 {"ok":true}	yes
6	injection payload	200 {"ok":true}	yes, neutralized
7	a*b@x.com	200 {"ok":true}	yes, see N3
8	300-char name	200 {"ok":true}	yes, capped to exactly 120
9-16	8 identical POSTs	200 x5, then 429 x3	5

Sink recorded 10 hits total across the session, which matches 1 warm-up + #5 + #6 + 2x #7/#8 + 5 from the flood. The 405, both 400s and the honeypot contributed zero, so the "silently accept and drop" claim holds at the wire level, not just in the unit test.

What actually arrived in the Chat message

Valid lead:

*📨 New Keploy blog subscriber*
*Name:* Amaan Bhati
*Email:* amaan.bhati@keploy.io
*Company:* Keploy
*Source:* blog-newsletter
*Page:* /blog/technology/api-testing?utm_source=q3_launch
*Submitted:* 2026-08-20T16:20:52.190Z

Sent as Amaan.Bhati@Keploy.IO, delivered lowercased. utm_source=q3_launch kept its underscore, which is the F1 decision doing its job on a real request.

Injection attempt. Sent:

{"fullName":"Evil*bold*\n*Email:* attacker@evil.com\n*Company:* Forged",
 "email":"a_b~c@x.com",
 "companyName":"<https://evil.com|CLICK ME>",
 "page":"/p?a=`code`&b=*x*",
 "source":"admin-override"}

Arrived:

*📨 New Keploy blog subscriber*
*Name:* Evil bold Email: attacker@evil.com Company: Forged
*Email:* a_b~c@x.com
*Company:* https://evil.com CLICK ME
*Source:* blog-newsletter
*Page:* /p?a= code &b= x
*Submitted:* 2026-08-20T16:20:52.224Z

Every vector is dead in a way that's now observable rather than argued:

• Field forgery collapsed onto one line. The forged *Email:* / *Company:* labels lost their * and their newlines, so they read as inert text inside the Name value and cannot be mistaken for real fields.
• Fake link <https://evil.com|CLICK ME> lost <, > and |, so it renders as plain text, not a clickable label.
• Code span and bold both defused.
• source: "admin-override" was pinned back to blog-newsletter.
• a_b~c@x.com survived intact, both the _ and the ~. This is the exact case F1's comment argues for, and it is now demonstrated rather than reasoned about.

N1 confirmed: the production gap is real

next dev forces NODE_ENV=development, so I drove the handler directly with a mocked req/res (the shape tests/lib/blog-lead-notify.test.ts already uses) and flipped only NODE_ENV:

================= NODE_ENV=development =================
  network failure (catch branch)     HTTP 200  logged: YES -> [blog-lead] delivery to Google Chat failed...
  HTTP 500 (!chatRes.ok branch)      HTTP 200  logged: YES -> [blog-lead] Google Chat rejected the message (HTTP 500)...

================= NODE_ENV=production ==================
  network failure (catch branch)     HTTP 200  logged: *** NOTHING ***
  HTTP 500 (!chatRes.ok branch)      HTTP 200  logged: YES -> [blog-lead] Google Chat rejected the message (HTTP 500)...

Same code, one env var apart. In production a Chat 500 is visible and a DNS failure, refused connection, TLS error or abort is completely silent, while both return 200 to the user either way. That is the asymmetry N1 describes, and the fail-open behaviour is confirmed correct on both paths.

N3 confirmed

a*b@x.com passes EMAIL_RE (* is neither @ nor whitespace), then sanitize turns the * into a space:

*Email:* a b@x.com

The *Email:* line is delivering something that is not an email address. Validating the sanitized value instead would have returned a 400. Rare input, one line to make impossible, and unrelated to the F1 question of which characters to strip.

Unchanged

Verdict stays 💬 COMMENT, 0 blocking. The abuse controls, sanitizer, source pinning, caps and fail-open all behave exactly as documented under real requests. N1 moves from reasoned to reproduced; N2 (the 15s abort sitting at or above the platform ceiling) is the one thing here I can't demonstrate locally, since it needs Vercel's function timeout to bite.

[amaan-bhati]

@dhananjay6561 Have added a comment for the recaptcha test failure, explained properly in the comment, kindly address. Rest of the things lgtm, this is the only thing i am concerned about, Rest all LGTM!

[amaan-bhati]

@dhananjay6561 I think that there is a failing test, I think it is realted to the recaptcha failure, this is although not really possible to test locally, since there wont be any recaptcha token generated locally, we can only test this on prod, but i want to understand why gate with recaptcha here? Did we have this earlier on the newsletter or not? Why include recaptcha here? Do try and ideate with claude on this, lmk what you think about this. If not relevant anymore then also let me know.

[amaan-bhati]

Verified things on local end-to-end

Ran the endpoint for real rather than reading it, since the last two rounds were static analysis. df949a1, isolated worktree, next dev on :3111.

GOOGLE_CHAT_WEBHOOK_URL pointed at a local sink on :4599 that echoes each payload, so no traffic left the machine and no real Chat space was touched. Endpoint is /blog/api/blog-lead-notify (the basePath: '/blog', which subscribe-newsletter.tsx:137 gets right via router.basePath).

Results

#	Request	Response	Delivered?
1	GET	405 + Allow: POST	no
2	POST {}	400 {"ok":false,"error":"validation"}	no
3	POST invalid email	400 {"ok":false,"error":"validation"}	no
4	POST + company_website (honeypot)	200 {"ok":true}	no
5	valid lead	200 {"ok":true}	yes
6	injection payload	200 {"ok":true}	yes, neutralized
7	a*b@x.com	200 {"ok":true}	yes, see N3
8	300-char name	200 {"ok":true}	yes, capped to exactly 120
9-16	8 identical POSTs	200 x5, then 429 x3	5

Sink recorded 10 hits total across the session, which matches 1 warm-up + #5 + #6 + 2x #7/#8 + 5 from the flood. The 405, both 400s and the honeypot contributed zero, so the "silently accept and drop" claim holds at the wire level, not just in the unit test.

What actually arrived in the Chat message

Valid lead:

*📨 New Keploy blog subscriber*
*Name:* Amaan Bhati
*Email:* amaan.bhati@keploy.io
*Company:* Keploy
*Source:* blog-newsletter
*Page:* /blog/technology/api-testing?utm_source=q3_launch
*Submitted:* 2026-08-20T16:20:52.190Z

Sent as Amaan.Bhati@Keploy.IO, delivered lowercased. utm_source=q3_launch kept its underscore, which is the F1 decision doing its job on a real request.

Injection attempt. Sent:

{"fullName":"Evil*bold*\n*Email:* attacker@evil.com\n*Company:* Forged",
 "email":"a_b~c@x.com",
 "companyName":"<https://evil.com|CLICK ME>",
 "page":"/p?a=`code`&b=*x*",
 "source":"admin-override"}

Arrived:

*📨 New Keploy blog subscriber*
*Name:* Evil bold Email: attacker@evil.com Company: Forged
*Email:* a_b~c@x.com
*Company:* https://evil.com CLICK ME
*Source:* blog-newsletter
*Page:* /p?a= code &b= x
*Submitted:* 2026-08-20T16:20:52.224Z

Every vector is dead in a way that's now observable rather than argued:

• Field forgery collapsed onto one line. The forged *Email:* / *Company:* labels lost their * and their newlines, so they read as inert text inside the Name value and cannot be mistaken for real fields.
• Fake link <https://evil.com|CLICK ME> lost <, > and |, so it renders as plain text, not a clickable label.
• Code span and bold both defused.
• source: "admin-override" was pinned back to blog-newsletter.
• a_b~c@x.com survived intact, both the _ and the ~. This is the exact case F1's comment argues for, and it is now demonstrated rather than reasoned about.

N1 confirmed: the production gap is real

next dev forces NODE_ENV=development, so I drove the handler directly with a mocked req/res (the shape tests/lib/blog-lead-notify.test.ts already uses) and flipped only NODE_ENV:

================= NODE_ENV=development =================
  network failure (catch branch)     HTTP 200  logged: YES -> [blog-lead] delivery to Google Chat failed...
  HTTP 500 (!chatRes.ok branch)      HTTP 200  logged: YES -> [blog-lead] Google Chat rejected the message (HTTP 500)...

================= NODE_ENV=production ==================
  network failure (catch branch)     HTTP 200  logged: *** NOTHING ***
  HTTP 500 (!chatRes.ok branch)      HTTP 200  logged: YES -> [blog-lead] Google Chat rejected the message (HTTP 500)...

Same code, one env var apart. In production a Chat 500 is visible and a DNS failure, refused connection, TLS error or abort is completely silent, while both return 200 to the user either way. That is the asymmetry N1 describes, and the fail-open behaviour is confirmed correct on both paths.

N3 confirmed

a*b@x.com passes EMAIL_RE (* is neither @ nor whitespace), then sanitize turns the * into a space:

*Email:* a b@x.com

The *Email:* line is delivering something that is not an email address. Validating the sanitized value instead would have returned a 400. Rare input, one line to make impossible, and unrelated to the F1 question of which characters to strip.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhananjay6561]

@amaan-bhati dug into the recaptcha thing, few things:

first, there isn't a failing test on the current head. the e2e (chromium) check is green on df949a1, the exact commit you reviewed, all four LeadCapture tests pass headless in CI. if you saw a red run it was on an earlier commit or a flake, current state is clean.

on "can't test locally without a real token": that's actually not needed here. the spec stubs grecaptcha in beforeEach, it fulfills the loader with an empty script and injects execute to return 'e2e-stub-token', so the token is a fake and the whole thing runs fully offline. that's why it passes in CI with no prod and no real google call. so it is testable, it's just stubbed rather than live.

on why recaptcha is here at all: it's not from this pr. it came in with #403 (the /blog-mql lead capture), already on main. that's what gates the lead path, client mints an invisible token and telemetry verifies it server side. this pr doesn't touch any recaptcha code, and the chat notify path i added doesn't use recaptcha at all, the notify fetch fires outside the getToken().then() so it carries no token. i only extended the existing #403 spec to also cover the notify call.

so nothing to fix here re recaptcha, it's pre-existing and still relevant to the blog-mql lead, just unrelated to what this pr changes.

separately, pushed 253920b addressing the iteration-6 review: log silent notify failures in prod too (n1), maxDuration=10 + 5s abort so they're picked together (n2), validate email after sanitizing (n3), and documented the rate-limit hit counting + map cap (n4/n5). lmk if you want me to walk through any of it.

[dhananjay6561]

@amaan-bhati verified the two fixes the same way you did, drove the handler with a mocked req/res and flipped only NODE_ENV, against the patched code in 253920b.

n1, the prod gap you found is closed:

================= NODE_ENV=development =================
  network failure (catch branch)     HTTP 200  logged: YES -> [blog-lead] delivery to Google Chat failed (network error)...
  HTTP 500 (!chatRes.ok branch)      HTTP 200  logged: YES -> [blog-lead] Google Chat rejected the message (HTTP 500)...

================= NODE_ENV=production ==================
  network failure (catch branch)     HTTP 200  logged: YES -> [blog-lead] delivery to Google Chat failed (network error)...
  HTTP 500 (!chatRes.ok branch)      HTTP 200  logged: YES -> [blog-lead] Google Chat rejected the message (HTTP 500)...

the prod catch branch that logged *** NOTHING *** in your run now logs, symmetric with the !chatRes.ok branch. the raw error is only attached in dev, so prod stays PII-free, checked the log for the webhook key/token and the full url, none of it leaks.

n3, a*b@x.com now gets rejected instead of delivered as 'a b@x.com':

a*b@x.com  -> HTTP 400 {"ok":false,"error":"validation"}  delivered: no

both behaviours are also locked into tests/lib/blog-lead-notify.test.ts so they can't regress (48/48 pass). n2 is the one that still can't be shown locally since it needs vercel's function timeout to bite, but maxDuration=10 now sits explicitly above the 5s abort so they're chosen together.

[dhananjay6561]

verified n1 and n3 locally the same way you did, mocked req/res + flipped only NODE_ENV so i could hit the prod path (next dev forces development).

[amaan-bhati]

Verified locally at 253920b. Holding approval on one line

Drove the handler directly with a mocked req/res and flipped only NODE_ENV, same approach as your screenshot, and checked the maxDuration export against Next's own config extractor.

npm run test:unit 48/48, npx tsc --noEmit clean.

Finding	Status
N1 catch branch silent in production	✅ fixed, verified in both envs
N3 EMAIL_RE before sanitize	✅ fixed, verified, plus a test
N4 rate-limited request records a hit	✅ documented as deliberate
N5 map cap exceedable within a window	✅ documented, follow-up named
N2 abort vs function ceiling	❌ abort fixed, but the maxDuration export is inert

N1 and N3 are properly done. The one thing below needs fixing before I approve.

---

⛔ The blocker: export const maxDuration = 10 is silently ignored here

This is a Pages Router API route. In Next 14.0.1 the bare export is read for app router only; for pages it comes solely from export const config = {…}. Next's source is explicit: AUTHORIZED_EXTRA_ROUTER_PROPS is read from the bare export when pageType === "app", and from config when pageType === "pages".

Confirmed against the extractor on this exact file:

pageType="pages"  as written (bare export)       extraConfig = {}
pageType="pages"  with export const config = {}  extraConfig = {"maxDuration":10}
pageType="app"    as written (bare export)       extraConfig = {"maxDuration":10}

-export const maxDuration = 10;
+export const config = { maxDuration: 10 };

To be clear about the severity: no request fails today. The abort is 5s and every Vercel plan's default ceiling is 10s or more, so "the abort trips before the platform kills the invocation" happens to hold anyway. What's wrong is that the comment above it states a guarantee that isn't actually in force, and the whole point of N2 was to make that guarantee explicit. If the abort is ever raised trusting the declared 10s, the catch stops logging again, which is precisely the silent-failure mode N1 just fixed. That's why I'd rather it be real than incidental.

Verified after applying it: 48/48 tests still pass, tsc clean.

---

Two nits, take or leave

name still has the pre-N3 ordering. The fix landed for email but not the field beside it, so a required field can arrive blank:

fullName "***"      HTTP 200  "*Name:* "
fullName "<>|`"     HTTP 200  "*Name:* "

!name passes on the raw value, then sanitize maps every character to a space and collapses it away. No injection risk, just the same ordering one field over.

-  const name = String(data.fullName ?? "").trim();
+  // Sanitize before validating, same reason as the email below: sanitize() maps
+  // < > | * ` and newlines to spaces, so a raw-passing value like "***" would
+  // collapse to "" and ship a blank *Name:* line despite the non-empty check.
+  const name = sanitize(String(data.fullName ?? ""), 120);
...
   const lead = {
-    name: sanitize(name, 120),
+    name,

With that applied:

fullName "***"          HTTP 400  (not delivered)
fullName "<>|`"         HTTP 400  (not delivered)
fullName " Jane "       HTTP 200  "*Name:* Jane"
fullName "Ann O'Reilly" HTTP 200  "*Name:* Ann O'Reilly"
300-char name           HTTP 200  capped at 120

The apostrophe case is deliberate: ' isn't in the strip set, so real names are unaffected.

The catch comment says "the 15s abort" while the abort is now 5000 (lines 234 vs 201). Introduced by this commit.

---

The evidence on what's already fixed

N1, production observability. All three failure modes now surface in production, the abort is distinguished from a network error, and the webhook secret never reaches the log:

=== NODE_ENV=development ===
  network   HTTP 200  logged=YES  secretLeak=no
            "[blog-lead] delivery to Google Chat failed (network error) — verify GOOGLE_CHAT_WEB…"
  abort     HTTP 200  logged=YES  secretLeak=no
            "[blog-lead] delivery to Google Chat failed (timed out) — verify GOOGLE_CHAT_WEBHOOK…"
  http500   HTTP 200  logged=YES  secretLeak=no

=== NODE_ENV=production ===
  network   HTTP 200  logged=YES  secretLeak=no
  abort     HTTP 200  logged=YES  secretLeak=no
  http500   HTTP 200  logged=YES  secretLeak=no

Production previously logged nothing for the network and abort cases. Fail-open is intact, every path still returns 200. I asserted against the literal key= / token= values in the webhook URL rather than just the message shape, so the no-leak result is a real check.

N3, sanitize before validate.

email a*b@x.com     HTTP 400  (not delivered)
email a_b~c@x.com   HTTP 200  "*Email:* a_b~c@x.com"
email A.B@Keploy.IO HTTP 200  "*Email:* a.b@keploy.io"

The mangled-address case 400s instead of delivering a b@x.com, _/~ still survive per the earlier decision, and server-side lowercasing still happens.

Happy to approve as soon as the export const config line lands.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[amaan-bhati]

Re-checked at 4542438. The blocker is unchanged

The two new commits are a main merge (which brought in #416), so nothing here addresses the previous round. Re-verified on this head rather than assuming:

The blocker, still present. blog-lead-notify.ts:42 is still the bare export, and Next still ignores it for a Pages Router route:

pageType="pages"  extraConfig = {}
pageType="app"    extraConfig = {"maxDuration":10}

-export const maxDuration = 10;
+export const config = { maxDuration: 10 };

Line 198's comment still promises maxDuration (10s) is in force. It isn't.

Both nits also unchanged. name still validates raw and delivers sanitized (:155 / :171):

fullName '***'     HTTP 200  "*Name:* "
fullName '<>|`'    HTTP 200  "*Name:* "
fullName 'Jane'    HTTP 200  "*Name:* Jane"

And :234 still reads "the 15s abort" while the abort is 5000.

The merge itself is clean: npm run test:unit 52/52 (up from 48, picking up #416's cases), npx tsc --noEmit clean. CI: build, E2E chromium, DCO pass; lighthouse still running.

Same as before — happy to approve once the export const config line lands. The other two are optional.

[dhananjay6561]

hey you're looking at 4542438 which is the main merge commit, one behind. the actual fix is in 8f4ff8b sitting right on top of it and it's already on the remote head. can you take a pull and recheck at 8f4ff8b?

all three are in there: line 47 is now export const config = { maxDuration: 10 } with a note above explaining the pages vs app extractor thing so it doesn't creep back, name is sanitized before validate at line 165, and the stale 15s comment is now 5s at line 244. if you run the extractor again at that head you'll get extraConfig with maxDuration for pageType pages.

the mixup is just timing, the main merge got pushed to the branch while the last round was open and my fix went on after it, so it's the newest commit. fetch and you should see all three resolved.

[amaan-bhati]

Approving. Full local verification at 8f4ff8b

Verified the whole surface, not just the last delta. Isolated worktree, next dev, a local sink on :4601 standing in for the Chat webhook so nothing left the machine and no real space was touched, plus headless Chromium for the UI path.

npm run test:unit 52/52 · npx tsc --noEmit clean · CI green on this head: build, lighthouse, E2E chromium, DCO.

Rejection paths — nothing delivered

GET  (wrong method)              405  {"ok":false,"error":"method_not_allowed"}
POST {}                          400  {"ok":false,"error":"validation"}
POST invalid email               400  {"ok":false,"error":"validation"}
POST fullName "***"              400  {"ok":false,"error":"validation"}
POST email a*b@x.com             400  {"ok":false,"error":"validation"}
POST honeypot filled             200  {"ok":true}

sink hits from all six: 0

The two 400s in the middle are the validate-after-sanitize ordering on both fields: each would previously have passed validation and delivered a mangled value.

Delivery paths — what the sink actually received

*Name:* Amaan Bhati
*Email:* amaan.bhati@keploy.io                        <- sent Amaan.Bhati@Keploy.IO
*Page:* /blog/technology/x?utm_source=q3_launch       <- underscore preserved

Injection attempt, sent with forged *Email:* / *Company:* lines containing real newlines, a <url|label> link, a code span, bold, and source: "admin-override":

*Name:* Evil bold Email: fake@evil.com Company: Forged   <- collapsed inline, no forged fields
*Email:* a_b~c@x.com                                     <- '_' and '~' both survive
*Company:* https://evil.com CLICK                        <- link defused
*Page:* /p?a= code &b= x                                 <- backtick + bold stripped
*Source:* blog-newsletter                                <- forged source pinned

Caps, measured on the delivered payload:

Name delivered length = 120   (sent 300)
Page delivered length = 500   (sent 600)

Abuse controls

req 1..5 -> 200 {"ok":true}
req 6    -> 429 {"ok":false,"error":"rate_limited"}
req 7    -> 429 {"ok":false,"error":"rate_limited"}

Observability, both envs

next dev forces development, so I drove the handler directly with a mocked req/res and flipped only NODE_ENV:

NODE_ENV=development
  network  HTTP 200  logged=YES  secretLeak=no  "…failed (network error) — verify GOOGLE…"
  abort    HTTP 200  logged=YES  secretLeak=no  "…failed (timed out) — verify GOOGLE_CHA…"
  http500  HTTP 200  logged=YES  secretLeak=no  "…rejected the message (HTTP 500) — …"

NODE_ENV=production
  network  HTTP 200  logged=YES  secretLeak=no
  abort    HTTP 200  logged=YES  secretLeak=no
  http500  HTTP 200  logged=YES  secretLeak=no

All three visible in production, abort distinguished from network error, fail-open intact on every path. I asserted against the literal key= / token= values in the webhook URL, not just the message shape, so secretLeak=no is a real check.

Function ceiling

pageType="pages"  extraConfig = {"maxDuration":10}

:47   export const config = { maxDuration: 10 };
:211  const timer = setTimeout(() => controller.abort(), 5000);

Read back through Next's own static-info extractor, so the 10s ceiling is genuinely in force and the 5s abort sits under it — the catch can run and log before the platform reclaims the invocation.

UI double-submit guard

same-tick x3 (the race the ref guards)   POSTs = 1
sequential x3, 400ms apart               POSTs = 2   gap = 888ms

The same-tick case is the one the ref exists for and it holds at exactly one POST. The sequential case is an in-flight guard behaving correctly — the second POST landed 888ms later, after the first had settled. Worth noting I could not verify whether the success state locks the form, because the newsletter subscription host isn't reachable from this environment, so subscribed never flips locally. In production that path is what would swap the UI.

---

Everything from the six rounds is closed and verified: the sanitizer contract and its _/~ decision, source pinning, honeypot, per-IP rate limit, field caps, fail-open, production observability, validate-before-sanitize on both fields, and the function-ceiling guarantee now being real rather than incidental. Approving.

[nehagup]

Review — newsletter form → Google Chat notify

Recommendation: Approve with nits. Well-engineered and hardened across the review iterations; the security-critical items all pass at head, and CI is green (E2E / build / Lighthouse / DCO).

Security / abuse — passes

• No committed secret — the webhook is process.env.GOOGLE_CHAT_WEBHOOK_URL (never NEXT_PUBLIC_*); .env.local.example ships it commented-out as a placeholder. Error logs are PII- and secret-free.
• Endpoint is defended — honeypot, per-IP rate limit (5/min, with correct x-real-ip / last-XFF handling against spoofing), field caps, a fetch timeout, and fail-open so a Chat outage never blocks a subscription. User input is sanitized before it enters the Chat card.

Should-fix

• File the durable fix as a tracked issue, not a // follow-up. The rate limiter is in-memory per serverless instance, so its effective ceiling rises as Vercel scales out under load. The real mitigation — send this notification server-to-server from the already-reCAPTCHA-verified /blog-mql path and delete this public endpoint — is only referenced in comments. Reasonable stopgap for an internal-channel notifier, but "follow-up" should be a real ticket.

Process — worth reconciling before merge

The PR shows an APPROVED decision, but every formal review on the thread is COMMENTED, and the last one explicitly held approval pending the maxDuration fix. That fix landed in a later commit, but no re-approval was submitted afterward. Worth confirming the final commit is actually approved before merging on the strength of that state.

Nits

• Rate-map eviction only prunes expired IPs above a 10k-entry threshold, so >10k IPs rotated within one 60s window could grow the map unbounded for the instance lifetime — add a hard cap / LRU if the /blog-mql refactor slips.
• Autolinking of bare URLs in the name / company fields inside the Chat message is by design — no deceptive-label vector (<, >, | are stripped) — noted for the record, no action.

Scope

Clean — 517 of the 628 additions are the handler + 48 unit tests; the client change is small and entirely in service of the feature.

_{Related (not for this PR): this endpoint is better protected than landing's /trial/submit, which still lacks a honeypot / rate limit — worth backporting these defenses there.}

_{🤖 Assisted review via Claude Code.}