522 implement and enforce domain whitelist validation #533
Conversation
Add comprehensive domain whitelist validation system to enhance API security by restricting access to sender endpoints based on configured domain whitelists. ## New Features ### Core Implementation - **Domain Whitelist Middleware**: New middleware that validates Origin/Referer headers against sender's configured domain whitelist - **Domain Utility Functions**: Comprehensive domain extraction, normalization, and validation utilities - **CORS Integration**: Updated CORS middleware to respect domain whitelist for proper origin validation ### Security Features - **Block Non-whitelisted Domains**: Returns 403 Forbidden for requests from unauthorized domains - **Subdomain Support**: Automatically allows subdomains (e.g., api.example.com matches example.com) - **Case Insensitive**: Handles different case variations in domain names - **WWW Normalization**: Normalizes www.example.com to example.com for consistent matching ### Backward Compatibility - **Empty Whitelist Support**: Empty domain whitelist allows all domains (backward compatibility) - **Inactive Profile Bypass**: Domain validation is skipped for inactive sender profiles - **Graceful Error Handling**: Invalid domains are handled gracefully with appropriate logging ## Files Added - - Main domain whitelist middleware - - Comprehensive middleware tests - - Domain extraction and validation utilities - - Domain utility function tests ## Files Modified - - Integrated domain whitelist middleware into sender routes - - Updated CORS to respect domain whitelist - - Updated configuration for testing ## Testing - **Unit Tests**: Comprehensive test coverage for domain utilities and middleware - **Integration Tests**: Verified with real API calls using curl - **Acceptance Criteria**: All user story requirements met: ✅ Whitelisted domains allowed ✅ Non-whitelisted domains blocked (403 Forbidden) ✅ Empty whitelist allows all domains ✅ Subdomain matching works correctly ## Security Benefits - Prevents unauthorized domain access to sensitive payment APIs - Reduces risk of CSRF attacks through proper origin validation - Provides granular access control per sender profile - Protects against API abuse from unauthorized sources ## Usage Domain whitelist validation is automatically applied to all endpoints when sender profiles are active. Configure domains via the sender profile settings API. Resolves: #522
Joshua-Ogbonna
requested review from
5ran6,
chibie and
onahprosper
as code owners
WalkthroughAdds domain extraction utilities, enforces a domain whitelist via new middleware applied to sender routes, refactors CORS to compute per-request allowed origins, adds unit tests, and comments out a tmpfs build-cache mount in docker-compose.yml. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant G as Gin Router
participant A as Auth MW
participant D as DomainWhitelist MW
participant CORS as CORS MW
participant H as Handler
Note over C,G: Request to sender route (/v1/sender/...)
C->>G: HTTP Request (Origin/Referer, maybe API-Key)
G->>A: DynamicAuth / OnlySender
A-->>G: Sender context set
G->>D: DomainWhitelistMiddleware
D->>D: Extract domain (Origin → Referer) / Validate against whitelist
alt Allowed
D-->>G: Next
G->>CORS: Resolve allowed origin (sender/API-Key)
CORS->>G: Set CORS headers (handle OPTIONS)
G->>H: Handler
H-->>C: 200 OK
else Not allowed
D-->>C: 403 Forbidden
end
sequenceDiagram
autonumber
participant U as User (Slack)
participant S as Server (SlackInteractionHandler)
participant DB as KYB Storage
participant E as Email Service
participant Slack as Slack API
Note over U,S: Slack approve_ action
U->>S: Block action (approve_<id>)
S->>DB: Check processed status
alt Already processed
S-->>U: Respond "already processed"
else Not processed
S->>DB: Mark processed (approve)
S-->>U: "Approving submission..."
par
S->>Slack: async replace message (if response_url)
and
S->>DB: Update KYB profile/status (Approved)
S->>E: Send approval email
S->>Slack: Send approval feedback
end
S-->>U: Finalize
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
routers/middleware/cors.go (1)
29-36: CORS: "*" with credentials is invalid; add Vary and gate credentials.Per CORS, you cannot use Access-Control-Allow-Origin: * with Allow-Credentials: true. This will break browser CORS and is a security posture smell.
- ctx.Writer.Header().Set("Access-Control-Allow-Origin", allowedOrigin) + ctx.Header("Access-Control-Allow-Origin", allowedOrigin) ctx.Writer.Header().Set("Access-Control-Max-Age", "86400") ctx.Writer.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE, UPDATE") ctx.Writer.Header().Set("Access-Control-Allow-Headers", "Origin, Content-Type, api_key, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization, API-Key, Client-Type") ctx.Writer.Header().Set("Access-Control-Expose-Headers", "Content-Length") - ctx.Writer.Header().Set("Access-Control-Allow-Credentials", "true") + // CORS best practice: gate credentials and add Vary + if allowedOrigin != "*" { + ctx.Header("Vary", "Origin, Access-Control-Request-Method, Access-Control-Request-Headers") + ctx.Writer.Header().Set("Access-Control-Allow-Credentials", "true") + } else { + ctx.Writer.Header().Set("Access-Control-Allow-Credentials", "false") + }
🧹 Nitpick comments (11)
routers/middleware/cors.go (4)
94-101: Mirror CORS fixes for API‑Key CORS path.Apply the same credentials gating + Vary header here too.
- ctx.Writer.Header().Set("Access-Control-Allow-Origin", allowedOrigin) + ctx.Header("Access-Control-Allow-Origin", allowedOrigin) ctx.Writer.Header().Set("Access-Control-Max-Age", "86400") ctx.Writer.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE, UPDATE") ctx.Writer.Header().Set("Access-Control-Allow-Headers", "Origin, Content-Type, api_key, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization, API-Key, Client-Type") ctx.Writer.Header().Set("Access-Control-Expose-Headers", "Content-Length") - ctx.Writer.Header().Set("Access-Control-Allow-Credentials", "true") + if allowedOrigin != "*" { + ctx.Header("Vary", "Origin, Access-Control-Request-Method, Access-Control-Request-Headers") + ctx.Writer.Header().Set("Access-Control-Allow-Credentials", "true") + } else { + ctx.Writer.Header().Set("Access-Control-Allow-Credentials", "false") + }
118-126: Minor: Avoid parsing empty API key.Check for empty
API-Keybeforeuuid.Parseto skip needless parsing.- apiKey := ctx.GetHeader("API-Key") - apiKeyUUID, err := uuid.Parse(apiKey) - if err != nil { - return "*" - } - - if apiKey == "" { - return "*" - } + apiKey := ctx.GetHeader("API-Key") + if apiKey == "" { + return "*" + } + apiKeyUUID, err := uuid.Parse(apiKey) + if err != nil { + return "*" + }
37-43: Use project logger for OPTIONS logs; reduce noise.Swap
log.Println("OPTIONS")for your structured logger at debug level to keep logs consistent.
25-27: Route detection via substring is brittle.
strings.Contains(path, "/sender/")can false‑match. Prefer mounting this middleware only on the sender route group or using a robust prefix check.Also applies to: 90-92
utils/domain.go (3)
3-6: Prepare to strip ports during normalization.Add net import to support port-stripping in NormalizeDomain.
-import ( - "net/url" - "strings" -) +import ( + "net" + "net/url" + "strings" +)
51-57: Normalize domains by stripping ports and IPv6 brackets.This lets whitelist entries like "example.com" match origins like "https://example.com:3000" and handles IPv6 hosts.
domain = strings.ToLower(domain) if strings.HasPrefix(domain, "www.") { domain = domain[4:] } - return domain + // Strip port if present (e.g., example.com:3000 or [::1]:3000) + if h, _, err := net.SplitHostPort(domain); err == nil { + domain = h + } else if strings.HasPrefix(domain, "[") && strings.HasSuffix(domain, "]") { + // IPv6 host without port like [::1] + domain = strings.Trim(domain, "[]") + } + return domain
70-93: Confirm desired behavior for empty/invalid headers.Current logic allows when requestDomain is empty or parsing fails upstream. If a whitelist is configured, consider default‑deny on parse errors to avoid easy bypass via malformed headers (non‑browser clients).
Do you want me to adjust IsDomainAllowed to treat parse errors as deny when whitelist is non‑empty? I can provide a patch and tests.
routers/middleware/domain_whitelist_test.go (1)
35-66: Consider adding port and referer scenarios.Add cases for:
- Origin with port (https://example.com:8443) vs whitelist ["example.com"].
- Missing Origin with Referer present.
- Invalid Origin when whitelist is non‑empty (expect 403 if you adopt default‑deny on parse errors).
I can extend the table tests if you want a patch.
utils/domain_test.go (1)
103-156: Add host:port matching tests to lock in behavior after normalization change.Propose tests:
- requestDomain "example.com:3000", whitelist ["example.com"] -> true
- requestDomain "[::1]:3000", whitelist ["::1"] -> true
I can add these table rows if you proceed with port-stripping normalization.
routers/middleware/domain_whitelist.go (2)
114-124: Apply the same default‑deny on parse errors for API‑Key path.Mirror the fail‑closed behavior for API‑key middleware.
- requestDomain, err := u.ExtractDomainFromRequest(origin, referer) - if err != nil { - logger.WithFields(logger.Fields{ - "origin": origin, - "referer": referer, - "error": err.Error(), - }).Warnf("Failed to extract domain from request headers") - - c.Next() - return - } + requestDomain, err := u.ExtractDomainFromRequest(origin, referer) + if err != nil { + logger.WithFields(logger.Fields{ + "origin": origin, + "referer": referer, + "error": err.Error(), + }).Warnf("Failed to extract domain from request headers") + if len(senderProfile.DomainWhitelist) > 0 { + u.APIResponse(c, http.StatusForbidden, "error", + "Access denied: Invalid Origin/Referer header", map[string]interface{}{ + "origin": origin, "referer": referer, + }) + c.Abort() + return + } + c.Next() + return + }
78-89: Operational: Avoid duplicate DB lookups by caching sender in context.If API‑Key auth already resolved the sender elsewhere, consider setting it in context and reusing rather than re‑querying here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docker-compose.yml(1 hunks)routers/index.go(1 hunks)routers/middleware/cors.go(2 hunks)routers/middleware/domain_whitelist.go(1 hunks)routers/middleware/domain_whitelist_test.go(1 hunks)utils/domain.go(1 hunks)utils/domain_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
routers/index.go (1)
routers/middleware/domain_whitelist.go (1)
DomainWhitelistMiddleware(17-76)
routers/middleware/domain_whitelist_test.go (1)
routers/middleware/domain_whitelist.go (1)
DomainWhitelistMiddleware(17-76)
utils/domain_test.go (1)
utils/domain.go (3)
ExtractDomainFromOrigin(8-19)NormalizeDomain(46-57)IsDomainAllowed(70-93)
routers/middleware/domain_whitelist.go (3)
utils/domain.go (2)
ExtractDomainFromRequest(34-44)IsDomainAllowed(70-93)utils/logger/logger.go (4)
WithFields(76-109)Fields(73-73)Warnf(133-138)Debugf(117-122)utils/http.go (1)
APIResponse(23-29)
routers/middleware/cors.go (2)
utils/domain.go (2)
ExtractDomainFromOrigin(8-19)IsDomainAllowed(70-93)ent/client.go (1)
Client(48-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
docker-compose.yml (1)
41-41: Remove dead tmpfs-cache volume configuration
Remove the commented-out mount (line 41) and the unused named volume definition (lines 52–56) to avoid dead config.services: @@ volumes: - .:/app - # - tmpfs-cache:/root/.cache/go-build # Add tmpfs volume for build cache @@ -# Define volumes -volumes: - tmpfs-cache: - driver: local - driver_opts: - type: tmpfs - device: tmpfsrouters/index.go (2)
127-129: LGTM: Whitelist middleware mounted on sender routes.Good placement after auth and OnlySender. This enforces domain checks where intended.
123-129: No additional whitelist middleware needed for API-Key flows
DynamicAuthMiddleware invokes APIKeyMiddleware for “/sender/” requests, settingsenderin context before DomainWhitelistMiddleware, so API-Key requests are already subject to domain whitelist enforcement.routers/middleware/domain_whitelist_test.go (1)
24-33: Nice: Focused middleware test harness.Injecting a mock sender into context and exercising the whitelist permutations is clean and effective.
utils/domain_test.go (1)
7-44: Solid coverage for ExtractDomainFromOrigin.Good mix of valid/invalid and empty inputs.
…ls and blocks bypass for malformed headers
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
routers/middleware/cors.go (1)
21-23: Path check is brittlestrings.Contains on URL path can misfire. Prefer group-based middleware or HasPrefix on a known base path (e.g., /v1/sender/).
routers/middleware/domain_whitelist.go (1)
19-22: Path gating could be more preciseUse router grouping or c.FullPath() and prefix match to avoid accidental matches on paths that merely contain “/sender/”.
Also applies to: 24-28, 32-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
routers/middleware/cors.go(1 hunks)routers/middleware/domain_whitelist.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
routers/middleware/cors.go (2)
utils/domain.go (2)
ExtractDomainFromOrigin(8-19)IsDomainAllowed(70-93)ent/client.go (1)
Client(48-104)
routers/middleware/domain_whitelist.go (3)
utils/domain.go (2)
ExtractDomainFromRequest(34-44)IsDomainAllowed(70-93)utils/logger/logger.go (4)
WithFields(76-109)Fields(73-73)Warnf(133-138)Debugf(117-122)utils/http.go (1)
APIResponse(23-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
routers/middleware/cors.go (2)
37-49: Duplicate domain/whitelist logic with DomainWhitelist middlewaregetCORSOrigin/validateDomain* repeat checks already in DomainWhitelistMiddleware. Extract a shared helper (e.g., utils.ResolveAllowedOrigin) to remove duplication.
79-83: Fix: ent .Only requires stdlib context, not gin.ContextReplace .Only(ctx) with the request context to compile.
- .Only(ctx) + .Only(ctx.Request.Context())routers/middleware/domain_whitelist.go (3)
40-57: LGTM: Fail-closed behavior when whitelist configuredGood: invalid Origin/Referer now blocks when a whitelist exists; permissive only when empty whitelist.
59-74: LGTM: Explicit 403 on whitelist violations with useful context logsClear response and structured logs.
104-108: Fix: ent .Only requires stdlib context, not gin.ContextReplace .Only(c) with request context.
- .Only(c) + .Only(c.Request.Context())
…d add new migration for kyb_rejection_comment
… Blockscout API - Update GetAddressTransactionHistory to GetAddressTokenTransfers for clarity. - Adjust API endpoint and error messages to reflect the change in functionality. - Remove block range filtering logic as it is not applicable to token transfers.
…fication - Fix shell escaping issues with exclamation marks in feat! pattern - Separate BREAKING CHANGE detection to avoid space-related issues - Use wc -l instead of grep -c for proper counting - Ensure release notes will now properly populate with commit information
- Add edge loading for provider and currency in ValidateAndFixBalances to prevent race conditions. - Implement checks in validateBalanceConsistencyInternal to ensure edges are loaded, preventing nil pointer dereference during validation.
- Include transaction hash in the payment order update process to enhance traceability and integrity of order transactions.
- Update account name extraction logic to exclude OK as a valid account name, ensuring more accurate validation of account data.
- Update the token query in GetProviderProfile to only include enabled tokens, enhancing data accuracy and relevance in the response.
- Move wallet address from error message to log fields for better grouping - Standardize error messages to group by chain ID and error type - Only modify Errorf calls, keeping warnings unchanged - Error details still available in log fields for debugging This fixes the issue where each unique wallet address created separate error groups in Glitchtip monitoring dashboard.
…pdate in payment tables migration
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/index.go(1 hunks)routers/index.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- routers/index.go
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/index.go (6)
utils/logger/logger.go (3)
Warnf(133-138)Errorf(141-146)Infof(125-130)ent/kybprofile/where.go (3)
CompanyName(75-77)IDEQ(20-22)ID(15-17)ent/client.go (1)
Client(48-104)storage/database.go (1)
Client(21-21)ent/user/where.go (2)
IDEQ(20-22)ID(15-17)ent/user/user.go (1)
KybVerificationStatusApproved(140-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
Description
This PR implements domain whitelist validation for sender API endpoints to enhance security by restricting access based on configured domain whitelists.
Added domain whitelist middleware that validates Origin/Referer headers against sender's configured domain whitelist. Created domain utility functions for extraction, normalization, and validation with subdomain support. Updated CORS middleware to respect domain whitelist for proper origin validation.
References
Closes #522
Testing
Unit tests added for domain utilities and middleware validation. Integration tests verified with real API calls using curl on localhost:8000. Tested with both JWT and API key authentication methods.
Manual testing steps:
Environment: Go 1.24.6, macOS, PostgreSQL 16
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit