feat(account): auth nostr#990
Draft
escapedcat wants to merge 12 commits into
Draft
Conversation
Contributor
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
✅ Deploy Preview for btcmap ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds Nostr authentication to the login flow via NIP-98 and NIP-07 standards, enabling users to sign in with a Nostr extension or by providing an ChangesNostr Authentication Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (1)
src/lib/nostr.ts (1)
15-18: 💤 Low value
signEventreturn type may be overstated.NIP-07 extensions return a plain signed
Event(withidandsig); they do not set thenostr-tools"verified" brand thatVerifiedEventcarries. Typing the extension method as returningPromise<VerifiedEvent>is misleading, especially sincesignAuthWithSecretKeyusesfinalizeEvent()which properly sets the brand, creating an inconsistency between the two signing paths that both claim to returnVerifiedEvent.In practice this works because the values are JSON-serialized and verified server-side, but the inaccurate type can mislead future code that might check the
[verifiedSymbol]brand. Consider returningEventfromsignEventand verifying explicitly only when a brandedVerifiedEventis needed.🤖 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/nostr.ts` around lines 15 - 18, The NIP-07 extension type NostrExtension currently types signEvent as returning Promise<VerifiedEvent>, but extensions only return a plain Event (no verified brand); change signEvent's return type to Promise<Event> in the NostrExtension definition and update any callers that need a branded VerifiedEvent (e.g., code paths like signAuthWithSecretKey) to explicitly call finalizeEvent(event) to produce a VerifiedEvent where required; reference symbols to change: NostrExtension, signEvent, EventTemplate, VerifiedEvent, signAuthWithSecretKey, finalizeEvent.
🤖 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/components/auth/NostrLoginForm.svelte`:
- Around line 77-94: Add a re-entry guard at the start of the async login
handlers to prevent duplicate executions when the submit event races the
disabled UI: in loginWithNsec() add an early check like "if (nsecLoading)
return;" before any work starts (and likewise add a guard in
loginWithExtension(), e.g. "if (extensionLoading) return;"). This ensures the
function returns immediately if an in-progress flag
(nsecLoading/extensionLoading) is already set, preventing double decode/sign and
duplicate network requests; keep the existing loading flag management and
cleanup as-is.
In `@src/lib/i18n/locales/en.json`:
- Around line 86-95: Add the ten new Nostr i18n keys introduced in en.json to
every non-English locale file so keys remain aligned; specifically add
nostrExtension, nostrSigning, nostrFailed, nostrError, nsecToggle, nsecLabel,
nsecWarning, nsecSubmit, nsecInvalid, and or to each of bg.json, de.json,
es.json, fr.json, nl.json, pt-BR.json, and ru.json with appropriate translated
strings (or temporary placeholders), ensuring the JSON structure and ordering
match the existing locale files.
In `@src/routes/api/session/nostr/`+server.ts:
- Around line 42-51: The fetch to `${API_BASE}/v4/auth/nostr` currently has no
timeout; update the call in the block that defines `res` to use an abort signal
(e.g., `AbortSignal.timeout(timeoutMs)` or an `AbortController`) passed to fetch
so the request is cancelled after a short timeout; catch aborts and other errors
and ensure you still call `error(502, "Failed to authenticate with Nostr")` (and
log the abort/detail via `console.error`) so the route fails fast instead of
hanging when `API_BASE` is slow or unresponsive, referencing the existing `res`,
`API_BASE`, `eventB64`, and `error(502, ...)` symbols.
- Around line 21-24: The current content-length guard uses
Number(request.headers.get("content-length")) which yields 0 when the header is
missing, allowing chunked/unknown-length bodies to bypass the MAX_BODY_BYTES
check; update the check in the handler so you first read the raw header (e.g.,
const rawCL = request.headers.get("content-length")), reject if rawCL is
null/empty with error(413, "Request body too large"), then parse to a number
(const contentLength = Number(rawCL)) and validate
Number.isFinite(contentLength) && contentLength <= MAX_BODY_BYTES before
proceeding to request.json(); reference the contentLength variable,
MAX_BODY_BYTES constant, and request.json() so the guard explicitly fails when
the header is absent or invalid.
---
Nitpick comments:
In `@src/lib/nostr.ts`:
- Around line 15-18: The NIP-07 extension type NostrExtension currently types
signEvent as returning Promise<VerifiedEvent>, but extensions only return a
plain Event (no verified brand); change signEvent's return type to
Promise<Event> in the NostrExtension definition and update any callers that need
a branded VerifiedEvent (e.g., code paths like signAuthWithSecretKey) to
explicitly call finalizeEvent(event) to produce a VerifiedEvent where required;
reference symbols to change: NostrExtension, signEvent, EventTemplate,
VerifiedEvent, signAuthWithSecretKey, finalizeEvent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb9a640c-fef9-4ca0-a352-a488f3baa0c4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
package.jsonsrc/components/auth/NostrLoginForm.sveltesrc/components/layout/UserMenu.sveltesrc/lib/i18n/locales/en.jsonsrc/lib/nostr.tssrc/routes/api/session/nostr/+server.tssrc/routes/login/+page.svelte
escapedcat
added a commit
that referenced
this pull request
May 10, 2026
Per CodeRabbit review on PR #990: the button's `disabled` prop propagates asynchronously, so a second click (or Enter on a focused button) can fire the handler again before the disabled state takes effect. Without a guard, loginWithExtension would open two extension- signing popups for one user intent, and loginWithNsec would decode + sign the same nsec twice and double-fire /api/session/nostr. Add `if (loading) return;` at the top of both handlers. Cheap, no behavior change on the happy path.
escapedcat
added a commit
that referenced
this pull request
May 10, 2026
…903 Per CodeRabbit nitpick on PR #990: NIP-07 extensions return a plain signed Event without nostr-tools' verifiedSymbol brand, so typing window.nostr.signEvent as `Promise<VerifiedEvent>` was misleading. finalizeEvent (used in the nsec path) does set the brand, so the two signing paths actually returned subtly different shapes despite both being annotated as VerifiedEvent. Introduce a shared SignedAuthEvent alias (= Event) in $lib/nostr, type both signing functions and the extension contract as that, and update the NostrLoginForm callsite. Behavior unchanged: callers still JSON-serialize the event and let btcmap-api re-verify the Schnorr signature, so the brand was never load-bearing — but accurate types prevent any future code from mistakenly relying on a brand the extension path doesn't provide.
escapedcat
added a commit
that referenced
this pull request
May 10, 2026
Per CodeRabbit review on PR #990: when content-length is absent (e.g. chunked transfer encoding), Number(null) ⇒ 0, which is finite and ≤ 4096, so the size cap was silently bypassed. Oversized chunked bodies would fall through to request.json() and only be caught by SvelteKit's default body limit — much further down the chain. Reject the missing-header case explicitly with 411 Length Required, keeping the explicit guard meaningful for the documented happy path.
escapedcat
added a commit
that referenced
this pull request
May 10, 2026
Per CodeRabbit review on PR #990: the cross-origin call to btcmap-api had no timeout, so a slow or hung API would tie up the SvelteKit request handler indefinitely and leave the user staring at a "Signing…" UI. Wire up AbortSignal.timeout(10_000) so the fetch fails fast. The existing catch already surfaces network errors as 502; AbortError (timeout) lands in the same branch with no special handling needed. 10s is generous for the API path here — sync DB lookup + token mint, no network.
escapedcat
added a commit
that referenced
this pull request
May 10, 2026
Per CodeRabbit review on PR #990: the 10 new login.* keys introduced in en.json (nostrExtension, nostrSigning, nostrFailed, nostrError, nsecToggle, nsecLabel, nsecWarning, nsecSubmit, nsecInvalid, or) were missing from bg, de, es, fr, nl, pt-BR, ru. SvelteKit's i18n falls back silently to English when keys are missing, but per CLAUDE.md keys should stay aligned across locales so reviewers and translators can see what needs translating. Use English text as placeholder values; translators can refine later. nl.json uses HTML entities for diacritics per the file's existing convention (…, —); other locales use literal Unicode (existing style). Note: many other login keys (login.username, login.password, etc.) are also missing from these locales — pre-existing gap, out of scope here. The nl.json line-count delta is mostly biome reformatting unrelated JSON entries to consistent style; the actual key additions are 10 lines.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Nostr-based authentication to the web app’s login flow, including a server-side proxy endpoint to exchange NIP-98 signed events for a BTC Map API session token, and adjusts logout to navigate back to /login to avoid stale authenticated UI.
Changes:
- Added
NostrLoginFormto the login page, supporting NIP-07 browser extensions and localnsecsigning. - Introduced
/api/session/nostrserver route to forward NIP-98 auth viaAuthorization: Nostr <base64>. - Updated logout behavior to redirect to
/loginafter clearing the session.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/login/+page.svelte | Adds Nostr login UI above the existing username/password form with an “or” divider. |
| src/components/auth/NostrLoginForm.svelte | Implements extension-based and nsec-based signing flows and exchanges the signed event for a session token. |
| src/routes/api/session/nostr/+server.ts | Adds server proxy endpoint to exchange signed NIP-98 event for API token with size/time guards. |
| src/lib/nostr.ts | Adds shared Nostr helpers for building/signing auth events and decoding nsec. |
| src/components/layout/UserMenu.svelte | Redirects to /login after logout to avoid inconsistent authenticated-page UI. |
| src/lib/i18n/locales/en.json | Adds login-related Nostr strings and login.or. |
| src/lib/i18n/locales/nl.json | Adds login-related Nostr strings and reformats some JSON blocks. |
| src/lib/i18n/locales/bg.json | Adds login-related Nostr strings and login.or. |
| src/lib/i18n/locales/de.json | Adds login-related Nostr strings and login.or. |
| src/lib/i18n/locales/es.json | Adds login-related Nostr strings and login.or. |
| src/lib/i18n/locales/fr.json | Adds login-related Nostr strings and login.or. |
| src/lib/i18n/locales/pt-BR.json | Adds login-related Nostr strings and login.or. |
| src/lib/i18n/locales/ru.json | Adds login-related Nostr strings and login.or. |
| package.json | Adds nostr-tools dependency. |
| pnpm-lock.yaml | Locks nostr-tools and its dependency graph. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
escapedcat
added a commit
that referenced
this pull request
May 10, 2026
Per Copilot review on PR #990: svelte-i18n's `_` store returns the raw translation string, and Svelte's `{$_(...)}` text bindings render text without decoding HTML entities. So `Signing…` and `—` would have shown up literally in the Dutch UI rather than as `…` and `—`. Convert just my new keys (nostrSigning, nsecWarning) to literal Unicode, matching what en.json already does. Pre-existing entity strings elsewhere in nl.json (Kopiëren, Verifiëren, etc.) are left as-is — fixing those is a separate, broader concern about CLAUDE.md's "use entity style for diacritics" guidance not matching the actual render path.
escapedcat
added a commit
that referenced
this pull request
May 10, 2026
…903 Per Copilot review on PR #990: $lib/nostr adds auth-critical helpers (NIP-98 template build, signing, nsec decoding) without coverage. Adding tests now prevents regressions in the most likely failure modes — wrong HRP, malformed bech32, the wrong shape of signed event. decodeNsec: - valid nsec → 32-byte Uint8Array, round-trips an encode/decode pair - whitespace tolerance - rejects npub (HRP mismatch) with generic "not an nsec" error - rejects malformed bech32 (propagates the underlying parse failure) - rejects empty input signAuthWithSecretKey: - produces a kind-27235 event with the correct NIP-98 tags (u points at /v4/auth/nostr; method is POST) - created_at lands in [now-1, now+1] seconds - id and sig hex-formatted; pubkey matches getPublicKey(sk) - two events from the same key get distinct created_at / id (regression guard against accidental memoization) 8 tests, all green. Full suite: 336 passed, was 328. The NIP-07 path (signAuthWithExtension) requires a window.nostr mock and is intentionally not covered here — would test the mock, not the production code path. Worth adding when an integration harness exists.
Member
|
How do we feel about not raw-dogging nsecs and relying on extensions and/or bunker? |
Member
|
Maybe not ready yet, but raw-dogging a rando nsec or extension auth not working for me. |
Contributor
Author
|
No, API needs to be merged first, sorry, this should be a draft |
Member
|
Ah, I think it's merged but not deployed. We ideally need to get API hooked up to CI. |
Contributor
Author
|
Ah sorry, no, that was just a prep PR, we now need this: |
First in a small series adding NIP-98 Nostr login alongside the
existing username/password flow. Lands the foundations only:
- nostr-tools@^2.23.3: Schnorr signing, NIP-19 (nsec/npub) decoding,
and the EventTemplate/finalizeEvent surface used for browser-side
signing of the kind 27235 auth event.
- src/lib/nostr.ts:
- NOSTR_AUTH_URL derived from API_BASE so local dev signs for the
same origin the API verifies (BTCMAP_API_BASE_URL on the server,
VITE_API_BASE_URL here).
- getNostrExtension() — NIP-07 browser-extension detection.
- signAuthWithExtension() — sign via window.nostr (Alby, nos2x).
- signAuthWithSecretKey() — sign locally with a raw nsec-decoded key.
- decodeNsec() — bech32 nsec → Uint8Array, rejects npub/malformed
input with a generic message (no detail leak).
The browser-facing /api/session/nostr proxy and the login-page UI
land in follow-up commits.
…1004 SvelteKit server route that bridges the browser's {signed_event} JSON contract (no CORS preflight) to btcmap-api's canonical NIP-98 Authorization: Nostr <base64(event)> header. The browser-side login UI POSTs the signed kind 27235 event as JSON; this route base64-encodes it and forwards to POST /v4/auth/nostr with the header (no body). The API verifies the Schnorr signature against the URL pinned by ApiBaseUrl, then either looks up the user by npub or auto-creates one. The response carries {token, username, npub}. The 4 KB content-length cap rejects obvious junk before we parse the JSON envelope. 401/403 from the API surface as 401 Invalid Nostr signature; transport / non-2xx fall through to 502. Same shape as login/+server.ts (native fetch, error/json helpers). The endpoint name follows api-base.ts policy: never hardcode api.btcmap.org, always use API_BASE so VITE_API_BASE_URL works for local dev.
Component for NIP-98 sign-in mirroring main's <LoginForm> contract:
exposes onSuccess(session) so callers (e.g. /login, save-flow modal)
own the post-login navigation.
Two paths, both resolving to the same /api/session/nostr proxy:
1. NIP-07 browser extension (Alby, nos2x). Detected on mount with a
single 300ms re-check to handle slow injection without blocking
first paint.
2. nsec paste fallback. Decoded once via decodeNsec, used to sign
exactly one event, then the secretKey buffer is best-effort
zeroed (V8 may have copies elsewhere — comment notes this).
Login error classification: 401 from the proxy maps to
nostrFailed ("Nostr signature was rejected"); transport / other
HTTP errors fall through to nostrError ("Could not complete Nostr
login"). Decode failures route to nsecInvalid.
Session shape unchanged: session.login(username, "", token) sets
autoGenerated=false so BackupModal won't show a password — the
user's Nostr key is their backup.
i18n: 9 new keys under "login" (nostrExtension, nostrSigning,
nostrFailed, nostrError, nsecToggle, nsecLabel, nsecWarning,
nsecSubmit, nsecInvalid). en.json updated; other locales fall
back to English at runtime per CLAUDE.md.
Component is not yet rendered anywhere — the /login page wires
it up in a follow-up commit.
…903 Two-path /login: Nostr (extension or nsec paste) at the top, then a divider with "or", then the username/password <LoginForm>. Both forms share the same handleSuccess(current) which hydrates saved items and navigates to /user/activity. The Nostr block is wrapped in a `space-y-3` container so the extension button + nsec toggle stack consistently when both render. i18n: one new "login.or" key for the divider label. en.json updated; other locales fall back to English at runtime.
Without an explicit navigation, clicking Logout from /user/saved or
/user/activity cleared the session in localStorage but kept the user
on the same page. Those pages only check for a session in onMount,
so they kept rendering the post-login UI — and any subsequent
saved-* / activity fetch fired without a token and surfaced as
"Failed to load…" toasts.
After session.clear(), goto('/login') so the user lands on a clean
state and can sign back in (with username/password OR Nostr).
Per CodeRabbit review on PR #990: the button's `disabled` prop propagates asynchronously, so a second click (or Enter on a focused button) can fire the handler again before the disabled state takes effect. Without a guard, loginWithExtension would open two extension- signing popups for one user intent, and loginWithNsec would decode + sign the same nsec twice and double-fire /api/session/nostr. Add `if (loading) return;` at the top of both handlers. Cheap, no behavior change on the happy path.
…903 Per CodeRabbit nitpick on PR #990: NIP-07 extensions return a plain signed Event without nostr-tools' verifiedSymbol brand, so typing window.nostr.signEvent as `Promise<VerifiedEvent>` was misleading. finalizeEvent (used in the nsec path) does set the brand, so the two signing paths actually returned subtly different shapes despite both being annotated as VerifiedEvent. Introduce a shared SignedAuthEvent alias (= Event) in $lib/nostr, type both signing functions and the extension contract as that, and update the NostrLoginForm callsite. Behavior unchanged: callers still JSON-serialize the event and let btcmap-api re-verify the Schnorr signature, so the brand was never load-bearing — but accurate types prevent any future code from mistakenly relying on a brand the extension path doesn't provide.
Per CodeRabbit review on PR #990: when content-length is absent (e.g. chunked transfer encoding), Number(null) ⇒ 0, which is finite and ≤ 4096, so the size cap was silently bypassed. Oversized chunked bodies would fall through to request.json() and only be caught by SvelteKit's default body limit — much further down the chain. Reject the missing-header case explicitly with 411 Length Required, keeping the explicit guard meaningful for the documented happy path.
Per CodeRabbit review on PR #990: the cross-origin call to btcmap-api had no timeout, so a slow or hung API would tie up the SvelteKit request handler indefinitely and leave the user staring at a "Signing…" UI. Wire up AbortSignal.timeout(10_000) so the fetch fails fast. The existing catch already surfaces network errors as 502; AbortError (timeout) lands in the same branch with no special handling needed. 10s is generous for the API path here — sync DB lookup + token mint, no network.
Per CodeRabbit review on PR #990: the 10 new login.* keys introduced in en.json (nostrExtension, nostrSigning, nostrFailed, nostrError, nsecToggle, nsecLabel, nsecWarning, nsecSubmit, nsecInvalid, or) were missing from bg, de, es, fr, nl, pt-BR, ru. SvelteKit's i18n falls back silently to English when keys are missing, but per CLAUDE.md keys should stay aligned across locales so reviewers and translators can see what needs translating. Use English text as placeholder values; translators can refine later. nl.json uses HTML entities for diacritics per the file's existing convention (…, —); other locales use literal Unicode (existing style). Note: many other login keys (login.username, login.password, etc.) are also missing from these locales — pre-existing gap, out of scope here. The nl.json line-count delta is mostly biome reformatting unrelated JSON entries to consistent style; the actual key additions are 10 lines.
Per Copilot review on PR #990: svelte-i18n's `_` store returns the raw translation string, and Svelte's `{$_(...)}` text bindings render text without decoding HTML entities. So `Signing…` and `—` would have shown up literally in the Dutch UI rather than as `…` and `—`. Convert just my new keys (nostrSigning, nsecWarning) to literal Unicode, matching what en.json already does. Pre-existing entity strings elsewhere in nl.json (Kopiëren, Verifiëren, etc.) are left as-is — fixing those is a separate, broader concern about CLAUDE.md's "use entity style for diacritics" guidance not matching the actual render path.
…903 Per Copilot review on PR #990: $lib/nostr adds auth-critical helpers (NIP-98 template build, signing, nsec decoding) without coverage. Adding tests now prevents regressions in the most likely failure modes — wrong HRP, malformed bech32, the wrong shape of signed event. decodeNsec: - valid nsec → 32-byte Uint8Array, round-trips an encode/decode pair - whitespace tolerance - rejects npub (HRP mismatch) with generic "not an nsec" error - rejects malformed bech32 (propagates the underlying parse failure) - rejects empty input signAuthWithSecretKey: - produces a kind-27235 event with the correct NIP-98 tags (u points at /v4/auth/nostr; method is POST) - created_at lands in [now-1, now+1] seconds - id and sig hex-formatted; pubkey matches getPublicKey(sk) - two events from the same key get distinct created_at / id (regression guard against accidental memoization) 8 tests, all green. Full suite: 336 passed, was 328. The NIP-07 path (signAuthWithExtension) requires a window.nostr mock and is intentionally not covered here — would test the mock, not the production code path. Worth adding when an integration harness exists.
escapedcat
force-pushed
the
feature/auth-nostr-on-main
branch
from
c6d13c1 to
6176e4d
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
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
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 by CodeRabbit
New Features
Bug Fixes