fetch with cors proxy#604
Conversation
WalkthroughA CORS proxy mechanism is introduced for LNURL HTTP fetches via a new ChangesCORS Proxy Infrastructure
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Request Changes — Security Review
CRITICAL: All user traffic routed through a third-party personal proxy
src/lib/lnurl.ts:29-31 — Every LNURL and Ark address fetch now passes through https://cors-header-proxy.bordalix.workers.dev/proxy. This is a man-in-the-middle by design. The proxy operator (or anyone who compromises the Cloudflare Worker) can:
- Swap Lightning invoices — Replace the
prfield inLnUrlCallbackResponsewith a different bolt11 invoice, redirecting payments to an attacker wallet. AffectsfetchLnUrlInvoice(line 46) andfetchInvoice(line 89). - Swap Ark addresses — Replace the
addressfield inArkMethodResponse, redirecting Ark deposits. AffectsfetchArkAddress(line 101). This is protocol-critical. - Harvest metadata — See every LNURL, Lightning address, payment amount, and comment the user sends.
Even if the current proxy is benign, hardcoding a personal free-tier Cloudflare Worker into a financial wallet is unacceptable. The proxy source isn't auditable from this PR, there's no SLA, and the domain could be transferred or hijacked.
HIGH: URL injection via missing encodeURIComponent
src/lib/lnurl.ts:30:
const proxyUrl = `https://cors-header-proxy.bordalix.workers.dev/proxy?apiurl=${url}`The url is interpolated raw. If the upstream callback URL contains & (common in query strings, e.g. ?amount=1000&comment=hi), those parameters bleed into the proxy's query string instead of being part of apiurl. This breaks fetchLnUrlInvoice (line 46) which appends ?amount=...&comment=... to the callback URL before passing it to the proxy.
Fix if this approach were acceptable: encodeURIComponent(url).
HIGH: No configuration or opt-out
The proxy URL is hardcoded with no environment variable, feature flag, or build-time configuration. Users cannot:
- Disable the proxy
- Use their own proxy
- Verify what proxy is in use
MEDIUM: Tests don't cover the new behavior
src/test/lib/lnurl.test.ts — Tests use vitest-fetch-mock to mock fetch globally. Since fetchWithCorsProxy calls fetch internally, the mock still intercepts — but the tests never assert that requests go to the proxy URL, and never verify the URL rewriting is correct. The URL injection bug above would pass all existing tests silently.
MEDIUM: Availability — single point of failure
Free-tier personal Cloudflare Worker. If it goes down or hits rate limits, the entire wallet loses LNURL and Ark address resolution. No fallback, no retry logic.
Recommendation
CORS issues with LNURL are a real problem for browser-based wallets, but the fix must not introduce a trusted third party into the payment flow. Options:
- Self-hosted proxy under the org's infrastructure with the source code in-repo or a separate auditable repo, behind the project's domain.
- Server-side relay — if the wallet has a backend, route LNURL fetches through it.
- Configurable proxy URL at minimum, so users/deployers control the trust boundary — but even this is a band-aid.
None of these should land without the encodeURIComponent fix regardless.
fetchArkAddress routes Ark address resolution through this proxy. Requires human sign-off even if reworked.
Reviewed by Arkana 🔍
There was a problem hiding this comment.
🔴 Request Changes — Security Review
CRITICAL: All user traffic routed through a third-party personal proxy
src/lib/lnurl.ts:29-31 — Every LNURL and Ark address fetch now passes through https://cors-header-proxy.bordalix.workers.dev/proxy. This is a man-in-the-middle by design. The proxy operator (or anyone who compromises the Cloudflare Worker) can:
- Swap Lightning invoices — Replace the
prfield inLnUrlCallbackResponsewith a different bolt11 invoice, redirecting payments to an attacker wallet. AffectsfetchLnUrlInvoice(line 46) andfetchInvoice(line 89). - Swap Ark addresses — Replace the
addressfield inArkMethodResponse, redirecting Ark deposits. AffectsfetchArkAddress(line 101). This is protocol-critical. - Harvest metadata — See every LNURL, Lightning address, payment amount, and comment the user sends.
Even if the current proxy is benign, hardcoding a personal free-tier Cloudflare Worker into a financial wallet is unacceptable. The proxy source isn't auditable from this PR, there's no SLA, and the domain could be transferred or hijacked.
HIGH: URL injection via missing encodeURIComponent
src/lib/lnurl.ts:30:
const proxyUrl = `https://cors-header-proxy.bordalix.workers.dev/proxy?apiurl=${url}`The url is interpolated raw. If the upstream callback URL contains & (common in query strings, e.g. ?amount=1000&comment=hi), those parameters bleed into the proxy's query string instead of being part of apiurl. This breaks fetchLnUrlInvoice (line 46) which appends ?amount=...&comment=... to the callback URL before passing it to the proxy.
Fix if this approach were acceptable: encodeURIComponent(url).
HIGH: No configuration or opt-out
The proxy URL is hardcoded with no environment variable, feature flag, or build-time configuration. Users cannot:
- Disable the proxy
- Use their own proxy
- Verify what proxy is in use
MEDIUM: Tests don't cover the new behavior
src/test/lib/lnurl.test.ts — Tests use vitest-fetch-mock to mock fetch globally. Since fetchWithCorsProxy calls fetch internally, the mock still intercepts — but the tests never assert that requests go to the proxy URL, and never verify the URL rewriting is correct. The URL injection bug above would pass all existing tests silently.
MEDIUM: Availability — single point of failure
Free-tier personal Cloudflare Worker. If it goes down or hits rate limits, the entire wallet loses LNURL and Ark address resolution. No fallback, no retry logic.
Recommendation
CORS issues with LNURL are a real problem for browser-based wallets, but the fix must not introduce a trusted third party into the payment flow. Options:
- Self-hosted proxy under the org's infrastructure with the source code in-repo or a separate auditable repo, behind the project's domain.
- Server-side relay — if the wallet has a backend, route LNURL fetches through it.
- Configurable proxy URL at minimum, so users/deployers control the trust boundary — but even this is a band-aid.
None of these should land without the encodeURIComponent fix regardless.
fetchArkAddress routes Ark address resolution through this proxy. Requires human sign-off even if reworked.
Reviewed by Arkana 🔍
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/lnurl.ts`:
- Around line 29-31: The proxy URL is built without encoding the full
callback/api URL which allows user-controlled characters (e.g., from
fetchInvoice's note -> &comment=${note}) to break the query string; update
fetchWithCorsProxy to call encodeURIComponent on the url when constructing
proxyUrl (so use encodeURIComponent(url) for the apiurl query value) and ensure
all callers (e.g., fetchInvoice) continue to pass the raw callback URL — only
fetchWithCorsProxy should perform the encoding.
- Around line 29-32: The hardcoded proxy in fetchWithCorsProxy should be
replaced with an environment-configurable URL and a direct-fetch fallback:
change fetchWithCorsProxy to read a proxy URL from an environment variable (e.g.
LNURL_CORS_PROXY_URL) and, if that value is falsy, call fetch(url, options)
directly; if the env var is set, construct the proxy request (preserving the
original options and encoding the original url) and call fetch(proxyUrl,
options). Update all call sites that use fetchWithCorsProxy (the existing usages
referenced in this module) to keep using fetchWithCorsProxy unchanged so they
inherit the new behavior. Ensure you do not alter request headers or body
handling when proxying and document the new env var behavior in a comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
76c153c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9f311a8c.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://cors-proxy.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
76c153c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2e9da7bf.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://cors-proxy.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
76c153c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c8a95b95.arkade-wallet.pages.dev |
| Branch Preview URL: | https://cors-proxy.arkade-wallet.pages.dev |
There was a problem hiding this comment.
Reviewed by Arkana 🔍 (incremental — new commit 3e758e1)
✅ Fixed
src/lib/lnurl.ts:30 — encodeURIComponent(url) now correctly encodes the target URL. The URL injection issue I flagged previously is resolved.
🔴 Still Outstanding
The CRITICAL and HIGH findings from my previous review remain unaddressed:
-
CRITICAL: Third-party MitM — All LNURL/Ark traffic still routes through
bordalix.workers.dev, a personal free-tier Cloudflare Worker. This proxy can swap invoices, swap Ark addresses, and harvest payment metadata. For a financial wallet handling real funds this is unacceptable. -
HIGH: No configuration — Proxy URL is still hardcoded. No env var, no feature flag, no opt-out.
-
MEDIUM: No test coverage for proxy URL rewriting logic.
-
MEDIUM: Single point of failure — no fallback if the Worker is down.
The encodeURIComponent fix is good, but the core trust/security problem persists. This PR should not be merged until the proxy is either org-controlled infrastructure or configurable.
fetchArkAddress routes Ark address resolution through the proxy — this touches the Ark deposit path. Requires human sign-off.
There was a problem hiding this comment.
Reviewed by Arkana 🔍 (incremental — new commit 239597d)
New in this commit
src/lib/lnurl.ts:30 — Adds PLAYWRIGHT_TEST env var check to bypass the proxy during Playwright tests.
src/lib/constants.ts:16 — Proxy URL extracted to constants.ts.
Observations on the new changes
-
src/lib/lnurl.ts:30—process.env.PLAYWRIGHT_TESTis a Node.js idiom. This is a Vite/browser app. In production builds,process.envwon't exist unless explicitly shimmed. The check will silently fail (falsy), so the proxy will always be used in prod — which is probably the intent — but in dev mode behavior depends on the Vite config. Not a blocker, just fragile. If the goal is test-only bypass, useimport.meta.env.VITE_PLAYWRIGHT_TESTto match the rest of the codebase's env var pattern. -
src/lib/constants.ts:16— The proxy URL is no longer inline but still hardcoded. This is cosmetic, not a fix for the configurability issue.
🔴 All previous critical findings remain unaddressed
| Severity | Issue | Status |
|---|---|---|
| CRITICAL | Third-party MitM — all LNURL/Ark traffic through bordalix.workers.dev |
❌ Not addressed |
| HIGH | No configuration / opt-out for proxy URL | ❌ Not addressed (moved to constants, still hardcoded) |
| MEDIUM | No test coverage for proxy URL rewriting | ❌ Not addressed (tests now bypass the proxy entirely) |
| MEDIUM | Single point of failure, no fallback | ❌ Not addressed |
| ✅ | encodeURIComponent fix |
Fixed in 3e758e1 |
The test bypass (PLAYWRIGHT_TEST check) actually makes the test coverage worse — tests now never exercise the proxy path at all, so regressions in URL construction would be invisible.
This PR should not be merged. The proxy must be org-controlled infrastructure or at minimum configurable via VITE_CORS_PROXY_URL env var with a direct-fetch fallback when unset.
fetchArkAddress (line 108) routes Ark address resolution through this proxy. Requires human sign-off.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/lnurl.ts`:
- Around line 50-51: The comment value is appended unencoded to the LNURL
callback URL (variable note) which allows characters like & to split into extra
query params; update the code that builds url (where note is appended) to encode
the comment value with encodeURIComponent (e.g., change url +=
`&comment=${note}` to url += `&comment=${encodeURIComponent(note)}`) so the
LNURL server receives the full comment string; this interacts safely with the
existing fetchWithCorsProxy call and checkResponse<LnUrlCallbackResponse>.
- Around line 30-34: The code in fetchWithCorsProxy currently reads
process.env.PLAYWRIGHT_TEST which fails in the browser; change it to use Vite's
import.meta.env with a VITE_ prefix (check import.meta.env.VITE_PLAYWRIGHT_TEST)
and treat it as a string comparison to '1' when deciding whether to bypass the
proxy; update any Playwright test webServer.env to set VITE_PLAYWRIGHT_TEST=1 so
tests still bypass the proxy. Ensure you only change the environment-access
expression in fetchWithCorsProxy and leave the proxyUrl logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dfb08c1-ec94-453e-a35f-0e089044f20e
📒 Files selected for processing (2)
src/lib/constants.tssrc/lib/lnurl.ts
| if (note) url += `&comment=${note}` | ||
| const res = await fetch(url).then(checkResponse<LnUrlCallbackResponse>) | ||
| const res = await fetchWithCorsProxy(url).then(checkResponse<LnUrlCallbackResponse>) |
There was a problem hiding this comment.
note is still appended unencoded, breaking the comment value at the LNURL server.
encodeURIComponent(url) in fetchWithCorsProxy correctly protects the proxy query-string structure (previous review ✅). However, the LNURL server decodes the forwarded URL and still sees comment=test&evil=1 as two separate query parameters when note contains &. The comment value received by the server is silently truncated.
🐛 Proposed fix
- if (note) url += `&comment=${note}`
+ if (note) url += `&comment=${encodeURIComponent(note)}`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/lnurl.ts` around lines 50 - 51, The comment value is appended
unencoded to the LNURL callback URL (variable note) which allows characters like
& to split into extra query params; update the code that builds url (where note
is appended) to encode the comment value with encodeURIComponent (e.g., change
url += `&comment=${note}` to url += `&comment=${encodeURIComponent(note)}`) so
the LNURL server receives the full comment string; this interacts safely with
the existing fetchWithCorsProxy call and checkResponse<LnUrlCallbackResponse>.
There was a problem hiding this comment.
Reviewed by Arkana 🔍 (incremental — new commits 855af8d, 50346ff)
New in these commits
-
src/lib/lnurl.ts:31— Addedtypeof process !== 'undefined'guard before accessingprocess.env. ✅ This addresses my previous note about the bareprocess.envreference crashing in browsers. Good. -
src/lib/constants.ts:16— Proxy URL switched fromhttps://cors-header-proxy.bordalix.workers.dev/proxy?apiurl=tohttp://localhost:8787/proxy?apiurl=, with the old URL left as a comment. -
50346ff— Removed debugconsole.log. ✅ Cleanup, fine.
🔴 NEW: Proxy URL points to localhost
src/lib/constants.ts:16 — The proxy URL is now http://localhost:8787/.... This is a dev-only URL that will break every deployed instance of the wallet. LNURL, Lightning invoice fetching, and Ark address resolution will all fail in production since no service is listening on localhost:8787 in the user's browser context.
This was likely a temporary dev switch that shouldn't be committed. The commented-out Cloudflare Worker URL above it confirms this is debugging state left in the PR.
🔴 All previous critical findings remain unaddressed
| Severity | Issue | Status |
|---|---|---|
| CRITICAL | Proxy URL now points to localhost — breaks all production use |
🆕 New regression |
| CRITICAL | Third-party MitM risk (when switched back to bordalix.workers.dev) |
❌ Not addressed |
| HIGH | No configuration / opt-out for proxy URL | ❌ Not addressed |
| MEDIUM | No test coverage for proxy URL rewriting | ❌ Not addressed |
| MEDIUM | Single point of failure, no fallback | ❌ Not addressed |
| ✅ | encodeURIComponent fix |
Fixed in 3e758e1 |
| ✅ | typeof process guard |
Fixed in 855af8d |
The PR description says "DO NOT MERGE" which is correct — this is clearly WIP. The typeof process guard is a good improvement, but the localhost URL and all previous findings mean this remains unshippable.
fetchArkAddress (line ~108) routes Ark address resolution through this proxy. Requires human sign-off before any merge.
There was a problem hiding this comment.
Reviewed by Arkana 🔍 (incremental — new commit 76c153c)
What changed
-
src/lib/constants.ts— RemovedcorsProxyUrlentirely (both thelocalhostURL and the commented-out production URL). ✅ Fixes the localhost regression from last commit. -
src/lib/lnurl.ts:3— MovedcorsProxyUrlback into the file as a module-level constant, pointing tohttps://cors-header-proxy.bordalix.workers.dev/proxy?apiurl=. Dropped theconstants.tsimport.
Assessment
The localhost regression is fixed. The proxy URL is back to the Cloudflare Worker.
However, all previously flagged issues from my earlier reviews remain unaddressed:
| Severity | Issue | Status |
|---|---|---|
localhost proxy URL breaks production |
✅ Fixed in 76c153c |
|
| CRITICAL | Third-party MitM risk — all LNURL and Ark address requests route through bordalix.workers.dev, a personal account proxy. An attacker (or the proxy operator) can intercept/modify Lightning invoices and Ark addresses, redirecting funds. |
❌ Not addressed |
| HIGH | No configuration / env-var override for proxy URL — operators can't point to their own proxy or disable it | ❌ Not addressed |
| MEDIUM | No test coverage for fetchWithCorsProxy URL rewriting logic |
❌ Not addressed |
| MEDIUM | Single point of failure — if the Worker is down, all LNURL/Ark resolution fails with no fallback | ❌ Not addressed |
The PR description still says "DO NOT MERGE", which remains correct. The code is functional but the security posture is not merge-ready.
fetchArkAddress (line ~110) routes Ark address resolution through this third-party proxy. Requires human sign-off before any merge.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/lnurl.ts (1)
52-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
notemust be encoded before appending to the callback URL.Special characters (
&,=,+,#, spaces) innotebreak the query string, causing the LNURL server to receive a silently truncated or incorrectcommentvalue.🐛 Proposed fix
- if (note) url += `&comment=${note}` + if (note) url += `&comment=${encodeURIComponent(note)}`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/lnurl.ts` at line 52, The code appends raw note text into the callback URL ("if (note) url += `&comment=${note}`"), which breaks the query string for special characters; update the append to URL-encode the note before concatenation (use JavaScript's encodeURIComponent on the note variable) so the comment query parameter is safely escaped when building the url.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/lnurl.ts`:
- Line 3: The constant corsProxyUrl (in lnurl.ts) must not point to a hardcoded
personal proxy; replace this by reading a configurable value (e.g., an
environment/runtime config or injected setting) and avoid silently forcing all
LNURL traffic through it—if no project-owned proxy is configured, fall back to
direct requests or fail with a clear error; ensure all call sites that reference
corsProxyUrl use the new config accessor and add validation that the configured
proxy is organization-owned (or empty) and documented so operators can supply
their own proxy or backend relay.
---
Duplicate comments:
In `@src/lib/lnurl.ts`:
- Line 52: The code appends raw note text into the callback URL ("if (note) url
+= `&comment=${note}`"), which breaks the query string for special characters;
update the append to URL-encode the note before concatenation (use JavaScript's
encodeURIComponent on the note variable) so the comment query parameter is
safely escaped when building the url.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| @@ -1,5 +1,7 @@ | |||
| import { bech32, utf8 } from '@scure/base' | |||
|
|
|||
| const corsProxyUrl = 'https://cors-header-proxy.bordalix.workers.dev/proxy?apiurl=' | |||
There was a problem hiding this comment.
Hardcoded personal CORS proxy is a critical trust, privacy, and reliability blocker.
All LNURL-related network traffic — including payment callbacks, Lightning invoices, and Ark addresses — is routed through cors-header-proxy.bordalix.workers.dev, a proxy on an individual developer's personal account. This carries several hard blockers for any merge:
- Trust/Privacy: Users' payment amounts, invoice data, and receiving addresses are all readable by the proxy operator.
- Availability: The Workers Free plan has a daily request limit of 100,000 requests, and when this limit is exceeded, Cloudflare returns Error 1027 — causing silent failures for all wallet users after the cap is hit.
- Single point of failure: If the worker is deleted, redeployed, or misconfigured, the entire LNURL feature breaks for all users with no fallback.
Before this can be merged, the solution needs either:
- A properly governed, project-owned proxy (or a server-side relay endpoint in the wallet's own backend), or
- A CORS-native approach (e.g., configuring LNURL servers/LN addresses you control to emit correct headers, or using a user-configurable proxy setting).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/lnurl.ts` at line 3, The constant corsProxyUrl (in lnurl.ts) must not
point to a hardcoded personal proxy; replace this by reading a configurable
value (e.g., an environment/runtime config or injected setting) and avoid
silently forcing all LNURL traffic through it—if no project-owned proxy is
configured, fall back to direct requests or fail with a clear error; ensure all
call sites that reference corsProxyUrl use the new config accessor and add
validation that the configured proxy is organization-owned (or empty) and
documented so operators can supply their own proxy or backend relay.
1 participant
Fetches LNURL via a CORS proxy.
Warning: I'm using a proxy in my personal account (free tier) for tests.
DO NOT MERGE
Summary by CodeRabbit