fix: forward SMTP attachments - #405
Conversation
Entire-Checkpoint: 7d7cb24de113
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe SMTP server now extracts email attachments parsed by mailparser and forwards them to the useSend API. The Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying usesend with
|
| Latest commit: |
e5ffda8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://998a15dd.usesend.pages.dev |
| Branch Preview URL: | https://fix-smtp-forward-attachments.usesend.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/smtp-server/src/server.ts (1)
38-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact attachment content from error logs.
The error logging now includes the full
emailDatawhich contains base64-encoded attachment content (lines 102-108). This creates both privacy and performance concerns:
- Privacy risk: Attachments may contain sensitive or confidential data (PII, documents, images) that should not be logged.
- Log bloat: Large attachments (approaching the 10MB limit at line 129) will create enormous log entries, making logs difficult to manage and consuming excessive storage.
- Compliance: Logging user data without redaction conflicts with the repository-wide PII redaction policy.
🔒 Proposed fix to redact attachments from logs
if (!response.ok) { const errorData = await response.text(); + const redactedEmailData = { ...emailData }; + if (redactedEmailData.attachments) { + redactedEmailData.attachments = redactedEmailData.attachments.map( + (att: any) => ({ filename: att.filename, size: att.content?.length || 0 }) + ); + } console.error( "useSend API error response: error:", JSON.stringify(errorData, null, 4), - `\nemail data: ${emailDataText}`, + `\nemail data: ${JSON.stringify(redactedEmailData, null, 2)}`, );Based on learnings: Actionable guideline: enforce PII data redaction across the codebase by centralizing with a Pino serializer rather than ad-hoc fixes. Treat logging of full email addresses at info level as a repository-wide concern and audit all ts files that log user identifiers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/smtp-server/src/server.ts` around lines 38 - 42, The error log currently prints full emailData (via emailDataText/JSON.stringify) which includes base64 attachments; update the logging in server.ts where console.error is called (the useSend API error path) to redact attachment bodies before stringifying by removing or replacing the attachment content field(s) (e.g., set attachment.data or attachment.content to "<redacted_attachment>" or omit the attachment payload entirely) and instead include non-sensitive metadata (filename, contentType, size); prefer using a centralized redaction helper or pino serializer for emailData to ensure attachments are never logged and replace the current JSON.stringify(emailData, ...) with the sanitized version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/smtp-server/src/server.ts`:
- Around line 102-108: The attachments block assumes parsed.attachments exists
and uses parsed.attachments.length directly; update the guard to use optional
chaining and safe mapping: check parsed.attachments?.length or equivalent and
only map when parsed.attachments is defined (e.g., use
parsed.attachments?.length ? parsed.attachments.map(...) : undefined or map over
parsed.attachments ?? [] and convert to undefined if empty), referring to the
attachments mapping in server.ts where the parsed variable is used.
---
Outside diff comments:
In `@apps/smtp-server/src/server.ts`:
- Around line 38-42: The error log currently prints full emailData (via
emailDataText/JSON.stringify) which includes base64 attachments; update the
logging in server.ts where console.error is called (the useSend API error path)
to redact attachment bodies before stringifying by removing or replacing the
attachment content field(s) (e.g., set attachment.data or attachment.content to
"<redacted_attachment>" or omit the attachment payload entirely) and instead
include non-sensitive metadata (filename, contentType, size); prefer using a
centralized redaction helper or pino serializer for emailData to ensure
attachments are never logged and replace the current JSON.stringify(emailData,
...) with the sanitized version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3d129e2-862c-475e-ad5c-287230dc3daa
📒 Files selected for processing (1)
apps/smtp-server/src/server.ts
Entire-Checkpoint: 0b350a0d62e6
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/smtp-server/src/server.ts (1)
127-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact email addresses from info-level logs.
This line logs full email addresses (
emailObject.to) at info level, which violates the repository's PII handling policy.🔒 Recommended fix
- .then(() => console.log("Email sent successfully to: ", emailObject.to)) + .then(() => console.log("Email sent successfully"))Based on learnings: The repository standard is to enforce PII data redaction across the codebase by centralizing with a Pino serializer rather than ad-hoc fixes, treating logging of full email addresses at info level as a repository-wide concern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/smtp-server/src/server.ts` at line 127, This logs full email addresses via console.log(emailObject.to) which exposes PII; replace the ad‑hoc console.log call in apps/smtp-server/src/server.ts with the project logger (e.g., processLogger or the module's pino logger) and emit a non-PII message (e.g., "Email sent") or pass a redacted recipient value; do not log raw emailObject.to. Ensure the change relies on the centralized Pino serializer/redaction policy rather than embedding a manual redact routine so email addresses are consistently redacted across the service.
♻️ Duplicate comments (1)
apps/smtp-server/src/server.ts (1)
117-122:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against undefined
parsed.attachmentswith optional chaining.The code accesses
parsed.attachments.lengthdirectly without checking ifattachmentsis defined. According to the@types/mailparsertype definitions, theattachmentsproperty in ParsedMail is optional (attachments?: Attachment[]) and may be undefined. This will cause a runtime TypeError if no attachments are present.🛡️ Recommended fix
attachments: - parsed.attachments.length > 0 + parsed.attachments?.length ? parsed.attachments.map((attachment, index) => ({ filename: attachment.filename || `attachment-${index + 1}`, content: attachment.content.toString("base64"), })) : undefined,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/smtp-server/src/server.ts` around lines 117 - 122, The code accesses parsed.attachments.length directly and can throw if attachments is undefined; update the block that builds the attachments array (the parsed.attachments mapping logic in server.ts) to use optional chaining and/or a safe check (e.g., parsed.attachments?.length or parsed.attachments?.map) so it only maps when attachments exists, and return undefined otherwise; ensure the mapping still applies the filename fallback (`attachment.filename || \`attachment-${index + 1}\``) and base64 encoding of attachment.content when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/smtp-server/src/server.ts`:
- Around line 38-55: The log currently prints emailDataForLog (constructed from
emailData and its attachments) directly in the console.error call, which leaks
PII in the to/from/replyTo fields; update the construction of emailDataForLog
before the console.error so that any addresses in emailData.to, emailData.from,
and emailData.replyTo are redacted (e.g., replace local-part with **** or only
keep domain) while preserving other non-PII fields and the existing attachment
sanitization; modify the logic that builds emailDataForLog (the ternary that
maps attachments) to also call a small sanitizer function applied to those three
fields and use the sanitized emailDataForLog in the console.error call (or,
preferably, switch to structured logging/serializer if a Pino serializer is
available in the codebase).
---
Outside diff comments:
In `@apps/smtp-server/src/server.ts`:
- Line 127: This logs full email addresses via console.log(emailObject.to) which
exposes PII; replace the ad‑hoc console.log call in
apps/smtp-server/src/server.ts with the project logger (e.g., processLogger or
the module's pino logger) and emit a non-PII message (e.g., "Email sent") or
pass a redacted recipient value; do not log raw emailObject.to. Ensure the
change relies on the centralized Pino serializer/redaction policy rather than
embedding a manual redact routine so email addresses are consistently redacted
across the service.
---
Duplicate comments:
In `@apps/smtp-server/src/server.ts`:
- Around line 117-122: The code accesses parsed.attachments.length directly and
can throw if attachments is undefined; update the block that builds the
attachments array (the parsed.attachments mapping logic in server.ts) to use
optional chaining and/or a safe check (e.g., parsed.attachments?.length or
parsed.attachments?.map) so it only maps when attachments exists, and return
undefined otherwise; ensure the mapping still applies the filename fallback
(`attachment.filename || \`attachment-${index + 1}\``) and base64 encoding of
attachment.content when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3698b68-7150-4f32-9afb-08ac3c5ebb8e
📒 Files selected for processing (1)
apps/smtp-server/src/server.ts
Entire-Checkpoint: 05a0a1779c5e
|
Addressed the latest CodeRabbit review notes:
Verification rerun:
|
|
Scope correction: reverted the logging/redaction follow-up changes. The PR diff is now limited to issue #399 only: forwarding parsed SMTP attachments to the API as base64 attachment payloads. Verification rerun:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/smtp-server/src/server.ts (1)
102-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAttachment payload now leaks sensitive content through error logs.
Because attachments are now embedded as base64 in
emailObject, the existing raw log at Line 41 (emailDataText) will leak attachment bodies (and email PII) on API failures. Please sanitize before logging.Suggested fix
async function sendEmailToUseSend(emailData: any, apiKey: string) { try { @@ - const emailDataText = JSON.stringify(emailData); + const emailDataText = JSON.stringify(emailData); + const emailDataForLog = { + ...emailData, + to: emailData.to ? "[REDACTED]" : undefined, + from: emailData.from ? "[REDACTED]" : undefined, + replyTo: emailData.replyTo ? "[REDACTED]" : undefined, + attachments: Array.isArray(emailData.attachments) + ? emailData.attachments.map((a: any) => ({ + filename: a.filename, + contentType: a.contentType, + size: + typeof a.content === "string" + ? Buffer.byteLength(a.content, "base64") + : undefined, + })) + : undefined, + }; @@ - `\nemail data: ${emailDataText}`, + `\nemail data: ${JSON.stringify(emailDataForLog)}`, );Based on learnings: enforce repository-wide PII redaction in logs and avoid ad-hoc exposure of email identifiers/content.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/smtp-server/src/server.ts` around lines 102 - 108, The code is embedding full attachment bodies as base64 into emailObject (via parsed.attachments) which causes emailDataText logs to leak PII and attachment content; fix by removing or redacting attachment content before any logging: change the construction that maps parsed.attachments so it only includes safe metadata (e.g., filename, contentType, size) and omits or replaces content with a placeholder like "<redacted_attachment>" and update any code that builds emailDataText to use this sanitized version (reference parsed.attachments, emailObject, and emailDataText) so no raw attachment bytes or sensitive fields are ever logged; apply the same redaction pattern for other personal fields when assembling logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/smtp-server/src/server.ts`:
- Around line 104-107: The attachment mapping in parsed.attachments currently
builds objects with filename and base64 content but omits the attachment MIME
type; update the mapper used where parsed.attachments.map(...) is defined so
each returned object also includes the original attachment.contentType (e.g.,
contentType: attachment.contentType) or a sensible fallback, preserving MIME
metadata for downstream MIME handling in the code paths that use this mapped
array.
---
Duplicate comments:
In `@apps/smtp-server/src/server.ts`:
- Around line 102-108: The code is embedding full attachment bodies as base64
into emailObject (via parsed.attachments) which causes emailDataText logs to
leak PII and attachment content; fix by removing or redacting attachment content
before any logging: change the construction that maps parsed.attachments so it
only includes safe metadata (e.g., filename, contentType, size) and omits or
replaces content with a placeholder like "<redacted_attachment>" and update any
code that builds emailDataText to use this sanitized version (reference
parsed.attachments, emailObject, and emailDataText) so no raw attachment bytes
or sensitive fields are ever logged; apply the same redaction pattern for other
personal fields when assembling logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 073e351d-3242-4831-a684-acdd40fb05c1
📒 Files selected for processing (1)
apps/smtp-server/src/server.ts
| ? parsed.attachments.map((attachment, index) => ({ | ||
| filename: attachment.filename || `attachment-${index + 1}`, | ||
| content: attachment.content.toString("base64"), | ||
| })) |
There was a problem hiding this comment.
contentType metadata is being dropped from forwarded attachments.
The mapped object includes filename and content, but not contentType. This misses the stated objective to preserve attachment metadata and may break downstream MIME handling.
Suggested fix
attachments:
parsed.attachments.length > 0
? parsed.attachments.map((attachment, index) => ({
filename: attachment.filename || `attachment-${index + 1}`,
+ contentType: attachment.contentType,
content: attachment.content.toString("base64"),
}))
: undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ? parsed.attachments.map((attachment, index) => ({ | |
| filename: attachment.filename || `attachment-${index + 1}`, | |
| content: attachment.content.toString("base64"), | |
| })) | |
| ? parsed.attachments.map((attachment, index) => ({ | |
| filename: attachment.filename || `attachment-${index + 1}`, | |
| contentType: attachment.contentType, | |
| content: attachment.content.toString("base64"), | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/smtp-server/src/server.ts` around lines 104 - 107, The attachment
mapping in parsed.attachments currently builds objects with filename and base64
content but omits the attachment MIME type; update the mapper used where
parsed.attachments.map(...) is defined so each returned object also includes the
original attachment.contentType (e.g., contentType: attachment.contentType) or a
sensible fallback, preserving MIME metadata for downstream MIME handling in the
code paths that use this mapped array.
Summary
Verification
Closes #399
Summary by CodeRabbit