Feat: expand Mail API ecosystem with BYOK, Broadcasts, and robust security fixes#172
Merged
Conversation
yash-pouranik
changed the title
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a Mail Platform: MailLog model and queue persistence, public-api webhook/batch send/audiences/contacts/broadcasts with BYOK and svix verification, dashboard-api project proxy routes for mail operations, and a web dashboard UI for managing logs/audiences/broadcasts. ChangesMail Platform Feature Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… feat/mail-platform-expansion
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the existing Mail functionality into a broader “mail platform” across the public API, dashboard API, shared common package, and web dashboard UI—adding mail delivery logging, batch sending, webhook-driven status updates, BYOK-gated audiences/contacts, and marketing broadcasts, plus security hardening around webhook verification and project-scoped live-status lookups.
Changes:
- Added persistent
MailLogstorage and integrated mail logging from the async queue worker. - Introduced new mail platform API surface area (webhook receiver, batch send, logs/status, audiences/contacts, broadcasts) and dashboard proxy endpoints + UI page.
- Added webhook raw-body parsing support and Svix dependency for signature verification.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/queues/publicEmailQueue.js | Creates MailLog entries after successful queue-driven sends. |
| packages/common/src/models/MailLog.js | New Mongoose schema for per-project mail delivery logs. |
| packages/common/src/models/index.js | Exposes MailLog from the models barrel. |
| packages/common/src/index.js | Exports MailLog from the common package entrypoint. |
| package-lock.json | Adds svix dependency resolution. |
| apps/web-dashboard/src/pages/OtpVerification.jsx | Adjusts resend-enable state update timing. |
| apps/web-dashboard/src/pages/MailPlatform.jsx | New dashboard UI for logs, audiences/contacts, and broadcasts. |
| apps/web-dashboard/src/components/Layout/ProjectNavbar.jsx | Adds “Mail” navigation link/icon. |
| apps/web-dashboard/src/App.jsx | Registers the new /project/:projectId/mail route. |
| apps/public-api/src/routes/mail.js | Adds mail platform routes (webhook, batch, logs/status, audiences/contacts, broadcasts). |
| apps/public-api/src/controllers/mail.controller.js | Implements webhook verification, logs/status, batch send, audiences/contacts, and broadcasts logic. |
| apps/public-api/src/app.js | Adds express.raw() middleware for the webhook route before express.json(). |
| apps/public-api/package.json | Adds svix dependency. |
| apps/dashboard-api/src/routes/projects.js | Adds dashboard mail proxy routes (logs, live status, audiences/contacts, broadcasts). |
| apps/dashboard-api/src/controllers/project.controller.js | Implements dashboard mail proxy endpoints and live-status ownership check. |
| apps/dashboard-api/src/tests/routes.projects.storage.test.js | Updates controller mocks to include new mail proxy handlers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // -------------------- MAIL TEMPLATES (Phase 2 feature) -------------------- | ||
|
|
||
| const { MailTemplate } = require("@urbackend/common"); | ||
| const { MailTemplate, MailLog } = require("@urbackend/common"); |
| console.error("Failed to decrypt project resend key", e); | ||
| } | ||
| } | ||
| return { key: process.env.RESEND_API_KEY, isByok: false }; |
Comment on lines
+2425
to
+2432
| return res.json({ | ||
| success: true, | ||
| data: { | ||
| id: resendId, | ||
| last_event: "delivered (simulated / test pool)", | ||
| to: ["Queried successfully from local MailLog edge"], | ||
| created_at: new Date().toISOString(), | ||
| note: "Resend API Edge returned 404 Not Found. Since this dispatch used a shared sandbox or test pool token, real-time edge tracing logs are ephemeral and not persisted on external Resend matrix servers." |
Comment on lines
+733
to
+739
| const { segmentId, from, subject, html, scheduledAt } = req.body; | ||
| if (!segmentId || !subject || !html) { | ||
| return res.status(400).json({ success: false, data: {}, message: "segmentId, subject, and html are required." }); | ||
| } | ||
|
|
||
| const payload = { | ||
| audienceId: segmentId, |
Comment on lines
+451
to
+456
| module.exports.handleResendWebhook = async (req, res) => { | ||
| const secret = process.env.RESEND_WEBHOOK_SECRET; | ||
| if (!secret) { | ||
| return res.status(200).json({ success: true, message: "Webhook ignored: secret not configured." }); | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/dashboard-api/src/controllers/project.controller.js (2)
2557-2596: ⚖️ Poor tradeoffConsider middleware for Pro plan check.
Line 2570 fetches the Developer document for every broadcast request. Since Pro plan gating is a recurring pattern, consider adding a middleware that attaches
req.developer(similar to howplanEnforcement.attachDeveloperworks in routes). This would eliminate the repeated DB query and centralize plan checks.Current implementation is functional and correct, so this optimization is not urgent.
🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 2557 - 2596, The sendMarketingBroadcast controller currently re-queries the Developer model inside the handler (Developer.findById) to enforce the Pro plan; extract that repeated pattern into middleware that loads and attaches the developer to the request (e.g., req.developer) and/or performs plan checks so controllers can use the attached developer, similar to planEnforcement.attachDeveloper; update sendMarketingBroadcast to use req.developer (and remove the Developer.findById call and plan check there) and register the new middleware on routes that need Pro gating to avoid redundant DB queries and centralize enforcement.
2441-2474: 💤 Low valueConsider Express-native method routing.
The manual
req.methodchecks work, but Express routing would be cleaner. Instead of one handler checking method, use separaterouter.get()androuter.post()in the routes file. This improves clarity and leverages Express's built-in routing.Current pattern works and is consistent with
manageContacts, so deferring this is acceptable.🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 2441 - 2474, Split the single manageAudiences handler into two Express route handlers (one for GET and one for POST) and update routing to use router.get(...) and router.post(...) instead of inspecting req.method; extract the shared preflight logic (Project.findOne(...) with select(...), getResolvedResendKey(project) and the BYOK check) into a small helper used by both handlers so you keep the ownership and key validation consistent, then move the axios GET call to the new GET handler and the name validation + axios POST call to the new POST handler and remove the req.method branches and the final 405 response from manageAudiences.apps/public-api/src/controllers/mail.controller.js (2)
401-404: 💤 Low valueNo pagination on
getMailLogs.Hardcoded
.limit(50)with noskip/cursor leaves users unable to view older logs. The dashboard UI will silently cap. Consider accepting?limit(capped, e.g., 200) and?before=<ObjectId|Date>for keyset pagination over the(projectId, sentAt)compound index.🤖 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/public-api/src/controllers/mail.controller.js` around lines 401 - 404, The getMailLogs handler currently hardcodes .limit(50) and must be converted to keyset pagination: accept optional query params limit (parse and enforce an upper cap, e.g., 200) and before (accept either an ISO date string or an ObjectId-like value), compute a beforeDate/from ObjectId timestamp, and change the MailLog.find filter to include { projectId, sentAt: { $lt: beforeDate } } (or when supporting tie-breakers add _id < beforeId for equal sentAt), then .sort({ sentAt: -1, _id: -1 }).limit(limit). Ensure parsing/validation of limit and before and fall back to sensible defaults; update the getMailLogs controller and the MailLog.find call accordingly.
489-496: ⚡ Quick winBatch email schema lacks validation that single-send path enforces — fix inconsistency.
sendBatchSchemaacceptstoas unvalidatedz.string() | z.array(z.string()), whilesendMailSchema(imported from@urbackend/common) validates withz.string().email(). The batch endpoint will accept malformed addresses and pass them to Resend. Additionally,sendMailSchemapreprocesses subject with trim, butsendBatchSchemadoes not.Update
totoz.union([z.email(), z.array(z.email()).min(1)])and add subject preprocessing to match the single-send contract.♻️ Proposed fix
const sendBatchSchema = z.array( z.object({ - to: z.union([z.string(), z.array(z.string())]), - subject: z.string().min(1, "Subject is required"), + to: z.union([z.email(), z.array(z.email()).min(1)]), + subject: z.preprocess( + (value) => { + if (typeof value !== "string") return value; + return value.trim() === "" ? undefined : value; + }, + z.string().min(1, "Subject is required") + ), html: z.string().optional(), text: z.string().optional() }) ).min(1).max(100);🤖 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/public-api/src/controllers/mail.controller.js` around lines 489 - 496, sendBatchSchema currently allows unvalidated strings for the "to" field and doesn't trim "subject", causing inconsistency with sendMailSchema; update sendBatchSchema so "to" is z.union([z.string().email(), z.array(z.string().email()).min(1)]) (or use z.email() and z.array(z.email()).min(1)) to enforce valid emails for single and multiple recipients, and apply the same subject preprocessing used in sendMailSchema (e.g., a trim preprocess) so subjects are validated/normalized the same way as in sendMailSchema.
🤖 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/dashboard-api/src/controllers/project.controller.js`:
- Line 1471: There is a duplicate import of MailLog: remove the redundant const
{ MailTemplate, MailLog } = require("@urbackend/common"); declaration so the
file relies on the original MailLog import already present (keep MailTemplate
import where needed or merge into the existing require if you intended to add
MailTemplate there); ensure only a single MailLog symbol is imported to
eliminate the SyntaxError.
In `@apps/public-api/src/controllers/mail.controller.js`:
- Around line 394-446: Both getMailLogs and getMailStatus violate the response
envelope and leak raw DB errors: ensure every success response includes a
message field (e.g., "Mail logs retrieved successfully." / "Mail status
retrieved successfully.") and replace the catch blocks with the AppError-safe
pattern used elsewhere (check err.isOperational and return err.statusCode with
err.message; otherwise console.error with context like "[mail.getMailLogs]" or
"[mail.getMailStatus]" and return res.status(500).json({ success: false, data:
{}, message: "Internal server error." })), so you never expose raw
Mongoose/Mongo errors to clients.
- Around line 451-455: The handler handleResendWebhook currently returns a 200
success when RESEND_WEBHOOK_SECRET is unset; change this to fail closed by
rejecting requests when the secret is not configured: in handleResendWebhook
check process.env.RESEND_WEBHOOK_SECRET and if missing respond with a 500 (or
503/403 per policy) and a descriptive error message and log the condition (don’t
return { success: true }), so no incoming webhook is treated as verified when
the secret is absent; ensure the change is limited to the initial secret-check
branch and does not alter downstream signature verification or normal success
responses.
- Around line 468-486: Wrap the MailLog.updateOne call in a try/catch so DB
failures are logged but not rethrown (so the webhook still receives a 200);
e.g., surround MailLog.updateOne(...) with try { await MailLog.updateOne(...) }
catch (err) { logger.error?.('MailLog update failed', err) ||
console.error('MailLog update failed', err) } and do not throw or call next(err)
from the catch. Also ensure every response in this controller uses the standard
envelope { success, data, message } (replace bare res.status(...).json({
success: true }) with res.status(200).json({ success: true, data: {}, message:
'' }) and apply the same pattern to the other return points referenced around
MailLog.updateOne and the responses near lines 454/465/485).
- Around line 738-744: createBroadcast is building the payload but ignores the
project's resolved resendFromEmail; use the already-computed fromAddress from
resolveResendClient instead of falling back to a hardcoded string. Update the
payload construction in createBroadcast to set from: from || fromAddress ||
process.env.EMAIL_FROM || "urBackend <urbackend@apps.bitbros.in>" (preserving
existing fallbacks), ensuring you reference the resolved fromAddress variable
returned by resolveResendClient and keep the scheduledAt assignment as-is.
In `@packages/common/src/queues/publicEmailQueue.js`:
- Around line 87-89: The catch block that currently does console.error for
MailLog persistence failures must not drop the error; when creating MailLog (on
data.id) fails (catching logErr), route the failed record to a compensating
retry path or DLQ instead of only logging — for example push the payload and
error metadata into a dedicated retry queue or DLQ (or rethrow the error so the
job framework will retry) and include data.id and logErr in the queued message
to preserve context; update the catch in the MailLog creation code to enqueue
the failure to the retry/DLQ mechanism (or rethrow) rather than silently
continuing.
---
Nitpick comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 2557-2596: The sendMarketingBroadcast controller currently
re-queries the Developer model inside the handler (Developer.findById) to
enforce the Pro plan; extract that repeated pattern into middleware that loads
and attaches the developer to the request (e.g., req.developer) and/or performs
plan checks so controllers can use the attached developer, similar to
planEnforcement.attachDeveloper; update sendMarketingBroadcast to use
req.developer (and remove the Developer.findById call and plan check there) and
register the new middleware on routes that need Pro gating to avoid redundant DB
queries and centralize enforcement.
- Around line 2441-2474: Split the single manageAudiences handler into two
Express route handlers (one for GET and one for POST) and update routing to use
router.get(...) and router.post(...) instead of inspecting req.method; extract
the shared preflight logic (Project.findOne(...) with select(...),
getResolvedResendKey(project) and the BYOK check) into a small helper used by
both handlers so you keep the ownership and key validation consistent, then move
the axios GET call to the new GET handler and the name validation + axios POST
call to the new POST handler and remove the req.method branches and the final
405 response from manageAudiences.
In `@apps/public-api/src/controllers/mail.controller.js`:
- Around line 401-404: The getMailLogs handler currently hardcodes .limit(50)
and must be converted to keyset pagination: accept optional query params limit
(parse and enforce an upper cap, e.g., 200) and before (accept either an ISO
date string or an ObjectId-like value), compute a beforeDate/from ObjectId
timestamp, and change the MailLog.find filter to include { projectId, sentAt: {
$lt: beforeDate } } (or when supporting tie-breakers add _id < beforeId for
equal sentAt), then .sort({ sentAt: -1, _id: -1 }).limit(limit). Ensure
parsing/validation of limit and before and fall back to sensible defaults;
update the getMailLogs controller and the MailLog.find call accordingly.
- Around line 489-496: sendBatchSchema currently allows unvalidated strings for
the "to" field and doesn't trim "subject", causing inconsistency with
sendMailSchema; update sendBatchSchema so "to" is z.union([z.string().email(),
z.array(z.string().email()).min(1)]) (or use z.email() and
z.array(z.email()).min(1)) to enforce valid emails for single and multiple
recipients, and apply the same subject preprocessing used in sendMailSchema
(e.g., a trim preprocess) so subjects are validated/normalized the same way as
in sendMailSchema.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2549f6e6-2997-415d-93c3-a016ef832cab
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
apps/dashboard-api/src/__tests__/routes.projects.storage.test.jsapps/dashboard-api/src/controllers/project.controller.jsapps/dashboard-api/src/routes/projects.jsapps/public-api/package.jsonapps/public-api/src/app.jsapps/public-api/src/controllers/mail.controller.jsapps/public-api/src/routes/mail.jsapps/web-dashboard/src/App.jsxapps/web-dashboard/src/components/Layout/ProjectNavbar.jsxapps/web-dashboard/src/pages/MailPlatform.jsxapps/web-dashboard/src/pages/OtpVerification.jsxpackages/common/src/index.jspackages/common/src/models/MailLog.jspackages/common/src/models/index.jspackages/common/src/queues/publicEmailQueue.js
Comment on lines
+394
to
+446
| module.exports.getMailLogs = async (req, res) => { | ||
| try { | ||
| const projectId = req.project?._id; | ||
| if (!projectId) { | ||
| return res.status(401).json({ success: false, data: {}, message: "Project context missing." }); | ||
| } | ||
|
|
||
| const logs = await MailLog.find({ projectId }) | ||
| .sort({ sentAt: -1 }) | ||
| .limit(50) | ||
| .lean(); | ||
|
|
||
| return res.status(200).json({ | ||
| success: true, | ||
| data: logs, | ||
| message: "Mail logs retrieved successfully." | ||
| }); | ||
| } catch (err) { | ||
| return res.status(500).json({ success: false, data: {}, message: err.message || "Failed to retrieve mail logs." }); | ||
| } | ||
| }; | ||
|
|
||
| // GET /api/mail/logs/:resendId | ||
| module.exports.getMailStatus = async (req, res) => { | ||
| try { | ||
| const { resendId } = req.params; | ||
| if (!resendId) return res.status(400).json({ success: false, data: {}, message: "resendId is required." }); | ||
|
|
||
| const projectId = req.project?._id; | ||
| const logEntry = await MailLog.findOne({ resendEmailId: resendId, projectId }).lean(); | ||
| if (!logEntry) { | ||
| return res.status(404).json({ success: false, data: {}, message: "Mail log entry not found for this project." }); | ||
| } | ||
|
|
||
| const { resend } = await resolveResendClient(req); | ||
| const { data, error } = await resend.emails.get(resendId); | ||
| if (error) { | ||
| return res.status(error.statusCode || 500).json({ success: false, data: {}, message: error.message || "Failed to fetch email status from Resend." }); | ||
| } | ||
|
|
||
| return res.status(200).json({ | ||
| success: true, | ||
| data: { | ||
| dbLog: logEntry, | ||
| last_event: data?.last_event || logEntry.status, | ||
| resendStatus: data | ||
| }, | ||
| message: "Mail status retrieved successfully." | ||
| }); | ||
| } catch (err) { | ||
| return res.status(500).json({ success: false, data: {}, message: err.message || "Failed to fetch mail status." }); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Coding-guideline violations across new handlers: response envelope + Mongo error exposure.
Two systemic issues across getMailLogs, getMailStatus, and all audience/contact/broadcast handlers:
- Success responses like
return res.status(200).json({ success: true, data });(lines 595, 598, 607, 619, 631, 654, 666, 679, 699, 712, 749, 761, 773, 785, 797) omit the requiredmessagefield. The envelope must be{ success, data, message }. - Catch blocks return
err.messagedirectly to the client (e.g., lines 412, 444, 597, 609, 621, 633, 656, 668, 681, 701, 714, 751, 763, 775, 787, 799). For DB-driven paths (getMailLogs,getMailStatus) this can leak raw Mongoose/MongoDB error text (cast errors, validation messages, connection details). Route throughAppError(already used elsewhere — seeapp.js's operational-error branch at line 102) and return a sanitized message.
Suggested pattern:
} catch (err) {
if (err.isOperational) {
return res.status(err.statusCode).json({ success: false, data: {}, message: err.message });
}
console.error("[mail.<handler>] error:", err);
return res.status(500).json({ success: false, data: {}, message: "Internal server error." });
}As per coding guidelines: "All API endpoints return: { success: bool, data: {}, message: "" }. Use AppError class for errors — never raw throw, never expose MongoDB errors to client."
Also applies to: 586-801
🤖 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/public-api/src/controllers/mail.controller.js` around lines 394 - 446,
Both getMailLogs and getMailStatus violate the response envelope and leak raw DB
errors: ensure every success response includes a message field (e.g., "Mail logs
retrieved successfully." / "Mail status retrieved successfully.") and replace
the catch blocks with the AppError-safe pattern used elsewhere (check
err.isOperational and return err.statusCode with err.message; otherwise
console.error with context like "[mail.getMailLogs]" or "[mail.getMailStatus]"
and return res.status(500).json({ success: false, data: {}, message: "Internal
server error." })), so you never expose raw Mongoose/Mongo errors to clients.
Comment on lines
+451
to
+455
| module.exports.handleResendWebhook = async (req, res) => { | ||
| const secret = process.env.RESEND_WEBHOOK_SECRET; | ||
| if (!secret) { | ||
| return res.status(200).json({ success: true, message: "Webhook ignored: secret not configured." }); | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Webhook fails OPEN when RESEND_WEBHOOK_SECRET is unset.
If the env var is ever missing/misconfigured in production, every incoming request is acknowledged as { success: true } without signature verification, and MailLog will silently stop reflecting Resend events. This also lets anyone POST to /api/mail/webhook and receive a 200 (useful for probing). Fail closed instead.
🔒 Proposed fix
- const secret = process.env.RESEND_WEBHOOK_SECRET;
- if (!secret) {
- return res.status(200).json({ success: true, message: "Webhook ignored: secret not configured." });
- }
+ const secret = process.env.RESEND_WEBHOOK_SECRET;
+ if (!secret) {
+ console.error("[Webhook] RESEND_WEBHOOK_SECRET is not configured; refusing request.");
+ return res.status(500).json({ success: false, data: {}, message: "Webhook secret not configured." });
+ }🤖 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/public-api/src/controllers/mail.controller.js` around lines 451 - 455,
The handler handleResendWebhook currently returns a 200 success when
RESEND_WEBHOOK_SECRET is unset; change this to fail closed by rejecting requests
when the secret is not configured: in handleResendWebhook check
process.env.RESEND_WEBHOOK_SECRET and if missing respond with a 500 (or 503/403
per policy) and a descriptive error message and log the condition (don’t return
{ success: true }), so no incoming webhook is treated as verified when the
secret is absent; ensure the change is limited to the initial secret-check
branch and does not alter downstream signature verification or normal success
responses.
Comment on lines
+468
to
+486
| const { type, data } = evt; | ||
| if (data && data.email_id) { | ||
| let statusUpdate; | ||
| if (type === 'email.sent') statusUpdate = 'sent'; | ||
| else if (type === 'email.delivered') statusUpdate = 'delivered'; | ||
| else if (type === 'email.bounced') statusUpdate = 'bounced'; | ||
| else if (type === 'email.complained') statusUpdate = 'complained'; | ||
| else if (type === 'email.delivery_delayed') statusUpdate = 'queued'; | ||
|
|
||
| if (statusUpdate) { | ||
| await MailLog.updateOne( | ||
| { resendEmailId: data.email_id }, | ||
| { $set: { status: statusUpdate, updatedAt: new Date() } } | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return res.status(200).json({ success: true }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Wrap the DB update and complete the response envelope.
Two issues:
MailLog.updateOne(...)is awaited but not guarded — any Mongo failure will bubble out and the global error handler will return a 500 to Resend, causing automatic retries that may amplify load. Catch+log so the webhook still acks.- The success/failure responses here omit
data: {}and inconsistently include/omitmessage. Per coding guidelines, all controller responses should be{ success, data, message }. Same applies to line 454/465/485.
🛠️ Suggested fix
if (statusUpdate) {
- await MailLog.updateOne(
- { resendEmailId: data.email_id },
- { $set: { status: statusUpdate, updatedAt: new Date() } }
- );
+ try {
+ await MailLog.updateOne(
+ { resendEmailId: data.email_id },
+ { $set: { status: statusUpdate, updatedAt: new Date() } }
+ );
+ } catch (e) {
+ console.error("[Webhook] MailLog update failed:", e.message);
+ }
}
}
- return res.status(200).json({ success: true });
+ return res.status(200).json({ success: true, data: {}, message: "Webhook processed." });As per coding guidelines: "All API endpoints return: { success: bool, data: {}, message: "" }. Use AppError class for errors — never raw throw, never expose MongoDB errors to client."
🤖 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/public-api/src/controllers/mail.controller.js` around lines 468 - 486,
Wrap the MailLog.updateOne call in a try/catch so DB failures are logged but not
rethrown (so the webhook still receives a 200); e.g., surround
MailLog.updateOne(...) with try { await MailLog.updateOne(...) } catch (err) {
logger.error?.('MailLog update failed', err) || console.error('MailLog update
failed', err) } and do not throw or call next(err) from the catch. Also ensure
every response in this controller uses the standard envelope { success, data,
message } (replace bare res.status(...).json({ success: true }) with
res.status(200).json({ success: true, data: {}, message: '' }) and apply the
same pattern to the other return points referenced around MailLog.updateOne and
the responses near lines 454/465/485).
Comment on lines
+738
to
+744
| const payload = { | ||
| audienceId: segmentId, | ||
| from: from || process.env.EMAIL_FROM || "urBackend <urbackend@apps.bitbros.in>", | ||
| subject, | ||
| html | ||
| }; | ||
| if (scheduledAt) payload.scheduledAt = scheduledAt; |
Contributor
There was a problem hiding this comment.
createBroadcast ignores the project's resendFromEmail.
resolveResendClient already computes fromAddress from the project's BYOK config (or env fallback), but this handler doesn't destructure it and falls back to a hardcoded chain. A project with a configured resendFromEmail will see broadcasts go out from the global default instead of their domain — surprising and likely to harm BYOK deliverability.
♻️ Proposed fix
module.exports.createBroadcast = async (req, res) => {
try {
- const resend = await requireBroadcastGate(req);
+ const { resend, usingByok, fromAddress } = await resolveResendClient(req);
+ if (!usingByok || !req.planLimits?.byokEnabled) {
+ return res.status(403).json({
+ success: false, data: {},
+ message: "Broadcasts require both a BYOK Resend key and a Pro plan."
+ });
+ }
const { segmentId, from, subject, html, scheduledAt } = req.body;
if (!segmentId || !subject || !html) {
return res.status(400).json({ success: false, data: {}, message: "segmentId, subject, and html are required." });
}
const payload = {
audienceId: segmentId,
- from: from || process.env.EMAIL_FROM || "urBackend <urbackend@apps.bitbros.in>",
+ from: from || fromAddress,
subject,
html
};🤖 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/public-api/src/controllers/mail.controller.js` around lines 738 - 744,
createBroadcast is building the payload but ignores the project's resolved
resendFromEmail; use the already-computed fromAddress from resolveResendClient
instead of falling back to a hardcoded string. Update the payload construction
in createBroadcast to set from: from || fromAddress || process.env.EMAIL_FROM ||
"urBackend <urbackend@apps.bitbros.in>" (preserving existing fallbacks),
ensuring you reference the resolved fromAddress variable returned by
resolveResendClient and keep the scheduledAt assignment as-is.
Comment on lines
+87
to
+89
| } catch (logErr) { | ||
| console.error(`[Queue] Failed to create MailLog for emailId ${data.id}:`, logErr); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t silently drop MailLog persistence failures.
At Line 87–Line 89, logging and continuing means a successfully sent email can become permanently untraceable in MailLog, which can cause downstream status checks to return false 404s for legitimate sends. Route this to a compensating retry path (or dedicated DLQ) instead of only console.error.
🤖 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 `@packages/common/src/queues/publicEmailQueue.js` around lines 87 - 89, The
catch block that currently does console.error for MailLog persistence failures
must not drop the error; when creating MailLog (on data.id) fails (catching
logErr), route the failed record to a compensating retry path or DLQ instead of
only logging — for example push the payload and error metadata into a dedicated
retry queue or DLQ (or rethrow the error so the job framework will retry) and
include data.id and logErr in the queued message to preserve context; update the
catch in the MailLog creation code to enqueue the failure to the retry/DLQ
mechanism (or rethrow) rather than silently continuing.
…forgery' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…forgery' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…forgery' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Collaborator
Author
Agent-Logs-Url: https://github.com/geturbackend/urBackend/sessions/7e734a64-3505-452a-9560-8c2b7901d1db Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
Agent-Logs-Url: https://github.com/geturbackend/urBackend/sessions/7e734a64-3505-452a-9560-8c2b7901d1db Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
yash-pouranik
temporarily deployed
to
feat/mail-platform-expansion - urBackend-frankfrut PR #172
Render
Destroyed
Agent-Logs-Url: https://github.com/geturbackend/urBackend/sessions/7e734a64-3505-452a-9560-8c2b7901d1db Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
Agent-Logs-Url: https://github.com/geturbackend/urBackend/sessions/7e734a64-3505-452a-9560-8c2b7901d1db Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
Contributor
Apply usage limits to broadcast routes so BYOK Pro users pass gating, and use effective plan (with expiry) for marketing broadcasts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/dashboard-api/src/controllers/project.controller.js`:
- Around line 2394-2408: Update the getMailLogs controller to always return the
standardized envelope { success, data: {}, message: "" } and replace direct
error responses with AppError instances: when Project is not found, return
res.status(404).json({ success: false, data: {}, message: "Project not found" })
(or throw new AppError("Project not found", 404) and let middleware handle it),
and on any catch block wrap/throw an AppError rather than returning err.message;
ensure successful path returns res.json({ success: true, data: { logs },
message: "" }); apply the same pattern to the other related handlers referenced
(lines ~2411-2625) so all use Project, MailLog, AppError, and the consistent
envelope.
- Around line 2430-2432: The axios calls to the Resend API (e.g., the axios.get
using safeResendId and Authorization `key`) must be bounded with a shared
timeout option (e.g., timeout in ms) and error-handled to map
timeout/ETIMEDOUT/ECONNABORTED style errors to a 502 or 504 HTTP response rather
than hanging; update the axios invocation(s) at the shown call and the other
occurrences (around lines 2465-2477, 2503-2505, 2530-2543, 2574-2576, 2616-2618)
to pass the timeout option and catch errors, detecting timeout-specific error
codes/messages and returning res.status(502 or 504). Ensure the same shared
timeout value is reused across all Resend proxy calls for consistency.
- Around line 1439-1452: The current validation rejects empty resendApiKey and
prevents clearing an existing BYOK entry; change the logic in the block handling
resendApiKey so that when resendApiKey is provided but empty/null (e.g., after
trimming) you treat it as a clear operation instead of returning 400: set
updateFields.resendApiKey to null (or delete the field) to indicate removal, and
only call encrypt(trimmedKey) and assign updateFields.resendApiKey when
trimmedKey is non-empty and passes the /^re_[A-Za-z0-9_]+$/ check; keep the
existing error response for non-empty values that fail the format regex.
- Around line 2382-2392: getResolvedResendKey currently falls back to the global
RESEND_API_KEY when decrypt(project.resendApiKey) throws, which causes
wrong-account queries; change getResolvedResendKey so that if
project.resendApiKey exists but decrypt() fails it does NOT return the global
key but instead returns a null (or explicit "no usable key") result while
marking that a project key was configured (e.g. return { key: null, isByok: true
} or similar) so callers can distinguish "configured-but-invalid" from
"not-configured"; then update callers such as getResendLiveStatus to check for a
null/invalid key and avoid querying the global Resend account (handle as
missing/invalid project key rather than falling back to shared key).
In `@apps/public-api/src/__tests__/mail.controller.test.js`:
- Around line 346-358: The test is only toggling req.planLimits.byokEnabled so
it can get a 403 from BYOK logic instead of the Pro entitlement; update the
broadcast tests that call mailController.createBroadcast to explicitly set both
plan signals: keep BYOK satisfied (req.planLimits.byokEnabled = true) and set
the Pro gate value the controller expects (e.g., req.planLimits.proEnabled =
false for the test that must enforce the Pro gate, and proEnabled = true for the
positive case), so the assertion failures come from the Pro entitlement path
rather than BYOK; ensure decrypt.mockReturnValue and mockProjectConfig remain
unchanged so BYOK stays satisfied.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c2b5a87-493a-4113-884f-71d94e017f27
📒 Files selected for processing (4)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/__tests__/mail.controller.test.jsapps/public-api/src/controllers/mail.controller.jsapps/public-api/src/routes/mail.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/public-api/src/controllers/mail.controller.js
Comment on lines
1439
to
+1452
| if (resendApiKey !== undefined) { | ||
| if (typeof resendApiKey !== "string" || !resendApiKey.trim()) { | ||
| const trimmedKey = typeof resendApiKey === "string" ? resendApiKey.trim() : ""; | ||
| if (!trimmedKey) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "resendApiKey must be a non-empty string." }); | ||
| } | ||
| updateFields.resendApiKey = encrypt(resendApiKey.trim()); | ||
|
|
||
| // Sanitize the key: Prevent CRLF (HTTP Header Injection) and invalid characters | ||
| if (!/^re_[A-Za-z0-9_]+$/.test(trimmedKey)) { | ||
| return res.status(400).json({ error: "Invalid Resend API Key format." }); | ||
| } | ||
|
|
||
| updateFields.resendApiKey = encrypt(trimmedKey); |
Contributor
There was a problem hiding this comment.
Allow resendApiKey to be unset.
After this change, a project that already has BYOK configured cannot revert to the shared-key path via updateProject; sending an empty value just returns 400 and leaves the old encrypted key in place. Treat empty/null as a clear operation instead of forcing the previous key to stick around.
🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 1439 -
1452, The current validation rejects empty resendApiKey and prevents clearing an
existing BYOK entry; change the logic in the block handling resendApiKey so that
when resendApiKey is provided but empty/null (e.g., after trimming) you treat it
as a clear operation instead of returning 400: set updateFields.resendApiKey to
null (or delete the field) to indicate removal, and only call
encrypt(trimmedKey) and assign updateFields.resendApiKey when trimmedKey is
non-empty and passes the /^re_[A-Za-z0-9_]+$/ check; keep the existing error
response for non-empty values that fail the format regex.
Comment on lines
+2382
to
+2392
| const getResolvedResendKey = (project) => { | ||
| if (project?.resendApiKey?.encrypted) { | ||
| try { | ||
| const key = decrypt(project.resendApiKey); | ||
| if (key) return { key, isByok: true }; | ||
| } catch (e) { | ||
| console.error("Failed to decrypt project resend key", e); | ||
| } | ||
| } | ||
| return { key: process.env.RESEND_API_KEY_2 || process.env.RESEND_API_KEY, isByok: false }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Do not silently fall back to the shared key when a stored BYOK key can't be decrypted.
When project.resendApiKey exists but decrypt() fails, getResolvedResendKey() returns the global key. getResendLiveStatus() then queries the wrong Resend account and can incorrectly report a project-scoped mail log as missing. Only use the global fallback when no project key is configured at all.
Also applies to: 2426-2449
🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 2382 -
2392, getResolvedResendKey currently falls back to the global RESEND_API_KEY
when decrypt(project.resendApiKey) throws, which causes wrong-account queries;
change getResolvedResendKey so that if project.resendApiKey exists but decrypt()
fails it does NOT return the global key but instead returns a null (or explicit
"no usable key") result while marking that a project key was configured (e.g.
return { key: null, isByok: true } or similar) so callers can distinguish
"configured-but-invalid" from "not-configured"; then update callers such as
getResendLiveStatus to check for a null/invalid key and avoid querying the
global Resend account (handle as missing/invalid project key rather than falling
back to shared key).
Comment on lines
+2394
to
+2408
| module.exports.getMailLogs = async (req, res) => { | ||
| try { | ||
| const { projectId } = req.params; | ||
| const project = await Project.findOne({ _id: projectId, owner: req.user._id }); | ||
| if (!project) return res.status(404).json({ success: false, message: "Project not found" }); | ||
|
|
||
| const logs = await MailLog.find({ projectId: project._id }) | ||
| .sort({ sentAt: -1 }) | ||
| .limit(50) | ||
| .lean(); | ||
|
|
||
| return res.json({ success: true, data: { logs } }); | ||
| } catch (err) { | ||
| return res.status(500).json({ success: false, message: err.message }); | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize the new mail proxy response envelope and error handling.
These handlers mix { success, message }, { success, data }, and raw err.message passthroughs. That makes the new endpoints inconsistent for clients and bypasses the controller error-handling contract. Normalize every branch to { success, data: {}, message } and funnel failures through AppError instead of serializing arbitrary upstream text directly.
As per coding guidelines, All API endpoints return: { success: bool, data: {}, message: "" }. Use AppError class for errors — never raw throw, never expose MongoDB errors to client.
Also applies to: 2411-2450, 2453-2485, 2488-2511, 2514-2551, 2554-2582, 2585-2625
🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 2394 -
2408, Update the getMailLogs controller to always return the standardized
envelope { success, data: {}, message: "" } and replace direct error responses
with AppError instances: when Project is not found, return
res.status(404).json({ success: false, data: {}, message: "Project not found" })
(or throw new AppError("Project not found", 404) and let middleware handle it),
and on any catch block wrap/throw an AppError rather than returning err.message;
ensure successful path returns res.json({ success: true, data: { logs },
message: "" }); apply the same pattern to the other related handlers referenced
(lines ~2411-2625) so all use Project, MailLog, AppError, and the consistent
envelope.
Comment on lines
+2430
to
+2432
| const response = await axios.get(`https://api.resend.com/emails/${safeResendId}`, { | ||
| headers: { Authorization: `Bearer ${key}` } | ||
| }); |
Contributor
There was a problem hiding this comment.
Bound the new Resend proxy calls with a timeout.
Every new axios call here uses the default unbounded wait. A slow or hung Resend edge can pin dashboard-api request workers indefinitely and turn one upstream stall into a cascading outage. Add a shared timeout and translate timeout failures into a 502/504.
Also applies to: 2465-2477, 2503-2505, 2530-2543, 2574-2576, 2616-2618
🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 2430 -
2432, The axios calls to the Resend API (e.g., the axios.get using safeResendId
and Authorization `key`) must be bounded with a shared timeout option (e.g.,
timeout in ms) and error-handled to map timeout/ETIMEDOUT/ECONNABORTED style
errors to a 502 or 504 HTTP response rather than hanging; update the axios
invocation(s) at the shown call and the other occurrences (around lines
2465-2477, 2503-2505, 2530-2543, 2574-2576, 2616-2618) to pass the timeout
option and catch errors, detecting timeout-specific error codes/messages and
returning res.status(502 or 504). Ensure the same shared timeout value is reused
across all Resend proxy calls for consistency.
Comment on lines
+346
to
+358
| test('enforces Pro plan gate for broadcast creation', async () => { | ||
| const req = makeReq(); | ||
| req.planLimits = { byokEnabled: false }; | ||
| req.body = { audienceId: 'aud_1', subject: 'Hello', html: '<p>Hi</p>' }; | ||
| const res = makeRes(); | ||
|
|
||
| mockProjectConfig({ _id: 'proj_1', resendApiKey: { encrypted: 'x', iv: 'y', tag: 'z' } }); | ||
| decrypt.mockReturnValue('byok-key'); | ||
|
|
||
| await mailController.createBroadcast(req, res); | ||
|
|
||
| expect(res.status).toHaveBeenCalledWith(403); | ||
| expect(mockResendClient.broadcasts.create).not.toHaveBeenCalled(); |
Contributor
There was a problem hiding this comment.
These broadcast tests aren't exercising the Pro-plan gate yet.
Both cases drive req.planLimits.byokEnabled, but the feature contract here is "BYOK + Pro". That means these assertions can still pass if a BYOK-related branch returns 403, even when the actual Pro entitlement check regresses. Seed the plan signal the controller uses for broadcasts and keep BYOK satisfied separately.
Also applies to: 361-375
🤖 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/public-api/src/__tests__/mail.controller.test.js` around lines 346 -
358, The test is only toggling req.planLimits.byokEnabled so it can get a 403
from BYOK logic instead of the Pro entitlement; update the broadcast tests that
call mailController.createBroadcast to explicitly set both plan signals: keep
BYOK satisfied (req.planLimits.byokEnabled = true) and set the Pro gate value
the controller expects (e.g., req.planLimits.proEnabled = false for the test
that must enforce the Pro gate, and proEnabled = true for the positive case), so
the assertion failures come from the Pro entitlement path rather than BYOK;
ensure decrypt.mockReturnValue and mockProjectConfig remain unchanged so BYOK
stays satisfied.
Collaborator
Author
|
HOW many issues still pending?? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me fetch the current review comments on this PR to give you an accurate count. 🏁 Script executed: Length of output: 91 🏁 Script executed: Length of output: 2247 🏁 Script executed: Length of output: 3403 🏁 Script executed: Length of output: 4945
🔴 Critical (1)
🟠 Major (7)
🟡 Minor (2)
Would you like me to plan and apply fixes for all 10 of these? (´･ω･`) 🐇 |
Collaborator
Author
|
@coderabbitai |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainGot it! Let me read the relevant code sections first so the fixes are accurate. 🏁 Script executed: Length of output: 15170 Let me grab one more snippet to see 🏁 Script executed: Length of output: 9009
Fix Plan: All 10 Pending Issues
(ˆ˘ˆ) 🐇 The requested changes have been implemented and a pull request has been created: View PR |
Collaborator
Author
|
So I should merge this?? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1339
Recommended merge sequenceStep 1 — Merge Step 2 — Then merge Before you merge
|
| Item | Why it matters |
|---|---|
RESEND_WEBHOOK_SECRET is set in prod env |
Fix #1 now returns 500 instead of passing through when it's missing — so set the secret or webhooks will error |
RESEND_API_KEY_2 / RESEND_API_KEY are still configured |
Fix #6 now throws on decryption failure instead of silently falling back — make sure env vars are present |
MailLog re-throw in queue (Fix #4) |
If BullMQ job retries are configured, a failed MailLog write could cause email re-sends — confirm attempts: 1 on the queue |
If you're good with all of the above, merge #175 first → then merge #172. 🐇
Collaborator
Author
|
ok to merge? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1747
Step 1 — Merge
|
| Check | Status |
|---|---|
| CodeQL (JS/TS) | ✅ Pass |
| dashboard-api-tests | ✅ Pass |
| public-api-tests | ✅ Pass |
| frontend-lint | ✅ Pass |
| sdk-tests | ✅ Pass |
| python-sdk-tests | ✅ Pass |
Before you hit merge, just confirm these 3 env vars are set in your production environment:
| Variable | Why |
|---|---|
RESEND_WEBHOOK_SECRET |
Fix #1 now returns 500 if this is missing — webhooks will break without it |
RESEND_API_KEY_2 or RESEND_API_KEY |
Fix #6 now throws on BYOK decrypt failure instead of silently falling back |
If those are already configured — merge away! 🐇
📝 CodeRabbit Chat: Implement requested code changes
yash-pouranik
temporarily deployed
to
feat/mail-platform-expansion - urBackend-frankfrut PR #172
Render
Destroyed
Collaborator
Author
|
@coderabbitai |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1768
Current status
All 10 fixes from (∩˃ω˂∩) 🐇 |
Collaborator
Author
|
@coderabbitai |
5 tasks
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainSure! Let me create a comprehensive documentation issue now. 🏁 Script executed: Length of output: 53
Here's a quick summary of what the issue covers:
The issue also has a Definition of Done checklist with curl + SDK code examples, an architecture diagram, and publishing requirements. 🐇 |
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.
🚀 Overview
This PR significantly expands the urBackend Mail API Ecosystem, transforming it from a basic email sender into a comprehensive, developer-first Mail Platform. It introduces asynchronous batch sending, audience management, and marketing broadcasts.
Crucially, it also addresses high-priority security vulnerabilities related to webhook validation and cross-project log snooping identified during the code review.
🔒 Security & Bug Fixes
P1): Bypassed the globalexpress.json()middleware for the/api/mail/webhookroute usingexpress.raw(). This preserves the raw request buffer, ensuring that Svix/Resend webhook signature verification succeeds in production.P2): Fixed an authorization flaw ingetResendLiveStatus. The endpoint now strictly queries theMailLogcollection to verify that theresendIdbelongs to the requesting user's project before making the network request to the Resend API.✨ New Features
MailLogmongoose model to track delivery states, sender info, and webhook updates (delivered,bounced,complained)./api/mail/send-batchendpoint for high-volume, concurrent email dispatching.MailPlatform.jsxpage and updatedProjectNavbarto expose the new mail capabilities within the urBackend dashboard.🛠️ Changes
apps/public-api/src/app.js: Configured raw body parsing for webhooks.apps/public-api/src/controllers/mail.controller.js: Added webhook logic, batch sending, audience, and broadcast logic.apps/dashboard-api/src/controllers/project.controller.js: Added ownership verification to live-status and exposed dashboard APIs for the mail platform.packages/common/src/models/MailLog.js: New database schema.apps/web-dashboard/src/pages/MailPlatform.jsx: Frontend implementation.🧪 Verification
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests