fix(api): derive SIWE allowed domains from CORS origins#207
Merged
Conversation
Replace single SIWE_DOMAIN env var with dynamic domain list derived from CORS_ALLOWED_ORIGINS. This allows wallet auth from any trusted origin (e.g. localhost during dev against staging API) without separate config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6f5ec9ce41a3
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (9)
🚥 Pre-merge checks | ✅ 3✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/.env.example (1)
7-8: Consider whether this change belongs in this PR.Adding
VITE_PINATA_GATEWAY_URLappears unrelated to the main PR objective of deriving SIWE allowed domains from CORS origins. While the PR summary mentions this as a previously missing variable, bundling unrelated configuration changes can make the PR harder to review and understand.Additionally, the specific gateway URL (
sapphire-fashionable-antelope-504.mypinata.cloud) looks like a project-specific Pinata gateway. Confirm whether:
- This specific URL should be in the example file, or if a placeholder/generic format would be more appropriate
- This gateway URL is intended to be public/shared across environments
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/.env.example` around lines 7 - 8, The VITE_PINATA_GATEWAY_URL addition is unrelated to the SIWE/CORS change; remove or convert it to a generic placeholder in the env example to keep the PR focused: replace the concrete value "sapphire-fashionable-antelope-504.mypinata.cloud" with a clear placeholder like "<YOUR_PINATA_GATEWAY_URL>" (referencing the variable name VITE_PINATA_GATEWAY_URL) or move this change into a separate follow-up PR and update any docs mentioning VITE_PINATA_GATEWAY_URL accordingly; ensure the example does not expose a project-specific gateway URL and confirm whether the gateway should be public before adding a real URL.apps/api/src/auth/controllers/identity.controller.ts (1)
256-273: Extract domain derivation logic to avoid duplication.This domain extraction logic is duplicated verbatim in
auth.service.ts(lines 421-437). Consider extracting this into a shared helper method inSiweServiceor a configuration utility to follow DRY principles and ensure consistent behavior across both call sites.♻️ Suggested approach: Add helper to SiweService
// In siwe.service.ts, add: + /** + * Derive allowed SIWE domains from CORS origins configuration. + * Filters wildcards and extracts host:port from URLs. + */ + static deriveAllowedDomains( + corsOrigins: string | undefined, + webAppUrl: string | undefined + ): string[] { + const rawOrigins = corsOrigins || webAppUrl; + if (!rawOrigins) { + return ['localhost:5173', 'localhost:4173', 'localhost']; + } + return rawOrigins + .split(',') + .map((o) => o.trim()) + .filter((o) => !o.includes('*')) + .map((o) => { + try { + return new URL(o).host; + } catch { + return o; + } + }); + }Then in both controller and service:
- const rawOrigins = - this.configService.get<string>('CORS_ALLOWED_ORIGINS') || - this.configService.get<string>('WEB_APP_URL'); - const allowedDomains = rawOrigins - ? rawOrigins - .split(',') - .map((o) => o.trim()) - .filter((o) => !o.includes('*')) - .map((o) => { - try { - const url = new URL(o); - return url.host; - } catch { - return o; - } - }) - : ['localhost:5173', 'localhost:4173', 'localhost']; + const allowedDomains = SiweService.deriveAllowedDomains( + this.configService.get<string>('CORS_ALLOWED_ORIGINS'), + this.configService.get<string>('WEB_APP_URL') + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/controllers/identity.controller.ts` around lines 256 - 273, The domain-extraction block is duplicated; extract it into a single helper (e.g., parseAllowedDomains or deriveAllowedDomains) on SiweService or a small config utility, implement the logic there (read CORS_ALLOWED_ORIGINS/WEB_APP_URL, split by comma, trim, filter out wildcards, try new URL() to return url.host or fallback to the original token, and preserve the same default array ['localhost:5173','localhost:4173','localhost']), then replace the inline logic in identity.controller.ts and auth.service.ts to call that helper (importing the new function or SiweService method) so both call sites reuse the same implementation and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/auth/controllers/identity.controller.ts`:
- Around line 260-273: When parsing CORS_ALLOWED_ORIGINS into allowedDomains,
handle the edge case where filtering out wildcards yields an empty array: after
computing allowedDomains from rawOrigins (variable names: rawOrigins,
allowedDomains, CORS_ALLOWED_ORIGINS), check if allowedDomains.length === 0 and
then emit a clear warning (e.g., console.warn or the module logger) that
rawOrigins contained only wildcard entries and wildcard domains were ignored,
include the rawOrigins value in the message, and then set allowedDomains to the
same safe fallback used when rawOrigins is falsy
(['localhost:5173','localhost:4173','localhost']) so SIWE verification
(SiweService.verifySiweMessage) does not silently reject all requests without
context.
---
Nitpick comments:
In `@apps/api/src/auth/controllers/identity.controller.ts`:
- Around line 256-273: The domain-extraction block is duplicated; extract it
into a single helper (e.g., parseAllowedDomains or deriveAllowedDomains) on
SiweService or a small config utility, implement the logic there (read
CORS_ALLOWED_ORIGINS/WEB_APP_URL, split by comma, trim, filter out wildcards,
try new URL() to return url.host or fallback to the original token, and preserve
the same default array ['localhost:5173','localhost:4173','localhost']), then
replace the inline logic in identity.controller.ts and auth.service.ts to call
that helper (importing the new function or SiweService method) so both call
sites reuse the same implementation and behavior.
In `@apps/web/.env.example`:
- Around line 7-8: The VITE_PINATA_GATEWAY_URL addition is unrelated to the
SIWE/CORS change; remove or convert it to a generic placeholder in the env
example to keep the PR focused: replace the concrete value
"sapphire-fashionable-antelope-504.mypinata.cloud" with a clear placeholder like
"<YOUR_PINATA_GATEWAY_URL>" (referencing the variable name
VITE_PINATA_GATEWAY_URL) or move this change into a separate follow-up PR and
update any docs mentioning VITE_PINATA_GATEWAY_URL accordingly; ensure the
example does not expose a project-specific gateway URL and confirm whether the
gateway should be public before adding a real URL.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/deploy-staging.ymlapps/api/.env.exampleapps/api/src/auth/auth.service.tsapps/api/src/auth/controllers/identity.controller.spec.tsapps/api/src/auth/controllers/identity.controller.tsapps/api/src/auth/services/siwe.service.spec.tsapps/api/src/auth/services/siwe.service.tsapps/web/.env.example
💤 Files with no reviewable changes (2)
- apps/api/.env.example
- .github/workflows/deploy-staging.yml
WEB_APP_URL was a legacy single-origin fallback superseded by CORS_ALLOWED_ORIGINS. Remove all references to consolidate on a single env var for trusted origins (CORS + SIWE domain validation). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: a2f44c18551c
SiweService now injects ConfigService and resolves allowed domains from CORS_ALLOWED_ORIGINS internally. Callers no longer need to build and pass the domain list — verifySiweMessage handles it as an implementation detail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d681ad360619
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.
Summary
SIWE_DOMAINenv var with dynamic domain list derived fromCORS_ALLOWED_ORIGINSSIWE_DOMAINfrom.env.example, deploy workflow, and all call sitesVITE_PINATA_GATEWAY_URLto web.env.example(was missing)Motivation
When running the web app locally (
localhost:5173) pointed at the staging API, wallet login fails with 401 because the SIWE message domain (localhost:5173) doesn't match the configuredSIWE_DOMAIN(app-staging.cipherbox.cc). Since CORS already defines the trusted origin list, SIWE should reuse it rather than requiring a separate config.Test plan
localhost:5173against staging APIapp-staging.cipherbox.cc🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests