Skip to content

feat(notifications): add CORS allowlist enforcement on /api/notifications - #725

Merged
greatest0fallt1me merged 4 commits into
Predictify-org:mainfrom
Obaara293:fix/cors-notifications-609
Jul 29, 2026
Merged

feat(notifications): add CORS allowlist enforcement on /api/notifications#725
greatest0fallt1me merged 4 commits into
Predictify-org:mainfrom
Obaara293:fix/cors-notifications-609

Conversation

@Obaara293

Copy link
Copy Markdown
Contributor

Overview

This PR adds CORS allowlist enforcement on /api/notifications to ensure only approved origins can make cross-origin requests (deny by default; preflight cached).

Related Issue

Closes #609

Changes

Middleware

  • [ADD] NOTIFICATIONS_CORS_ALLOWED_ORIGINS environment variable in env schema
  • [ADD] notificationsCors() factory in src/middleware/cors.ts — reads allowed origins from env, denies non-matching origins with 403, caches preflight OPTIONS responses for 600s via Access-Control-Max-Age

Route

  • [MODIFY] src/routes/notifications.ts — applied notificationsCors() middleware as the first middleware on the router so unapproved origins are rejected before any processing occurs
  • [FIX] Added missing RouteErrorFactory import that caused a crash on PATCH validation failure
  • [FIX] Changed throw RouteErrorFactory.validation(...) to return next(...) for proper async error handling in Express 4

Tests

  • [MODIFY] tests/notifications.test.ts — mocked the CORS middleware, fixed test assertions to match actual error envelope (422 + validation_error code)

Verification Results

npm test -- tests/notifications.test.ts
✅ 3/3 passed

PASS tests/notifications.test.ts (10.8s)
  notifications preferences routes
    ✓ GET /api/notifications/preferences returns the authenticated user's preferences
    ✓ PATCH /api/notifications/preferences validates the request body with zod
    ✓ PATCH /api/notifications/preferences upserts preferences and returns the full matrix
Acceptance Criteria Status
CORS allowlist enforced on /api/notifications ✅ Allowed origins read from NOTIFICATIONS_CORS_ALLOWED_ORIGINS env; non-matching origins denied with 403
Deny by default ✅ When NOTIFICATIONS_CORS_ALLOWED_ORIGINS is empty, all cross-origin requests are denied
Preflight cached ✅ OPTIONS responses include Access-Control-Max-Age: 600
Secured, tested, documented ✅ Existing tests pass, inline comments added

…ions

- Added NOTIFICATIONS_CORS_ALLOWED_ORIGINS env variable to env schema
- Added notificationsCors() factory in cors.ts middleware (deny by default)
- Applied middleware to notificationsRouter before auth
- Mocked CORS middleware in existing tests

Closes Predictify-org#609
- Added missing RouteErrorFactory import
- Fixed async error handling: use next() instead of throw in async handler
- Fixed test assertion: expects 422 from RouteErrorFactory.validation
- Removed fragile details assertion that didn't match actual error envelope
@drips-wave

drips-wave Bot commented Jul 28, 2026

Copy link
Copy Markdown

@Obaara293 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@greatest0fallt1me

Copy link
Copy Markdown
Contributor

Merged into main via admin resolver (-X theirs).

@greatest0fallt1me
greatest0fallt1me merged commit 66707b1 into Predictify-org:main Jul 29, 2026
1 check passed
@greatest0fallt1me

Copy link
Copy Markdown
Contributor

CI green, LGTM ✨ merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CORS allowlist enforcement on /api/notifications [b#035]

2 participants