Optional HTTP Basic Authorization configuration#619
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds HTTP Basic Auth support across the application by introducing a Vite plugin for dev/preview servers and a Cloudflare Pages middleware for production edge requests. Both implementations read credentials from environment variables and are no-ops if either credential is unset. ChangesHTTP Basic Authentication across Vite and Cloudflare
🎯 2 (Simple) | ⏱️ ~8 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 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.
Code Review — #619
Scope: Frontend deployment auth — NOT protocol-critical. No VTXO, signing, or round-lifecycle code touched. This adds optional HTTP Basic Auth gating on the wallet SPA bundle (defense-in-depth for private instances).
🔴 Must Fix
1. PagesFunction type is unresolved — build will break
functions/_middleware.ts:18 — PagesFunction is used but never imported, and @cloudflare/workers-types is not in package.json or referenced in tsconfig.json. The tsconfig include: ["**/*.ts"] will pick up this file during type-checking.
Fix: either add @cloudflare/workers-types as a dev dependency and include it in tsconfig compilerOptions.types, or exclude functions/ from tsconfig and add a separate functions/tsconfig.json for CF Pages.
2. Timing-unsafe credential comparison
functions/_middleware.ts:27 and plugins/vite-plugin-basic-auth.ts:18 — Both use === to compare the Authorization header against the expected value. This is theoretically vulnerable to timing side-channel attacks.
Practical risk is low over HTTP (network jitter dwarfs the timing delta), but it's trivial to fix and this is an auth boundary:
// Cloudflare Workers (functions/_middleware.ts)
// Use crypto.timingSafeEqual via the Web Crypto API or a polyfill
const encoder = new TextEncoder()
const a = encoder.encode(auth)
const b = encoder.encode(expected)
if (a.byteLength !== b.byteLength || !crypto.subtle.timingSafeEqual(a, b)) return unauthorized()
// Node/Vite (plugins/vite-plugin-basic-auth.ts)
import { timingSafeEqual } from 'node:crypto'
const a = Buffer.from(auth ?? '')
const b = Buffer.from(expected)
if (a.byteLength !== b.byteLength || !timingSafeEqual(a, b)) return unauthorized()🟡 Should Fix
3. No brute-force protection
Neither implementation has rate limiting. An attacker can hammer the endpoint with credential guesses. For Cloudflare, consider Cloudflare's built-in rate limiting rules. For the Vite dev server this matters less, but worth noting.
4. No tests
No test coverage for either the Vite plugin or the Cloudflare middleware. At minimum, unit tests should cover:
- Passthrough when env vars are unset
- 401 when no
Authorizationheader is provided - 401 when wrong credentials are provided
- 200 (pass-through to
next()) when correct credentials are provided
✅ Looks Good
- Plugin ordering in
vite.config.ts— auth runs first, correct. - No-op when env vars are unset — clean opt-in behavior.
- Consistent implementation between Vite (Node
Buffer) and CF Workers (btoa) — appropriate for each runtime. WWW-Authenticateheader on 401 — proper RFC 7617 behavior, triggers browser credential prompt.- No protocol-level code touched.
Verdict: Request changes on items 1 and 2. The type error is a concrete build break; the timing comparison is a cheap fix on an auth boundary that should be done right. Items 3–4 are recommended but non-blocking.
![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
🤖 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 `@functions/_middleware.ts`:
- Line 28: Replace the direct `!==` comparison between `auth` and `expected` in
the middleware with a constant-time comparison to avoid timing attacks:
implement (or import) a `constantTimeCompare(a, b)` helper and call it where `if
(auth !== expected) return unauthorized()` currently is (i.e., use `if
(!constantTimeCompare(auth, expected)) return unauthorized()`), ensuring the
helper handles differing lengths and performs byte-by-byte constant-time checks
(Cloudflare Workers do not provide crypto.timingSafeEqual, so implement or
vendor a small constant-time routine).
- Line 27: The current computation of expected using
btoa(`${username}:${password}`) fails for non-ASCII characters; update the Basic
auth encoding to use a UTF-8 aware base64 encoder (e.g., replace the btoa usage
with a Buffer-based encoding or TextEncoder approach) so that expected is
computed as base64 of the UTF-8 bytes of `${username}:${password}` (ensure you
change the expression where expected is assigned and remove the btoa call,
referencing the existing variables username and password to produce the RFC
7617-compliant header).
In `@plugins/vite-plugin-basic-auth.ts`:
- Line 18: Replace the insecure direct comparison if (auth === expected) with a
constant-time comparison: hash both auth and expected (e.g., SHA-256) into
fixed-size buffers and use crypto.timingSafeEqual to compare the digests so
timing cannot reveal credential details; update the code around the
auth/expected check (the branch that currently uses if (auth === expected)) to
compute the digests and call timingSafeEqual, handling any crypto imports and
edge cases (ensure both inputs are converted to Buffers/strings before hashing).
🪄 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: ad34f9ca-14e6-4fa2-8016-6c3d02ff4897
📒 Files selected for processing (3)
functions/_middleware.tsplugins/vite-plugin-basic-auth.tsvite.config.ts
| const auth = context.request.headers.get('Authorization') | ||
| if (!auth) return unauthorized() | ||
|
|
||
| const expected = 'Basic ' + btoa(`${username}:${password}`) |
There was a problem hiding this comment.
Critical: Encoding inconsistency with Vite plugin breaks non-ASCII passwords.
btoa() doesn't handle UTF-8 characters properly, while the Vite plugin uses Buffer.from() which correctly encodes UTF-8. This causes the dev server and production to compute different expected values for passwords containing non-ASCII characters (e.g., "café", "münchen"), breaking authentication inconsistently across environments.
Per RFC 7617, HTTP Basic Auth should support UTF-8.
🔧 Proposed fix using TextEncoder for UTF-8 support
- const expected = 'Basic ' + btoa(`${username}:${password}`)
+ const credentials = `${username}:${password}`
+ const utf8Bytes = new TextEncoder().encode(credentials)
+ const binaryString = String.fromCharCode(...utf8Bytes)
+ const expected = 'Basic ' + btoa(binaryString)Or, if non-ASCII passwords aren't required, document that only ASCII is supported and validate accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const expected = 'Basic ' + btoa(`${username}:${password}`) | |
| const credentials = `${username}:${password}` | |
| const utf8Bytes = new TextEncoder().encode(credentials) | |
| const binaryString = String.fromCharCode(...utf8Bytes) | |
| const expected = 'Basic ' + btoa(binaryString) |
🤖 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 `@functions/_middleware.ts` at line 27, The current computation of expected
using btoa(`${username}:${password}`) fails for non-ASCII characters; update the
Basic auth encoding to use a UTF-8 aware base64 encoder (e.g., replace the btoa
usage with a Buffer-based encoding or TextEncoder approach) so that expected is
computed as base64 of the UTF-8 bytes of `${username}:${password}` (ensure you
change the expression where expected is assigned and remove the btoa call,
referencing the existing variables username and password to produce the RFC
7617-compliant header).
…d crypto safe timing equality validation
Deploying wallet-staging with
|
| Latest commit: |
6acd835
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b506f4a6.wallet-staging-95g.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
6acd835
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6def9145.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://basic-auth.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
6acd835
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8a3f7fb7.arkade-wallet.pages.dev |
| Branch Preview URL: | https://basic-auth.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
6acd835
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6835c357.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://basic-auth.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Re-review — new commits address previous findings ✅
Both must-fix items from my prior review are resolved:
| # | Issue | Resolution |
|---|---|---|
| 1 | PagesFunction unresolved type |
Replaced with local EventContext interface — no external type dep needed |
| 2 | Timing-unsafe === comparison |
crypto.subtle.timingSafeEqual (CF Workers) and node:crypto.timingSafeEqual (Vite) — both correct |
Implementation looks clean:
functions/_middleware.ts:TextEncoder+crypto.subtle.timingSafeEqualwith length pre-check.btoaavailable in CF Workers runtime. ✓plugins/vite-plugin-basic-auth.ts:Buffer.from+timingSafeEqualwith length pre-check.expectedstring captured in closure, re-buffered per request (acceptable). ✓vite.config.ts: Plugin registered first in the array — middleware runs before other plugins. ✓- Both are no-ops when env vars are unset. ✓
Not protocol-critical — no VTXO, signing, or round-lifecycle code touched. This is deployment-layer auth gating on the SPA bundle.
Previous note on brute-force protection (item 3) still stands as a nice-to-have — recommend Cloudflare rate-limiting rules on the dashboard side rather than in-code.
LGTM. Approving.
2 participants
Adds optional configuration of HTTP basic authentication by setting
BASIC_AUTH_USERNAMEandBASIC_AUTH_PASSWORDas environment variables.Support for both the Vite server via a plugin, as well as Cloudflare Pages middleware.
Allows for the protection of private wallet instances.
Summary by CodeRabbit