Feat: one click connect via auth command#14
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant CLI as CLI
participant Browser as Wallet (Browser)
participant Relay as Relay
participant NWC as NWC Client
User->>CLI: auth https://hub.example.com --app-name MyApp --wallet-name alice
CLI->>CLI: generate keypair, write pending secret (~/.alby-cli/connection-secret-alice.pending)
CLI->>Browser: open approval URL (includes client pubkey)
Browser->>User: show approval UI
User->>Browser: approve
Browser->>Relay: publish NIP-47 info event (kind 13194, p:clientPubkey)
Relay-->>NWC: deliver info event
User->>CLI: auth --complete --relay-url wss://relay.getalby.com/v1 --wallet-name alice
CLI->>NWC: subscribe (kinds [13194], p:clientPubkey)
Relay-->>NWC: forward wallet's info event
NWC-->>CLI: wallet pubkey & relay info
CLI->>CLI: construct NWC URL, save connection-secret (0600), remove pending file
CLI-->>User: print connection complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rolznz
changed the title
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 8-10: The package.json bin mapping removed the historical
executable name "alby-cli", breaking backward compatibility; restore a
backward-compatible alias by adding "alby-cli" alongside "cli" in the "bin"
object so both "cli" and "alby-cli" point to "build/index.js" (update the "bin"
entry in package.json to include "alby-cli": "build/index.js" while keeping
"cli": "build/index.js").
In `@src/commands/auth.ts`:
- Around line 110-119: The 5s timeout in the approval wait timer is too short
for human wallet confirmations; update the timeout in the timer created in
src/commands/auth.ts (the const timer = setTimeout(...) block that uses settled,
unsub, and reject) to a larger value (e.g., 60000 ms or 120000 ms) or replace
the hardcoded 5000 with a named constant (e.g., APPROVAL_TIMEOUT_MS) or
config/env variable so it’s easier to tune; ensure the new timeout value is used
in the setTimeout call and that behavior still unsubscribes and rejects with the
same message if it elapses.
- Around line 18-30: The filename construction in getConnectionSecretPath and
getPendingConnectionSecretPath currently interpolates the unvalidated name
parameter and is vulnerable to path traversal; fix by validating/sanitizing the
incoming wallet name: either reject names containing path separators or
traversal tokens (e.g. '/', '\', '..') and only allow a safe whitelist like
/^[A-Za-z0-9._-]+$/, or normalize to a safe basename before interpolation;
update both functions (getConnectionSecretPath and
getPendingConnectionSecretPath) to perform this validation/sanitization and
throw or return an error for invalid names so secrets cannot escape the
~/.alby-cli directory.
In `@src/commands/connect.ts`:
- Around line 9-13: getConnectionSecretPath currently interpolates raw wallet
names into filenames, allowing path traversal; validate or sanitize the name
before composing the path in getConnectionSecretPath: reject or normalize names
that contain path separators ('/', '\\') or parent-segment sequences ('..'), or
enforce a strict allowlist (e.g., /^[A-Za-z0-9._-]+$/) and throw a clear error
if validation fails, then continue to generate filename
(`connection-secret-${name}.key`) and return join(homedir(), ".alby-cli",
filename); reference getConnectionSecretPath, filename, join, and homedir when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecdc6411-9691-4777-8ac4-8016b409b396
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
README.mdpackage.jsonsrc/commands/auth.tssrc/commands/connect.tssrc/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/auth.ts (1)
195-200: Consider including--relay-urlin the printed complete command if non-default.If a user needs a custom relay URL for step 2 (e.g., self-hosted relay), they must remember to pass it manually. The printed command could include the relay-url to improve the experience.
Suggested enhancement
const completeCmd = `npx `@getalby/cli` auth --complete` + (options.walletName - ? ` --wallet-name ${options.walletName}` - : ""); + ? ` --wallet-name ${options.walletName}` + : "") + + (options.relayUrl !== "wss://relay.getalby.com/v1" + ? ` --relay-url ${options.relayUrl}` + : "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/auth.ts` around lines 195 - 200, The printed one-liner built in completeCmd should include the relay URL when a non-default relay is used: detect the relay option (e.g., options.relayUrl or the variable that holds the chosen relay) and append ` --relay-url <relay>` to the template only when it is set/non-default; update the template construction around completeCmd (and any usage of options.walletName) so the printed command reflects both --wallet-name and --relay-url flags when applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/auth.ts`:
- Around line 75-78: The --wallet-name option in auth/connect saves named files
but getClient() (in src/utils.ts) only reads --connection-secret, NWC_URL, or
the hardcoded ~/.alby-cli/connection-secret.key, so named connections are never
loaded; fix by either removing the .option("--wallet-name") from the auth and
connect commands or by implementing support in getClient(): add an optional
walletName parameter (and/or read a global CLI option or env var like
WALLET_NAME), then have getClient() check for a named file path
~/.alby-cli/connection-secret-{walletName}.key before falling back to the
default, and wire the global --wallet-name option into the CLI entrypoint so all
commands can pass the wallet name into getClient().
- Around line 130-151: The subscribe call assigns unsub asynchronously via
.then(), creating a race where the timeout can fire before unsub is set; fix by
awaiting nwaClient.subscribe(...) to get the returned unsub before you start the
timeout/allow timeout to trigger, i.e. call const { unsub: u } = await
nwaClient.subscribe({ onSuccess: async (nwcClient) => { ... } }); unsub = u;
then start the timer and use settled/timer/unsub consistently; ensure both the
onSuccess path (testAndLogConnection, saveConnectionSecret, rmSync, resolve) and
the timeout/cleanup path call unsub() and clearTimeout(timer) to guarantee
subscription cleanup and avoid the race.
---
Nitpick comments:
In `@src/commands/auth.ts`:
- Around line 195-200: The printed one-liner built in completeCmd should include
the relay URL when a non-default relay is used: detect the relay option (e.g.,
options.relayUrl or the variable that holds the chosen relay) and append `
--relay-url <relay>` to the template only when it is set/non-default; update the
template construction around completeCmd (and any usage of options.walletName)
so the printed command reflects both --wallet-name and --relay-url flags when
applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
reneaaron
reviewed
Apr 2, 2026
Member
There was a problem hiding this comment.
cACK
Nice 🔥 can't wait to test it!
Support for multiple named wallet connections so you can manage multiple wallets independently.
Would you want to be able to manage multiple ones? (also for other commands?)
Added a new interactive wallet auth flow: generate an approval URL, approve in your wallet, then complete setup from the CLI.
What does then complete setup from the CLI exactly mean? How would the typical process look like when onboarding from an agent session?
Member
Author
true - the --wallet-name could apply to all commands. I'll see if I can update it.
after the user confirms the connection, the wallet connection pubkey will be made available. The "complete" command listens for that on the relay and then saves the full connection secret to the file. This is currently explicit, but doesn't actually need to be. It could run automatically next time any command is run (if there is a pending connection for that wallet name, then try to connect). This could be less confusing for the AI and human. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/commands/auth.ts (3)
125-145:⚠️ Potential issue | 🟠 MajorStabilize subscription lifecycle to prevent hangs/leaks.
Lines 125-144 assign
unsubasynchronously; timeout can fire before assignment. Also, if Line 133+ throws afterclearTimeout, the outer Promise may never settle.Suggested fix
await new Promise<void>((resolve, reject) => { let settled = false; + let unsub: (() => void) = () => {}; const timer = setTimeout(() => { if (settled) return; settled = true; - unsub?.(); + unsub(); reject( new Error( "Timed out waiting for wallet approval. Run `npx `@getalby/cli` auth --complete` to try again.", ), ); - }, 5000); - - let unsub: (() => void) | undefined; + }, APPROVAL_TIMEOUT_MS); - nwaClient - .subscribe({ + void nwaClient + .subscribe({ onSuccess: async (nwcClient) => { if (settled) return; - settled = true; - clearTimeout(timer); - unsub?.(); - - await testAndLogConnection(nwcClient); - saveConnectionSecret( - connectionSecretPath, - nwcClient.getNostrWalletConnectUrl(), - ); - rmSync(pendingSecretPath); - resolve(); + try { + settled = true; + clearTimeout(timer); + unsub(); + await testAndLogConnection(nwcClient); + saveConnectionSecret( + connectionSecretPath, + nwcClient.getNostrWalletConnectUrl(), + ); + rmSync(pendingSecretPath); + resolve(); + } catch (err) { + reject(err); + } }, }) .then(({ unsub: u }) => { - unsub = u; + if (settled) { + u(); + return; + } + unsub = u; + }) + .catch((err) => { + if (settled) return; + settled = true; + clearTimeout(timer); + reject(err); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/auth.ts` around lines 125 - 145, The subscription lifecycle is fragile: unsub is assigned only in the then callback of nwaClient.subscribe so the timeout may fire before unsub is set, and exceptions after clearTimeout can leave the outer Promise unsettled; fix by ensuring unsub is captured synchronously from nwaClient.subscribe (assign the returned unsub before awaiting any async work), and wrap the onSuccess handler body (calls to testAndLogConnection, saveConnectionSecret, rmSync) in a try/catch/finally that sets settled, clears timer, calls unsub if defined, and resolves or rejects the outer promise accordingly; update references around nwaClient.subscribe, unsub, timer, settled, testAndLogConnection, saveConnectionSecret, rmSync, resolve/reject to ensure cleanup always runs.
18-23:⚠️ Potential issue | 🟠 MajorHarden pending-secret path creation for
--wallet-name.Line 20 builds the filename from unsanitized user input, so traversal tokens can escape
~/.alby-cli.Suggested fix
+import { getConnectionSecretPath, handleError, normalizeWalletName } from "../utils.js"; + function getPendingConnectionSecretPath(name?: string) { - const filename = name - ? `pending-connection-secret-${name}.key` + const safeName = normalizeWalletName(name); + const filename = safeName + ? `pending-connection-secret-${safeName}.key` : "pending-connection-secret.key"; return join(homedir(), ".alby-cli", filename); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/auth.ts` around lines 18 - 23, The filename in getPendingConnectionSecretPath is built from unsanitized user input (name) which allows path traversal; sanitize or normalize name before constructing filename: strip any path separators (use path.basename or explicitly reject '/' and '\\'), restrict to a safe character set (e.g., allow only [A-Za-z0-9_.-] and replace others with '-' or fallback to a hash of the input), then build the filename variable (pending-connection-secret-${safeName}.key) and join with homedir() so no user input can escape the ~/.alby-cli directory.
112-121:⚠️ Potential issue | 🟠 MajorIncrease approval timeout to a human-realistic window.
Line 121 uses
5000ms, which is too short for wallet approval and will fail normal flows.Suggested fix
+const APPROVAL_TIMEOUT_MS = 180_000; ... - }, 5000); + }, APPROVAL_TIMEOUT_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/auth.ts` around lines 112 - 121, The timeout for waiting for wallet approval in the auth flow is too short (currently 5000ms) and should be increased to a human-realistic window; update the setTimeout in the block that defines timer (the callback that checks settled, calls unsub?.(), and reject(...)) to use a longer delay such as 60000ms (or another agreed value like 120000ms) so users have sufficient time to approve in their wallet before the promise is rejected.src/utils.ts (1)
7-12:⚠️ Potential issue | 🟠 MajorValidate
walletNamebefore composing secret file paths.Line 9 interpolates raw input into a filename. Values like
../xor path separators can escape~/.alby-cliand read/write unintended files.Suggested hardening
+export function normalizeWalletName(name?: string): string | undefined { + const trimmed = name?.trim(); + if (!trimmed) return undefined; + if (!/^[A-Za-z0-9._-]+$/.test(trimmed)) { + throw new Error( + "Invalid wallet name. Use only letters, numbers, dot, underscore, and hyphen.", + ); + } + return trimmed; +} + export function getConnectionSecretPath(name?: string) { - const filename = name - ? `connection-secret-${name}.key` + const safeName = normalizeWalletName(name); + const filename = safeName + ? `connection-secret-${safeName}.key` : "connection-secret.key"; return join(homedir(), ".alby-cli", filename); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 7 - 12, The getConnectionSecretPath function currently interpolates raw name into a filename allowing path traversal; validate or sanitize the walletName before composing the path by rejecting or normalizing unsafe values: ensure the input contains only allowed characters (e.g. letters, numbers, hyphen, underscore) and does not include path separators or "..", or else throw an error or fall back to a default name; alternatively, use path.basename on the provided name to strip any directory components before building the filename so join(homedir(), ".alby-cli", filename) cannot escape the intended folder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/commands/auth.ts`:
- Around line 125-145: The subscription lifecycle is fragile: unsub is assigned
only in the then callback of nwaClient.subscribe so the timeout may fire before
unsub is set, and exceptions after clearTimeout can leave the outer Promise
unsettled; fix by ensuring unsub is captured synchronously from
nwaClient.subscribe (assign the returned unsub before awaiting any async work),
and wrap the onSuccess handler body (calls to testAndLogConnection,
saveConnectionSecret, rmSync) in a try/catch/finally that sets settled, clears
timer, calls unsub if defined, and resolves or rejects the outer promise
accordingly; update references around nwaClient.subscribe, unsub, timer,
settled, testAndLogConnection, saveConnectionSecret, rmSync, resolve/reject to
ensure cleanup always runs.
- Around line 18-23: The filename in getPendingConnectionSecretPath is built
from unsanitized user input (name) which allows path traversal; sanitize or
normalize name before constructing filename: strip any path separators (use
path.basename or explicitly reject '/' and '\\'), restrict to a safe character
set (e.g., allow only [A-Za-z0-9_.-] and replace others with '-' or fallback to
a hash of the input), then build the filename variable
(pending-connection-secret-${safeName}.key) and join with homedir() so no user
input can escape the ~/.alby-cli directory.
- Around line 112-121: The timeout for waiting for wallet approval in the auth
flow is too short (currently 5000ms) and should be increased to a
human-realistic window; update the setTimeout in the block that defines timer
(the callback that checks settled, calls unsub?.(), and reject(...)) to use a
longer delay such as 60000ms (or another agreed value like 120000ms) so users
have sufficient time to approve in their wallet before the promise is rejected.
In `@src/utils.ts`:
- Around line 7-12: The getConnectionSecretPath function currently interpolates
raw name into a filename allowing path traversal; validate or sanitize the
walletName before composing the path by rejecting or normalizing unsafe values:
ensure the input contains only allowed characters (e.g. letters, numbers,
hyphen, underscore) and does not include path separators or "..", or else throw
an error or fall back to a default name; alternatively, use path.basename on the
provided name to strip any directory components before building the filename so
join(homedir(), ".alby-cli", filename) cannot escape the intended folder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e738fa04-ff47-4c6d-91ec-14332a48790f
📒 Files selected for processing (5)
README.mdsrc/commands/auth.tssrc/commands/connect.tssrc/index.tssrc/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- README.md
Member
Author
|
@reneaaron I updated it, now it's much more seamless - only use |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/utils.ts (2)
66-75:⚠️ Potential issue | 🟠 MajorKeep the approval timeout in minutes, not seconds.
Line 75 still uses
5000, but this is a human confirmation flow and the requirement calls for waiting a few minutes before timing out. Five seconds will fail normal browser approval round-trips.Suggested adjustment
+const APPROVAL_TIMEOUT_MS = 3 * 60 * 1000; + export async function completePendingConnection( pendingSecretPath: string, connectionSecretPath: string, relayUrl: string = "wss://relay.getalby.com/v1", ): Promise<NWCClient> { @@ - }, 5000); + }, APPROVAL_TIMEOUT_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 66 - 75, The approval timeout is currently set to 5000 (5 seconds) in the timer callback, which is too short for a human approval flow; update the timeout to wait minutes instead (e.g., 5 minutes = 300000 ms) or introduce a named constant like APPROVAL_TIMEOUT_MS and use that value in the setTimeout where timer, settled, unsub, and reject are used so the wallet approval flow has sufficient time to complete.
15-27:⚠️ Potential issue | 🔴 CriticalValidate
walletNamebefore composing secret paths.
nameis interpolated directly into the filename, so--wallet-name ../../foocan make laterreadFileSync,writeFileSync, andrmSynccalls escape~/.alby-cliand touch arbitrary user files. Reject path separators / traversal tokens before building either helper path.Suggested hardening
+function normalizeWalletName(name?: string) { + const value = name?.trim(); + if (!value) return undefined; + if (!/^[A-Za-z0-9._-]+$/.test(value)) { + throw new Error( + "Invalid wallet name. Use only letters, numbers, dot, underscore, and hyphen.", + ); + } + return value; +} + export function getConnectionSecretPath(name?: string) { - const filename = name - ? `connection-secret-${name}.key` + const safeName = normalizeWalletName(name); + const filename = safeName + ? `connection-secret-${safeName}.key` : "connection-secret.key"; return join(homedir(), ".alby-cli", filename); } export function getPendingConnectionSecretPath(name?: string) { - const filename = name - ? `pending-connection-secret-${name}.key` + const safeName = normalizeWalletName(name); + const filename = safeName + ? `pending-connection-secret-${safeName}.key` : "pending-connection-secret.key"; return join(homedir(), ".alby-cli", filename); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 15 - 27, The helper functions getConnectionSecretPath and getPendingConnectionSecretPath currently interpolate the optional name directly into filenames, allowing path traversal (e.g., "../../") or separators; add validation at the start of both functions to reject any name that contains path separators, backslashes, path traversal tokens (".."), or is an absolute path, or restrict to a safe whitelist (e.g., /^[A-Za-z0-9._-]+$/); if the name fails validation throw an Error (or return a safe default) before composing the filename so subsequent readFileSync/writeFileSync/rmSync cannot escape the ~/.alby-cli directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/auth.ts`:
- Around line 29-33: The pending auth flow currently only persists the secret
but not the relay URL, so flows started with a non-default --relay-url lose that
value; update the code that saves the pending auth (where the secret is written
in auth.ts) to also save relayUrl alongside the secret (e.g., include relayUrl
in the pending object you serialize), and update the auto-complete/load logic in
src/utils.ts (the code that reads the pending secret during completion) to read
the saved relayUrl and use it when rehydrating the pending connection so the
approval event is listened to on the original relay.
In `@src/utils.ts`:
- Around line 29-40: The human-readable status message in saveConnectionSecret
is written to stdout and will corrupt machine-parsed output from output() and
getClient() paths; change that console.log to emit to stderr (e.g.,
console.error or process.stderr.write) or gate it behind an existing verbose
flag so structured stdout remains untouched; do the same for other human-status
prints around getClient() and where output() is used (the pending-auth messages)
so only machine results go to stdout and all status/debug messages go to stderr
or verbose-controlled paths.
---
Duplicate comments:
In `@src/utils.ts`:
- Around line 66-75: The approval timeout is currently set to 5000 (5 seconds)
in the timer callback, which is too short for a human approval flow; update the
timeout to wait minutes instead (e.g., 5 minutes = 300000 ms) or introduce a
named constant like APPROVAL_TIMEOUT_MS and use that value in the setTimeout
where timer, settled, unsub, and reject are used so the wallet approval flow has
sufficient time to complete.
- Around line 15-27: The helper functions getConnectionSecretPath and
getPendingConnectionSecretPath currently interpolate the optional name directly
into filenames, allowing path traversal (e.g., "../../") or separators; add
validation at the start of both functions to reject any name that contains path
separators, backslashes, path traversal tokens (".."), or is an absolute path,
or restrict to a safe whitelist (e.g., /^[A-Za-z0-9._-]+$/); if the name fails
validation throw an Error (or return a safe default) before composing the
filename so subsequent readFileSync/writeFileSync/rmSync cannot escape the
~/.alby-cli directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c7a3d32-375a-4396-842a-73efb55db6d2
📒 Files selected for processing (18)
src/commands/auth.tssrc/commands/cancel-hold-invoice.tssrc/commands/connect.tssrc/commands/fetch-l402.tssrc/commands/get-balance.tssrc/commands/get-budget.tssrc/commands/get-info.tssrc/commands/get-wallet-service-info.tssrc/commands/list-transactions.tssrc/commands/lookup-invoice.tssrc/commands/make-hold-invoice.tssrc/commands/make-invoice.tssrc/commands/pay-invoice.tssrc/commands/pay-keysend.tssrc/commands/settle-hold-invoice.tssrc/commands/sign-message.tssrc/commands/wait-for-payment.tssrc/utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils.ts`:
- Around line 66-75: The 5-second timeout in the setTimeout used to wait for
wallet approval (the timer variable that checks settled/unsub and calls reject
with the "Timed out waiting for wallet approval..." Error) is too short;
increase the delay to a more reasonable default (e.g., 2–3 minutes) or make the
timeout configurable via a parameter or constant (exposed/used by the function
that creates timer) so callers can override it; ensure the existing logic that
clears the timer and honors settled/unsub remains unchanged and update any
docs/comments to reflect the new default.
- Around line 15-27: Both getConnectionSecretPath and
getPendingConnectionSecretPath currently interpolate the user-supplied name
directly into filenames, allowing path traversal or invalid characters; fix by
sanitizing the name before building the filename: strip any path separators and
parent-directory segments (e.g., remove occurrences of "../" and use only
path.basename-like behavior), replace or remove characters invalid for filenames
(such as / \ : * ? " < > |) with a safe substitute (e.g., dash or underscore),
and fall back to the default filename when the sanitized result is empty; apply
this sanitization in both functions (getConnectionSecretPath and
getPendingConnectionSecretPath) so only a safe filename fragment is used in
join(homedir(), ".alby-cli", filename).
- Around line 63-98: The timeout handler can fire before the
asynchronously-assigned unsub is set, leaving the relay subscribed; to fix, call
and await nwaClient.subscribe(...) first (or otherwise capture unsub
synchronously from the subscribe result) so you have the unsub function before
starting the timeout/timer, then start the timer and rely on settled/unsub to
clean up; specifically update the flow around nwaClient.subscribe, unsub, timer,
and settled so unsub is assigned from the subscribe result (e.g. const { unsub }
= await nwaClient.subscribe(...)) prior to creating the timer and before any
code that may call unsub().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 812744c2-b997-41e4-9bfe-99d800a996c5
📒 Files selected for processing (3)
.github/workflows/ci.ymlsrc/test/connection-secret.test.tssrc/utils.ts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
Member
|
Tested the full auth flow end-to-end — works well! 🎉 One UX issue on the Hub side: after clicking "Connect" on the approval page, the Hub seems to load forever and never redirects anywhere. The user is left wondering if it worked. It would be nice if the Hub could listen for incoming requests (similar to the normal connection creation flow) and then either redirect to the connection detail page or show a success confirmation. The CLI side works great — pending connection is picked up, finalized, and cleaned up properly on the next command. |
Member
Author
|
@reneaaron I added an issue already here: getAlby/hub#2197 but I'll add your suggestion too, thanks!
|
Member
Author
|
@reneaaron should we merge the l402 PR first? then I can update this one. |
Member
If you mean #7 I don't see why we'd wait as we need some upstream changes there as well (release js-lightning-tools, etc). 🚢 |
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
2 participants
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.
Closes #11
Step 1: run the auth command
Step 2: human confirms the connection inside Alby Hub
Step 3: run any command as normal - this will finalize the connection.
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Chores