auth/env hardening and reliability updates#44
Merged
Conversation
parsing, and IPv6 loopback filter
…put, and E2E scaffolding
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
📝 WalkthroughWalkthroughUpdated CI/workflows and Docker images; pinned Bun; added unit tests and type checks; hardened security audits and rate-limit headers; tightened auth, env, relay, and NIP‑46 validation; increased PBKDF2 iterations; refactored frontend callbacks/accessibility; added many tests, migrations, and utilities. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Member
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (18)
src/db/migrations/20250916_0004_audit_nip46_data_sizes.sql (1)
41-43: Consider centralizing repeated size expressions via a CTE.The same
LENGTH(CAST(... AS BLOB))expressions are repeated multiple times. Computing once reduces drift risk and makes future threshold updates safer.♻️ Proposed refactor
+WITH session_sizes AS ( + SELECT + id, + user_id, + client_pubkey, + COALESCE(LENGTH(CAST(relays AS BLOB)), 0) AS relay_size, + COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) AS methods_size, + COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) AS kinds_size + FROM nip46_sessions +) INSERT INTO nip46_data_audit ( session_id, user_id, client_pubkey, relay_size, methods_size, kinds_size, total_size, has_risk_old_limit, has_risk_new_limit ) SELECT - id, + id, user_id, client_pubkey, - CASE WHEN relays IS NOT NULL THEN LENGTH(CAST(relays AS BLOB)) ELSE 0 END as relay_size, - CASE WHEN policy_methods IS NOT NULL THEN LENGTH(CAST(policy_methods AS BLOB)) ELSE 0 END as methods_size, - CASE WHEN policy_kinds IS NOT NULL THEN LENGTH(CAST(policy_kinds AS BLOB)) ELSE 0 END as kinds_size, - CASE - WHEN relays IS NOT NULL OR policy_methods IS NOT NULL OR policy_kinds IS NOT NULL - THEN COALESCE(LENGTH(CAST(relays AS BLOB)), 0) + COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) + COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) - ELSE 0 - END as total_size, + relay_size, + methods_size, + kinds_size, + relay_size + methods_size + kinds_size AS total_size, CASE - WHEN COALESCE(LENGTH(CAST(relays AS BLOB)), 0) > 10000 - OR COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) > 10000 - OR COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) > 10000 + WHEN relay_size > 10000 + OR methods_size > 10000 + OR kinds_size > 10000 THEN 1 ELSE 0 END as has_risk_old_limit, CASE - WHEN COALESCE(LENGTH(CAST(relays AS BLOB)), 0) > 50000 - OR COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) > 50000 - OR COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) > 50000 + WHEN relay_size > 50000 + OR methods_size > 50000 + OR kinds_size > 50000 THEN 1 ELSE 0 END as has_risk_new_limit -FROM nip46_sessions; +FROM session_sizes;Also applies to: 46-46, 52-54, 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/migrations/20250916_0004_audit_nip46_data_sizes.sql` around lines 41 - 43, Repeated LENGTH(CAST(... AS BLOB)) expressions for relays, policy_methods, and policy_kinds should be computed once in a CTE and then referenced in the main SELECT to avoid duplication and drift; add a WITH block (e.g., audit_blob_sizes) that selects id plus computed relay_size, methods_size, kinds_size using LENGTH(CAST(column AS BLOB)) and then join or select from that CTE in place of repeating the expressions in the existing SELECT (referencing the same column names: relays, policy_methods, policy_kinds and the output aliases relay_size, methods_size, kinds_size); apply the same CTE approach to the other duplicated regions noted (lines ~46, 52–54, 60–62).src/routes/update.ts (1)
43-43: Make prerelease policy runtime-configurable.Line 43 hard-codes prerelease behavior, so Lines 158 and 179 still require a code change to toggle policy. Prefer an env-driven value.
Proposed change
-const ALLOW_PRERELEASE_UPDATES = false; +const ALLOW_PRERELEASE_UPDATES = parseBoolean(process.env['ALLOW_PRERELEASE_UPDATES']);Also applies to: 158-158, 179-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/update.ts` at line 43, The code currently hard-codes prerelease behavior via the constant ALLOW_PRERELEASE_UPDATES; make this a runtime-configurable boolean by reading an environment/config value (e.g., process.env.ALLOW_PRERELEASE_UPDATES) parsed to boolean with a safe default of false, replace the const declaration with that runtime value, and update any places that reference ALLOW_PRERELEASE_UPDATES (the prerelease-check logic in this file — around the update handling branches that currently use that constant) to use the new runtime-configured variable so toggling prerelease policy requires only an env change rather than a code change.frontend/components/Onboarding.tsx (1)
147-147: Avoidanytype; use an explicit type instead.The
anytype violates the coding guideline requiring explicit types. Based on the usage at line 158, this should be typed with the expected response shape.♻️ Suggested fix
- let data: any = {}; + let data: { error?: string } = {};As per coding guidelines: "Enable TypeScript strict mode, declare explicit types, and avoid any"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Onboarding.tsx` at line 147, Replace the loose any-typed variable `data` in the Onboarding component with a concrete type matching the expected response shape used later (e.g., define an interface like `OnboardingResponse { /* fields used at usage site */ }` or use `Partial<OnboardingResponse>` if initially empty) and change the declaration `let data: any = {};` to `let data: OnboardingResponse` or `let data: Partial<OnboardingResponse> = {}`; ensure the declared fields match the properties accessed around the later usage (the code referencing `data` in the Onboarding component).frontend/components/ui/collapsible.tsx (1)
41-41: Pairaria-expandedwitharia-controlsfor a complete disclosure relationship.Nice accessibility improvement. Consider adding an
idon the content container andaria-controlson this trigger so assistive tech can map controller → region explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/collapsible.tsx` at line 41, The trigger currently sets aria-expanded={isExpanded} but lacks aria-controls; add a stable id for the collapsible content region (e.g., using React's useId or a generated contentId) and assign that id to the content container element, then add aria-controls={contentId} alongside aria-expanded on the trigger so the trigger element (where aria-expanded is used) explicitly references the controlled content region; update any props or state in the Collapsible/CollapsibleTrigger/CollapsibleContent components (names from this file) to pass and consume the contentId.frontend/components/Configure.tsx (2)
331-363: Solid accessibility implementation for focus trapping.The focus trap correctly handles:
- Initial focus on the primary action button
- Tab cycling within the dialog
- Escape key to close
- Focus restoration on unmount
One minor observation: Line 361's focus restoration
(clearTriggerButtonRef.current ?? previousFocus)inverts the typical pattern where you'd prioritize the capturedpreviousFocus. SincepreviousFocusalready falls back to the trigger button (Line 333), you could simplify to justpreviousFocus?.focus().♻️ Optional simplification
return () => { window.cancelAnimationFrame(raf); window.removeEventListener('keydown', handleKeyDown); - (clearTriggerButtonRef.current ?? previousFocus)?.focus(); + previousFocus?.focus(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 331 - 363, The cleanup in the useEffect focus-trap currently restores focus using (clearTriggerButtonRef.current ?? previousFocus)?.focus(), which inverts the intended priority because previousFocus already falls back to clearTriggerButtonRef; update the cleanup to call previousFocus?.focus() instead so the originally captured element (previousFocus) is prioritized when unmounting; modify the return cleanup inside the useEffect that references previousFocus, clearTriggerButtonRef, and clearConfirm/clearCancel refs accordingly.
160-163: Avoidanyin error handling.The
(error as any)?.namepattern can be replaced with a type guard to satisfy strict TypeScript requirements.As per coding guidelines: "TypeScript strict mode; explicit types, avoid
any".🔧 Suggested type-safe alternative
} catch (error) { - if ((error as any)?.name === 'AbortError') { + if (error instanceof DOMException && error.name === 'AbortError') { return; // newer request superseded this one }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 160 - 163, Replace the unsafe `(error as any)?.name === 'AbortError'` check with a proper type guard: add an isAbortError(err: unknown): boolean function that returns true when err is a DOMException or Error and its name equals 'AbortError', then use `if (isAbortError(error)) return;` inside the catch in the Configure component (the catch block around the request). This removes `any` and satisfies TypeScript strict typing while preserving the abort-detection behavior.src/routes/peers.ts (2)
131-137: Remove duplicate fallback persistence branch.Both branches call
saveFallbackPeerPolicies(hasPolicies ? sanitizedPolicies : null)identically, so the conditional is unnecessary.Suggested refactor
const success = await updateUserPeerPolicies(userId, sanitizedPolicies); if (!success) { console.warn('Failed to persist peer policies for user', userId); - await saveFallbackPeerPolicies(hasPolicies ? sanitizedPolicies : null); - } else { - await saveFallbackPeerPolicies(hasPolicies ? sanitizedPolicies : null); } + await saveFallbackPeerPolicies(hasPolicies ? sanitizedPolicies : null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/peers.ts` around lines 131 - 137, The conditional around saveFallbackPeerPolicies is redundant: after calling updateUserPeerPolicies(userId, sanitizedPolicies) both the true and false branches call saveFallbackPeerPolicies(hasPolicies ? sanitizedPolicies : null). Remove the duplicate branch by calling saveFallbackPeerPolicies(hasPolicies ? sanitizedPolicies : null) once after the updateUserPeerPolicies call and keep the existing warning log when success is false (console.warn('Failed to persist peer policies for user', userId)); ensure variables updateUserPeerPolicies, saveFallbackPeerPolicies, userId, sanitizedPolicies and hasPolicies are used as before.
646-649: ReusesafeNormalizePubkeyin ping error paths.Both catch blocks reimplement normalize+try/catch. Using
safeNormalizePubkeykeeps behavior consistent and reduces duplication.Suggested refactor
- try { - const normalizedPubkey = normalizePubkey(pubkey); - context.peerStatuses.set(normalizedPubkey, updatedStatus); - } catch {} + const normalizedPubkey = safeNormalizePubkey(pubkey); + if (normalizedPubkey) { + context.peerStatuses.set(normalizedPubkey, updatedStatus); + }- try { - const normalizedPubkey = normalizePubkey(target); - context.peerStatuses.set(normalizedPubkey, updatedStatus); - } catch {} + const normalizedPubkey = safeNormalizePubkey(target); + if (normalizedPubkey) { + context.peerStatuses.set(normalizedPubkey, updatedStatus); + }Also applies to: 724-727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/peers.ts` around lines 646 - 649, Replace the try/catch around normalizePubkey in the ping error paths with the existing safeNormalizePubkey helper: call safeNormalizePubkey(pubkey) and if it returns a falsy/undefined normalized value, bail out (or skip setting status), otherwise use that value to call context.peerStatuses.set(normalizedPubkey, updatedStatus); apply the same change to both places that currently wrap normalizePubkey in try/catch (the blocks handling the ping error paths noted around where pubkey, context.peerStatuses and updatedStatus are used) so behavior is consistent and duplication removed.frontend/components/ui/tooltip.tsx (2)
41-45: Prefer a runtime fallback label whenariaLabelis missing.At Line 41 the check only logs and still renders a focusable trigger; JS callers (or unsafe casts) can still ship an unlabeled control. Consider applying a safe fallback label for resilience.
Suggested patch
- const ariaLabel = props.ariaLabel; + const ariaLabel = props.ariaLabel?.trim(); + const resolvedAriaLabel = focusable ? (ariaLabel || 'Tooltip trigger') : undefined; useEffect(() => { if (focusable && !ariaLabel) { console.error('[Tooltip] focusable tooltips require ariaLabel for accessibility.'); } }, [focusable, ariaLabel]); ... - aria-label={ariaLabel} + aria-label={resolvedAriaLabel}Also applies to: 142-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/tooltip.tsx` around lines 41 - 45, The existing useEffect in the Tooltip component only logs when focusable is true and ariaLabel is missing but still renders an unlabeled focusable trigger; update Tooltip so when focusable && !ariaLabel it computes a safe runtime fallback (e.g., derive from the tooltip content or a generic string like "Tooltip") and uses that fallback value as the aria-label passed to the focusable trigger element; adjust the code paths where the trigger is rendered (the trigger element around line ~142) to consume this computed ariaLabelFallback instead of the possibly undefined ariaLabel, and keep the console.error for developer visibility.
1-1: Rename this component file to PascalCase.
frontend/components/ui/tooltip.tsxshould beTooltip.tsxto match the repository convention for React component files.As per coding guidelines,
frontend/**/*.tsx: React component file names use PascalCase (e.g.,Configure.tsx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/tooltip.tsx` at line 1, Rename the component file from tooltip.tsx to Tooltip.tsx and ensure the component export (e.g., the React component named Tooltip) keeps the same name; then update all imports and barrel exports that reference './ui/tooltip' (or similar paths) to use './ui/Tooltip' so module resolution remains correct throughout the codebase.package.json (1)
76-82: Use exact versions inoverridesfor deterministic hardening.npm best practices recommend pinning exact versions in
overridesto enforce a known-good version and prevent drift. Currently, the caret ranges (^10.5.0,^10.2.3, etc.) allow patch/minor updates, which can bypass the security/compatibility intent. The lock file confirms this—minimatchresolved to10.2.4instead of the^10.2.3range, andfast-xml-parserto5.4.1instead of^5.3.8.Suggested change
"overrides": { - "glob": "^10.5.0", - "minimatch": "^10.2.3", - "js-yaml": "^4.1.1", - "undici": "^6.23.0", - "ajv": "^8.18.0", - "fast-xml-parser": "^5.3.8" + "glob": "10.5.0", + "minimatch": "10.2.3", + "js-yaml": "4.1.1", + "undici": "6.23.0", + "ajv": "8.18.0", + "fast-xml-parser": "5.3.8" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 76 - 82, Update the package.json "overrides" entries to use exact pinned versions (no caret ranges) so the override enforces a single known-good version; replace "glob": "^10.5.0", "minimatch": "^10.2.3", "js-yaml": "^4.1.1", "undici": "^6.23.0", "ajv": "^8.18.0", and "fast-xml-parser": "^5.3.8" with exact versions (e.g., "glob": "10.5.0", "minimatch": "10.2.4" or the audited/resolved version you intend to lock to) inside the "overrides" object in package.json so npm will pin those transitive dependencies deterministically.src/routes/sign.ts (1)
130-134: Rate-limit header calculation looks correct, butMath.ceilis redundant.
Number.parseIntalready returns an integer, soMath.ceil(retryAfterWindow)has no effect. Consider simplifying:const retryAfterWindow = Number.parseInt(process.env.RATE_LIMIT_WINDOW || '900', 10); -const retryAfterSeconds = Number.isFinite(retryAfterWindow) ? Math.ceil(retryAfterWindow) : 900; +const retryAfterSeconds = Number.isFinite(retryAfterWindow) && retryAfterWindow > 0 ? retryAfterWindow : 900;Adding
retryAfterWindow > 0also guards against negative or zero values that would be invalid forRetry-After.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/sign.ts` around lines 130 - 134, The retry header calculation uses Math.ceil unnecessarily and doesn't guard against non-positive values; update the logic around retryAfterWindow/retryAfterSeconds so retryAfterSeconds is an integer derived from Number.parseInt(process.env.RATE_LIMIT_WINDOW || '900', 10) and replaced with a positive check (e.g., if retryAfterWindow > 0 then use retryAfterWindow else fall back to 900) before converting to a string for the 'Retry-After' header in the Response.json call; adjust the code that sets retryAfterSeconds and the headers passed to Response.json accordingly.src/routes/nip04.ts (1)
90-96: Consider usingresetAtfrom rate limit result for accurate Retry-After.The
checkRateLimitfunction returnsresetAt(seesrc/routes/auth.tslines 473-496), which represents the actual reset timestamp. Using this would be more accurate than re-parsingRATE_LIMIT_WINDOW:const rate = await checkRateLimit(req, 'crypto', { clientIp: context.clientIp }); if (!rate.allowed) { - const retryAfterWindow = Number.parseInt(process.env.RATE_LIMIT_WINDOW || '900', 10) - const retryAfter = Number.isFinite(retryAfterWindow) ? Math.ceil(retryAfterWindow) : 900 + const retryAfter = rate.resetAt + ? Math.max(1, Math.ceil((rate.resetAt - Date.now()) / 1000)) + : 900; return Response.json({ error: 'Rate limit exceeded. Try again later.' }, { status: 429, headers: { ...headers, 'Retry-After': retryAfter.toString() } }) }This applies to
src/routes/sign.tsas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip04.ts` around lines 90 - 96, The rate-limit handler currently computes Retry-After from RATE_LIMIT_WINDOW; update the block that checks rate.allowed to use the checkRateLimit result's resetAt timestamp (e.g., the value returned alongside rate.allowed) to compute a precise Retry-After: calculate seconds = Math.max(0, Math.ceil((resetAt - Date.now()) / 1000)), fall back to the existing RATE_LIMIT_WINDOW parsing if resetAt is missing or invalid, and send that seconds value in the 'Retry-After' header on the Response.json call (same change should be applied to the analogous logic in sign.ts).tests/routes/admin.whoami.session.spec.ts (1)
45-47: Test teardown no longer closes the database.The
afterAllnow only stops auth cleanup but doesn't calldatabase.default.close(). While the temp directory is removed, the SQLite connection may remain open. If this causes resource warnings or flaky tests, consider adding:afterAll(async () => { try { const auth = await import('../../src/routes/auth'); auth.stopAuthCleanup(); } catch {} + try { const db = await import('../../src/db/database'); db.default.close(); } catch {} })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/admin.whoami.session.spec.ts` around lines 45 - 47, The test teardown in afterAll currently only calls auth.stopAuthCleanup() and doesn't close the SQLite connection; update the afterAll to also import and call database.default.close() (or the appropriate close method on the database export) after stopping auth cleanup, and wrap both calls in try/catch to swallow errors during teardown so tests don't fail due to cleanup race conditions; reference the existing afterAll, auth.stopAuthCleanup(), and database.default.close() when making the change.src/routes/onboarding.ts (1)
271-274: Limited special character set may restrict user password choices.The regex only accepts
@$!%*?&as special characters, rejecting common characters like#,^,-,_,(,), etc. This could frustrate users who use password managers generating broader character sets.Consider whether this restriction is intentional for compatibility reasons or if the set should be expanded:
-const PASSWORD_REGEX = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&]).*$/; +const PASSWORD_REGEX = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[!@#$%^&*()_+\-=\[\]{}|;':",./<>?]).*$/;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/onboarding.ts` around lines 271 - 274, PASSWORD_REGEX currently requires one of @$!%*?& which unnecessarily limits allowed special characters; update the regex used in onboarding.ts (PASSWORD_REGEX) to require at least one non-alphanumeric (but allow whitespace) instead of restricting to that small set—for example replace the lookahead (?=.*[@$!%*?&]) with (?=.*[^A-Za-z0-9\s]) so any punctuation/symbol counts as a special character while still preserving whitespace and keeping the existing letter/number lookaheads.src/routes/admin.ts (1)
456-467: Consider hoistingknownAdminPathsto module scope.The
Setis recreated on every request that falls through the switch. Moving it to module scope avoids repeated allocations:+const KNOWN_ADMIN_PATHS = new Set([ + '/api/admin/whoami', + '/api/admin/users', + '/api/admin/users/delete', + '/api/admin/api-keys', + '/api/admin/api-keys/revoke', + '/api/admin/status', +]); + export async function handleAdminRoute(...) { // ... - const knownAdminPaths = new Set([...]); - const isKnownPath = knownAdminPaths.has(url.pathname); + const isKnownPath = KNOWN_ADMIN_PATHS.has(url.pathname);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/admin.ts` around lines 456 - 467, Hoist the constant Set currently created per-request by moving the declaration of knownAdminPaths out of the request handler into module scope (top of the file) so it is instantiated once; keep the same contents and usage (referenced by isKnownPath and the Response block) and ensure it remains a const to preserve immutability and avoid repeated allocations on each request.src/routes/nip44.ts (1)
41-45: VerifyRetry-Afterheader calculation when env var is unset.When
RATE_LIMIT_WINDOWis undefined,parseInt(undefined)returnsNaN, andMath.ceil(NaN)producesNaN, resulting in"NaN"in the header. Consider adding a fallback:- headers: { ...headers, 'Retry-After': Math.ceil(parseInt(process.env.RATE_LIMIT_WINDOW || '900')).toString() } + headers: { ...headers, 'Retry-After': String(Math.ceil(Number(process.env.RATE_LIMIT_WINDOW) || 900)) }The
admin.tsfile in this PR implements a more robust pattern with explicit validation—consider aligning for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip44.ts` around lines 41 - 45, The Retry-After header generation can produce "NaN" when process.env.RATE_LIMIT_WINDOW is undefined; update the calculation in the rate-limit response to validate and default the window (e.g., read const windowSec = Number.parseInt(process.env.RATE_LIMIT_WINDOW || '', 10); if (!Number.isFinite(windowSec)) use a safe default like 900) then set 'Retry-After': Math.ceil(windowSec).toString(); ensure you update the block that constructs the Response.json (referencing headers and process.env.RATE_LIMIT_WINDOW) so it uses the validated/defaulted window value.tests/routes/helpers/script-runner.ts (1)
41-51: Good secret redaction in error previews.The regex patterns in
toSafePreviewcatch common secret patterns:
- Key-value pairs for
admin_secret,session_secret,password,api_key,token- Bearer tokens
This prevents accidental secret exposure in test failure logs.
Consider adding coverage for
derived_keyorencryption_keypatterns if those appear in error output, though the current patterns likely cover most scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/helpers/script-runner.ts` around lines 41 - 51, Update toSafePreview to also redact keys named derived_key and encryption_key (including variants like derived-key, encryption-key, with or without quotes) by adding them to the first secret-key alternation in the regex used to replace key-value secrets; keep the existing flags and replacement string '$1<redacted>' and preserve trimming/truncation behavior so other logic (compact, ERROR_PREVIEW_MAX_CHARS) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Line 32: CI pins Bun via the bun-version key (e.g., in release.yml and ci.yml)
but the main Dockerfile and Umbrel Dockerfile use different Bun images; update
either the CI workflows' bun-version to match the main Dockerfile's Bun (1.3.10)
and the Umbrel Dockerfile to the same version, or conversely update the
Dockerfiles to the CI pinned version—ensure bun-version in release.yml and
ci.yml and the FROM lines in the Dockerfile and Umbrel Dockerfile are
consistent, or add a clear comment documenting an intentional, justified
divergence.
In `@frontend/components/NIP46.tsx`:
- Around line 244-245: Before starting the async operations that call
fetch(`/api/nip46/sessions/${pubkey}`) (the try block at the start of the
session/revoke/update flows), clear any stale error state by resetting
connectError via setConnectError(null) (or the equivalent state setter)
immediately before the try. Do this in both locations referenced (the try around
the fetch using pubkey at line ~244 and the other operation at ~262-263) so
previous errors are not shown during a new attempt.
- Around line 245-246: The code interpolates raw pubkey into API paths (e.g.,
the fetch call using "/api/nip46/sessions/${pubkey}" and the second similar
call) which can produce malformed URLs; fix by encoding the pubkey before
inserting into the path (use encodeURIComponent(pubkey)) wherever
"/api/nip46/sessions/${pubkey}" is built (search for fetch calls and functions
in NIP46.tsx that reference that string) so both the DELETE and the other
request use the encoded value.
In `@frontend/components/Signer.tsx`:
- Around line 947-951: The code currently places raw backend/text error details
(the variable detail and message derived from response.text() or thrown errors)
directly into the UI via setCredentialSaveError; instead, keep the detailed text
only in internal logs (console.error or a logger) and set a generic, user-safe
message (e.g., "Unable to save relays. Please try again.") into
setCredentialSaveError; update all occurrences that follow the same pattern (the
blocks using response.text().catch(() => '') and setCredentialSaveError in this
file/Signer.tsx) to log the full detail internally and pass the sanitized
message to the UI.
In `@frontend/components/ui/event-log.tsx`:
- Line 40: The state declaration for isExpanded should include an explicit
boolean type to satisfy TS strict-mode rules; update the useState call that
initializes isExpanded (currently using useState(hideHeader)) to use
useState<boolean>(hideHeader) so isExpanded and setIsExpanded are typed as
boolean and (boolean)=>void respectively, matching the project guideline and the
adjacent explicitly typed state on the next line.
In `@src/const.ts`:
- Around line 26-27: The HOST_PORT parsing is too lenient: stop using parseInt
and instead validate process.env['HOST_PORT'] as a strictly numeric integer
string, convert with Number or parseInt only after verifying /^\d+$/ and then
ensure the resulting port is within valid bounds (1–65535); update the logic
that produces parsedHostPort and export HOST_PORT so malformed strings (e.g.
"8002abc"), empty values, zero, or out-of-range numbers fall back to the default
8002 and only assign a validated integer to HOST_PORT.
In `@src/db/database.ts`:
- Around line 12-21: The current isEnvPathFile heuristic misclassifies
dot-containing directory names as files; update the logic in the
isEnvPathFile/DB_DIR/DB_FILE block so a value is considered a file only when it
has an explicit extension or is explicitly a .db file (e.g. replace the
basename-includes-dot check with a stricter test such as path.extname(envPath)
!== '' || envPath.endsWith('.db')), keeping DB_DIR and DB_FILE assignment
semantics using that updated isEnvPathFile flag and still falling back to
defaultDbDir when envPath is falsy.
In `@src/db/migrator.ts`:
- Line 31: The lexical check using dir.startsWith(projectRoot + path.sep) in
src/db/migrator.ts is vulnerable to symlink traversal; canonicalize both paths
with fs.realpathSync (or fs.promises.realpath) before comparing to ensure
symlinks are resolved, but preserve current behavior for missing dirs by first
checking existence (fs.existsSync or try stat) and only calling realpathSync
inside a try/catch; update the comparison logic that references dir,
projectRoot, path.sep and the startsWith check to use the resolvedRealDir and
resolvedProjectRoot values so non-existent directories still return [] and
realpath errors are handled gracefully.
In `@src/node/manager.ts`:
- Around line 2188-2193: The patch replaces loose any typings that break strict
mode: change the occurrences of "any" used for poolProto, originalSubscribeMany,
and the normalizedSubscribeMany function parameters to "unknown" (or tighter
specific types) and then narrow/cast where needed; update SimplePool?.prototype
typing to a suitable unknown | Record<string, unknown> and type the
originalSubscribeMany variable as unknown before checking typeof, and give
normalizedSubscribeMany parameters (relays, filters, params, this) unknown types
and narrow them inside the function so the subscribeMany override compiles under
strict TS without noImplicitAny errors.
In `@src/routes/index.ts`:
- Around line 231-234: The 401 response is returning the full auth status object
via getAuthStatus(), but the API expects authMethods as a string[]; update the
Response.json call so authMethods returns only the array of method names (e.g.,
use getAuthStatus().methods or map the result to a string[]), and ensure the
value matches the documented Unauthorized response shape and typing for
authMethods in the handler that builds the 401 (the Response.json call inside
the route that uses authResult and getAuthStatus).
In `@src/routes/peers.ts`:
- Around line 125-128: The call to updateUserPeerPolicies(userId, null) is
currently ignored so failures silently leave stale DB state; modify the peers
route to explicitly handle failures from updateUserPeerPolicies and
saveFallbackPeerPolicies by awaiting their results and checking for error/false
return values or catching thrown exceptions, and if either fails return an error
response (or throw) instead of continuing—update the branch around the condition
that calls updateUserPeerPolicies and saveFallbackPeerPolicies to propagate or
surface errors from those functions (refer to updateUserPeerPolicies and
saveFallbackPeerPolicies in your changes).
- Around line 370-372: The catch block that builds the HTTP response currently
includes raw error text in the warnings array (see the catch (error) handler and
the Response.json call), which can leak internal parsing details; change it to
avoid returning the exception message to clients — instead return a generic
error/warning string (e.g. "Invalid credentials format") and move the detailed
error.message into server-side logging (use processLogger or console.error
inside the same catch). Ensure the Response.json payload uses only the generic
text and keep status 400 and existing headers unchanged.
In `@src/utils/rate-limiter.ts`:
- Around line 252-258: The identifier extraction using
key.slice(key.lastIndexOf(':') + 1) fails for identifiers containing ":" (e.g.,
IPv6); update the logic that iterates this.fallbackStore.keys() (the keys loop
with const keyIdentifier = ...) to split on the first colon instead of the last
one—e.g., find the first ':' via indexOf or use key.split(':', 2) and take the
second part—so keyIdentifier equals the original identifier even when it
contains colons, then continue deleting matching keys as before.
---
Nitpick comments:
In `@frontend/components/Configure.tsx`:
- Around line 331-363: The cleanup in the useEffect focus-trap currently
restores focus using (clearTriggerButtonRef.current ?? previousFocus)?.focus(),
which inverts the intended priority because previousFocus already falls back to
clearTriggerButtonRef; update the cleanup to call previousFocus?.focus() instead
so the originally captured element (previousFocus) is prioritized when
unmounting; modify the return cleanup inside the useEffect that references
previousFocus, clearTriggerButtonRef, and clearConfirm/clearCancel refs
accordingly.
- Around line 160-163: Replace the unsafe `(error as any)?.name ===
'AbortError'` check with a proper type guard: add an isAbortError(err: unknown):
boolean function that returns true when err is a DOMException or Error and its
name equals 'AbortError', then use `if (isAbortError(error)) return;` inside the
catch in the Configure component (the catch block around the request). This
removes `any` and satisfies TypeScript strict typing while preserving the
abort-detection behavior.
In `@frontend/components/Onboarding.tsx`:
- Line 147: Replace the loose any-typed variable `data` in the Onboarding
component with a concrete type matching the expected response shape used later
(e.g., define an interface like `OnboardingResponse { /* fields used at usage
site */ }` or use `Partial<OnboardingResponse>` if initially empty) and change
the declaration `let data: any = {};` to `let data: OnboardingResponse` or `let
data: Partial<OnboardingResponse> = {}`; ensure the declared fields match the
properties accessed around the later usage (the code referencing `data` in the
Onboarding component).
In `@frontend/components/ui/collapsible.tsx`:
- Line 41: The trigger currently sets aria-expanded={isExpanded} but lacks
aria-controls; add a stable id for the collapsible content region (e.g., using
React's useId or a generated contentId) and assign that id to the content
container element, then add aria-controls={contentId} alongside aria-expanded on
the trigger so the trigger element (where aria-expanded is used) explicitly
references the controlled content region; update any props or state in the
Collapsible/CollapsibleTrigger/CollapsibleContent components (names from this
file) to pass and consume the contentId.
In `@frontend/components/ui/tooltip.tsx`:
- Around line 41-45: The existing useEffect in the Tooltip component only logs
when focusable is true and ariaLabel is missing but still renders an unlabeled
focusable trigger; update Tooltip so when focusable && !ariaLabel it computes a
safe runtime fallback (e.g., derive from the tooltip content or a generic string
like "Tooltip") and uses that fallback value as the aria-label passed to the
focusable trigger element; adjust the code paths where the trigger is rendered
(the trigger element around line ~142) to consume this computed
ariaLabelFallback instead of the possibly undefined ariaLabel, and keep the
console.error for developer visibility.
- Line 1: Rename the component file from tooltip.tsx to Tooltip.tsx and ensure
the component export (e.g., the React component named Tooltip) keeps the same
name; then update all imports and barrel exports that reference './ui/tooltip'
(or similar paths) to use './ui/Tooltip' so module resolution remains correct
throughout the codebase.
In `@package.json`:
- Around line 76-82: Update the package.json "overrides" entries to use exact
pinned versions (no caret ranges) so the override enforces a single known-good
version; replace "glob": "^10.5.0", "minimatch": "^10.2.3", "js-yaml": "^4.1.1",
"undici": "^6.23.0", "ajv": "^8.18.0", and "fast-xml-parser": "^5.3.8" with
exact versions (e.g., "glob": "10.5.0", "minimatch": "10.2.4" or the
audited/resolved version you intend to lock to) inside the "overrides" object in
package.json so npm will pin those transitive dependencies deterministically.
In `@src/db/migrations/20250916_0004_audit_nip46_data_sizes.sql`:
- Around line 41-43: Repeated LENGTH(CAST(... AS BLOB)) expressions for relays,
policy_methods, and policy_kinds should be computed once in a CTE and then
referenced in the main SELECT to avoid duplication and drift; add a WITH block
(e.g., audit_blob_sizes) that selects id plus computed relay_size, methods_size,
kinds_size using LENGTH(CAST(column AS BLOB)) and then join or select from that
CTE in place of repeating the expressions in the existing SELECT (referencing
the same column names: relays, policy_methods, policy_kinds and the output
aliases relay_size, methods_size, kinds_size); apply the same CTE approach to
the other duplicated regions noted (lines ~46, 52–54, 60–62).
In `@src/routes/admin.ts`:
- Around line 456-467: Hoist the constant Set currently created per-request by
moving the declaration of knownAdminPaths out of the request handler into module
scope (top of the file) so it is instantiated once; keep the same contents and
usage (referenced by isKnownPath and the Response block) and ensure it remains a
const to preserve immutability and avoid repeated allocations on each request.
In `@src/routes/nip04.ts`:
- Around line 90-96: The rate-limit handler currently computes Retry-After from
RATE_LIMIT_WINDOW; update the block that checks rate.allowed to use the
checkRateLimit result's resetAt timestamp (e.g., the value returned alongside
rate.allowed) to compute a precise Retry-After: calculate seconds = Math.max(0,
Math.ceil((resetAt - Date.now()) / 1000)), fall back to the existing
RATE_LIMIT_WINDOW parsing if resetAt is missing or invalid, and send that
seconds value in the 'Retry-After' header on the Response.json call (same change
should be applied to the analogous logic in sign.ts).
In `@src/routes/nip44.ts`:
- Around line 41-45: The Retry-After header generation can produce "NaN" when
process.env.RATE_LIMIT_WINDOW is undefined; update the calculation in the
rate-limit response to validate and default the window (e.g., read const
windowSec = Number.parseInt(process.env.RATE_LIMIT_WINDOW || '', 10); if
(!Number.isFinite(windowSec)) use a safe default like 900) then set
'Retry-After': Math.ceil(windowSec).toString(); ensure you update the block that
constructs the Response.json (referencing headers and
process.env.RATE_LIMIT_WINDOW) so it uses the validated/defaulted window value.
In `@src/routes/onboarding.ts`:
- Around line 271-274: PASSWORD_REGEX currently requires one of @$!%*?& which
unnecessarily limits allowed special characters; update the regex used in
onboarding.ts (PASSWORD_REGEX) to require at least one non-alphanumeric (but
allow whitespace) instead of restricting to that small set—for example replace
the lookahead (?=.*[@$!%*?&]) with (?=.*[^A-Za-z0-9\s]) so any
punctuation/symbol counts as a special character while still preserving
whitespace and keeping the existing letter/number lookaheads.
In `@src/routes/peers.ts`:
- Around line 131-137: The conditional around saveFallbackPeerPolicies is
redundant: after calling updateUserPeerPolicies(userId, sanitizedPolicies) both
the true and false branches call saveFallbackPeerPolicies(hasPolicies ?
sanitizedPolicies : null). Remove the duplicate branch by calling
saveFallbackPeerPolicies(hasPolicies ? sanitizedPolicies : null) once after the
updateUserPeerPolicies call and keep the existing warning log when success is
false (console.warn('Failed to persist peer policies for user', userId)); ensure
variables updateUserPeerPolicies, saveFallbackPeerPolicies, userId,
sanitizedPolicies and hasPolicies are used as before.
- Around line 646-649: Replace the try/catch around normalizePubkey in the ping
error paths with the existing safeNormalizePubkey helper: call
safeNormalizePubkey(pubkey) and if it returns a falsy/undefined normalized
value, bail out (or skip setting status), otherwise use that value to call
context.peerStatuses.set(normalizedPubkey, updatedStatus); apply the same change
to both places that currently wrap normalizePubkey in try/catch (the blocks
handling the ping error paths noted around where pubkey, context.peerStatuses
and updatedStatus are used) so behavior is consistent and duplication removed.
In `@src/routes/sign.ts`:
- Around line 130-134: The retry header calculation uses Math.ceil unnecessarily
and doesn't guard against non-positive values; update the logic around
retryAfterWindow/retryAfterSeconds so retryAfterSeconds is an integer derived
from Number.parseInt(process.env.RATE_LIMIT_WINDOW || '900', 10) and replaced
with a positive check (e.g., if retryAfterWindow > 0 then use retryAfterWindow
else fall back to 900) before converting to a string for the 'Retry-After'
header in the Response.json call; adjust the code that sets retryAfterSeconds
and the headers passed to Response.json accordingly.
In `@src/routes/update.ts`:
- Line 43: The code currently hard-codes prerelease behavior via the constant
ALLOW_PRERELEASE_UPDATES; make this a runtime-configurable boolean by reading an
environment/config value (e.g., process.env.ALLOW_PRERELEASE_UPDATES) parsed to
boolean with a safe default of false, replace the const declaration with that
runtime value, and update any places that reference ALLOW_PRERELEASE_UPDATES
(the prerelease-check logic in this file — around the update handling branches
that currently use that constant) to use the new runtime-configured variable so
toggling prerelease policy requires only an env change rather than a code
change.
In `@tests/routes/admin.whoami.session.spec.ts`:
- Around line 45-47: The test teardown in afterAll currently only calls
auth.stopAuthCleanup() and doesn't close the SQLite connection; update the
afterAll to also import and call database.default.close() (or the appropriate
close method on the database export) after stopping auth cleanup, and wrap both
calls in try/catch to swallow errors during teardown so tests don't fail due to
cleanup race conditions; reference the existing afterAll,
auth.stopAuthCleanup(), and database.default.close() when making the change.
In `@tests/routes/helpers/script-runner.ts`:
- Around line 41-51: Update toSafePreview to also redact keys named derived_key
and encryption_key (including variants like derived-key, encryption-key, with or
without quotes) by adding them to the first secret-key alternation in the regex
used to replace key-value secrets; keep the existing flags and replacement
string '$1<redacted>' and preserve trimming/truncation behavior so other logic
(compact, ERROR_PREVIEW_MAX_CHARS) remains unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (71)
.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/umbrel-dev.yml.gitignoreDockerfiledocs/DEPLOY.mddocs/openapi/README.mddocs/openapi/openapi.jsondocs/openapi/openapi.yamlenv.examplefrontend/App.tsxfrontend/components/ApiKeys.tsxfrontend/components/Configure.tsxfrontend/components/Login.tsxfrontend/components/NIP46.tsxfrontend/components/Onboarding.tsxfrontend/components/Recover.tsxfrontend/components/Signer.tsxfrontend/components/nip46/Permissions.tsxfrontend/components/nip46/RelaySettings.tsxfrontend/components/nip46/Requests.tsxfrontend/components/ui/card.tsxfrontend/components/ui/collapsible.tsxfrontend/components/ui/event-log.tsxfrontend/components/ui/icon-button.tsxfrontend/components/ui/input-with-validation.tsxfrontend/components/ui/peer-list.tsxfrontend/components/ui/tooltip.tsxfrontend/types/index.tsllm/implementation/node-lifecycle-implementation.mdllm/implementation/umbrel-implementation.mdpackage.jsonscripts/fetch-swagger-ui.mjsscripts/patch-zod-compat.mjsscripts/release.shsrc/class/relay.test.tssrc/class/relay.tssrc/config/crypto.tssrc/const.tssrc/db/database.tssrc/db/migrations/20250916_0004_audit_nip46_data_sizes.sqlsrc/db/migrator.tssrc/db/nip46.tssrc/db/ui-event-log.tssrc/nip46/service.tssrc/node/manager.tssrc/routes/admin.tssrc/routes/auth.tssrc/routes/env.tssrc/routes/index.tssrc/routes/nip04.tssrc/routes/nip44.tssrc/routes/nip46.tssrc/routes/onboarding.tssrc/routes/peers.tssrc/routes/sign.tssrc/routes/status.tssrc/routes/update.tssrc/routes/user.tssrc/routes/utils.test.tssrc/routes/utils.tssrc/server.tssrc/utils/rate-limiter.tstests/routes/admin.whoami.session.spec.tstests/routes/auth.rehydrate.spec.tstests/routes/body-limit.spec.tstests/routes/env.db-mode.spec.tstests/routes/helpers/script-runner.spec.tstests/routes/helpers/script-runner.tstests/routes/onboarding.spec.tstests/routes/status-env.test.ts
Member
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi/openapi.json (1)
3565-3594:⚠️ Potential issue | 🟠 MajorPotential breaking API change:
client_pubkeyrenamed topubkeywithout compatibility path.At Line 3565 and Line 3593, requiring only
pubkeycan break existing NIP-46 clients still sendingclient_pubkey. If this isn’t a versioned breaking change, keep a deprecated alias during transition.Suggested transitional schema patch
"Nip46SessionInput": { "type": "object", "description": "Client-supplied fields when creating or updating a NIP‑46 session", "properties": { "pubkey": { "type": "string", "description": "Client public key (hex encoded)" + }, + "client_pubkey": { + "type": "string", + "deprecated": true, + "description": "Deprecated alias for pubkey; retained for backward compatibility" }, "status": { "type": "string", "enum": [ "pending", "active", "revoked" ], "description": "Desired session status (defaults to pending)" }, "profile": { "$ref": "#/components/schemas/Nip46Profile" }, "relays": { "type": "array", "items": { "type": "string" }, "description": "Preferred relays for the session" }, "policy": { "$ref": "#/components/schemas/Nip46Policy" } }, - "required": [ - "pubkey" - ] + "anyOf": [ + { "required": ["pubkey"] }, + { "required": ["client_pubkey"] } + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 3565 - 3594, Add a deprecated alias property so existing clients sending client_pubkey continue working: in the same object that defines "pubkey" add a "client_pubkey" property with the same schema (type: string, description: "DEPRECATED: client_pubkey (hex encoded) — use pubkey") and then replace the single "required": ["pubkey"] constraint with a oneOf/anyOf that accepts either required ["pubkey"] or required ["client_pubkey"] (or otherwise accept either field), preserving "pubkey" as the preferred name while allowing the deprecated "client_pubkey" during transition.
♻️ Duplicate comments (1)
src/db/database.ts (1)
12-15:⚠️ Potential issue | 🟡 Minor
DB_PATHfile detection still misclassifies dot-named directories.Line 14 still treats values like
/data/my.folderas files becausepath.extname(...) !== '', so directory targets with dots can resolve incorrectly.🔧 Safer extension-based file detection
+const DB_FILE_EXTENSIONS = new Set(['.db', '.sqlite', '.sqlite3']) +const envExt = envPath ? path.extname(envPath).toLowerCase() : '' const isEnvPathFile = !!envPath && ( - envPath.endsWith('.db') || - (path.extname(envPath) !== '' && !envPath.endsWith(path.sep)) + !envPath.endsWith(path.sep) && + DB_FILE_EXTENSIONS.has(envExt) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/database.ts` around lines 12 - 15, The current isEnvPathFile logic misclassifies dot-containing directory names (e.g., /data/my.folder) as files because path.extname(envPath) !== '' is true; update the condition used in isEnvPathFile to only treat path.extname(envPath) as indicating a file when the final path segment (path.basename(envPath)) does not start with a dot (to exclude dot-named dirs) and/or when the path actually exists and fs.statSync(envPath).isFile() confirms it's a file; modify the isEnvPathFile computation to include a check on path.basename(envPath) (and optionally use fs.existsSync + fs.statSync for existing paths) so envPath like '/data/my.folder' is not misclassified while still recognizing real files and .db endings.
🧹 Nitpick comments (14)
frontend/components/ui/input-with-validation.tsx (1)
1-55: Rename component file to PascalCase to match repo convention.Please rename
frontend/components/ui/input-with-validation.tsxtofrontend/components/ui/InputWithValidation.tsxand update imports accordingly.As per coding guidelines,
frontend/**/*.tsx: React component file names use PascalCase (e.g.,Configure.tsx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/input-with-validation.tsx` around lines 1 - 55, Rename the file to PascalCase and update all imports: move frontend/components/ui/input-with-validation.tsx → frontend/components/ui/InputWithValidation.tsx, keep the component name InputWithValidation and its named export unchanged, then search the repo for any imports using "input-with-validation" and update them to import { InputWithValidation } from "./InputWithValidation" (or the correct relative path); also update any barrel/index files or tests that re-export or reference the old filename to use the new filename.src/db/migrations/20250916_0004_audit_nip46_data_sizes.sql (1)
41-43: Reduce repeated size expressions with a CTE.The same
COALESCE(LENGTH(CAST(... AS BLOB)), 0)expressions are repeated many times. Consider computing them once, then reusing aliases fortotal_sizeand risk checks.♻️ Suggested refactor
-SELECT - id, - user_id, - client_pubkey, - CASE WHEN relays IS NOT NULL THEN LENGTH(CAST(relays AS BLOB)) ELSE 0 END as relay_size, - CASE WHEN policy_methods IS NOT NULL THEN LENGTH(CAST(policy_methods AS BLOB)) ELSE 0 END as methods_size, - CASE WHEN policy_kinds IS NOT NULL THEN LENGTH(CAST(policy_kinds AS BLOB)) ELSE 0 END as kinds_size, - CASE - WHEN relays IS NOT NULL OR policy_methods IS NOT NULL OR policy_kinds IS NOT NULL - THEN COALESCE(LENGTH(CAST(relays AS BLOB)), 0) + COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) + COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) - ELSE 0 - END as total_size, +WITH sized AS ( + SELECT + id, + user_id, + client_pubkey, + COALESCE(LENGTH(CAST(relays AS BLOB)), 0) AS relay_size, + COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) AS methods_size, + COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) AS kinds_size + FROM nip46_sessions +) +SELECT + id, + user_id, + client_pubkey, + relay_size, + methods_size, + kinds_size, + relay_size + methods_size + kinds_size AS total_size, CASE - WHEN COALESCE(LENGTH(CAST(relays AS BLOB)), 0) > 10000 - OR COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) > 10000 - OR COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) > 10000 + WHEN relay_size > 10000 OR methods_size > 10000 OR kinds_size > 10000 THEN 1 ELSE 0 END as has_risk_old_limit, CASE - WHEN COALESCE(LENGTH(CAST(relays AS BLOB)), 0) > 50000 - OR COALESCE(LENGTH(CAST(policy_methods AS BLOB)), 0) > 50000 - OR COALESCE(LENGTH(CAST(policy_kinds AS BLOB)), 0) > 50000 + WHEN relay_size > 50000 OR methods_size > 50000 OR kinds_size > 50000 THEN 1 ELSE 0 END as has_risk_new_limit -FROM nip46_sessions; +FROM sized;Also applies to: 46-46, 52-54, 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/migrations/20250916_0004_audit_nip46_data_sizes.sql` around lines 41 - 43, Compute the repeated BLOB length expressions once in a CTE and reuse aliases instead of repeating COALESCE(LENGTH(CAST(... AS BLOB)), 0): add a CTE (e.g., sized_rows) that SELECTs the primary key (id) and computes relay_size, methods_size, kinds_size using COALESCE(LENGTH(CAST(relays AS BLOB)),0) etc., then in the main query reference those aliases for per-column sizes, SUM them into a single total_size alias, and use total_size and the sized alias names in the risk checks and CASE expressions (replace occurrences of the repeated LENGTH(CAST(... AS BLOB)) for relays, policy_methods, policy_kinds with the CTE aliases).frontend/components/ui/icon-button.tsx (1)
55-55: Minor: Trailing whitespace on export line.There's a trailing space at the end of this line. This is a very minor nit, but removing it maintains consistent formatting.
🧹 Suggested fix
-export { IconButton, iconButtonVariants }; +export { IconButton, iconButtonVariants };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/icon-button.tsx` at line 55, Remove the trailing whitespace at the end of the export statement for IconButton and iconButtonVariants (the line exporting IconButton, iconButtonVariants) so the export reads without any trailing space; update the export line for the IconButton/iconButtonVariants symbols to remove the extra space and save formatting.frontend/components/Configure.tsx (3)
1332-1332: Minor performance consideration.Using
JSON.stringifyfor object comparison on every render works for this small settings object but could be optimized withuseMemoand a shallow/deep equality check if the settings object grows significantly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` at line 1332, The code currently compares advancedSettings and originalAdvancedSettings using JSON.stringify on every render; replace this with a memoized equality check to avoid expensive stringify calls by computing a boolean like areAdvancedSettingsDirty using React's useMemo and a suitable shallow/deep equality comparer (e.g., lodash/isEqual or a custom shallowEqual) and then use that memoized boolean in the JSX conditional; update references to advancedSettings and originalAdvancedSettings inside useMemo so the memo recomputes only when those objects change.
301-310: Reminder: Address the TODO for fetching existing keyset names.The mock empty array means the duplicate name validation at line 375 (
existingNames.indexOf(nameWithoutShare)) will never prevent duplicate names.Would you like me to help implement the server API call for fetching existing keyset names, or open an issue to track this task?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 301 - 310, The code currently uses a mock empty shares array which prevents duplicate-name validation from working; update the Configure component to call the server API instead: replace the mock shares variable with an awaited fetch to '/api/env/shares' (or the real endpoint), parse JSON, extract names the same way (share.name.split(' share ')[0]), and call setExistingNames(names); ensure you handle fetch errors and non-OK responses (log or surface via state) and keep existingNames and the duplicate check (existingNames.indexOf(nameWithoutShare)) intact so validation works correctly.
337-369: Good accessibility implementation with a minor consideration.The focus trap correctly handles Escape key, Tab navigation, and focus restoration. Initial focus on the confirm button (destructive action) is intentional but some accessibility guidelines suggest focusing the cancel button first for destructive actions to reduce accidental confirmations.
Consider: The codebase already has a
ConfirmModalcomponent (used at line 1360). Consolidating both confirmation modals to use the same accessibility-enhanced pattern would improve consistency and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 337 - 369, The focus-trap useEffect for showClearConfirm currently sets initial focus to clearConfirmButtonRef (destructive action) and duplicates modal logic; change the initial focus to clearCancelButtonRef to reduce accidental confirmations and ensure focus restoration via clearTriggerButtonRef remains, and refactor Configure's clear-confirm flow to reuse the existing ConfirmModal component (used elsewhere as ConfirmModal) instead of duplicating the keyboard/focus logic so both modals share the same accessibility pattern and handlers.frontend/components/Signer.tsx (1)
996-1002: Return values from save helpers are discarded.Both
saveRelaysToUserCredentialsandsaveRelaysToServerEnvnow returnbooleansuccess indicators, butsaveRelaysToEnvignores these values. The callers (handleAddRelay,handleRemoveRelay) optimistically update local state regardless of save outcome.This is acceptable because errors are surfaced via
credentialSaveErrorstate, but you may want to consider reverting the local relay list on failure for a more robust UX.♻️ Optional: propagate success/failure and revert on error
const saveRelaysToEnv = async (relays: string[]) => { + let success: boolean; if (isDatabaseMode()) { - await saveRelaysToUserCredentials(relays); + success = await saveRelaysToUserCredentials(relays); } else { - await saveRelaysToServerEnv(relays); + success = await saveRelaysToServerEnv(relays); } + return success; };Then in handlers, revert on failure:
const handleAddRelay = async () => { // ... const prevRelays = relayUrls; setRelayUrls(newRelays); setNewRelayUrl(""); const ok = await saveRelaysToEnv(newRelays); if (!ok) setRelayUrls(prevRelays); // revert };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Signer.tsx` around lines 996 - 1002, saveRelaysToEnv currently ignores the boolean results from saveRelaysToUserCredentials and saveRelaysToServerEnv, so update saveRelaysToEnv to return a boolean indicating overall success (true only if the called helper returns true), then update callers like handleAddRelay and handleRemoveRelay to await saveRelaysToEnv and, on false, revert the optimistic local state change (e.g., restore previous relayUrls) and rely on credentialSaveError for the error UI; reference the functions saveRelaysToEnv, saveRelaysToUserCredentials, saveRelaysToServerEnv, handleAddRelay, and handleRemoveRelay when making these changes.src/class/relay.test.ts (1)
128-175: Good coverage of limit behavior, consider asserting returned event id.The test correctly validates that
limitapplies only to matched events. For additional robustness, consider asserting the specific event id returned to verify the sorting order is respected (should bematchedBwith highestcreated_at).💡 Optional enhancement for stronger assertion
const eventMessages = messages.filter((msg) => msg[0] === 'EVENT'); expect(eventMessages).toHaveLength(1); expect(eventMessages[0]?.[1]).toBe('sub-limit'); expect((eventMessages[0]?.[2] as { kind?: number }).kind).toBe(1); + // Verify sorting returns newest matching event (matchedB has created_at=3) + expect((eventMessages[0]?.[2] as { id?: string }).id).toBe('e'.repeat(64)); expect(messages).toContainEqual(['EOSE', 'sub-limit']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/class/relay.test.ts` around lines 128 - 175, Add an assertion that the single returned event is the expected newest matched event by checking the event id equals matchedB.id: after decoding messages and filtering eventMessages, assert eventMessages[0]?.[2].id === matchedB.id (or equivalent) to ensure sorting by created_at returns matchedB when limit: 1 is used; reference variables matchedB and eventMessages in the test to locate where to add this check.frontend/components/nip46/RelaySettings.tsx (1)
78-84: Floating promise in onClick handler.The async arrow function returns a Promise, but React event handlers expect
void. While this works in practice, it creates an unhandled promise. For consistency with line 33 (void handleAdd()), consider wrapping invoid:Suggested fix
<IconButton variant="ghost" size="sm" icon={<X className="h-3 w-3" />} tooltip="Remove relay" - onClick={async () => { - try { - await onRemove(relay) - } catch { - // error surface handled via parent `error` - } - }} + onClick={() => { + void (async () => { + try { + await onRemove(relay) + } catch { + // error surface handled via parent `error` + } + })() + }} disabled={saving} />Alternatively, extract to a named handler like
handleRemovefor cleaner code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/nip46/RelaySettings.tsx` around lines 78 - 84, The onClick async arrow returns a Promise causing a floating promise; change it to match the pattern used for add (e.g. create a named handler handleRemove or call onRemove via void to ensure the event handler returns void). Specifically, replace the inline async onClick={() => { try { await onRemove(relay) } catch { ... } }} with either a top-level async function handleRemove(relay) that performs try/await/onRemove and is referenced as onClick={() => void handleRemove(relay)} or call onClick={() => { try { void onRemove(relay) } catch { /* keep parent error handling */ } }}, making sure to reference the existing onRemove and relay identifiers.env.example (1)
255-255: Consider using obviously placeholder values.The static analysis tool flagged these as potential secrets (false positive for an example file). However, realistic-looking hex strings could be accidentally copied to production. Consider using obviously fake placeholders like:
# SESSION_SECRET=<generate-64-hex-with-openssl-rand-hex-32>This makes it clearer that users must generate their own values rather than copy these examples.
Also applies to: 264-264, 274-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@env.example` at line 255, Replace realistic-looking hex example values for SESSION_SECRET (and other similar example secret values in this file) with clearly fake placeholders so users must generate their own keys; e.g., change the commented value to something like "# SESSION_SECRET=<generate-64-hex-with-openssl-rand-hex-32>" (and update the other flagged example secret entries in the same file similarly) so the examples are obviously placeholders and not copy-pasteable secrets.src/nip46/service.ts (1)
404-409: Add defensive close on failedconnectto avoid leaked transport state.The new catch block resets references, but Line 404 can still leave partially opened resources if
connectfails mid-handshake.♻️ Suggested cleanup hardening
} catch (error) { + try { + await this.agent?.close?.() + } catch (closeError) { + this.log('warn', 'Failed to close NIP-46 agent after connect failure', { + error: this.serializeError(closeError) + }) + } this.removeAgentListeners() this.agent = null this.signer = null this.started = false throw error }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nip46/service.ts` around lines 404 - 409, The catch block in the connect flow resets references but doesn't close the underlying transport, risking leaked resources; update the catch to defensively close the transport and any partially-initialized agent before nulling fields — e.g. call and await a close method on the transport (this.transport?.close() or this.closeTransport()) and, if available, close the agent (this.agent?.close()) inside a try/catch, then call removeAgentListeners(), set this.agent = null, this.signer = null, this.transport = null, and this.started = false before rethrowing the error so partially-opened resources are always cleaned up.src/db/nip46.ts (1)
57-59: Consider usingnormalizeClientPubkeyingetSessionfor consistency.The new helper is used consistently in
updatePolicy,updateStatus,deleteSession, andlogSessionEvent, butgetSession(line 256) duplicates the normalization logic inline. Using the helper would improve maintainability.♻️ Suggested refactor
export function getSession(userId: number | bigint, client_pubkey: string): Nip46Session | null { - const key = (client_pubkey || '').trim().toLowerCase() + const key = normalizeClientPubkey(client_pubkey) if (!key || !/^[0-9a-f]{64}$/.test(key)) return null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 57 - 59, The getSession function duplicates client pubkey normalization inline instead of reusing normalizeClientPubkey; update getSession to call normalizeClientPubkey(clientPubkey) where it currently trims/lowers the value so behavior matches updatePolicy, updateStatus, deleteSession, and logSessionEvent and to centralize normalization logic in normalizeClientPubkey for maintainability.frontend/components/Onboarding.tsx (1)
108-117: Cleanup in useEffect doesn't match the timeout source.The cleanup clears
timeoutRef.current, but this timeout is set increateUser(line 256) for the auto-redirect, not incheckStatus. This cleanup will effectively be a no-op when the component unmounts during the status check phase. While not a bug (the timeout increateUseris still cleaned up on unmount), the comment is misleading.Consider either removing the misleading comment or moving the cleanup to a separate effect that depends on the completion step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Onboarding.tsx` around lines 108 - 117, The cleanup in the useEffect that invokes checkStatus is misleading because it clears timeoutRef.current (which is actually set in createUser for auto-redirect), so update the Onboarding component to either remove or correct the comment and move the timeout cleanup into a separate effect that watches the completion/step state (or the variable that createUser sets) so clearTimeout(timeoutRef.current) runs only for the timer created by createUser; reference the existing useEffect(checkStatus), createUser, timeoutRef.current, and the completion/step state to locate and relocate or remove the cleanup logic and adjust the comment accordingly.tests/routes/helpers/script-runner.ts (1)
41-51: Consider redactingBasicauth tokens in preview output.Line [49] redacts
Bearertokens, butBasiccredentials can still appear in previews.🔒 Proposed hardening
- .replace(/(bearer\s+)[a-z0-9._-]+/ig, '$1<redacted>'); + .replace(/((?:bearer|basic)\s+)[a-z0-9._~+/=-]+/ig, '$1<redacted>');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/helpers/script-runner.ts` around lines 41 - 51, toSafePreview currently redacts bearer tokens but not HTTP Basic credentials, so add a second replacement in the toSafePreview function to detect and redact Basic auth values (e.g., "Basic <base64>") alongside the existing bearer redaction; update the regex used in toSafePreview (the replacement after the secret key regex) to also match /(basic\s+)[A-Za-z0-9._-]+=*/i and replace with '$1<redacted>' (or combine into a single regex that handles both 'bearer' and 'basic') so previews never expose Basic auth payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/ApiKeys.tsx`:
- Around line 227-229: When isAdminUser flips to false the code only toggles
initialAdminLoadRef.current and returns but leaves sensitive state (keys and
issuedKey) populated; update the branch that checks isAdminUser to also clear
the admin-derived state by resetting keys and issuedKey (e.g. call the state
setters used in this component such as setKeys([]) and
setIssuedKey(null/undefined)) before returning so no admin secrets remain in
memory when access is lost.
In `@src/db/migrator.ts`:
- Around line 33-42: The catch block in the realpath validation wraps both
realpathSync errors and the explicit security Error thrown when resolvedRealDir
is outside resolvedProjectRoot, which hides the original security message/stack;
update the try/catch in migrator.ts (around realpathSync, resolvedRealDir,
resolvedProjectRoot and the thrown Error about "Migration directory must be
within project root") so that if the caught error is the specific security Error
(e.g., instanceof Error and message startsWith "Security:" or better: tag the
thrown security error with a unique type/class or symbol) you re-throw it
unchanged, otherwise wrap/throw a new error that includes the original error
details; ensure you reference the same identifiers (dir, projectRoot,
resolvedRealDir, resolvedProjectRoot) when implementing this conditional
re-throw.
In `@src/routes/onboarding.ts`:
- Around line 271-274: The password regex (PASSWORD_REGEX) currently allows any
non-alphanumeric, non-whitespace special character but the user-facing policy
text still references a limited set (e.g., "@$!%*?&"); update the displayed
message to match the validation by changing the password policy string to state
"a special character (any non-alphanumeric character, excluding whitespace)" or
similar wording, or alternatively restrict PASSWORD_REGEX to the exact
characters listed—ensure the change is made where the message/constant that
describes the password requirements is defined and that it references
PASSWORD_REGEX so both validation and text remain consistent.
In `@tests/routes/admin.whoami.session.spec.ts`:
- Around line 45-47: The afterAll block currently swallows errors from the
dynamic import and cleanup call (import('../../src/routes/auth') and
auth.stopAuthCleanup()), which can hide teardown failures; change it to await
the import and call stopAuthCleanup() inside a try/catch that does not swallow
the error—either log the error and rethrow (or call fail) so the test run
surface the failure. Ensure you reference the same symbols (afterAll,
import('../../src/routes/auth'), auth.stopAuthCleanup) and propagate the
exception instead of an empty catch to avoid masking leaked async work.
---
Outside diff comments:
In `@docs/openapi/openapi.json`:
- Around line 3565-3594: Add a deprecated alias property so existing clients
sending client_pubkey continue working: in the same object that defines "pubkey"
add a "client_pubkey" property with the same schema (type: string, description:
"DEPRECATED: client_pubkey (hex encoded) — use pubkey") and then replace the
single "required": ["pubkey"] constraint with a oneOf/anyOf that accepts either
required ["pubkey"] or required ["client_pubkey"] (or otherwise accept either
field), preserving "pubkey" as the preferred name while allowing the deprecated
"client_pubkey" during transition.
---
Duplicate comments:
In `@src/db/database.ts`:
- Around line 12-15: The current isEnvPathFile logic misclassifies
dot-containing directory names (e.g., /data/my.folder) as files because
path.extname(envPath) !== '' is true; update the condition used in isEnvPathFile
to only treat path.extname(envPath) as indicating a file when the final path
segment (path.basename(envPath)) does not start with a dot (to exclude dot-named
dirs) and/or when the path actually exists and fs.statSync(envPath).isFile()
confirms it's a file; modify the isEnvPathFile computation to include a check on
path.basename(envPath) (and optionally use fs.existsSync + fs.statSync for
existing paths) so envPath like '/data/my.folder' is not misclassified while
still recognizing real files and .db endings.
---
Nitpick comments:
In `@env.example`:
- Line 255: Replace realistic-looking hex example values for SESSION_SECRET (and
other similar example secret values in this file) with clearly fake placeholders
so users must generate their own keys; e.g., change the commented value to
something like "# SESSION_SECRET=<generate-64-hex-with-openssl-rand-hex-32>"
(and update the other flagged example secret entries in the same file similarly)
so the examples are obviously placeholders and not copy-pasteable secrets.
In `@frontend/components/Configure.tsx`:
- Line 1332: The code currently compares advancedSettings and
originalAdvancedSettings using JSON.stringify on every render; replace this with
a memoized equality check to avoid expensive stringify calls by computing a
boolean like areAdvancedSettingsDirty using React's useMemo and a suitable
shallow/deep equality comparer (e.g., lodash/isEqual or a custom shallowEqual)
and then use that memoized boolean in the JSX conditional; update references to
advancedSettings and originalAdvancedSettings inside useMemo so the memo
recomputes only when those objects change.
- Around line 301-310: The code currently uses a mock empty shares array which
prevents duplicate-name validation from working; update the Configure component
to call the server API instead: replace the mock shares variable with an awaited
fetch to '/api/env/shares' (or the real endpoint), parse JSON, extract names the
same way (share.name.split(' share ')[0]), and call setExistingNames(names);
ensure you handle fetch errors and non-OK responses (log or surface via state)
and keep existingNames and the duplicate check
(existingNames.indexOf(nameWithoutShare)) intact so validation works correctly.
- Around line 337-369: The focus-trap useEffect for showClearConfirm currently
sets initial focus to clearConfirmButtonRef (destructive action) and duplicates
modal logic; change the initial focus to clearCancelButtonRef to reduce
accidental confirmations and ensure focus restoration via clearTriggerButtonRef
remains, and refactor Configure's clear-confirm flow to reuse the existing
ConfirmModal component (used elsewhere as ConfirmModal) instead of duplicating
the keyboard/focus logic so both modals share the same accessibility pattern and
handlers.
In `@frontend/components/nip46/RelaySettings.tsx`:
- Around line 78-84: The onClick async arrow returns a Promise causing a
floating promise; change it to match the pattern used for add (e.g. create a
named handler handleRemove or call onRemove via void to ensure the event handler
returns void). Specifically, replace the inline async onClick={() => { try {
await onRemove(relay) } catch { ... } }} with either a top-level async function
handleRemove(relay) that performs try/await/onRemove and is referenced as
onClick={() => void handleRemove(relay)} or call onClick={() => { try { void
onRemove(relay) } catch { /* keep parent error handling */ } }}, making sure to
reference the existing onRemove and relay identifiers.
In `@frontend/components/Onboarding.tsx`:
- Around line 108-117: The cleanup in the useEffect that invokes checkStatus is
misleading because it clears timeoutRef.current (which is actually set in
createUser for auto-redirect), so update the Onboarding component to either
remove or correct the comment and move the timeout cleanup into a separate
effect that watches the completion/step state (or the variable that createUser
sets) so clearTimeout(timeoutRef.current) runs only for the timer created by
createUser; reference the existing useEffect(checkStatus), createUser,
timeoutRef.current, and the completion/step state to locate and relocate or
remove the cleanup logic and adjust the comment accordingly.
In `@frontend/components/Signer.tsx`:
- Around line 996-1002: saveRelaysToEnv currently ignores the boolean results
from saveRelaysToUserCredentials and saveRelaysToServerEnv, so update
saveRelaysToEnv to return a boolean indicating overall success (true only if the
called helper returns true), then update callers like handleAddRelay and
handleRemoveRelay to await saveRelaysToEnv and, on false, revert the optimistic
local state change (e.g., restore previous relayUrls) and rely on
credentialSaveError for the error UI; reference the functions saveRelaysToEnv,
saveRelaysToUserCredentials, saveRelaysToServerEnv, handleAddRelay, and
handleRemoveRelay when making these changes.
In `@frontend/components/ui/icon-button.tsx`:
- Line 55: Remove the trailing whitespace at the end of the export statement for
IconButton and iconButtonVariants (the line exporting IconButton,
iconButtonVariants) so the export reads without any trailing space; update the
export line for the IconButton/iconButtonVariants symbols to remove the extra
space and save formatting.
In `@frontend/components/ui/input-with-validation.tsx`:
- Around line 1-55: Rename the file to PascalCase and update all imports: move
frontend/components/ui/input-with-validation.tsx →
frontend/components/ui/InputWithValidation.tsx, keep the component name
InputWithValidation and its named export unchanged, then search the repo for any
imports using "input-with-validation" and update them to import {
InputWithValidation } from "./InputWithValidation" (or the correct relative
path); also update any barrel/index files or tests that re-export or reference
the old filename to use the new filename.
In `@src/class/relay.test.ts`:
- Around line 128-175: Add an assertion that the single returned event is the
expected newest matched event by checking the event id equals matchedB.id: after
decoding messages and filtering eventMessages, assert eventMessages[0]?.[2].id
=== matchedB.id (or equivalent) to ensure sorting by created_at returns matchedB
when limit: 1 is used; reference variables matchedB and eventMessages in the
test to locate where to add this check.
In `@src/db/migrations/20250916_0004_audit_nip46_data_sizes.sql`:
- Around line 41-43: Compute the repeated BLOB length expressions once in a CTE
and reuse aliases instead of repeating COALESCE(LENGTH(CAST(... AS BLOB)), 0):
add a CTE (e.g., sized_rows) that SELECTs the primary key (id) and computes
relay_size, methods_size, kinds_size using COALESCE(LENGTH(CAST(relays AS
BLOB)),0) etc., then in the main query reference those aliases for per-column
sizes, SUM them into a single total_size alias, and use total_size and the sized
alias names in the risk checks and CASE expressions (replace occurrences of the
repeated LENGTH(CAST(... AS BLOB)) for relays, policy_methods, policy_kinds with
the CTE aliases).
In `@src/db/nip46.ts`:
- Around line 57-59: The getSession function duplicates client pubkey
normalization inline instead of reusing normalizeClientPubkey; update getSession
to call normalizeClientPubkey(clientPubkey) where it currently trims/lowers the
value so behavior matches updatePolicy, updateStatus, deleteSession, and
logSessionEvent and to centralize normalization logic in normalizeClientPubkey
for maintainability.
In `@src/nip46/service.ts`:
- Around line 404-409: The catch block in the connect flow resets references but
doesn't close the underlying transport, risking leaked resources; update the
catch to defensively close the transport and any partially-initialized agent
before nulling fields — e.g. call and await a close method on the transport
(this.transport?.close() or this.closeTransport()) and, if available, close the
agent (this.agent?.close()) inside a try/catch, then call
removeAgentListeners(), set this.agent = null, this.signer = null,
this.transport = null, and this.started = false before rethrowing the error so
partially-opened resources are always cleaned up.
In `@tests/routes/helpers/script-runner.ts`:
- Around line 41-51: toSafePreview currently redacts bearer tokens but not HTTP
Basic credentials, so add a second replacement in the toSafePreview function to
detect and redact Basic auth values (e.g., "Basic <base64>") alongside the
existing bearer redaction; update the regex used in toSafePreview (the
replacement after the secret key regex) to also match
/(basic\s+)[A-Za-z0-9._-]+=*/i and replace with '$1<redacted>' (or combine into
a single regex that handles both 'bearer' and 'basic') so previews never expose
Basic auth payloads.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (72)
.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/umbrel-dev.yml.gitignoreDockerfiledocs/DEPLOY.mddocs/openapi/README.mddocs/openapi/openapi.jsondocs/openapi/openapi.yamlenv.examplefrontend/App.tsxfrontend/components/ApiKeys.tsxfrontend/components/Configure.tsxfrontend/components/Login.tsxfrontend/components/NIP46.tsxfrontend/components/Onboarding.tsxfrontend/components/Recover.tsxfrontend/components/Signer.tsxfrontend/components/nip46/Permissions.tsxfrontend/components/nip46/RelaySettings.tsxfrontend/components/nip46/Requests.tsxfrontend/components/ui/card.tsxfrontend/components/ui/collapsible.tsxfrontend/components/ui/event-log.tsxfrontend/components/ui/icon-button.tsxfrontend/components/ui/input-with-validation.tsxfrontend/components/ui/peer-list.tsxfrontend/components/ui/tooltip.tsxfrontend/types/index.tsllm/implementation/node-lifecycle-implementation.mdllm/implementation/umbrel-implementation.mdpackage.jsonpackages/umbrel/igloo/Dockerfilescripts/fetch-swagger-ui.mjsscripts/patch-zod-compat.mjsscripts/release.shsrc/class/relay.test.tssrc/class/relay.tssrc/config/crypto.tssrc/const.tssrc/db/database.tssrc/db/migrations/20250916_0004_audit_nip46_data_sizes.sqlsrc/db/migrator.tssrc/db/nip46.tssrc/db/ui-event-log.tssrc/nip46/service.tssrc/node/manager.tssrc/routes/admin.tssrc/routes/auth.tssrc/routes/env.tssrc/routes/index.tssrc/routes/nip04.tssrc/routes/nip44.tssrc/routes/nip46.tssrc/routes/onboarding.tssrc/routes/peers.tssrc/routes/sign.tssrc/routes/status.tssrc/routes/update.tssrc/routes/user.tssrc/routes/utils.test.tssrc/routes/utils.tssrc/server.tssrc/utils/rate-limiter.tstests/routes/admin.whoami.session.spec.tstests/routes/auth.rehydrate.spec.tstests/routes/body-limit.spec.tstests/routes/env.db-mode.spec.tstests/routes/helpers/script-runner.spec.tstests/routes/helpers/script-runner.tstests/routes/onboarding.spec.tstests/routes/status-env.test.ts
Member
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
frontend/components/ApiKeys.tsx (1)
227-231:⚠️ Potential issue | 🟠 MajorClear revoke modal state when admin access is lost.
On Line 227, you now clear
keysandissuedKey, butrevokeTarget/revokeReasonare still retained. If the revoke modal was open, key metadata can remain visible after admin access is removed.🔧 Suggested fix
useEffect(() => { if (!isAdminUser) { initialAdminLoadRef.current = false setKeys([]) setIssuedKey(null) + setRevokeTarget(null) + setRevokeReason('') return } if (initialAdminLoadRef.current) return initialAdminLoadRef.current = true loadKeys().catch(err => console.error('Failed to load keys:', err)) }, [isAdminUser, loadKeys])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ApiKeys.tsx` around lines 227 - 231, When admin access is lost in ApiKeys.tsx (the block using initialAdminLoadRef.current, setKeys, setIssuedKey), also clear the revoke modal state so sensitive key metadata isn't left visible: call the state setters that control the revoke modal (e.g., setRevokeTarget and setRevokeReason or clear revokeTarget/revokeReason) along with setKeys([]) and setIssuedKey(null) to fully reset UI state when !isAdminUser.
🟡 Minor comments (5)
frontend/components/ui/button.tsx-44-46 (1)
44-46:⚠️ Potential issue | 🟡 Minor
typegets dropped inasChildmode.Line 42 extracts
type, but Line 45 only spreadsprops, so explicittypeis lost whenasChildistrue.Proposed fix
- const componentProps = asChild - ? props - : { ...props, type: type ?? "button" } + const componentProps = asChild + ? { ...props, ...(type !== undefined ? { type } : {}) } + : { ...props, type: type ?? "button" }Also applies to: 51-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/button.tsx` around lines 44 - 46, The component currently drops the extracted `type` when `asChild` is true because it spreads `props` directly; update the `componentProps` assignment(s) so the extracted `type` is preserved in both modes: when `asChild` is true include the extracted `type` (only if defined) in the spread, and when not `asChild` keep the fallback `type: type ?? "button"`; apply this change to the `componentProps` expression(s) (search for `componentProps`, `asChild`, `props`, and `type` in this file, including the second occurrence around line 51) so the explicit `type` is never lost.src/routes/docs.ts-46-47 (1)
46-47:⚠️ Potential issue | 🟡 MinorFallback guidance is inconsistent with the current asset allowlist.
Line 91 suggests vendoring full local Swagger UI assets, but Line 46 only permits serving
swagger-ui.cssfrom/api/docs/assets/*. Either re-enable JS asset serving or update the fallback text to avoid suggesting a non-working local path.Also applies to: 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/docs.ts` around lines 46 - 47, The fallback message in the docs route currently recommends vendoring full local Swagger UI assets under /api/docs/assets/* but the asset allowlist only permits 'swagger-ui.css', so the described local path won't serve required JS; either add the missing JS files (e.g., 'swagger-ui-bundle.js' and 'swagger-ui-standalone-preset.js') to the same allowlist where 'swagger-ui.css' is listed so the local vendor path works, or alter the fallback text (the message around lines 91-92) to stop suggesting the non-working /api/docs/assets/* path and instead point to the correct external CDN or the single CSS-only option. Ensure changes reference the same allowlist array that currently contains 'swagger-ui.css' and update the fallback string accordingly.tests/routes/env.db-mode.spec.ts-10-13 (1)
10-13:⚠️ Potential issue | 🟡 MinorHard-coded fallback secret violates coding guidelines.
The fallback hex string on line 13 is a hard-coded secret. Per coding guidelines, secrets should be loaded from environment variables or data/ fixtures, not hard-coded—even in tests.
Consider moving this to a fixture file (e.g.,
data/test-keyset-secret.txt) or require the environment variable to be set for these tests:🔧 Suggested approach
+import { readFileSync } from 'fs'; +import path from 'path'; + +const FIXTURE_SECRET_PATH = path.join(process.cwd(), 'data', 'test-keyset-secret.txt'); + +function loadTestKeysetSecret(): string { + const fromEnv = normalizeOptionalEnv(process.env.TEST_KEYSET_SECRET) ?? + normalizeOptionalEnv(process.env.TEST_NSEC_HEX); + if (fromEnv) return fromEnv; + try { + return readFileSync(FIXTURE_SECRET_PATH, 'utf8').trim(); + } catch { + throw new Error('TEST_KEYSET_SECRET env var or data/test-keyset-secret.txt fixture required'); + } +} + const TEST_KEYSET_SECRET = - normalizeOptionalEnv(process.env.TEST_KEYSET_SECRET) ?? - normalizeOptionalEnv(process.env.TEST_NSEC_HEX) ?? - 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'; + loadTestKeysetSecret();As per coding guidelines: "Never hard-code secrets; load from environment or data/ fixtures"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/env.db-mode.spec.ts` around lines 10 - 13, The TEST_KEYSET_SECRET fallback is a hard-coded secret; update the TEST_KEYSET_SECRET initialization (and any use of normalizeOptionalEnv) to load the secret from a fixture or require the env var: add a data fixture (e.g., data/test-keyset-secret.txt) and read it when normalizeOptionalEnv(process.env.TEST_KEYSET_SECRET) and normalizeOptionalEnv(process.env.TEST_NSEC_HEX) are undefined, or fail the test early with a clear error if neither env var nor fixture exists; ensure references to TEST_KEYSET_SECRET and normalizeOptionalEnv are updated to use the fixture-loading logic (or throw) rather than the hard-coded hex.src/routes/index.ts-129-131 (1)
129-131:⚠️ Potential issue | 🟡 MinorWebsocket upgrade detection should be case-insensitive (and tolerate token lists).
Upgradeheader values can vary in casing and may contain multiple tokens. Current!== 'websocket'can incorrectly 404 legitimate websocket upgrades.Proposed fix
- if (url.pathname === '/api/events' && req.headers.get('upgrade') !== 'websocket') { - return Response.json({ error: 'Not Found' }, { status: 404, headers }); - } + if (url.pathname === '/api/events') { + const upgrade = req.headers.get('upgrade'); + const isWebsocket = upgrade + ?.split(',') + .some(token => token.trim().toLowerCase() === 'websocket'); + if (!isWebsocket) { + return Response.json({ error: 'Not Found' }, { status: 404, headers }); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/index.ts` around lines 129 - 131, The current check in the if-block that inspects url.pathname === '/api/events' uses req.headers.get('upgrade') !== 'websocket', which fails for varied casing or multiple tokens; update the condition in the same block to treat the Upgrade header case-insensitively and accept token lists (e.g., normalize the header value to lowercase, split on commas, trim tokens and check includes('websocket')) before returning Response.json({ error: 'Not Found' }, ...). Ensure you only reject when no token equals 'websocket' so legitimate websocket upgrade requests are not 404'd.src/routes/peers.ts-204-216 (1)
204-216:⚠️ Potential issue | 🟡 MinorUse
typeof globalThis.structuredCloneinstead of barestructuredClonereference to avoid ReferenceError.In JavaScript, referencing a bare identifier (like
structuredClone) throwsReferenceErrorbefore the ternary operator can evaluate, if that identifier is undefined. This happens in runtimes that don't havestructuredClone(available only in Node.js 17.3+). Usetypeof globalThis.structuredClone === 'function'for safe feature detection.Proposed fix
function summarizePolicyForLog(policy: NodePolicyInput | null | undefined): Record<string, unknown> | null { if (!policy || typeof policy !== 'object') { return null; } - const summary = structuredClone ? structuredClone(policy) : { ...policy }; + const summary = typeof globalThis.structuredClone === 'function' + ? globalThis.structuredClone(policy) + : { ...policy }; if ('note' in summary) { delete (summary as { [key: string]: unknown }).note; } if ('metadata' in summary) { delete (summary as { [key: string]: unknown }).metadata; } return summary; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/peers.ts` around lines 204 - 216, The function summarizePolicyForLog references structuredClone directly which can throw ReferenceError in environments without it; update the feature check to use typeof globalThis.structuredClone === 'function' and call globalThis.structuredClone(policy) when available, otherwise fall back to shallow copy ({ ...policy }); keep the existing removal of 'note' and 'metadata' from the summary and preserve the function name summarizePolicyForLog and its input/output shapes.
🧹 Nitpick comments (29)
frontend/components/ui/input-with-validation.tsx (1)
55-55: Trailing whitespace.There appears to be a trailing space after the semicolon on this line.
🧹 Proposed fix
-export { InputWithValidation }; +export { InputWithValidation };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/input-with-validation.tsx` at line 55, Remove the trailing whitespace after the semicolon in the export statement for InputWithValidation; locate the export { InputWithValidation }; line and delete the extra space so it ends immediately after the semicolon.scripts/api/test-get-endpoints.ts (2)
14-14: Consider replacinganywithunknownfor type safety.The
anytype is used in several places throughout this file (lines 14, 46, 56, 65, 86, 126), which bypasses TypeScript's type checking. Usingunknownwith explicit type narrowing would align with the coding guidelines.♻️ Suggested approach
type TestCase = { name: string; path: string; expectStatus?: number | ((status: number) => boolean); expectContentType?: RegExp; // e.g., /^application\/json/ - validate?: (jsonOrText: any, res: Response) => void | Promise<void>; + validate?: (jsonOrText: unknown, res: Response) => void | Promise<void>; };Then update validators to narrow the type:
validate: (data: unknown) => { + const obj = data as Record<string, unknown>; - if (typeof data?.serverRunning !== 'boolean') throw new Error('serverRunning missing'); + if (typeof obj?.serverRunning !== 'boolean') throw new Error('serverRunning missing'); },For error handling (line 126):
- } catch (err: any) { + } catch (err: unknown) { - return { name: tc.name, path: tc.path, ok: false, status: -1, detail: err?.message || String(err) }; + return { name: tc.name, path: tc.path, ok: false, status: -1, detail: err instanceof Error ? err.message : String(err) }; }As per coding guidelines: "Enable TypeScript strict mode, declare explicit types, and avoid any".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/api/test-get-endpoints.ts` at line 14, Replace the use of the any type in the validate function signature and all validator functions with unknown (e.g., change validate?: (jsonOrText: any, res: Response) => ... to validate?: (jsonOrText: unknown, res: Response) => ...), then update each validator (the functions referenced where validate is called/implemented) to perform explicit type narrowing/guards (typeof checks, Array.isArray, instance checks, or schema validation) before accessing properties or casting to concrete types; ensure the error-handling block that processes validator failures also narrows the unknown value before using it.
119-123: Silent JSON parse failure may obscure root cause.The
.catch(() => ({}))swallows JSON parse errors, which would then manifest as validation failures with "field missing" errors rather than indicating malformed JSON.♻️ Optional: surface JSON parse errors explicitly
if (tc.validate) { const isJson = /json/i.test(ctype); - const body = isJson ? await res.json().catch(() => ({})) : await res.text(); + let body: unknown; + if (isJson) { + const text = await res.text(); + try { + body = JSON.parse(text); + } catch { + return { name: tc.name, path: tc.path, ok: false, status, detail: 'Invalid JSON response' }; + } + } else { + body = await res.text(); + } await tc.validate(body, res); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/api/test-get-endpoints.ts` around lines 119 - 123, The current JSON parsing in the test runner silently swallows parse errors (the isJson branch using res.json().catch(() => ({}))) which hides malformed JSON; update the logic in the block around tc.validate / isJson so that JSON parsing errors are not swallowed—use a try/catch around await res.json() (or await res.json().catch(err => { throw new Error(`Invalid JSON response: ${err.message}`) })) and either rethrow or attach the parse error to the body/result passed into tc.validate so tests surface malformed JSON explicitly; locate the code around the tc.validate check and the isJson/body assignment to implement this change.frontend/components/ui/peer-list.tsx (2)
377-417: Good refactor with safer parsing and normalized key matching.The
useCallbackwrapper is appropriate for the event listener usage. TheNumber.isFinitecheck prevents NaN latency values, andtoPolicyKeynormalization handles compressed pubkey format differences.Consider extracting the repeated update logic to reduce duplication:
♻️ Optional: Extract update logic
+ const buildUpdatedPeer = (peer: PeerStatus, status: any): PeerStatus => { + const hasLatency = status.latency !== undefined && status.latency !== null; + const parsedLatency = Number(status.latency); + const latency = hasLatency && Number.isFinite(parsedLatency) ? parsedLatency : peer.latency; + return { + ...peer, + online: Boolean(status.online), + lastSeen: parseDate(status.lastSeen) ?? peer.lastSeen, + latency, + lastPingAttempt: parseDate(status.lastPingAttempt) ?? peer.lastPingAttempt + }; + }; const handlePeerUpdate = useCallback((event: CustomEvent) => { const { pubkey, status } = event.detail; setPeers(prev => { const updated = prev.map(peer => { if (peer.pubkey === pubkey) { - // ... duplicated logic + return buildUpdatedPeer(peer, status); } const peerNormalized = toPolicyKey(peer.pubkey); const pingNormalized = toPolicyKey(pubkey); if (peerNormalized !== '' && peerNormalized === pingNormalized) { - // ... duplicated logic + return buildUpdatedPeer(peer, status); } return peer; }); return updated; }); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/peer-list.tsx` around lines 377 - 417, The peer update mapping in handlePeerUpdate duplicates the same logic for exact match and normalized-match; refactor by extracting a helper (e.g., updatePeerStatus) that accepts a peer and status and returns the updated PeerStatus (using parseDate, Number parsing/Number.isFinite, and preserving fallback fields), then call that helper from both match branches inside handlePeerUpdate and return peer unchanged otherwise; keep helper name and handlePeerUpdate, toPolicyKey, parseDate, PeerStatus, and setPeers references so it's easy to locate and replace the duplicated blocks.
32-32: Theanytype violates strict mode guidelines.While the comment indicates this is kept for compatibility, consider using a more specific type or
unknown:- node: any; // Not used in server architecture, kept for compatibility + node: unknown; // Not used in server architecture, kept for compatibilityUsing
unknownis safer thananyas it requires explicit type checks before use, though since it's unused this is a minor concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/peer-list.tsx` at line 32, The declaration "node: any" should not use any; change the type to a safer alternative (for example replace with "node: unknown") or remove the unused declaration entirely; locate the "node" symbol in peer-list.tsx and either change its type from any to unknown (or a specific interface if you know the shape) or delete the unused variable/comment to satisfy strict mode.frontend/components/ui/modal.tsx (1)
83-110: Consider extracting the close button to reduce duplication.The close button JSX is duplicated between lines 84-93 and 99-108. A small refactor could improve maintainability:
♻️ Optional refactor to extract close button
+ const closeButton = showCloseButton ? ( + <Button + variant="ghost" + size="sm" + onClick={onClose} + aria-label="Close" + title="Close" + className="text-gray-400 hover:text-gray-300 hover:bg-gray-800 h-8 w-8 p-0" + > + <X className="h-4 w-4" /> + </Button> + ) : null; {title && ( <div className="flex justify-between items-center border-b border-gray-800 p-4"> <h3 id={titleId} className="text-xl font-semibold text-blue-200">{title}</h3> - {showCloseButton && ( - <Button - variant="ghost" - ... - </Button> - )} + {closeButton} </div> )} - {!title && showCloseButton && ( + {!title && closeButton && ( <div className="flex justify-end border-b border-gray-800 p-4"> - <Button - ... - </Button> + {closeButton} </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/modal.tsx` around lines 83 - 110, The close button JSX is duplicated; extract it into a small reusable component or render function (e.g., const CloseButton = () => (...) or function renderCloseButton()) and use that in place of the two duplicated blocks; preserve the same props and symbols (showCloseButton, onClose, Button, X, aria-label/title/className/variant/size) so both conditional branches render <CloseButton /> (or call renderCloseButton()) without changing behavior.frontend/components/ui/collapsible.tsx (1)
26-27: Good accessibility improvements withuseIdand ARIA attributes.The addition of
aria-expanded,aria-controls, and correspondingidattributes follows WAI-ARIA disclosure pattern correctly.For TypeScript strict mode consistency, consider adding explicit type:
- const [isExpanded, setIsExpanded] = useState(defaultOpen); + const [isExpanded, setIsExpanded] = useState<boolean>(defaultOpen);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/collapsible.tsx` around lines 26 - 27, Add an explicit boolean type to the useState hook to satisfy TypeScript strict mode: change the isExpanded/setIsExpanded state declaration (currently using useState(defaultOpen)) to explicitly use useState<boolean>, and keep the contentId from useId as-is; this ensures isExpanded is typed as boolean and avoids implicit any/union types in the Collapsible component.frontend/components/ui/event-log.tsx (1)
40-42: Explicit type annotation added forisExpanded- good fix.Line 40 now correctly uses
useState<boolean>(hideHeader)addressing the previous review feedback.However, for consistency with the coding guidelines requiring explicit types, line 42 should also have an explicit type annotation:
- const [showFilters, setShowFilters] = useState(false); + const [showFilters, setShowFilters] = useState<boolean>(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/event-log.tsx` around lines 40 - 42, The showFilters state lacks an explicit type annotation for consistency; update the useState call that defines showFilters/setShowFilters to include a boolean generic (e.g., useState<boolean>(false)) so it matches the explicit typing used for isExpanded and activeFilters and makes the types of showFilters and setShowFilters explicit.frontend/components/Recover.tsx (1)
561-586: Consider auto-hiding revealed NSEC after a short timeout.Once shown, the secret stays visible until manually hidden. A short auto-hide timer would improve shoulder-surfing resistance without changing core UX.
🔐 Optional hardening snippet
useEffect(() => { if (!showRecoveredNsec || !recoveredNsec) return; const timer = window.setTimeout(() => { setShowRecoveredNsec(false); }, 30000); // 30s return () => window.clearTimeout(timer); }, [showRecoveredNsec, recoveredNsec]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Recover.tsx` around lines 561 - 586, Add an auto-hide effect so the revealed NSEC (recoveredNsec) is hidden after a short timeout when showRecoveredNsec becomes true: implement a useEffect that watches showRecoveredNsec and recoveredNsec, starts a timer (e.g., 30s) to call setShowRecoveredNsec(false) when shown, and clears the timer on cleanup or when showRecoveredNsec/recoveredNsec change; update the component that renders the recovered NSEC and toggles visibility (references: showRecoveredNsec, recoveredNsec, setShowRecoveredNsec) to include this effect so the secret auto-hides.Dockerfile (1)
2-24: Good version pin; consider digest pinning for immutable builds.Lines 2 and 24 improve reproducibility by pinning Bun to
1.3.10. For stronger supply-chain guarantees, pin the image by digest as well.Example diff (resolved digest)
-FROM oven/bun:1.3.10 AS build +FROM oven/bun:1.3.10@sha256:b86c67b531d87b4db11470d9b2bd0c519b1976eee6fcd71634e73abfa6230d2e AS build ... -FROM oven/bun:1.3.10 AS production +FROM oven/bun:1.3.10@sha256:b86c67b531d87b4db11470d9b2bd0c519b1976eee6fcd71634e73abfa6230d2e AS production🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 2 - 24, Replace the two FROM lines that use the tag "oven/bun:1.3.10" (the lines "FROM oven/bun:1.3.10 AS build" and "FROM oven/bun:1.3.10 AS production") with the corresponding image digest pins (e.g., "oven/bun@sha256:<digest>") to ensure immutable builds; obtain the correct sha256 digest for the 1.3.10 image from your registry or the image manifest and use that digest in both FROM instructions so both the build and production stages reference the exact same immutable image.src/nip46/index.ts (1)
22-25: Prefer structured server logging over rawconsole.warnhere.When init options drift, logging through
opts.addServerLogkeeps these warnings observable in the same pipeline as other server events.🔧 Suggested refinement
if (service) { if (!serviceInitOptions || !areInitOptionsEquivalent(serviceInitOptions, opts)) { - console.warn('Reusing existing Nip46Service with different init options.') + try { + opts.addServerLog('warning', 'Reusing existing Nip46Service with different init options.') + } catch { + console.warn('Reusing existing Nip46Service with different init options.') + } } return service }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nip46/index.ts` around lines 22 - 25, Replace the console.warn call with a structured server log via opts.addServerLog so the warning is captured in the server pipeline: inside the block that checks service && (!serviceInitOptions || !areInitOptionsEquivalent(serviceInitOptions, opts)), call opts.addServerLog(...) with a clear message and include contextual fields (e.g., previous init options from serviceInitOptions and current opts) and a severity/category flag instead of console.warn; ensure opts.addServerLog is present or gate the call if opts is undefined to avoid errors.src/routes/nip44.ts (1)
41-46: Use limiter reset time for more accurateRetry-Afterwhen present.Right now the header always reflects the configured window. Using
rate.resetAt(if provided) improves client retry behavior and keeps this route consistent with/api/nip04/*.🔁 Suggested Retry-After refinement
if (!rate.allowed) { - const retryAfterWindow = Number.parseInt(process.env.RATE_LIMIT_WINDOW || '', 10); - const retryAfterSeconds = Number.isFinite(retryAfterWindow) && retryAfterWindow > 0 ? retryAfterWindow : 900; + const resetAt = typeof rate.resetAt === 'number' && Number.isFinite(rate.resetAt) ? rate.resetAt : null; + const retryAfterFromReset = resetAt !== null + ? Math.max(0, Math.ceil((resetAt - Date.now()) / 1000)) + : null; + const retryAfterWindow = Number.parseInt(process.env.RATE_LIMIT_WINDOW || '', 10); + const retryAfterFallback = Number.isFinite(retryAfterWindow) && retryAfterWindow > 0 ? retryAfterWindow : 900; + const retryAfterSeconds = retryAfterFromReset !== null ? retryAfterFromReset : retryAfterFallback; return Response.json({ error: 'Rate limit exceeded. Try again later.' }, { status: 429, headers: { ...headers, 'Retry-After': Math.ceil(retryAfterSeconds).toString() } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip44.ts` around lines 41 - 46, The Retry-After header should prefer the limiter's reset time when available: in the Response.json call where you compute retryAfterWindow/retryAfterSeconds, check for a provided rate.resetAt (or rate?.resetAt) and compute retrySeconds = Math.max(0, Math.ceil((new Date(rate.resetAt).getTime() - Date.now()) / 1000)); if that yields a finite positive number use it, otherwise fall back to the existing retryAfterWindow / retryAfterSeconds logic; then set the 'Retry-After' header to retrySeconds.toString() in the Response.json response.src/routes/onboarding.ts (1)
273-273: Use a single password regex source to prevent policy drift.This file defines a local
PASSWORD_REGEXwhilesrc/config/crypto.tsalso defines password policy. Keeping both invites future mismatch between setup validation and shared/server validation behavior.Proposed refactor
-const PASSWORD_REGEX = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^A-Za-z0-9\s]).*$/; +const PASSWORD_REGEX = VALIDATION.PASSWORD_REGEX;Also applies to: 348-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/onboarding.ts` at line 273, Replace the local PASSWORD_REGEX in onboarding.ts with the canonical password policy from the shared config so validation stays consistent: remove the local const PASSWORD_REGEX and import the password policy (e.g., the exported regex/constant defined in src/config/crypto.ts) and use that in any places that currently reference PASSWORD_REGEX (notably the checks around the symbols at lines shown, e.g., where PASSWORD_REGEX is referenced near the handlers in onboarding.ts). Ensure the import name matches the exported identifier from crypto.ts and update both occurrences (the current local declaration and the second usage) to use the imported constant.src/routes/user.ts (2)
241-241: Avoidanytype per coding guidelines.The
bodyvariable usesanytype. Consider using a more specific type orunknownwith runtime validation.♻️ Suggested type improvement
- let body: any; + let body: unknown; try { body = await parseJsonRequestBody(req); } catch (error) {Then update the property accesses to use type guards or assertions after validation.
As per coding guidelines: "Enable TypeScript strict mode, declare explicit types, and avoid any"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/user.ts` at line 241, The local variable "body" is declared as any; replace it with a stricter type (e.g., unknown or a specific interface like UserRequestBody) and perform runtime validation before accessing properties. Update the code that assigns to and reads from body (the variable named "body" in the route handler) to validate shape (using a validator function or type guard) and then narrow the type (assert or use a typed variable) so subsequent property accesses are type-safe and no longer use any. Ensure all references to body use the narrowed/validated type or guarded access.
473-473: Avoidanytype per coding guidelines.Same issue as above - the
bodyvariable usesanytype.As per coding guidelines: "Enable TypeScript strict mode, declare explicit types, and avoid any"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/user.ts` at line 473, The local variable "body" is typed as any; replace it with an explicit type instead (e.g., define an interface like UserRequestBody or use unknown and narrow it) and validate/cast before use. Change "let body: any;" to either "let body: UserRequestBody;" (create the UserRequestBody interface matching expected fields) or "let body: unknown;" then add a type guard/validation step (e.g., isUserRequestBody(body) or runtime checks) before assigning/using fields; update all downstream usages to rely on the explicit type or the narrowed type. Ensure the new type or guard is exported/declared near the route handler so the compiler can enforce strict typing.frontend/components/Configure.tsx (1)
603-633: Consider extractingparseRelayListto module scope for reuse.The
parseRelayListfunction is defined insidehandleCreateKeysetbut has no closure dependencies. Extracting it to module scope (nearisAbortError) would improve readability and enable reuse if needed elsewhere.♻️ Optional refactor
+function parseRelayList(raw: string): string[] | null { + try { + const parsed: unknown = JSON.parse(raw); + if (!Array.isArray(parsed)) return null; + const relays = parsed + .filter((relay): relay is string => typeof relay === 'string') + .map((relay) => relay.trim()) + .filter((relay) => relay.length > 0); + return relays.length > 0 ? relays : null; + } catch { + return null; + } +} + const Configure: React.FC<ConfigureProps> = ({ onKeysetCreated, onCredentialsSaved, onBack, authHeaders = {} }) => {Then remove the inline definition from
handleCreateKeyset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 603 - 633, The local parseRelayList function should be moved to module scope for reuse: extract the function currently declared inside handleCreateKeyset to the top-level (near isAbortError), keep its signature and behavior unchanged, and remove the inline declaration; verify handleCreateKeyset and resolveRelaysToSave still call parseRelayList, and confirm there are no closure dependencies on isHeadlessMode, existingRelays or advancedSettings.RELAYS before making it module-scoped.tests/routes/user-peers.spec.ts (1)
257-322: Important concurrency isolation test.This test verifies that when rollback occurs for peer A's failed update, it doesn't overwrite the concurrent successful update to peer B. The 50ms delay simulates the timing window where concurrent updates could interfere.
One consideration: The 50ms timeout is somewhat arbitrary and could be flaky on slow CI runners.
Consider documenting why 50ms was chosen, or use a more deterministic synchronization mechanism if flakiness is observed:
- await new Promise(resolve => setTimeout(resolve, 50)); + // Allow time for the first request to begin retrying before simulating concurrent update. + // This timing may need adjustment if test becomes flaky on slow runners. + await new Promise(resolve => setTimeout(resolve, 50));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/user-peers.spec.ts` around lines 257 - 322, The hard-coded 50ms sleep is flaky; replace it with a deterministic synchronization: instead of await new Promise(resolve => setTimeout(resolve, 50)), wait until the in-progress PUT has reached the retry/persistence phase by polling or using a test hook. Modify the test to either poll a clear condition (e.g., an "inFlightUpdate" flag exposed on the peers module or context) or use a test-only hook exported by peers (e.g., peers.__TEST__.waitForPersistenceRetry(pending)) so you call await peers.__TEST__.waitForPersistenceRetry(pending) before applying the concurrent setNodePolicies(context.node, ...) update; keep using pending (the Promise returned from peers.handlePeersRoute) and setNodePolicies as the change points. Ensure the polling/hook has a reasonable timeout to fail fast in CI.src/routes/auth.ts (2)
967-980: Login 429 should ideally useresetAt/computedretryAfter(not always the full window).Right now
Retry-Afteris always the entire configured window (Line 975), even if the client is near the end of the window. SincecheckRateLimit()now returnsresetAt, you can compute an accurateRetry-After(like you do inauthenticate()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/auth.ts` around lines 967 - 980, The 429 response always uses the full AUTH_CONFIG.RATE_LIMIT_WINDOW; instead compute an accurate Retry-After using the resetAt value returned by checkRateLimit(req). In the block where you handle !rate.allowed (around the Response.json call), calculate retryAfterSeconds = Math.max(1, Math.ceil((rate.resetAt - Date.now())/1000)) (or fallback to Math.ceil(AUTH_CONFIG.RATE_LIMIT_WINDOW/1000) if resetAt is missing) and use that value for the 'Retry-After' header (same approach used in authenticate()). Ensure you still include the Set-Cookie and other headers unchanged.
319-333: Constant-time compare + derived-key vault limits look like a solid hardening pass.
compareConstantTime()removes the “length leak” edge case fromtimingSafeEqualusage by hashing first (Line 319-323).- Clamping TTL and max reads (Line 326-333) helps keep the derived-key vault bounded.
Based on learnings: “Tune FROSTR_SIGN_TIMEOUT, SIGN_TIMEOUT_MS, AUTH_DERIVED_KEY_TTL_MS, and AUTH_DERIVED_KEY_MAX_READS per environment”.
Also applies to: 326-333, 348-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/auth.ts` around lines 319 - 333, compareConstantTime currently hashes inputs before timingSafeEqual which is good; ensure it always accepts/normalizes inputs (convert non-strings to strings) to avoid throws. For the derived-key limits, ensure environment tuning is respected by validating and clamping parsed values for AUTH_DERIVED_KEY_TTL_MS and AUTH_DERIVED_KEY_MAX_READS (using parseEnvInt) as shown, and apply the same validated/clamped pattern to FROSTR_SIGN_TIMEOUT and SIGN_TIMEOUT_MS so all timeouts are parsed, bounded and safe per environment; locate and update the compareConstantTime function and the constants AUTH_DERIVED_KEY_TTL_MS, AUTH_DERIVED_KEY_MAX_READS, FROSTR_SIGN_TIMEOUT, SIGN_TIMEOUT_MS to normalize inputs and enforce min/max bounds.src/routes/nip46.ts (3)
329-344: Consider not returning raw parse/validation errors from transport PUT.On Line 341-343,
error.messagecan include low-level details (e.g., JSON parser text). Elsewhere in this file you return a generic"Invalid JSON"message, which is usually safer/cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip46.ts` around lines 329 - 344, Replace returning raw error.message in the PUT /api/nip46/transport handler with a generic validation message: when parseJsonWithLimit (and related validation) fails, return Response.json({ error: 'Invalid JSON' }, { status: 400, headers }) instead of exposing error.message; keep the null-size check that returns 413, and still call setTransportKey(userId, sk) on success. Update the catch around parseJsonWithLimit/setTransportKey to always send the generic 'Invalid JSON' response for parse/validation failures (or log the real error internally if needed) rather than returning error.message.
582-590: Good: consistent 413/400 behavior for oversized or invalid JSON bodies.Nice consistency across session creation and policy/status updates: oversize → 413, invalid JSON → 400.
Also applies to: 678-687, 720-728
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip46.ts` around lines 582 - 590, The JSON parsing block using parseJsonWithLimit and NIP46_JSON_BODY_LIMIT_BYTES correctly returns 413 for oversized payloads and 400 for invalid JSON; ensure the exact same pattern (use parseJsonWithLimit, check for null -> Response.json(... status: 413), catch -> Response.json(... status: 400)) is applied to the other occurrences noted (around the handlers that reference session creation and policy/status update) so that functions handling request bodies (e.g., the handlers that currently contain the similar logic near lines 678-687 and 720-728) use the same parsedBody/null check and catch behavior and the same headers variable for consistency.
9-40:parseJsonWithLimit()is a good guard rail, but it’s still best-effort without streaming.If
content-lengthis missing (or lies),await req.text()(Line 24) can still read an arbitrarily large body into memory before the size check. If you already have (or can add) a shared streaming/size-limited body reader insrc/routes/utils.ts, it’d be worth reusing here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip46.ts` around lines 9 - 40, The parseJsonWithLimit function currently calls await req.text() which can load an arbitrarily large body into memory if content-length is missing or incorrect; replace that call with a streaming, size-limited reader (e.g., readRequestBodyWithLimit or readStreamWithLimit) from your shared utilities and use it to obtain the body text while enforcing maxBytes, keeping the existing content-length short-circuit; if such a helper doesn't exist, implement it to consume req.body as a ReadableStream, accumulate bytes up to maxBytes, abort and return null or throw when exceeded, then JSON.parse the returned text inside parseJsonWithLimit (preserve current error propagation).src/routes/index.ts (3)
68-85: Prefer derivingRetry-AfterfromAUTH_CONFIG(avoid parsing env twice).
AUTH_CONFIG.RATE_LIMIT_WINDOWalready centralizes parsing/defaults. Re-parsingprocess.env.RATE_LIMIT_WINDOWhere can drift from what the limiter actually uses.Proposed refactor
- const parsedRateLimitWindowSeconds = Number.parseInt(process.env.RATE_LIMIT_WINDOW ?? '', 10); - const retryAfterSeconds = ( - Number.isFinite(parsedRateLimitWindowSeconds) && parsedRateLimitWindowSeconds > 0 - ? Math.ceil(parsedRateLimitWindowSeconds) - : 900 - ).toString(); + const retryAfterSeconds = Math.ceil(AUTH_CONFIG.RATE_LIMIT_WINDOW / 1000).toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/index.ts` around lines 68 - 85, Replace the ad-hoc re-parsing of process.env.RATE_LIMIT_WINDOW in the retryAfterSeconds calculation with the centralized value in AUTH_CONFIG.RATE_LIMIT_WINDOW: use AUTH_CONFIG.RATE_LIMIT_WINDOW (or its numeric equivalent) to compute retryAfterSeconds (Math.ceil when >0, fallback to 900, then toString()), and keep the hasAuthHint logic unchanged (it should still read req.headers authorization, x-api-key, x-session-id and cookie). This ensures parsedRateLimitWindowSeconds/retryAfterSeconds derive from the same source as the limiter and avoids duplicating environment parsing.
139-165: AvoidretryAfterSecondsshadowing inside docs auth flow.Minor readability: the inner
const retryAfterSecondsshadows the outerretryAfterSecondsused elsewhere in this handler.Proposed refactor
- const retryAfterSeconds = typeof authResult.retryAfter === 'number' ? authResult.retryAfter : 60; + const docsRetryAfterSeconds = typeof authResult.retryAfter === 'number' ? authResult.retryAfter : 60; const retryHeaders = { ...headers, - 'Retry-After': String(retryAfterSeconds) + 'Retry-After': String(docsRetryAfterSeconds) }; return Response.json( { error: 'Rate limit exceeded', - retryAfter: retryAfterSeconds, + retryAfter: docsRetryAfterSeconds, resetAt: authResult.resetAt },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/index.ts` around lines 139 - 165, The inner const retryAfterSeconds in the docs auth rate-limit branch shadows an outer retryAfterSeconds; update the rate-limit handling to use a distinct name (e.g., retryAfterForDocs or retryAfterSecondsForDocs) or reuse the outer variable instead of redeclaring, and ensure the Retry-After header and JSON body use that renamed symbol; locate the block that checks authResult.rateLimited and modify the variable used when building retryHeaders and the Response.json call (references: authResult, headers, getAuthStatus, Response.json).
234-375: For 429s, preferauthResult.retryAfter/resetAtover a fixed window value.Good move putting
finalizeAuth()in a singlefinally(Line 373-375). One tweak:authenticate()already computes accurate backoff metadata; using the fixedretryAfterSecondshere can mislead clients (especially if the limiter isn’t a simple fixed window).Proposed fix (header accuracy)
if (authResult.rateLimited) { + const retryAfter = typeof authResult.retryAfter === 'number' + ? authResult.retryAfter + : Number.parseInt(retryAfterSeconds, 10) || 60; return Response.json({ error: 'Rate limit exceeded. Try again later.' }, { status: 429, headers: { ...headers, - 'Retry-After': retryAfterSeconds + 'Retry-After': String(retryAfter) } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/index.ts` around lines 234 - 375, The 429 response uses a fixed retryAfterSeconds which can be inaccurate; in the authenticate() branch where you check authResult.rateLimited, use the backoff metadata provided by authResult (e.g., authResult.retryAfter or authResult.resetAt) instead of the fixed retryAfterSeconds: set the Retry-After header to authResult.retryAfter (seconds) or convert authResult.resetAt to an HTTP-date string if present, and ensure you merge it into the response headers in the same place where you return the 429 inside the initial authenticate() block so clients receive the accurate retry window.src/db/nip46.ts (3)
387-419: Potential perf hot spot: JSON-scan up to 500 pending requests per lookup.
getPendingNip46RequestByClientId()can parse up to 500row.paramsblobs per call (Line 407-417). If this is on a frequently-hit path, consider reducing payload and/or storing the client request id in a queryable column.One low-impact improvement is selecting only
id, paramsand fetching the full row only on match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 387 - 419, getPendingNip46RequestByClientId currently JSON-parses up to 500 params blobs per call; change the DB access to only SELECT id, params (and any minimal tie-breaker columns) in the initial query inside getPendingNip46RequestByClientId to reduce data scanned, then iterate rows, parse JSON and when payloadId matches normalizedClientRequestId use the matching row id to run a second query (e.g. SELECT * FROM nip46_requests WHERE id = ?) to fetch and return the full Nip46RequestRecord; keep the same normalization logic (normalizeClientPubkey, normalizedClientRequestId) and error handling for JSON.parse, and consider adding a dedicated client_request_id column later for direct querying.
180-208: Tighten JSON parsing types + avoid__proto__key hazards in boolean maps.
JSON.parse()yields implicitany. Also, assigning into a normal{}can behave oddly for special keys like__proto__. Usingunknown+ a null-prototype object makes this more robust.Proposed hardening
function parseRelayArray(raw: string): string[] { - const parsed = JSON.parse(raw) + const parsed: unknown = JSON.parse(raw) if (!Array.isArray(parsed)) { throw new Error('[nip46] Data integrity violation: relays must be an array') } const relays: string[] = [] - for (const value of parsed) { + for (const value of parsed) { if (typeof value !== 'string') { throw new Error('[nip46] Data integrity violation: relay entries must be strings') } relays.push(value) } return relays } function parseBooleanMap(raw: string, label: string): Record<string, boolean> { - const parsed = JSON.parse(raw) + const parsed: unknown = JSON.parse(raw) if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { throw new Error(`[nip46] Data integrity violation: ${label} must be an object`) } - const normalized: Record<string, boolean> = {} - for (const [key, value] of Object.entries(parsed)) { + const normalized: Record<string, boolean> = Object.create(null) + for (const [key, value] of Object.entries(parsed as Record<string, unknown>)) { if (typeof value !== 'boolean') { throw new Error(`[nip46] Data integrity violation: ${label}.${key} must be a boolean`) } normalized[key] = value } return normalized }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 180 - 208, Use safer JSON parsing and a null-prototype map: change both JSON.parse(raw) calls to parse into unknown (e.g. const parsed = JSON.parse(raw) as unknown), then narrow via runtime checks (Array.isArray(parsed) and typeof element === 'string' in parseRelayArray; parsed !== null, typeof parsed === 'object' and !Array.isArray(parsed) in parseBooleanMap). For parseBooleanMap create the accumulator with no prototype (const normalized = Object.create(null) as Record<string, boolean>) and either skip or reject keys named "__proto__" before assignment; keep the current boolean-type check for values and assign into normalized. Also update variable names parseRelayArray and parseBooleanMap accordingly where you perform the narrowed checks and assignments.
57-59: Nice: consistentclient_pubkeynormalization across reads/writes.This should eliminate a whole class of “same key, different casing/whitespace” bugs.
Small nit:
upsertSession()still normalizes inline; consider reusingnormalizeClientPubkey()there too for a single source of truth.Also applies to: 294-300, 587-698, 731-743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 57 - 59, The file adds normalizeClientPubkey(clientPubkey) but several places still perform inline trimming/lowercasing (notably upsertSession()); update upsertSession() to call normalizeClientPubkey(clientPubkey) instead of duplicating trim()/toLowerCase(), and replace any other inline normalizations in this module with calls to normalizeClientPubkey to ensure a single source of truth for client_pubkey handling (search for other places that call .trim().toLowerCase() on clientPubkey and swap them to use the helper).docs/openapi/openapi.yaml (1)
2875-2882: Constrainnostrconnectinvite URI format in schema.
Nip46ConnectInput.uricurrently accepts any string, so contract-level validation is weak for a security-sensitive entry point.🧩 Proposed fix
Nip46ConnectInput: type: object properties: uri: type: string description: nostrconnect:// invite URI + pattern: '^nostrconnect://' required: [uri]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.yaml` around lines 2875 - 2882, Nip46ConnectInput.uri currently accepts any string; tighten the OpenAPI schema by adding a pattern (and optionally format/example) to validate nostrconnect invite URIs: update the Nip46ConnectInput object to include a "pattern" that enforces the "nostrconnect://" scheme and a sensible character set (e.g. ^nostrconnect://[A-Za-z0-9\-._~:/?#[\]@!$&'()*+,;=%]+$), and update the "description" to mention the enforced pattern and provide an example value so consumers and validators reject non‑nostrconnect URIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 507e08b4-32c4-4c51-b8c9-52608b06c394
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (101)
.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/umbrel-dev.yml.gitignoreDockerfilecompose.ymldocs/CONFIG.mddocs/DEPLOY.mddocs/RELEASE.mddocs/SECURITY.mddocs/openapi/README.mddocs/openapi/openapi.jsondocs/openapi/openapi.yamlenv.examplefrontend/App.tsxfrontend/components/ApiKeys.tsxfrontend/components/Configure.tsxfrontend/components/Login.tsxfrontend/components/NIP46.tsxfrontend/components/Onboarding.tsxfrontend/components/Recover.tsxfrontend/components/Signer.tsxfrontend/components/nip46/Permissions.tsxfrontend/components/nip46/QRScanner.tsxfrontend/components/nip46/RelaySettings.tsxfrontend/components/nip46/Requests.tsxfrontend/components/ui/button.tsxfrontend/components/ui/card.tsxfrontend/components/ui/collapsible.tsxfrontend/components/ui/event-log.tsxfrontend/components/ui/icon-button.tsxfrontend/components/ui/input-with-validation.tsxfrontend/components/ui/modal.tsxfrontend/components/ui/peer-list.tsxfrontend/components/ui/relay-input.tsxfrontend/components/ui/tooltip.tsxfrontend/styles.cssfrontend/tsconfig.jsonfrontend/types/index.tsfrontend/types/nostr-connect.d.tsllm/implementation/auth-implementation.mdllm/implementation/node-lifecycle-implementation.mdllm/implementation/umbrel-implementation.mdllm/workflows/RELEASE_PROCESS.mdpackage.jsonpackages/umbrel/igloo/Dockerfilescripts/api/README.mdscripts/api/test-get-endpoints.tsscripts/api/test-ws-events.tsscripts/fetch-swagger-ui.mjsscripts/patch-zod-compat.mjsscripts/release.shscripts/start-headless.jssrc/class/relay.test.tssrc/class/relay.tssrc/config/crypto.tssrc/const.tssrc/db/database.tssrc/db/migrations/20250916_0004_audit_nip46_data_sizes.sqlsrc/db/migrations/20250922_0007_add_nip46_relays.sqlsrc/db/migrator.tssrc/db/nip46.tssrc/db/ui-event-log.tssrc/nip46/index.tssrc/nip46/service.tssrc/node/manager.tssrc/routes/admin.tssrc/routes/auth-factory.tssrc/routes/auth.tssrc/routes/docs.tssrc/routes/env.tssrc/routes/index.tssrc/routes/nip04.tssrc/routes/nip44.tssrc/routes/nip46.tssrc/routes/node-manager.tssrc/routes/onboarding.tssrc/routes/peers.tssrc/routes/sign.tssrc/routes/status.tssrc/routes/update.tssrc/routes/user.tssrc/routes/utils.test.tssrc/routes/utils.tssrc/server.tssrc/types/global.d.tssrc/utils/rate-limiter.tsstatic/docs/swagger-ui-bundle.jsstatic/index.htmltests/routes/admin.api-keys.negatives.spec.tstests/routes/admin.whoami.session.spec.tstests/routes/auth.rehydrate.spec.tstests/routes/body-limit.spec.tstests/routes/env.db-mode.spec.tstests/routes/helpers/script-runner.spec.tstests/routes/helpers/script-runner.tstests/routes/nip46.spec.tstests/routes/onboarding.spec.tstests/routes/protected.api.spec.tstests/routes/status-env.test.tstests/routes/user-peers.spec.ts
💤 Files with no reviewable changes (1)
- frontend/styles.css
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
Verification
Notes
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Documentation
Tests / CI