feat(aggregator): enforce domain whitelist for sender API routes#723
Open
sundayonah wants to merge 1 commit intomainfrom
Open
feat(aggregator): enforce domain whitelist for sender API routes#723sundayonah wants to merge 1 commit intomainfrom
sundayonah wants to merge 1 commit intomainfrom
Conversation
- Add domain validation middleware (Origin/Referer vs sender whitelist) - Add utils/domain.go (ExtractRequestDomain, IsDomainAllowed, subdomain support) - Apply DomainWhitelistMiddleware to /v1/sender/ and /v2/sender/ - Update CORS to allow only whitelisted origins when API-Key present on sender paths - Empty whitelist allows all domains (backward compatible) - Add tests for domain utils and domain whitelist middleware - Guard ent runtime init when PaymentOrder has no hooks (fix test panic)
Contributor
📝 WalkthroughWalkthroughThis PR implements domain whitelist validation middleware to enforce sender profile domain restrictions, while refactoring PaymentOrder hook handling by removing exported Hooks and simplifying error handling in default initialization methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Router as Router/Handler
participant AuthMW as DynamicAuthMiddleware
participant WhitelistMW as DomainWhitelistMiddleware
participant Utils as Domain Utils
participant DB as Database
Client->>Router: Request with Origin/Referer
Router->>AuthMW: Process
AuthMW->>DB: Validate credentials
DB-->>AuthMW: Sender profile
AuthMW->>Router: Set sender in context
Router->>WhitelistMW: Process
alt Sender exists & whitelist non-empty
WhitelistMW->>Utils: ExtractRequestDomain(origin, referer)
Utils-->>WhitelistMW: Extracted domain
WhitelistMW->>Utils: IsDomainAllowed(domain, whitelist)
Utils-->>WhitelistMW: Validation result
alt Domain allowed
WhitelistMW->>Router: Continue
Router-->>Client: Process request normally
else Domain denied
WhitelistMW-->>Client: 403 Forbidden (Domain not allowed)
end
else No sender or empty whitelist
WhitelistMW->>Router: Continue (backward compatible)
Router-->>Client: Process request normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ent/paymentorder_create.go (1)
603-697:⚠️ Potential issue | 🔴 CriticalRestore nil-checks for runtime-wired PaymentOrder defaults to prevent panics on uninitialized package vars.
The
defaults()method calls package-level function variables (DefaultCreatedAt,DefaultUpdatedAt,DefaultAmountPaid, etc.) without checking if they are initialized. These variables are wired at runtime viaent/runtime/runtime.gousing type-asserted assignments from schema descriptors. If initialization is delayed or fails, calling a nil function will panic instead of returning a recoverable error.This affects both:
PaymentOrderCreate.Save()(line 605) which calls_c.defaults()fire-and-forgetPaymentOrderCreateBulk.Save()(line 2586) which callsbuilder.defaults()on each builder without error handlingReintroduce nil-checks by making
defaults()return anerrorand propagate it in bothSave()methods:Suggested pattern
func (_c *PaymentOrderCreate) Save(ctx context.Context) (*PaymentOrder, error) { - _c.defaults() + if err := _c.defaults(); err != nil { + return nil, err + } return withHooks(ctx, _c.sqlSave, _c.mutation, _c.hooks) } func (_c *PaymentOrderCreate) defaults() error { if _, ok := _c.mutation.CreatedAt(); !ok { + if paymentorder.DefaultCreatedAt == nil { + return fmt.Errorf("ent: uninitialized default for field %q", paymentorder.FieldCreatedAt) + } v := paymentorder.DefaultCreatedAt() _c.mutation.SetCreatedAt(v) } + // Apply same guard pattern to all function-type defaults + return nil }
🧹 Nitpick comments (1)
routers/middleware/domain_whitelist.go (1)
35-46: Consider reordering checks for clearer control flow.The current logic is correct, but the flow is slightly confusing:
IsDomainAllowedis called before checking if the domain is empty, even though an empty domain will always fail the allowlist check. Consider reordering to check for empty domain first:♻️ Optional: Clearer control flow
- if u.IsDomainAllowed(domain, whitelist) { - c.Next() - return - } - // When whitelist is set but no domain could be extracted, block (e.g. missing Origin/Referer). if domain == "" { u.APIResponse(c, http.StatusForbidden, "error", "Origin or Referer required when domain whitelist is configured", nil) c.Abort() return } + if u.IsDomainAllowed(domain, whitelist) { + c.Next() + return + } u.APIResponse(c, http.StatusForbidden, "error", "Domain not allowed", nil) c.Abort()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routers/middleware/domain_whitelist.go` around lines 35 - 46, Reorder the checks in the domain whitelist middleware so the empty-domain case is handled before calling IsDomainAllowed: first test if domain == "" and call u.APIResponse(c, http.StatusForbidden, "error", "Origin or Referer required when domain whitelist is configured", nil) followed by c.Abort() and return; only after that call u.IsDomainAllowed(domain, whitelist) and allow the request (c.Next()/return) or respond with the "Domain not allowed" message and abort. Ensure you update the control flow surrounding the domain variable, IsDomainAllowed, APIResponse, c.Abort, and c.Next to reflect this new order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@routers/middleware/domain_whitelist.go`:
- Around line 35-46: Reorder the checks in the domain whitelist middleware so
the empty-domain case is handled before calling IsDomainAllowed: first test if
domain == "" and call u.APIResponse(c, http.StatusForbidden, "error", "Origin or
Referer required when domain whitelist is configured", nil) followed by
c.Abort() and return; only after that call u.IsDomainAllowed(domain, whitelist)
and allow the request (c.Next()/return) or respond with the "Domain not allowed"
message and abort. Ensure you update the control flow surrounding the domain
variable, IsDomainAllowed, APIResponse, c.Abort, and c.Next to reflect this new
order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d06ed2c1-1433-4b3d-930e-dbb8a303bddf
📒 Files selected for processing (10)
ent/client.goent/paymentorder/paymentorder.goent/paymentorder_create.goent/paymentorder_update.goent/runtime/runtime.gorouters/index.gorouters/middleware/domain_whitelist.gorouters/middleware/domain_whitelist_test.goutils/domain.goutils/domain_test.go
💤 Files with no reviewable changes (2)
- ent/runtime/runtime.go
- ent/paymentorder/paymentorder.go
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
1 participant
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.
Description
This PR implements enforcement of the existing domain whitelist on sender API routes so that only requests from allowed domains (or any domain when the whitelist is empty) can access sensitive payment APIs.
Background: The
SenderProfiletable already had adomain_whitelistfield, but it was not enforced. CORS usedAccess-Control-Allow-Origin: *and there was no validation ofOrigin/Referer. Any domain could call the sender API with a valid API key.Changes:
routers/middleware/domain_whitelist.go): After auth, reads the sender profile from context. Ifdomain_whitelistis non-empty, it derives the request domain fromOriginorReferer, checks it against the whitelist (exact and subdomain), and returns 403 Forbidden when not allowed or when Origin/Referer is missing. Empty whitelist continues to allow all domains (backward compatible).utils/domain.go):ExtractRequestDomain(origin, referer)andIsDomainAllowed(host, whitelist)with subdomain support and case normalization.routers/index.go):DomainWhitelistMiddlewareis applied only to/v1/sender/and/v2/sender/(afterDynamicAuthMiddleware, beforeOnlySenderMiddleware). Provider and other routes are unchanged.routers/middleware/cors.go): For sender paths whenAPI-Keyis present,Access-Control-Allow-Originis set only when the requestOriginis in the sender’s whitelist; otherwise the header is omitted so the browser blocks the response. Non-sender paths and requests without API-Key keep previous behavior (e.g.*).utils/domain_test.goforExtractRequestDomainandIsDomainAllowed;routers/middleware/domain_whitelist_test.gofor the middleware (all three acceptance criteria covered). DuplicatedecodeResponseBodyin middleware tests removed in favor of the helper inrequest_test.go.ent/runtime/runtime.goso init does not panic whenPaymentOrderhas no hooks (enables tests that loadent).Behaviour summary:
Breaking changes: None for callers that use a non-empty whitelist correctly. Senders that previously had a whitelist configured but relied on it not being enforced will now get 403 for non-whitelisted origins; they must add those origins to the whitelist or clear the whitelist to allow all.
Alternatives considered: Keeping CORS as
*and only enforcing in middleware (403) was considered sufficient for security; CORS was updated as well for defense-in-depth and correct behaviour with credentials.References
Closes #522
Testing
Unit / package tests:
From repo root (e.g.
paycrest/aggregator):go test ./utils/... -run "TestExtractRequestDomain|TestIsDomainAllowed" -vgo test ./routers/middleware/... -run TestDomainWhitelist -vOr run all tests:
go test ./...This change adds test coverage for new/changed/fixed functionality
with valid domain
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Tests