Skip to content

Conversation

github-actions[bot]
Copy link
Contributor

This is an automated pull request to merge chas/resend-rate-limit-issue into dev.
It was created by the [Auto Pull Request] action.

@vercel
Copy link

vercel bot commented Oct 12, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
app Ready Ready Preview Comment Oct 16, 2025 2:54pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
portal Skipped Skipped Oct 16, 2025 2:54pm

@chasprowebdev chasprowebdev changed the title [dev] [chasprowebdev] chas/resend-rate-limit-issue Trigger 2 new-policy-email events per second at most due to resend rate limit issue Oct 12, 2025
@chasprowebdev chasprowebdev requested a review from Marfuen October 12, 2025 16:12
@comp-ai-code-review
Copy link

comp-ai-code-review bot commented Oct 16, 2025

🔒 Comp AI - Security Review

🔴 Risk Level: HIGH

No OSV/CVE findings reported, but the PR scan flags high-risk issues in apps/app/src/actions/policies/accept-requested-policy-changes.ts (authz, stored XSS, TOCTOU, email leakage, verbose logging).


📦 Dependency Vulnerabilities

✅ No known vulnerabilities detected in dependencies.


🛡️ Code Security Analysis

View 2 file(s) with issues

🔴 apps/app/src/actions/policies/accept-requested-policy-changes.ts (HIGH Risk)

# Issue Risk Level
1 Insufficient authorization: approverId not validated as acting user HIGH
2 Stored XSS: comment saved without sanitization HIGH
3 Race condition: TOCTOU between read and update may allow conflicting updates HIGH
4 Potential email disclosure: sends all employee emails directly as payload HIGH
5 Verbose error logging may leak sensitive info HIGH

Recommendations:

  1. Enforce that the authenticated user is the approver: verify the current user maps to the approver (e.g., resolve member for user.id and ensure member.id or member.userId matches policy.approverId) or otherwise verify the user has explicit approval permissions. Do not trust an approverId supplied by the client.
  2. Use DB-level conditional updates or transactions/optimistic locking to avoid TOCTOU: include conditions in the update (e.g., WHERE id = ? AND lastPublishedAt = OR use a version counter) or perform read+update in a transaction to ensure atomicity.
  3. Sanitize and/or encode comment content before storing and always escape on render. Prefer output encoding in the UI and enforce input validation (length, allowed characters). Consider using a vetted sanitizer library (e.g., DOMPurify on render or server-side sanitizers) and store only the sanitized/validated content.
  4. Avoid sending raw email addresses to external services or logging them. Send notifications server-side (look up emails server-side from member IDs) or use internal IDs/templates so third-party task payloads do not contain bulk email lists. If an external provider is required, minimize the exposed data and ensure payloads are encrypted and access-controlled.
  5. Reduce verbose error leakage: do not log full error stacks to places accessible by untrusted users. Use structured/application logging with sanitized error messages and store detailed stack traces only in secured audit logs. Consider adding an approval audit record (who approved, when, note) instead of relying on console logs.

🟡 apps/app/src/jobs/tasks/email/new-policy-email.ts (MEDIUM Risk)

# Issue Risk Level
1 Missing input validation for task payload (email, names, orgId) MEDIUM
2 Sensitive data logged (email, policyName) in cleartext MEDIUM
3 Detailed error messages written to logs and rethrown MEDIUM

Recommendations:

  1. Enforce schema validation on the task payload before use. Use a runtime schema (zod/joi/ajv) to validate email format, allowed notificationType enum, string length limits, and organizationId format. Reject or normalize invalid payloads before calling sendPolicyNotificationEmail.
  2. Perform input sanitization and canonicalization for strings (trim, strip control characters). Validate organizationId against expected formats (UUID) if applicable.
  3. Do not log raw PII. Mask or redact email addresses in logs (e.g., a****@example.com or show only domain). Avoid logging user names or policy titles unless necessary; if logged, redact or hash them.
  4. Avoid logging raw error messages or stack traces that may contain sensitive data. Log a safe error code/message and send full details to a secure error-tracking system (Sentry) with appropriate access controls. When rethrowing, throw a generic error or wrap the original error so internal details are not propagated to upstream consumers.
  5. Add defensive checks/guards before calling sendPolicyNotificationEmail (e.g., ensure payload has required fields and passes schema). If sendPolicyNotificationEmail is a third-party/in-project function, ensure it also validates inputs and handles malformed data safely.
  6. Fix the queue configuration comment vs actual behavior: the comment says 1 email/sec but concurrencyLimit is 2. Either adjust concurrencyLimit to match intended rate or enforce rate-limiting via a token bucket, delay, or an external rate-limiter so you don't exceed email provider limits.
  7. Harden producers of this task: ensure only authorized services can enqueue tasks with this payload (authenticate/authorize enqueue operations), and validate/normalize incoming data at the API boundary.
  8. Add unit/integration tests that assert invalid payloads are rejected and that logs do not contain unmasked PII.
  9. Consider using structured logging with automatic PII redaction middleware or a logging wrapper that masks sensitive fields consistently.

💡 Recommendations

View 3 recommendation(s)
  1. Enforce approver authorization server-side: do not trust a client-supplied approverId — resolve the authenticated user to a member/approver record and assert member.id or member.userId matches policy.approverId (or check an approval-permission) before making changes.
  2. Sanitize and validate comment content before persisting and always perform output encoding on render: apply a vetted sanitizer (or whitelist) and enforce length/character limits server-side so stored XSS cannot be injected.
  3. Make state changes atomic and avoid leaking bulk emails: perform read+update inside a DB transaction or use conditional updates/optimistic locking (e.g., WHERE id=? AND version=? or lastPublishedAt=?), and stop including raw employee email lists in external task payloads — resolve emails server-side and pass only internal IDs or minimal data.

Powered by Comp AI - AI that handles compliance for you. Reviewed Oct 16, 2025

@vercel vercel bot temporarily deployed to Preview – portal October 16, 2025 02:56 Inactive
@chasprowebdev chasprowebdev changed the title Trigger 2 new-policy-email events per second at most due to resend rate limit issue Create trigger.dev task to send policy email due to resend rate limit issue Oct 16, 2025
@vercel vercel bot temporarily deployed to Preview – portal October 16, 2025 14:50 Inactive
@Marfuen Marfuen merged commit b737ba3 into main Oct 16, 2025
7 of 8 checks passed
@Marfuen Marfuen deleted the chas/resend-rate-limit-issue branch October 16, 2025 14:50
@claudfuen
Copy link
Contributor

🎉 This PR is included in version 1.56.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants