Add loops email service - #38
Conversation
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
REQUEST_CHANGES — one blocking security issue remains.
Finding
- [P1] Redact the Axios error before logging it —
server/src/app.ts:81. The new catch path passes the rawAxiosErrorto Winston. Axios errors retain bothconfig.headers.Authorizationand the serialized request headers, and the repository's currentformat.errors+format.splat+ JSON logger emits those fields. A controlled failed-request reproduction with a canary credential confirmed that the completeBearervalue appears in the log output. Any Loops timeout, authentication error, or other request failure would therefore copyNEWSLETTER_API_TOKENinto the rotating application logs. Please log only explicitly sanitized fields such as the error message/code and upstream status, or add central header redaction, and cover the failure path so credentials cannot regress into logs.
Validation
git diff --check ed2c8c9...6102fd9— passed.npx --yes bun run testinserver— passed (5/5 tests).npx --yes bun run build:releaseinserver— passed.npx --yes bun run buildinwebsite— passed.- Targeted Prettier check for all changed website files — passed. The repository-wide website check still reports 19 files outside this PR's diff.
- Verified the request method, endpoint, camel-case contact fields, and
mailingListspayload shape against the official Loops Update Contact API contract.
Aside from the credential leak above, I found no additional blocking issue in the exact base-to-head diff.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
APPROVE — the prior credential-leak blocker is resolved at b3d6567; no blocking findings remain.
The new toSafeLogError allow-list removes Axios request, response, config, and headers before the error reaches Winston, while retaining only the useful name, message, code, and upstream status. Its regression test serializes an Axios error containing a canary bearer token through the production logger format and confirms neither the token nor an authorization field is emitted. I also traced all three signup callers and verified the upsert endpoint and mailing-list payload against the official Loops Update Contact contract.
Validation:
git diff --check ed2c8c9...b3d6567— passed.npx --yes bun run testinserver— passed (10/10 tests).npx --yes bun run build:releaseinserver— passed.npx --yes bun run buildinwebsite— passed.- Prettier check for all changed website files — passed. The repository-wide website check still reports the same 19 files outside this PR's diff.
- GitHub currently reports no CI check runs for this head.
Non-blocking deployment note: the server now requires NEWSLETTER_MAILING_LIST_ID and LAUNCH_MAILING_LIST_ID; this repository has no tracked server deployment manifest against which to verify those external values.
Change email subscription to loops