fix(cln): accept caller-supplied preimage in SendKeysend (unbreaks NIP-47 pay_keysend) - #2521
fix(cln): accept caller-supplied preimage in SendKeysend (unbreaks NIP-47 pay_keysend)#2521welliv wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesKeysend preimage handling
Docker workflow permissions
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5af0815 to
cbd46a9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
README.md-39-42 (1)
39-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDocument the 8008 firewall requirement before funding.
docs/security.mdLines 11-16 state that*:8008exposes unauthenticated admin endpoints and must be blocked before the wallet holds sats. The quick start installs the daemons and starts the wizard without surfacing this requirement. Add a blocking warning before the connection and funding steps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 39 - 42, Update the README quick-start sequence around the daemon installation and Hub connection steps to add a prominent blocking warning that port 8008 must be firewalled before funding or allowing the wallet to hold sats, referencing the unauthenticated admin endpoints described in the security guidance. Place the warning before the connection and funding instructions.docs/security.md-5-7 (1)
5-7: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAlign the key model with the unauthenticated completion path.
Line 5 says that a foreign key is not usable and that a local key only works against this daemon. Lines 7 and 11 state that completion requests do not validate the key. Document that client records are daemon-local for usage accounting, but any caller who reaches a funded daemon can submit requests. Keep the firewall as the primary access control.
Proposed wording
- A key from another instance is not usable here, and a key minted here only works against this daemon. + Client records are local to the daemon for usage accounting. They do not authenticate completion requests; access depends on network reachability and the Cashu wallet balance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/security.md` around lines 5 - 7, Update the API key documentation around the daemon-local client registry and completion behavior to state that keys are used only for local usage accounting and identity, not request authentication. Clarify that any caller reaching a funded daemon can submit completion requests regardless of key validity, and preserve the firewall as the primary access control.SECURITY.md-12-14 (1)
12-14: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winProvide a private reporting channel for sensitive findings.
Do not direct sensitive reports to
Open an issue, because GitHub issues are public. Provide a security email address or private advisory process. Keep public issues for non-sensitive reports only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SECURITY.md` around lines 12 - 14, Update the Reporting section in SECURITY.md to remove “Open an issue” as an option for sensitive findings and provide a private channel, such as a security email address or private advisory process. Clarify that public GitHub issues are intended only for non-sensitive reports.docs/backup-restore.md-39-43 (1)
39-43: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReplace unauthenticated backup encryption before expanding the archive.
AES-256-OFB does not authenticate the archive. An attacker can modify restored databases or configuration without detection. PBKDF2 with 4096 iterations also weakens resistance to offline password guessing. Use a versioned AEAD format with a stronger password KDF, or exclude these secrets from this format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backup-restore.md` around lines 39 - 43, Replace the documented AES-256-OFB/PBKDF2 backup format with a versioned authenticated-encryption format using a stronger password KDF before supporting archive expansion, or remove secrets from this format. Update the “Crypto format” documentation to describe the new format and ensure tampered archives are rejected before restoration.docs/deploy.md-34-36 (1)
34-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winProtect the Routstr configuration file.
The configuration contains
nsecandnwc, but the installation steps do not require owner-only permissions. Require~/.routstrd/config.jsonto use mode0600, keep its directory private, and prevent secrets from entering shell history or logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/deploy.md` around lines 34 - 36, Update the deployment instructions around the Routstr configuration step to create `~/.routstrd` with owner-only directory permissions and enforce mode `0600` on `~/.routstrd/config.json`. Instruct users to provide `nsec` and `nwc` without exposing them in shell history or logs, using a secure non-echoed or file-based method.frontend/src/components/connections/routstr/RoutstrConnectionDetails.tsx-70-79 (1)
70-79: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the shadcn
Buttonfor the visibility action.Replace the native
buttonwith the existingButtoncomponent. Add anaria-labelwithonExtraAction.titlebecause this control has no visible text.As per coding guidelines, “Use shadcn/ui components for all UI; do not create custom components unless no shadcn equivalent exists.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/RoutstrConnectionDetails.tsx` around lines 70 - 79, Replace the native button in the onExtraAction rendering with the existing shadcn Button component, preserving its click handler, title, icon, and styling. Add aria-label={onExtraAction.title} to provide an accessible name for the icon-only control.Source: Coding guidelines
frontend/src/components/connections/routstr/constants.ts-14-18 (1)
14-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the Hub base path in the external endpoint.
If Hub runs below a subpath, this builds an endpoint at the origin root. For example, a Hub at
/hub/produces/routstr/v1instead of/hub/routstr/v1. Useimport.meta.env.BASE_URLwhen constructing this URL, asfrontend/platform_specific/http/src/utils/request.tsalready does.Proposed fix
export function getRoutstrHubEndpoint(): string { + const basePath = import.meta.env.BASE_URL.replace(/\/$/, ""); + const endpoint = `${basePath}${ROUTSTR_HUB_PROXY_PATH}`; + if (typeof window === "undefined") { - return ROUTSTR_HUB_PROXY_PATH; + return endpoint; } - return `${window.location.origin}${ROUTSTR_HUB_PROXY_PATH}`; + return `${window.location.origin}${endpoint}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/constants.ts` around lines 14 - 18, Update getRoutstrHubEndpoint to include import.meta.env.BASE_URL when constructing the browser-facing endpoint, preserving the Hub deployment subpath before ROUTSTR_HUB_PROXY_PATH while leaving the server-side proxy path behavior unchanged.frontend/src/hooks/useRoutstrd.ts-459-470 (1)
459-470: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftThe browser calls the mint directly.
getMeltQuoteFeerunsfetchagainstmintUrlfrom the user's browser. Two consequences follow:
- The mint learns the user's IP address. Every other Routstr call in this module goes through the Hub, which hides the user's network location.
- The request depends on the mint allowing cross-origin requests. A mint without permissive CORS headers makes this call fail, which triggers the
return 0path described in the other comment on this file.Route the melt quote through the Hub, next to the other
/api/routstrdendpoints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useRoutstrd.ts` around lines 459 - 470, Update getMeltQuoteFee so the melt quote request is sent through the Hub’s /api/routstrd endpoint instead of fetching mintUrl directly. Preserve the existing POST payload, headers, abort signal, and response handling while matching the routing pattern used by the other Routstr calls.frontend/src/screens/internal-apps/Routstr.tsx-193-227 (1)
193-227: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard against creating a second app when the user goes Back.
The "Top Up" step has a Back button that returns to "Configure".
handleConfigurealways callscreateApp. A user who steps back and submits again creates a second isolated Routstr connection, with a second NWC registration inroutstrd. The first app remains, holding any sats already transferred.Skip creation when
appIdis already set, or disable the Back button after the app exists.🐛 Proposed fix
const handleConfigure = async (e: React.FormEvent) => { e.preventDefault(); + if (appId) { + // The wallet already exists (user stepped back); do not create a second one. + setStep("topup"); + return; + } setConfigureLoading(true);Also applies to: 546-549
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/screens/internal-apps/Routstr.tsx` around lines 193 - 227, Update handleConfigure to avoid calling createApp when appId is already set, preserving the existing app and proceeding without a second NWC registration; alternatively, disable the Top Up step’s Back button once the app exists.frontend/src/screens/settings/RoutstrApiKeys.tsx-52-54 (1)
52-54: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace
useNavigatewith link components.Three navigations are plain internal links with no async work before them: the "Hub backup" text button at lines 66-72, "View Connection" at lines 164-171, and "Go to Backup" at lines 194-200. The repository standard is
<Link/>so that middle-click, open-in-new-tab, and copy-link work.♻️ Proposed refactor
-import { useNavigate } from "react-router"; +import { Link } from "react-router"; +import { LinkButton } from "src/components/ui/custom/link-button";- <button - type="button" - onClick={() => navigate("/settings/backup")} - className="text-foreground underline underline-offset-2 hover:no-underline" - > - Hub backup - </button> + <Link + to="/settings/backup" + className="text-foreground underline underline-offset-2 hover:no-underline" + > + Hub backup + </Link>- <Button - size="sm" - variant="secondary" - onClick={() => handleViewConnection(app.id)} - > + <LinkButton to={`/apps/${app.id}`} size="sm" variant="secondary"> <ExternalLinkIcon className="h-3 w-3 mr-1" /> View Connection - </Button> + </LinkButton>Confirm the exact export path of
LinkButtonbefore applying.As per coding guidelines: "Avoid using useNavigate; use component where possible to ensure good browser UX".
Also applies to: 66-72, 163-172, 193-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/screens/settings/RoutstrApiKeys.tsx` around lines 52 - 54, Replace the useNavigate-based handleViewConnection navigation and the three synchronous internal navigation controls (“Hub backup”, “View Connection”, and “Go to Backup”) with the repository’s Link/LinkButton components. Preserve each destination URL and visible styling, and confirm the exact LinkButton export path before importing it; remove useNavigate and handleViewConnection once no imperative navigation remains.Source: Coding guidelines
frontend/src/components/connections/routstr/TopUpDialog.tsx-43-74 (1)
43-74: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe fallback check can report a false success.
Two paths lead to a wrong "Deposited" result:
- If
getRoutstrdKeyBalances()at line 47 throws,beforestays0. The catch block then testsafter >= 0 + numAmount. A wallet that already holds at leastnumAmountsats satisfies that test, so a failed deposit reports success andonTopUppersists a wrong balance.- Auto top-up runs server-side on a loop. A refill that lands inside the 2 second wait also satisfies the test.
Track whether the snapshot succeeded, and skip the fallback when it did not.
🐛 Proposed fix
setIsProcessing(true); // Snapshot balance to confirm success - let before = 0; + let before: number | null = null; try { const bal = await getRoutstrdKeyBalances(); - before = bal?.total ?? 0; + before = bal?.total ?? null; } catch { /* ignore */ } try { // Direct Hub-to-mint path — pay from this Routstr connection's wallet await fundFromHub(numAmount, appId); onTopUp(numAmount); onOpenChange(false); toast.success(`Deposited ${numAmount} sats`); } catch (error) { // If Hub path failed, check if balance increased anyway - try { - await new Promise((r) => setTimeout(r, 2000)); - const afterBal = await getRoutstrdKeyBalances(); - const after = afterBal?.total ?? 0; - if (after >= before + numAmount) { + try { + if (before === null) { + throw error; + } + await new Promise((r) => setTimeout(r, 2000)); + const afterBal = await getRoutstrdKeyBalances(); + const after = afterBal?.total ?? 0; + if (after >= before + numAmount) { onTopUp(numAmount);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/TopUpDialog.tsx` around lines 43 - 74, Track whether the initial balance snapshot in the top-up flow succeeded, and only perform the post-failure balance-increase fallback when it did. Update the logic around getRoutstrdKeyBalances and the fundFromHub catch block so a failed snapshot cannot produce a success from the zero default; preserve the existing success handling when a valid snapshot confirms the balance increase.frontend/src/components/connections/routstr/ApiKeySection.tsx-306-353 (1)
306-353: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
app.metadatain the dependency list can drive a repeating PATCH loop.
refreshLiveBalancedoes not readapp.metadata; it fetches fresh state from the server. The dependency list still includesapp.metadata, so the callback identity changes whenever the parent re-renders with a new metadata object. The effect at lines 318-353 depends onrefreshLiveBalance, so it re-runs, callsrefreshLiveBalance({persist: true}), which PATCHes metadata and callsonMetadataUpdate(). That re-fetch produces a newapp.metadataobject identity, which changes the callback identity again.The cycle stops only when
silentsuppressesonMetadataUpdate, which is the current default. Any future call withsilent: falseon mount, or a parent that revalidates metadata on its own, turns this into a request loop against/api/apps/{pubkey}.Remove
app.metadataandhasKeyfrom the list, since neither is read inside the callback, and drop the eslint suppression.🐛 Proposed fix
- // eslint-disable-next-line react-hooks/exhaustive-deps - [hasKey, app.appPubkey, app.metadata, onMetadataUpdate] + [app.id, app.appPubkey, onMetadataUpdate] );Confirm that
onMetadataUpdateis stable in the parent. If it is not, wrap it inuseCallbackthere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/ApiKeySection.tsx` around lines 306 - 353, Update the dependency list for the refreshLiveBalance callback to remove app.metadata and hasKey, since neither is read by the callback, and remove the eslint suppression. Ensure onMetadataUpdate is stable in its parent, wrapping it with useCallback if necessary, while preserving the existing effect behavior.frontend/src/hooks/useRoutstrd.ts-453-488 (1)
453-488: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA failed fee quote returns 0 and starts a melt that cannot succeed.
getMeltQuoteFeeswallows every error and returns0. InrefundFromHub,computeRefundSend(walletBal, 0)then equals the full wallet balance. The mint cannot melt the full balance, because the melt itself needs a fee reserve. The melt fails, the shrink loop retries withsend - 1up tosend - 4, and those also fail for any wallet where the fee exceeds 4 sats. The user sees a generic failure and no sats move.Distinguish "fee is zero" from "fee is unknown". Return
nullon failure and skip or abort the pass.🐛 Proposed fix
-async function getMeltQuoteFee( - invoice: string, - mintUrl: string -): Promise<number> { +async function getMeltQuoteFee( + invoice: string, + mintUrl: string +): Promise<number | null> {} catch { - // fall through — fee unknown, treat as 0 + // fall through — fee unknown } finally { clearTimeout(timer); } - return 0; + return null; }Then in
refundFromHub:const quoteInvoice = await createAppScopedInvoice(walletBal, appId); - fee = await getMeltQuoteFee(quoteInvoice, mintUrl); + const quotedFee = await getMeltQuoteFee(quoteInvoice, mintUrl); + if (quotedFee === null) { + throw new Error("Could not read the mint melt fee. Try again later."); + } + fee = quotedFee; lastQuotedBalance = walletBal; lastFee = fee;Preserve the
PartialRefundErrorbehaviour whentotalRefunded > 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useRoutstrd.ts` around lines 453 - 488, Update getMeltQuoteFee to return null when the request fails or the response lacks a valid fee_reserve, while retaining numeric zero as a valid fee. In refundFromHub, detect the null result and skip or abort that refund pass before computeRefundSend, while preserving the existing PartialRefundError behavior when totalRefunded > 0.frontend/src/components/connections/routstr/ApiKeySection.tsx-135-165 (1)
135-165: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the typed
request()helper and type the health response.Lines 137-141 call
fetchdirectly and then useres.json(), which produces an untyped value. The subsequent property access onhealth?.alarmsand the inline(a: { kind: string })cast are unchecked. The repository standard is the typedrequest()helper, which also handles base URL and error mapping consistently across platforms.♻️ Proposed refactor
+type HealthAlarm = { + kind: string; + rawDetails?: { + routstrd_healthy?: boolean; + cocod_healthy?: boolean; + last_error?: string; + }; +}; + (async () => { try { - const res = await fetch("/api/health"); - if (!res.ok) { - return; - } - const health = await res.json(); + const health = await request<{ alarms?: HealthAlarm[] }>("/api/health"); if (!cancelled && health?.alarms) { - const routstrdAlarm = health.alarms.find( - (a: { kind: string }) => a.kind === "routstrd_offline" - ); + const routstrdAlarm = health.alarms.find( + (a) => a.kind === "routstrd_offline" + );As per coding guidelines: "HTTP requests use the typed request() helper in frontend/src/utils/request.ts" and "Use strict TypeScript; no any types".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/ApiKeySection.tsx` around lines 135 - 165, Replace the direct fetch and untyped JSON handling in the health-check IIFE with the typed request() helper from the repository utility. Define or reuse a strict health-response type covering alarms and rawDetails, pass it to request(), and update the alarm lookup to use the typed alarm shape without the inline cast; preserve the existing cancellation and daemon-status behavior.Source: Coding guidelines
frontend/src/components/connections/routstr/CreateKeyDialog.tsx-75-104 (1)
75-104: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA funding failure leaves an orphaned daemon client.
createRoutstrdClientruns first. IffundFromHubthrows, the catch resets the step tosetupand never callsonKeyCreated. The daemon keeps the created client, but Hub stores noclientIdfor it. Each retry creates another orphaned client, andDeleteKeyDialogcan never reach the earlier ones.Report the created key to the parent before funding, or delete the client when funding fails.
🐛 Proposed fix: surface the key before funding
if (!apiKey) { throw new Error("Failed to create API key"); } + // The key exists on the daemon from here on. Report it immediately so a + // funding failure cannot orphan it. + setCreatedKey(apiKey); + onKeyCreated(apiKey, clientId); + // 2. Fund via Hub's LN from this app's wallet if (Number(amount) > 0) { setStep("paying"); await fundFromHub(Number(amount), connectionAppId); } - setCreatedKey(apiKey); setStep("done"); - onKeyCreated(apiKey, clientId); toast.success("Deposit & key created!");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/CreateKeyDialog.tsx` around lines 75 - 104, Update handleCreateKey so the successfully created API key and clientId are reported through onKeyCreated before fundFromHub runs, while preserving the existing funding and success flow. Ensure funding failures do not discard the created client reference, allowing the parent and DeleteKeyDialog to manage it.frontend/src/components/connections/routstr/DeleteKeyDialog.tsx-46-59 (1)
46-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA transient balance check failure makes the dialog unusable.
For a non-"not found" error the catch sets
balancetonull.canDeleteat line 102 then staysfalse, and the render branch at lines 116-119 shows "Checking wallet balance…" forever. The user gets no error text and no retry control. The message at line 134 states that deletion is allowed when the daemon is unreachable, but that branch renders only whenbalance !== null, so it never appears in this case.Add an explicit unreachable state with a retry action, and show it when
balance === null && daemonUnreachable.🐛 Proposed fix for the render branch
- {balance === null ? ( + {balance === null && daemonUnreachable ? ( + <div className="space-y-2"> + <p className="text-xs text-amber-600 dark:text-amber-400"> + Could not read the wallet balance. The daemon may be + restarting. + </p> + <Button variant="outline" size="sm" onClick={checkBalance}> + Retry + </Button> + </div> + ) : balance === null ? ( <div className="text-muted-foreground"> Checking wallet balance… </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/DeleteKeyDialog.tsx` around lines 46 - 59, Update the balance-check state and render logic in DeleteKeyDialog so a transient failure represented by balance === null and daemonUnreachable shows an explicit daemon-unreachable message with a retry action instead of the indefinite “Checking wallet balance…” state. Wire the retry action to rerun the existing balance check, while preserving the confirmed “not found” zero-balance behavior and normal balance rendering.frontend/src/components/connections/routstr/RefundDialog.tsx-105-117 (1)
105-117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a timeout to the third-party mint fetch.
This route calls
activeMintdirectly withfetch, but the project guideline says HTTP requests should use the typedrequest()helper. Also add an abort signal/timeout so a stalled third-party mint cannot keeploadingBalancestrue, freeze the “Calculating fees…” state, and keep fetching after the dialog closes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/RefundDialog.tsx` around lines 105 - 117, Replace the direct fetch in the RefundDialog fee-quote flow with the project’s typed request() helper, using the appropriate melt quote endpoint and payload. Add an AbortSignal timeout to bound the third-party request, and ensure the timeout is cleaned up or cancellation is respected when the dialog closes so loadingBalances and the “Calculating fees…” state cannot remain stuck.Source: Coding guidelines
service/routstrd.go-638-651 (1)
638-651: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not put the daemon response body into the error.
Line 648 embeds the raw
nwc/connectresponse body in the error. The request body carries the NWC connection string, which contains a secret key. Daemons commonly echo the request in error responses. That error is logged at line 553 and published to the event bus at line 559. A secret can then reach logs and analytics.Return the status code only, and log the truncated body at debug level if needed.
🔒 Proposed fix
if resp.StatusCode < 200 || resp.StatusCode >= 300 { - b, _ := io.ReadAll(io.LimitReader(resp.Body, 2048)) - return fmt.Errorf("nwc/connect status %d: %s", resp.StatusCode, string(b)) + // The request body carries the NWC secret; the daemon may echo it. + // Never include the response body in a returned or published error. + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 2048)) + return fmt.Errorf("nwc/connect failed with status %d", resp.StatusCode) }Based on coding guidelines: "Never log sensitive data such as seeds, macaroons, or tokens".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 638 - 651, Update RoutstrdService.reconnectNwc so non-2xx responses return an error containing only the HTTP status code; remove the response-body text from the returned error. If response details are needed, log the already truncated body only at debug level without exposing it through the error propagated to callers.Source: Coding guidelines
service/routstrd.go-865-885 (1)
865-885: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPerform the metadata read-modify-write inside a transaction.
writeAutoRefillConfigunmarshalsapp.Metadatafrom an app record thatfindRoutstrAppread earlier, then writes the wholemetadatacolumn back. The comment states that the same column carriesapiKey,clientId, andbalance. If the UI updates any of those fields between the read and this write, this write discards them. The app instance passed in can also be stale by seconds.Re-read the row inside a transaction and write in the same transaction.
🔧 Proposed fix
func (r *RoutstrdService) writeAutoRefillConfig(app *db.App, cfg *AutoRefillConfig) error { - var meta map[string]interface{} - if err := json.Unmarshal(app.Metadata, &meta); err != nil { - return fmt.Errorf("read app metadata: %w", err) - } - routstrMeta, _ := meta["routstr"].(map[string]interface{}) - if routstrMeta == nil { - routstrMeta = map[string]interface{}{} - } - routstrMeta["autoRefill"] = cfg - meta["routstr"] = routstrMeta - bytes, err := json.Marshal(meta) - if err != nil { - return err - } - if err := r.svc.GetDB().Model(&db.App{}).Where("id = ?", app.ID).Update("metadata", datatypes.JSON(bytes)).Error; err != nil { - return fmt.Errorf("write app metadata: %w", err) - } - app.Metadata = bytes - return nil + var updated []byte + err := r.svc.GetDB().Transaction(func(tx *gorm.DB) error { + var current db.App + if err := tx.Where("id = ?", app.ID).First(¤t).Error; err != nil { + return fmt.Errorf("reload app: %w", err) + } + var meta map[string]interface{} + if err := json.Unmarshal(current.Metadata, &meta); err != nil { + return fmt.Errorf("read app metadata: %w", err) + } + routstrMeta, _ := meta["routstr"].(map[string]interface{}) + if routstrMeta == nil { + routstrMeta = map[string]interface{}{} + } + routstrMeta["autoRefill"] = cfg + meta["routstr"] = routstrMeta + encoded, err := json.Marshal(meta) + if err != nil { + return err + } + if err := tx.Model(&db.App{}).Where("id = ?", app.ID).Update("metadata", datatypes.JSON(encoded)).Error; err != nil { + return fmt.Errorf("write app metadata: %w", err) + } + updated = encoded + return nil + }) + if err != nil { + return err + } + app.Metadata = updated + return nil }This also removes the local variable
bytes, which shadows the importedbytespackage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 865 - 885, Update writeAutoRefillConfig to perform the metadata read-modify-write in a database transaction: re-read the current app row by app.ID inside the transaction, unmarshal that fresh metadata, update only the routstr.autoRefill value, and persist the resulting metadata using the same transaction handle. Update the passed app’s Metadata after a successful commit, and rename the marshaled payload variable to avoid shadowing the bytes package.api/backup.go-92-152 (1)
92-152: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe daemon databases can be archived in a torn state.
db.Stop(api.db)stops only the Hub database. The comment at lines 96-98 states that the daemons keep running. The checkpoint at line 103 therefore only guarantees a flushed WAL at that instant. The archive copy happens later, at line 188. Between the two points,cocodandroutstrdcan write again. Two results follow:
- A new WAL is created that is never archived, so those writes are lost on restore.
coco.dbcan be copied mid-transaction, which yields a corrupt database rather than an older one.For a Cashu wallet, a torn
coco.dbmeans lost proofs.Use the SQLite online backup API, or run
VACUUM INTOa temporary file and archive that file. Both produce a consistent snapshot while writers are active. If you keep the current approach, archive the-waland-shmfiles alongside each.db.Also note that
filepath.Globnever returnsfilepath.ErrBadPatternfor these inputs, so the daemon files added at lines 138-152 skip theos.Staterror rather than reporting it. That part is fine.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/backup.go` around lines 92 - 152, Update the backup flow around checkpointSqliteDatabase and the daemon database entries so each running daemon database is archived from a consistent SQLite snapshot rather than copying the live .db after checkpointing. Use SQLite’s online backup API or VACUUM INTO a temporary snapshot, add the snapshot files to filesToArchive, and clean them up afterward while preserving existing config-file handling.http/http_service.go-208-215 (1)
208-215: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBoth reverse proxies lack timeouts, error handling, and rate limiting.
httputil.NewSingleHostReverseProxyuseshttp.DefaultTransportwhenTransportis nil. That transport has no overall response timeout. If the daemon accepts a connection and then stalls, the Hub request goroutine is held indefinitely. Enough stalled requests exhaust the server.
ErrorHandleris also nil, so a daemon that is down produces a bare 502 with the error written to the standard logger rather than the structured logger this repository uses.The
/routstr/v1/*proxy at line 230 needs the most care. It is unauthenticated, so any client on the network can open connections through it.+ proxyTransport := &http.Transport{ + DialContext: (&net.Dialer{Timeout: 5 * time.Second}).DialContext, + ResponseHeaderTimeout: 30 * time.Second, + } + proxyErrorHandler := func(w http.ResponseWriter, r *http.Request, err error) { + logger.Logger.WithError(err).WithField("path", r.URL.Path).Error("routstrd proxy request failed") + w.WriteHeader(http.StatusBadGateway) + } routstrdProxy := httputil.NewSingleHostReverseProxy(&url.URL{ Scheme: "http", Host: "localhost:8008", }) + routstrdProxy.Transport = proxyTransport + routstrdProxy.ErrorHandler = proxyErrorHandlerApply the same settings to
routstrOpenAIProxy, and add a rate limiter to the public route. Do not setResponseHeaderTimeoutso low that a slow first token on a streaming completion is cut off.As per coding guidelines: "Wrap errors with fmt.Errorf("context: %w", err) for debugging" and use "structured logging via logrus with contextual fields".
Also applies to: 225-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/http_service.go` around lines 208 - 215, Configure both routstrProxy and routstrOpenAIProxy with a transport that enforces an overall response timeout while leaving ResponseHeaderTimeout high enough for streaming completions. Add ErrorHandler implementations that wrap errors with context using fmt.Errorf and report them through structured logrus fields instead of the standard logger. Protect the unauthenticated /routstr/v1/* route with rate limiting, while preserving the existing authenticated route behavior.Source: Coding guidelines
api/api.go-205-234 (1)
205-234: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe
autoRefill.enabledprotection covers only one payload shape.The comment states that the generic PATCH must never change
routstr.autoRefill.enabled. The code only enforces this when the incoming metadata already containsroutstr.autoRefillas an object. Two cases defeat it:
- The request omits
routstrentirely. The persistedroutstrsubtree, includingenabled, is dropped on write.- The request sends
routstrwithoutautoRefill. The storedautoRefillblock is dropped.The blur-save race described in the comment still resurrects or clears the value in those cases.
The error handling has the same problem.
tx.Firstandjson.Unmarshalfailures are ignored througherr == nil. If the read fails, the client-suppliedenabledvalue is persisted.Restructure so the server-owned field is always restored from the database value, and so a read failure aborts the update.
♻️ Sketch of the restructured merge
- incomingMeta := *updateAppRequest.Metadata - if incomingRoutstr, ok := incomingMeta["routstr"].(map[string]interface{}); ok { - if incomingAR, ok := incomingRoutstr["autoRefill"].(map[string]interface{}); ok { - var currentApp db.App - if err := tx.First(¤tApp, userApp.ID).Error; err == nil { - var existingMeta Metadata - if currentApp.Metadata != nil { - if err := json.Unmarshal(currentApp.Metadata, &existingMeta); err == nil { - if existingRoutstr, ok := existingMeta["routstr"].(map[string]interface{}); ok { - if existingAR, ok := existingRoutstr["autoRefill"].(map[string]interface{}); ok { - if existingEnabled, ok := existingAR["enabled"]; ok { - incomingAR["enabled"] = existingEnabled - } - } - } - } - } - } - } - } + incomingMeta := *updateAppRequest.Metadata + var currentApp db.App + if err := tx.First(¤tApp, userApp.ID).Error; err != nil { + return fmt.Errorf("failed to load current app metadata: %w", err) + } + existingEnabled, hasExisting, err := readAutoRefillEnabled(currentApp.Metadata) + if err != nil { + return fmt.Errorf("failed to decode current app metadata: %w", err) + } + if hasExisting { + // Always restore the server-owned flag, whatever shape the request has. + setAutoRefillEnabled(incomingMeta, existingEnabled) + }
readAutoRefillEnabledandsetAutoRefillEnabledcreate the intermediateroutstrandautoRefillmaps when they are absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api.go` around lines 205 - 234, Restructure the metadata merge around the existing PATCH handler so routstr.autoRefill.enabled is always restored from the persisted database metadata, even when routstr or autoRefill is absent from the request. Use the merge helpers readAutoRefillEnabled and setAutoRefillEnabled (or equivalent localized logic) to create missing intermediate maps, and make failures from tx.First or json.Unmarshal abort the update instead of persisting the client value.api/models.go-593-595 (1)
593-595: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
appIdbefore creating an invoice.
CreateInvoicepassesappIddirectly toMakeInvoicewithout checking the referenced app. Reject unknown or non-isolated app IDs before saving the invoice so sats cannot be credited to a stale wallet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/models.go` around lines 593 - 595, Update CreateInvoice to validate the optional AppId against an existing isolated app before calling MakeInvoice or persisting the invoice. Reject unknown or non-isolated app IDs, while preserving the current behavior when AppId is unset.Source: Coding guidelines
scripts/patch-routstrd-dist.sh-62-62 (1)
62-62: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrite the patched bundle atomically.
open(path, "w")truncates the daemon bundle before the new content is written. If the write fails part way,dist/daemon/index.jsis left truncated androutstrdno longer starts. The verification block at line 67 then reports a failure, but the original bundle is already gone and only a reinstall recovers it.Write to a temporary file in the same directory and replace the target.
-open(path, "w", encoding="utf-8").write(src) -print("routstrd dist patch applied") +import os, tempfile + +directory = os.path.dirname(path) +fd, tmp_path = tempfile.mkstemp(dir=directory, suffix=".tmp") +with os.fdopen(fd, "w", encoding="utf-8") as handle: + handle.write(src) +os.replace(tmp_path, path) +print("routstrd dist patch applied")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/patch-routstrd-dist.sh` at line 62, Update the bundle-writing logic around the open(path, "w") call to write the patched source to a temporary file in the target’s directory, then atomically replace the original path only after the write succeeds. Preserve the existing verification flow and ensure temporary-file cleanup on failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9e06d8b-b5d0-4de6-b767-11b79eae68a5
⛔ Files ignored due to path filters (13)
docs/images/conn-autotopup.pngis excluded by!**/*.pngdocs/images/conn-keysection.pngis excluded by!**/*.pngdocs/images/dialog-delete.pngis excluded by!**/*.pngdocs/images/dialog-models.pngis excluded by!**/*.pngdocs/images/dialog-refund.pngis excluded by!**/*.pngdocs/images/dialog-topup.pngis excluded by!**/*.pngdocs/images/wizard-1-configure.pngis excluded by!**/*.pngdocs/images/wizard-2-topup.pngis excluded by!**/*.pngdocs/images/wizard-3-createkey.pngis excluded by!**/*.pngdocs/images/wizard-4-fundkey.pngis excluded by!**/*.pngdocs/images/wizard-5-done.pngis excluded by!**/*.pngfrontend/src/assets/suggested-apps/routstr.pngis excluded by!**/*.pngfrontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (66)
.gitignoreCHANGELOG.mdCONTRIBUTING.mdREADME.mdSECURITY.mdapi/api.goapi/backup.goapi/models.goapi/transactions.godeploy.shdocs/architecture.mddocs/backup-restore.mddocs/daemon-patches.mddocs/deploy.mddocs/development.mddocs/reference.mddocs/security.mddocs/troubleshooting.mddocs/upstream.mddocs/user-flow.mdfrontend/package.jsonfrontend/platform_specific/http/src/utils/request.tsfrontend/src/components/TransactionsList.tsxfrontend/src/components/connections/AppTransactionList.tsxfrontend/src/components/connections/AppUsage.tsxfrontend/src/components/connections/DisconnectApp.tsxfrontend/src/components/connections/SuggestedAppData.tsxfrontend/src/components/connections/routstr/ApiKeySection.tsxfrontend/src/components/connections/routstr/CreateKeyDialog.tsxfrontend/src/components/connections/routstr/DeleteKeyDialog.tsxfrontend/src/components/connections/routstr/ModelDetailPanel.tsxfrontend/src/components/connections/routstr/ModelPricingStrip.tsxfrontend/src/components/connections/routstr/ModelSelect.tsxfrontend/src/components/connections/routstr/ModelSelectUtils.tsfrontend/src/components/connections/routstr/RefundDialog.tsxfrontend/src/components/connections/routstr/RoutstrConnectionDetails.tsxfrontend/src/components/connections/routstr/TopUpDialog.tsxfrontend/src/components/connections/routstr/constants.tsfrontend/src/components/connections/routstr/refundLogic.test.tsfrontend/src/components/connections/routstr/refundLogic.tsfrontend/src/components/layouts/SettingsLayout.tsxfrontend/src/hooks/useRoutstrd.tsfrontend/src/lib/clipboard.tsfrontend/src/routes.tsxfrontend/src/screens/ai/AI.tsxfrontend/src/screens/apps/AppDetails.tsxfrontend/src/screens/internal-apps/Routstr.tsxfrontend/src/screens/settings/RoutstrApiKeys.tsxfrontend/vitest.config.tshttp/http_service.golnclient/cln/cln.goscripts/patch-routstrd-dist.shservice/models.goservice/routstrd.goservice/routstrd_apps_test.goservice/routstrd_config_test.goservice/routstrd_recovery_test.goservice/routstrd_refill_test.goservice/routstrd_unix.goservice/routstrd_validation_test.goservice/routstrd_windows.goservice/service.goservice/start.goservice/stop.gotests/mocks/Service.gowails/wails_handlers.go
| extractZipEntry := func(zipFile *zip.File) error { | ||
| fsFilePath := filepath.Join(workDir, "restore", filepath.FromSlash(zipFile.Name)) | ||
| zipName := filepath.FromSlash(zipFile.Name) | ||
|
|
||
| // Entries produced from files outside workDir (routstrd/cocod state | ||
| // under $HOME, archived as "../../.routstrd/...") must resolve | ||
| // against the directory the ".." segments point to relative to | ||
| // workDir. Derive it by walking up from workDir once per ".." | ||
| // segment (with $HOME as the practical anchor when workDir sits | ||
| // directly under it). Resolving under workDir/restore would land | ||
| // them in the wrong place (e.g. /root/hub/.cocod instead of | ||
| // /root/.cocod) and the daemons would never find them, leaving a | ||
| // restored instance with an empty Cashu wallet and no API keys. | ||
| var fsFilePath string | ||
| if strings.HasPrefix(zipName, ".."+string(filepath.Separator)) { | ||
| trimmed := zipName | ||
| upDir := workDir | ||
| for strings.HasPrefix(trimmed, ".."+string(filepath.Separator)) { | ||
| trimmed = trimmed[len(".."+string(filepath.Separator)):] | ||
| upDir = filepath.Dir(upDir) | ||
| } | ||
| fsFilePath = filepath.Join(upDir, trimmed) | ||
| } else { | ||
| fsFilePath = filepath.Join(workDir, "restore", zipName) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Arbitrary file write during restore (Zip Slip).
The new branch honors .. segments taken from the archive entry name. The loop removes one .. per iteration and calls filepath.Dir(upDir) each time. filepath.Dir("/") returns "/", so extra .. segments do not fail. They saturate at the filesystem root. An entry named ../../../../../../etc/cron.d/pwn therefore resolves to /etc/cron.d/pwn.
The previous code joined against workDir/restore, which filepath.Join cleaned and confined. That confinement is now removed.
POST /api/restore is registered in RegisterSharedRoutes without JWT (http/http_service.go line 100). The caller supplies both the archive and the password. A user who restores an archive obtained from another party grants that party arbitrary file write with the Hub process privileges. deploy.sh runs the Hub from /root/hub, so that is root on the reference deployment.
Do not derive the destination from the entry name. Map the small set of known external files explicitly, and reject every other .. entry.
🔒 Proposed fix
extractZipEntry := func(zipFile *zip.File) error {
zipName := filepath.FromSlash(zipFile.Name)
- var fsFilePath string
- if strings.HasPrefix(zipName, ".."+string(filepath.Separator)) {
- trimmed := zipName
- upDir := workDir
- for strings.HasPrefix(trimmed, ".."+string(filepath.Separator)) {
- trimmed = trimmed[len(".."+string(filepath.Separator)):]
- upDir = filepath.Dir(upDir)
- }
- fsFilePath = filepath.Join(upDir, trimmed)
- } else {
- fsFilePath = filepath.Join(workDir, "restore", zipName)
- }
+ var fsFilePath string
+ if strings.Contains(zipName, "..") {
+ // Only the known daemon state files may land outside workDir.
+ rel, ok := allowedExternalRestorePath(zipName)
+ if !ok {
+ return fmt.Errorf("refusing to extract entry outside the restore directory: %s", zipFile.Name)
+ }
+ fsFilePath = filepath.Join(homeDir, rel)
+ } else {
+ fsFilePath = filepath.Join(workDir, "restore", zipName)
+ }
+ // Defence in depth: confirm the cleaned path stays under an allowed root.
+ if !isUnderAnyRoot(fsFilePath, filepath.Join(workDir, "restore"), homeDir) {
+ return fmt.Errorf("refusing to extract entry to %s", fsFilePath)
+ }allowedExternalRestorePath matches only .cocod/coco.db, .cocod/config.json, .routstrd/routstr.db and .routstrd/config.json, which are the exact files CreateBackup adds at lines 134-152. Resolve homeDir with os.UserHomeDir().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/backup.go` around lines 256 - 279, Replace the parent-segment traversal
in extractZipEntry with an explicit allowlist for the known external restore
files: .cocod/coco.db, .cocod/config.json, .routstrd/routstr.db, and
.routstrd/config.json. Add or reuse allowedExternalRestorePath, resolve the user
home via os.UserHomeDir(), map only those entries to their fixed destinations,
and reject every other archive name containing ..; retain workDir/restore
handling for non-external entries.
| // Public OpenAI-compatible proxy for external tools (Hermes, OpenCode, etc.). | ||
| // Auth is the routstrd API key in Authorization — NOT Hub JWT. | ||
| // /routstr/v1/chat/completions → http://127.0.0.1:8008/v1/chat/completions | ||
| // | ||
| // IMPORTANT: only /routstr/v1/* is exposed. The daemon binds *:8008 with | ||
| // unauthenticated admin endpoints (/stop, /wallet, /clients, /refund, ...); | ||
| // a catch-all /routstr/* proxy would let anyone reach those through the | ||
| // public Hub port. | ||
| routstrOpenAIProxy := httputil.NewSingleHostReverseProxy(&url.URL{ | ||
| Scheme: "http", | ||
| Host: "127.0.0.1:8008", | ||
| }) | ||
| openAIProxyHandler := echo.WrapHandler(http.StripPrefix("/routstr", routstrOpenAIProxy)) | ||
| e.Any("/routstr/v1/*", openAIProxyHandler) | ||
| e.Any("/routstr/v1/", openAIProxyHandler) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Echo does not clean dot-segments before routing, for the pinned version.
rg -n 'labstack/echo' go.modRepository: getAlby/hub
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod echo deps =="
rg -n 'github.com/labstack/echo/v4' go.mod go.sum | head -20
echo
echo "== http_service.go outline =="
ast-grep outline http/http_service.go --match 'routstrOpenAIProxy' --view expanded || true
echo
echo "== Relevant http_service.go lines =="
cat -n http/http_service.go | sed -n '180,250p'
echo
echo "== Route matching docs/usages around Any() =="
rg -n 'e\.Any|server\.Router|AddRouter|DefaultRouter|Match|stripPrefix|StripPrefix' --glob '*.go' | head -80Repository: getAlby/hub
Length of output: 4894
🌐 Web query:
Echo v4 router matches raw URL path dot segments default router
💡 Result:
In Echo v4, the default router behavior regarding path segments and URL encoding is designed to prioritize raw, encoded path matching to ensure consistency and prevent security bypasses [1][2]. By default, the Echo router matches routes using the raw request path (req.URL.RawPath) rather than the decoded path (req.URL.Path) [1][3]. Because the router treats the raw path as the authoritative source for matching, encoded characters—such as an encoded slash (%2F)—are not treated as path separators [1][2]. For example, a request to /admin%2Fsecret.txt will be treated as a single segment and will not match a protected route pattern like /admin/* [1][2]. This design choice was solidified to address security vulnerabilities where discrepancies between the router's path matching (which might ignore encoded characters) and static file handlers (which might unescape them) allowed attackers to bypass path-based access controls [1][2]. Consequently, Echo's static file serving and middleware now explicitly reject or handle encoded path separators to maintain this consistency [2][4]. Key technical details include: 1. Path Matching: The router typically uses req.URL.RawPath for routing [1][3]. If this is empty or UseEscapedPathForRouting is configured otherwise, behavior may shift, but the default state preserves encoded characters during the matching process [1][2]. 2. Path Segments: Because encoded slashes are not unescaped before matching, they are considered part of the current path segment rather than delimiters that would traverse into sub-routes [1][2]. 3. Consistency: To prevent security issues (such as GHSA-vfp3-v2gw-7wfq), modern versions of Echo ensure that static handlers and middleware are aligned with the router's treatment of these segments, often by rejecting encoded separators entirely when serving files [2][4]. If you are encountering issues where routes are not matching as expected due to encoded segments, it is generally recommended to keep the router's raw-path matching active for security, rather than attempting to force decoding within the routing logic itself [2][4].
Citations:
- 1: GHSA-vfp3-v2gw-7wfq
- 2: fix(static): reject encoded path separator to prevent route-level auth bypass labstack/echo#3009
- 3: https://github.com/labstack/echo/blob/master/router.go
- 4: https://github.com/labstack/echo/blob/v4.15.4/CHANGELOG.md
🌐 Web query:
Go net/http ReverseProxy TargetReq method path dot segments Clean EscapedPath behavior
💡 Result:
In Go's net/http/httputil.ReverseProxy, request path manipulation is primarily governed by how the Director (or the newer Rewrite function) updates req.URL.Path and req.URL.RawPath [1][2][3]. When using NewSingleHostReverseProxy, the internal Director function uses a helper called joinURLPath to combine the target URL and the incoming request URL [1][2]. The joinURLPath function is specifically designed to handle path concatenation while preserving or correctly managing escaped characters [4]. Key behaviors regarding path processing include: 1. Escaped Path Handling: To avoid double-escaping or losing information (e.g., %2F), joinURLPath uses both Path and RawPath [4]. If the input URLs have RawPath set, the resulting RawPath is concatenated similarly to the Path [1][5]. This prevents the common issue where encoded characters are incorrectly decoded or re-encoded during the proxying process [6][4][7]. 2. Path Cleaning and Slashes: The ReverseProxy itself does not automatically perform path cleaning (like removing dot segments or extra slashes) in the same way a ServeMux might [8]. The joinURLPath logic focuses on joining segments; it ensures a single slash is used as a separator between the target base path and the request path by checking both paths' prefixes and suffixes [1][5]. 3. Director vs. Rewrite: The Director field is deprecated in favor of the Rewrite hook [9][3]. While NewSingleHostReverseProxy still uses Director for backwards compatibility, it is recommended that custom implementations use Rewrite to perform safer and more explicit request modifications [9][3]. Unlike Director, which modifies the request in place, the Rewrite hook provides a cleaner interface (ProxyRequest) that avoids common pitfalls associated with modifying headers and URLs [3]. 4. Method Preservation: The ReverseProxy preserves the HTTP method of the original incoming request when forwarding to the target [1]. The Director (and Rewrite) functions primarily target URL and header modification, not method transformation [1][2]. In summary, Go's ReverseProxy preserves the integrity of escaped characters by managing RawPath alongside Path, and relies on explicit logic in its joining functions rather than automatic, hidden path cleaning behaviors [1][8][4].
Citations:
- 1: https://github.com/golang/go/blob/go1.20.12/src/net/http/httputil/reverseproxy.go
- 2: https://github.com/golang/go/blob/go1.16.14/src/net/http/httputil/reverseproxy.go
- 3: net/http/httputil: replace Director with Rewrite golang/go#53002
- 4: https://git.jordan.im/go/commit/?h=go1.15.7&id=2d323f900df420a29de29cbab949eea08e3d1a61
- 5: https://go.dev/src/net/http/httputil/reverseproxy.go?s=2534:2609
- 6: net/http/httputil: NewSingleHostReverseProxy doesn't set RawPath when creating Director golang/go#41082
- 7: v2: Use r.URL.RawPath for rewrite caddyserver/caddy#3596
- 8: net/http: ServeMux forcibly cleans double forward slash in URLs even when behaving as a gateway golang/go#42244
- 9: net/http/httputil: deprecate ReverseProxy.Director golang/go#73161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Echo package files if available =="
go env GOPATH
for d in $(go env GOPATH)/pkg/mod/github.com/labstack/echo* 2>/dev/null; do
echo "--- $d ---"
[ -d "$d" ] || continue
rg -n "UseEscapedPathForRouting|UseRawPath|Match|Router|ServeHTTP|ServeHTTP(" "$d" | head -120
done
echo
echo "== Go stdlib httputil joinURLPath if available =="
go env GOROOT
python3 - <<'PY'
import os, re
root = os.environ.get("GOROOT")
candidates=[]
for path in [f"{root}/src/net/http/httputil/reverseproxy.go", f"{root}/src/net/http/reverseproxy.go"]:
if os.path.isfile(path):
candidates.append(path)
for path in candidates:
print(f"--- {path} ---")
lines=open(path).readlines()
for i,l in enumerate(lines,1):
if re.search(r"(func\s+joinURLPath|func\s+ProxyRequest|func\s+NewSingleHostReverseProxy|Clean\(|EscapedPath)", l):
start=max(1,i-40); end=min(len(lines),i+90)
for n in range(start,end+1):
print(f"{n:5d}\t{lines[n-1]}", end="")
PYRepository: getAlby/hub
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== GOPATH =="
go env GOPATH
echo "== GOROOT =="
go env GOROOT
echo
echo "== Echo package source if cached =="
for d in $(go env GOPATH)/pkg/mod/github.com/labstack/echo* 2>/dev/null; do
echo "--- $d ---"
[ -d "$d" ] || continue
rg -n "UseEscapedPathForRouting|UseRawPath|Match|UseHandler|Find|ServeHTTP|Match\\(" "$d" | head -160
done
echo
echo "== Go stdlib reverse proxy path handling if cached =="
python3 - <<'PY'
import os, re
root = os.environ.get("GOROOT")
for path in [f"{root}/src/net/http/httputil/reverseproxy.go", f"{root}/src/net/http/reverseproxy.go"]:
if os.path.isfile(path):
print(f"--- {path} ---")
lines = open(path).readlines()
for i,l in enumerate(lines,1):
if re.search(r"(func\s+joinURLPath|func\s+ProxyRequest|func\s+NewSingleHostReverseProxy|path\.Clean|Clean\(|EscapedPath|RawPath)", l):
start=max(1,i-50); end=min(len(lines),i+120)
for n in range(start,end+1):
print(f"{n:5d}\t{lines[n-1]}", end="")
PYRepository: getAlby/hub
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -u
echo "== GOPATH and module cache =="
go env GOPATH GO111MODULE
echo
for d in $(find $(go env GOPATH)/pkg/mod -maxdepth 2 -type d -path '*github.com/labstack/echo*/v4' 2>/dev/null | sort -u | head -20); do
echo "--- $d ---"
rg -n "UseEscapedPathForRouting|UseRawPath|Match|UseHandler|ServeHTTP|ServeHTTP\\(" "$d" | head -120 || true
done
echo
echo "== Go stdlib reverse proxy path handling =="
python3 - <<'PY'
import os, re
root = os.environ["GOROOT"]
candidates = [
os.path.join(root, "src/net/http/httputil/reverseproxy.go"),
os.path.join(root, "src/net/http/reverseproxy.go"),
]
for path in candidates:
if os.path.isfile(path):
print(f"--- {path} ---")
lines = open(path).readlines()
for i, l in enumerate(lines, 1):
if re.search(r"(func\s+joinURLPath|func\s+ProxyRequest|func\s+NewSingleHostReverseProxy|path\.Clean|Clean\(|EscapedPath|RawPath|stripPrefix)", l):
start = max(1, i-60)
end = min(len(lines), i+150)
for n in range(start, end+1):
print(f"{n:5d}\t{lines[n-1]}", end="")
PYRepository: getAlby/hub
Length of output: 50367
🌐 Web query:
Echo v4.15.2 router.go UseRawPath Match UseEscapedPathForRouting
💡 Result:
In Echo v4.15.2, the routing logic primarily utilizes req.URL.RawPath for path matching when configured to use the raw path, which is a common source of security-related behavior regarding URL-encoded characters [1]. Technical Context: 1. Routing Logic: In the router's matching logic, the framework checks whether to use the raw, encoded path or the decoded path [1][2]. When useEscapedPathForRouting is false (the default in versions including v4.15.2), the router uses req.URL.RawPath for matching if it is available [1]. This means encoded characters like %2F are preserved during the routing process, causing them not to match against literal segments in route definitions [1]. 2. Security Implication: This behavior created a vulnerability (GHSA-vfp3-v2gw-7wfq) where the router would fail to match a protected route (e.g., /admin/*) if the path was encoded as /admin%2Fsecret.txt [1]. Because the router treats this as a different path, it bypasses route-level authorization middleware, while downstream handlers (like those serving static files) might later unescape the path, exposing sensitive files [1]. 3. Configuration: While some router implementations (or later versions/forks) explicitly expose configuration fields like UseEscapedPathForRouting in a RouterConfig struct [3], in the standard v4.x Echo codebase, this was often handled internally [1]. 4. Resolution in Later Versions: To resolve the inconsistency between how the router and other handlers (like Static file servers) interpret paths, newer versions (e.g., v4.15.4+) changed how paths are handled by default, moving toward consistent (typically unescaped) path interpretation and providing opt-in flags like EnablePathUnescapingStaticFiles for specific use cases [4][5]. For users of v4.15.2, it is highly recommended to upgrade to a version where these path matching inconsistencies are resolved, or to ensure that middleware is applied correctly to cover all potential path variations [4].
Citations:
- 1: GHSA-vfp3-v2gw-7wfq
- 2: https://github.com/labstack/echo/blob/v5.0.0/router.go
- 3: https://github.com/zxysilent/echo/blob/ba104908b9f6545d0b9d1a6695663c45cf952957/router.go
- 4: https://github.com/labstack/echo/releases/tag/v4.15.4
- 5: https://github.com/labstack/echo/blob/v4/echo.go
🏁 Script executed:
#!/bin/bash
set -u
MODCACHE="$GOPATH/pkg/mod"
if [ -n "$(go env GOPATH)" ]; then
MODCACHE="$(go env GOPATH)/pkg/mod"
fi
for d in $(find "$MODCACHE" -maxdepth 2 -type d -path '*github.com/labstack/echo*/v4' 2>/dev/null | sort -u | head -10); do
echo "--- $d ---"
if [ -f "$d/echo.go" ]; then
rg -n "UseEscapedPathForRouting|Escaped|RawPath|Disable..." "$d/echo.go" -A 5 -B 5 || true
fi
if [ -f "$d/router.go" ]; then
rg -n "func Match|func Find|UseRawPath|EscapedPath|UseEscapedPath" "$d/router.go" -A 8 -B 8 || true
fi
doneRepository: getAlby/hub
Length of output: 149
Normalize and validate the forwarded path before proxying.
/routstr/v1/* is only protected by Echo’s /routstr/v1/* route, while http.StripPrefix("/routstr", ...) forwards /v1/../stop as-is and httputil.ReverseProxy does not clean it. Reconstructing the RawPath before ReverseProxy may leave .. segments; reject paths that change after path.Clean or do not start with /routstr/v1/, using the escaped path as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/http_service.go` around lines 217 - 231, Update openAIProxyHandler to
validate and normalize the incoming escaped path before forwarding: reject
requests whose path changes after path.Clean or whose cleaned escaped path does
not remain under /routstr/v1/. Apply the validation to both Path and RawPath,
preserving the existing proxy behavior only for safe paths and preventing
traversal into daemon admin endpoints.
|
Addressing the CodeRabbit review — with full transparency: The original PR diff was wider than intended (my error: the branch was initially created on top of a fork of this repo that carries unrelated WIP). I have rebased the fix onto a clean The two CodeRabbit findings (both 🔴 Critical) targeted code that was mistakenly included in that polluted diff — they live in a different fork (
For the record: this PR's actual scope (CLN keysend preimage) is unaffected by those findings, and its upstream counterpart code ( |
|
Hi @welliv , sorry but this is not a fix. |
9208a07 to
40bde7e
Compare
|
You're right — thank you for the pushback. Silently ignoring the caller's preimage produced a transaction whose payment hash never settled on the network: the hub derives I've rewritten the fix (now at
Verified live on a CLN node: after The same fix ships for the Greenlight backend in my fork (identical CLN-family behavior). |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transactions/transactions_service.go (1)
1723-1723: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnsigned subtraction can underflow and suppress the budget warning.
budgetUsageSatanddbTransaction.AmountMsat/1000are bothuint64. If the transaction amount exceeds the current budget usage,budgetUsageSat-dbTransaction.AmountMsat/1000wraps to a very large value. The< warningUsagetest then fails and no warning is published.🐛 Proposed fix
- if budgetUsageSat >= warningUsage && budgetUsageSat-dbTransaction.AmountMsat/1000 < warningUsage { + transactionSat := dbTransaction.AmountMsat / 1000 + previousUsageSat := uint64(0) + if budgetUsageSat > transactionSat { + previousUsageSat = budgetUsageSat - transactionSat + } + if budgetUsageSat >= warningUsage && previousUsageSat < warningUsage {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transactions/transactions_service.go` at line 1723, Update the budget warning condition in the transaction handling flow to avoid unsigned underflow when calculating prior usage. Compare the transaction amount against budgetUsageSat before subtracting, or use an equivalent underflow-safe calculation, so warnings are still published when the transaction amount exceeds current usage.lnclient/ldk/ldk.go (1)
316-321: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid destroying the LDK node while
NextEventAsyncis parked.
Destroy()is called without ensuring the goroutine that parked inNextEventAsynchas returned first, so a final event can wake that goroutine and invoke the destroyed node objects. Stop the node before destroying it and keep the event goroutine away from the destroyed node.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lnclient/ldk/ldk.go` around lines 316 - 321, Update the shutdown flow around the NextEventAsync event goroutine so the node is stopped before Destroy() is called, and wait for the parked goroutine to return before destroying the node or allowing it to access node objects. Preserve the existing final-event handling while ensuring no event processing can occur after destruction.
🧹 Nitpick comments (1)
transactions/keysend_test.go (1)
119-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert against the persisted row, not only the returned struct.
transactionis the in-memory model that GORM updated in place. The assertions pass even if thepayment_hashwrite to the database is skipped or fails. Re-read the row by ID to make this a true regression test for the persisted values.Lines 129-133 also re-derive the hash from a value that line 121 already proved equals
realPreimage, so that block adds no coverage.♻️ Proposed change
// keysend construction: the payment hash is sha256(preimage) preImageBytes, err := hex.DecodeString(realPreimage) require.NoError(t, err) expectedHash := sha256.Sum256(preImageBytes) assert.Equal(t, hex.EncodeToString(expectedHash[:]), transaction.PaymentHash) - // and sha256(recorded preimage) must equal the recorded payment hash - recordedPreimageBytes, err := hex.DecodeString(*transaction.Preimage) - require.NoError(t, err) - recordedHash := sha256.Sum256(recordedPreimageBytes) - assert.Equal(t, transaction.PaymentHash, hex.EncodeToString(recordedHash[:])) + // the persisted row must carry the same backend values + persistedTransaction := db.Transaction{} + require.NoError(t, svc.DB.First(&persistedTransaction, transaction.ID).Error) + require.NotNil(t, persistedTransaction.Preimage) + assert.Equal(t, realPreimage, *persistedTransaction.Preimage) + assert.Equal(t, hex.EncodeToString(expectedHash[:]), persistedTransaction.PaymentHash) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transactions/keysend_test.go` around lines 119 - 133, Update the test around the existing transaction assertions to reload the persisted row by ID from the database, then assert its Preimage and PaymentHash values. Replace the redundant recorded-preimage hash block, while retaining checks that the persisted preimage matches realPreimage and its payment hash equals sha256(realPreimage).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lnclient/models.go`:
- Around line 200-206: Update LNDService.SendKeysend to populate
PayKeysendResponse.Preimage and PaymentHash from the successful keysend
response, preserving the actual network transaction values for callers and
transaction updates. Ensure all successful keysend paths return these fields
rather than relying on their implicit empty values.
In `@tests/mock_ln_client.go`:
- Around line 129-136: Update the preimage selection logic in the mock keysend
response so SendKeysendPreimage only overrides the caller-provided preimage when
it is non-empty. Preserve the documented fallback to the caller preimage when
the field is empty, and return the corresponding real payment hash so the
LDK-style settlement path is exercised.
In `@transactions/transactions_service.go`:
- Around line 596-599: The debug log in the keysend preimage recording path must
not expose payment preimages. Update the log fields in the surrounding
transaction service flow to record the corresponding payment hashes instead,
preserving the existing debug message and comparison context.
- Around line 595-619: In the actual-preimage reconciliation flow, ensure hex
decoding failures for preimage-derived payment hashes are returned instead of
leaving paymentHash empty. Replace the struct-based Updates call with a
map-based update that explicitly persists both payment_hash and preimage, and
return any decode or database error so execution cannot continue to
markTransactionSettled when persistence fails.
- Around line 601-615: Update the keysend settlement flow around
payKeysendResponse and dbTransaction so the initial transaction row is
constructed with the backend PaymentHash and Preimage returned by KeySend,
rather than first using the caller-derived hash and rewriting it afterward. Move
the keysend transaction creation or row data preparation into the same
settlement transaction, retain the SHA-256 fallback only when the backend hash
is absent, and remove the separate PaymentHash/Preimage rewrite that can create
duplicate settled transactions.
---
Outside diff comments:
In `@lnclient/ldk/ldk.go`:
- Around line 316-321: Update the shutdown flow around the NextEventAsync event
goroutine so the node is stopped before Destroy() is called, and wait for the
parked goroutine to return before destroying the node or allowing it to access
node objects. Preserve the existing final-event handling while ensuring no event
processing can occur after destruction.
In `@transactions/transactions_service.go`:
- Line 1723: Update the budget warning condition in the transaction handling
flow to avoid unsigned underflow when calculating prior usage. Compare the
transaction amount against budgetUsageSat before subtracting, or use an
equivalent underflow-safe calculation, so warnings are still published when the
transaction amount exceeds current usage.
---
Nitpick comments:
In `@transactions/keysend_test.go`:
- Around line 119-133: Update the test around the existing transaction
assertions to reload the persisted row by ID from the database, then assert its
Preimage and PaymentHash values. Replace the redundant recorded-preimage hash
block, while retaining checks that the persisted preimage matches realPreimage
and its payment hash equals sha256(realPreimage).
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efff9543-08e5-4ae6-b375-bd496bcbdc4a
📒 Files selected for processing (6)
lnclient/cln/cln.golnclient/ldk/ldk.golnclient/models.gotests/mock_ln_client.gotransactions/keysend_test.gotransactions/transactions_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- lnclient/cln/cln.go
|
Addressed all CodeRabbit comments from the 07:53 review (now at In-diff (all fixed):
Declined with reasoning:
Stale (from the original wider diff, files no longer in this PR):
|
|
This is not the right approach - only CLN code should be modified - to properly use the preimage if passed in. The last conclusion was that this was not possible. |
CLN's keysend RPC derives its own preimage server-side and cln-grpc has no override field. Rejecting non-empty preimages blocks NIP-47 pay_keysend entirely. With this fix, CLN ignores the caller preimage (like LDK/LND already do upstream — PayKeysendResponse carries only FeeMsat) and lets the node choose. The recorded preimage in the transactions table is the caller-derived one, not what the network used — this gap exists for all backends and should be fixed separately in the response contract.
659717e to
4c04dee
Compare
|
@rolznz — done. One file, four lines deleted. CLN-only. The node ignores the caller preimage (cln-grpc has no override field) and returns its own in the response. This PR just stops rejecting it. The recorded preimage won’t match what settled on the network — but that’s the same gap LDK and LND already have (PayKeysendResponse carries only FeeMsat upstream). Fixing that across all backends is a separate PR. |
The workflow has no permissions block, so GITHUB_TOKEN inherits the
repo's read-only default and cannot create the ghcr.io package
('denied: installation not allowed to Create organization package').
Grant contents: read + packages: write per GitHub's documented pattern
for publishing packages via GITHUB_TOKEN.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/build-docker.yaml (1)
4-6: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope
packages: writeto the publish step.This workflow has one
buildjob that both runs local tests and callsmr-smithers-excellent/docker-build-push@v6withsecrets.GITHUB_TOKEN. Since only the Docker push step needs GHCR write access, movepackages: writeto a job/steps scoped level so the test andactions/setup-go@v5steps do not inherit it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-docker.yaml around lines 4 - 6, Move packages: write from the workflow-level permissions into the narrowest job or step scope covering the Docker publish action, while retaining contents: read globally as needed. Ensure the build, test, and actions/setup-go@v5 steps do not inherit package write access, and the docker-build-push invocation still has the required GHCR permission.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/build-docker.yaml:
- Around line 4-6: Move packages: write from the workflow-level permissions into
the narrowest job or step scope covering the Docker publish action, while
retaining contents: read globally as needed. Ensure the build, test, and
actions/setup-go@v5 steps do not inherit package write access, and the
docker-build-push invocation still has the required GHCR permission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 757d5328-b40c-4222-8fee-189c907036f3
📒 Files selected for processing (1)
.github/workflows/build-docker.yaml
|
Closing — shallow/un-proven. Need to verify thoroughly and locally before submitting. Don't want to waste maintainer time. Will reopen from the correct fork once locally validated. |
2 participants
Summary
NIP-47
pay_keysendis broken on the CLN backend: every outgoing keysend fails withpreimage not supported for keysends.Root cause:
transactions/transactions_service.goSendKeysendalways synthesizes a preimage when none is provided (for transaction accounting), and passes it to the backend. The CLN backend (lnclient/cln/cln.go) rejects any non-empty preimage — so the transactions service's generated preimage makes every keysend fail before it even reaches the node.CLN's
keysendderives its own preimage server-side, and the cln-grpcKeysendRequesthas no field to override it. The fix accepts and ignores the caller-supplied preimage (with a debug log), matching how LDK/LND behave (they accept the preimage).Evidence
Verified end-to-end on a live regtest node (CLN backend → hub):
pay_keysendsettles: peer receives the keysend, hub recordsoutgoing/settledwith a preimage{"code":"INTERNAL","message":"preimage not supported for keysends"}{"result":{"preimage":"...","fees_paid":0},"result_type":"pay_keysend"}Notes
The same bug existed in a downstream Greenlight backend derived from this code and was fixed identically there (verified live).
Summary by CodeRabbit
Bug Fixes
Chores