Repository navigation
[API-506] Browser login with secure saved-session custody - #657
Conversation
pr-reviewer Summary for #bfd0fa7📝 3 findings Review completed. Please address the findings below. Findings by Severity
Review effort: 5/5 (Very Complex) SummaryThis PR introduces browser-based login with secure OS credential storage, replacing the previous plain-text However, the review rules for this repository explicitly require requesting changes when I recommend requesting changes. Findings
|
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Review A and dedicated security review at b907aff64eff2b521eea8f7da01118c42c76796b. Changes requested. Both reviewers completed independently before their reports were shared. The reports below preserve their findings, checks, and limitations.
Reviewer A
Factory Droid, claude-opus-5, requested high reasoning and high autonomy.
Intent: Independent general review A of nansen-cli PR 657 (API-506 browser login with secure session custody) at frozen head b907aff against actual base cc4aa18. Read AGENTS.md, CONTRIBUTING.md, the full API-503/504/505/506/507/508 tickets, API-506-plan-review.md, API-506-review-focus.md, API-506-implementation-handoff.md, API-506-storage-research.md and API-506-507-state-proposal.md in RUN. Traced the complete delta across all changed production files (auth-credentials, auth-state, auth-store, auth-store-worker, auth-device, auth-browser, auth-login, api.js, cli.js, doctor.js, commands/agent.js, commands/mcp.js, commands/completion.js, schema.json, package.json, docs, changeset) plus the replaced paths (saveConfig/deleteConfig/resolveAuthConfig/buildHeaders) and their real callers (trading.js, mcp-verify.js, swap-simulation.js). Executed npm test and npm run lint, plus six independent probes against the real resolver, the real macOS keychain through the shipped worker, the real native lock, and the frozen cross-repo contracts in api504-session-final-3e08f9e and api504-consumer-final-1e9bfcd. No source edits, no GitHub writes, no agent or model changes, no deployment.
Verdict: changes_requested.
A1 P2: src/auth-state.js:86
Every plain nansen login that fails before a token is issued tells the user that a remote revocation is unconfirmed and that they should go review an old CLI device in their account security settings. Nothing was issued, nothing needed revocation, and in a clean starting state there is no old device at all. begin() enrols the attempt's own generation in the journal so the storage probe gets cleaned up (line 148). On the failure path finish() runs cleanupGeneration() for that same generation, store.read() finds no manifest, and the remote value stays at its 'unconfirmed' initial value instead of a distinct "nothing was ever stored here" outcome. cleanupMessage() in src/auth-login.js line 10 then prints the revocation warning. The same false warning rides alongside the genuine one on the verification-failure, storage-failure and lost-race paths, which trains users to ignore it. Suggested fix: have cleanupGeneration return a third state when store.read finds no manifest for the generation, and suppress the revocation message for it.
Evidence:
Probe against the real filesystem, the real native lock and the real journal logic (only the keyring operation was in-memory, to avoid writing to the developer keychain; the journal code under test is identical either way). Clean temp directory, no prior credential:
=== approval denied before any token issuance ===
stdout: Approve nansen CLI in your browser: https://idp.nansen.ai/device?user_code=ABCD-EFGH
stdout: Remote revocation unconfirmed. Review the old CLI device in your Nansen account security settings.
THROWN PAIRING_DENIED: Browser approval was denied. The previous credential is unchanged.
leftover keychain accounts: []
config.json exists: false
=== approval code expired before issuance ===
stdout: Remote revocation unconfirmed. Review the old CLI device in your Nansen account security settings.
THROWN PAIRING_EXPIRED: The approval code expired or was consumed. Run nansen login again.
No test in src/tests/auth-login.test.js asserts the human progress lines on the failure paths, which is why this went unnoticed.
A2 P2: src/api.js:870
The session-specific 401 message is dead code. Line 870 sets message to "The selected browser session was rejected. Run: nansen login." for a session-selected request, and six lines later line 876 overwrites it with "Not logged in. Run: nansen login", because code is UNAUTHORIZED and this.apiKey is null for every session selection. A user whose saved session the API rejects is told they are not logged in, while nansen auth status in the same state reports logged_in: true with an active saved_session. docs/browser-login.md and the plan both describe the rejected-session wording that never reaches the user. Suggested fix: guard line 876 with this.selection.kind !== 'session', or set the session message after the UNAUTHORIZED enhancement block.
Evidence:
Probe with a valid saved-session bundle (real resolver, real validateSession, stubbed transport returning 401):
SENT HEADERS: {"X-Client-Type":"nansen-cli",...,"Authorization":"Bearer "}
CODE : UNAUTHORIZED
MESSAGE: Not logged in. Run: nansen login
DETAILS: {"attempt":1,"retryAfterMs":null}
The access token is correctly kept out of the message and details, so only the diagnosis is wrong. No test covers a session-selected 401 message.
A3 P2: src/auth-state.js:18
Legacy API-key storage and logout now hard-require the optional fs-native-extensions binding, so a user who already has a saved key in ~/.nansen/config.json can neither replace nor delete it when that binding is missing. Both cli.js login paths and logout go through locked(), which calls acquire(), which calls loadLocks(). In the base revision logout was a plain fs.unlinkSync and login --human a plain fs.writeFileSync, neither needing a native module. API-506 requires login --api-key and login --human to be preserved as legacy key-storage modes and requires logout to clear the active saved credential. Losing the ability to remove a local key copy is the security-relevant half. This bites npm install --omit=optional installs (which the repo's own package test uses), musl or Alpine images, and any architecture without a prebuild; fs-native-extensions@1.5.1 ships no musl-specific prebuild. The error text "Existing environment API keys still work" is accurate but useless to someone trying to delete a saved key. Suggested fix: keep the native lock mandatory for session generations, and fall back to a plain atomic file replacement plus an O_EXCL lockfile for key-only and logout mutations, or at minimum point the message at manually removing config.json.
Evidence:
Real CLI, real base code path, optional binding removed from node_modules and restored afterwards; HOME seeded with {"apiKey":"legacy-saved-key","baseUrl":"https://api.nansen.ai"}:
== logout (no native lock module) ==
{"success":false,"error":"Native authentication locking is unavailable. Reinstall nansen-cli with optional dependencies to save or remove authentication. Existing environment API keys still work.","code":"AUTH_LOCK_UNAVAILABLE","status":null}
== login --api-key (no native lock module) ==
{"success":false,"error":"Native authentication locking is unavailable...","code":"AUTH_LOCK_UNAVAILABLE","status":null}
== config still holds the saved key ==
{"apiKey":"legacy-saved-key","baseUrl":"https://api.nansen.ai"}
auth status still reads correctly in that state, so the key stays both visible and unremovable. src/tests/package.test.js asserts the AUTH_LOCK_UNAVAILABLE throw deliberately, so the write-path consequence is accepted rather than unnoticed. node_modules/fs-native-extensions/prebuilds contains darwin-arm64, darwin-x64, linux-arm64, linux-x64, win32-arm64, win32-x64 and mobile targets only.
A4 P2: src/api.js:763
Automatic x402 payment is now switched off for API-key-selected requests, and the 402 error loses its actionable guidance, but the changeset that becomes the published CHANGELOG entry does not mention it. mayAutoPay requires this.selection.kind === 'anonymous'; the base revision entered the wallet flow for any request without a manual signature, including one carrying an API key. A subscriber whose credits run out and who has a funded x402 wallet previously had the call paid automatically. Now the same call fails with whatever bare string the server sent, because line 973 also gates the "Sign the paymentRequirements below" hint on mayAutoPay || hasManualSignature. That leaves an error with no route forward, against the AGENTS.md rule that errors be actionable. The plan text only promises that invalid selected credentials never pay; this removes payment for valid keys too. README.md still claims the CLI "signs Payment-Signature headers automatically on 402 responses" without the anonymous-only qualifier. Suggested fix: state the removal in .changeset/browser-session-login.md and README option 2, and restore a pay-or-fund hint for the key-selected 402.
Evidence:
Probe with a stubbed 402 carrying an x402 accepts block and _x402Retry instrumented:
--- api-key selected ---
fetch #1 https://api.nansen.ai/api/v1/account payment-signature=false
code : PAYMENT_REQUIRED
message: "Payment required"
wallet payment attempts: none
--- anonymous ---
code : PAYMENT_REQUIRED
message: "Payment required (x402). Sign the paymentRequirements below per https://docs.x402.org and pass the result with --x402-payment-signature ."
wallet payment attempts: none
Corroborated by the diff to src/tests/api.test.js, where five x402 cases had to be switched from new NansenAPI('test-key', ...) to new NansenAPI(null, ...) and the assertions changed from expect(thrownError.message).toContain('auto-payment failed') plus expect(thrownError.details).toHaveProperty('paymentRequirements') to toContain('nansen login') plus paymentRequirements undefined. .changeset/browser-session-login.md contains no payment sentence.
A5 P3: src/auth-device.js:87
validateSession requires the access-token claim session_access_revocation_version === 1, which the pinned authservice emits only when SESSION_ACCESS_REVOCATION_ENABLED=true. That flag defaults to unset, and it is absent from the dev render config captured in RUN. With the flag off, a user completes browser approval, the IdP mints a real device session, and the CLI then rejects it locally with "The selected browser session is invalid. Run: nansen login." That message invites an unbounded retry loop, each pass burning a fresh approval and a fresh server-side family. The local check is consistent with the API side, which also returns 401 without the claim and keeps BROWSER_SESSION_ACCOUNT_ENABLED false by default, so the gate itself is coordinated rather than a CLI bug. What is missing is CLI-side diagnosis and documentation: docs/browser-login.md says only "API504 deployment/clock/retention/fleet gates remain" and never names either flag or this symptom. Suggested fix: distinguish a missing revocation-version claim from a malformed session and say the server has not enabled browser sessions yet, and name both flags as cohort prerequisites in docs/browser-login.md.
Evidence:
Frozen authservice 3e08f9e76a3fd785df9f641f640234e5a87e00a4: apps/idp/src/config.ts:1176 sets sessionAccessClockSkewSeconds only when SESSION_ACCESS_REVOCATION_ENABLED === 'true'; apps/idp/tests/config.test.ts:1607 asserts it "is off by default"; apps/idp/src/routes/auth-device.ts:620 adds accessRevocation only when that value is defined; apps/idp/src/tokens/signer.ts:134 emits the claim only then. grep -i 'SESSION_ACCESS' auth-dev-render-30cf.yaml returns nothing. Frozen API 1e9bfcdf3af14a6a51459561a6a5e075b32e3840, docs/browser-session-revocation-consumer.md: "Browser admission also requires ... session_access_revocation_version to be exactly integer 1. Missing claims ... return 401" and "BROWSER_SESSION_ACCOUNT_ENABLED remains false in deployment manifests and by default."
CLI probe over the real pairDevice poll path:
flag OFF (claim absent) - authservice default: REJECTED INVALID_BROWSER_SESSION - The selected browser session is invalid. Run: nansen login.
flag ON (claim present): ACCEPTED
pairDevice: INVALID_BROWSER_SESSION - The selected browser session is invalid. Run: nansen login.
server-side session was already minted and handed to cleanup: true
A6 P3: src/api.js:637
Every API request made under a saved session takes the process-wide exclusive auth.lock and spawns one credential-store helper per stored chunk plus one for the manifest. Measured on this macOS arm64 host that is about 150 ms of added latency per request, and concurrent commands serialise strictly. Sixteen parallel session-selected calls take 2.5 s of wall time. acquire() gives up after 15 s with AUTH_BUSY, so a high fan-out agent workload (roughly a hundred concurrent calls, or fewer when the keychain is slow or a bundle needs several chunks) can start failing commands that have nothing wrong with their credential. API-key and anonymous selections are unaffected. Suggested fix: cache the decrypted access token for the life of the process behind the epoch and generation check already in readSession, or take a shared rather than exclusive lock for reads.
Evidence:
Probe with the real native lock and the real macOS keychain, one installed session, disposable UUID accounts, verified clean afterwards:
concurrent= 1 wall=136ms failures=none
concurrent= 4 wall=607ms failures=none
concurrent=16 wall=2466ms failures=none
single readSession = 165ms (credential-store subprocess spawns per request)
keychain residue after logout: none
src/api.js:637 calls readSession on every request(), and src/auth-state.js:202 wraps it in locked(), which serialises in-process through the queues map and then takes the OS advisory lock.
A7 P3: src/doctor.js:263
Three pieces of user-facing text now contradict the shipped behaviour. First, doctor tells a user with a non-default baseUrl in config.json to "Run: nansen login (re-saves the default)", but plain login in exactly that state refuses with AUTH_ORIGIN_UNSUPPORTED before doing anything, and its message points at NANSEN_BASE_URL even though the origin came from config.json. Only the legacy --human or --api-key path still rewrites the base URL, which the hint does not say. Second, README.md lines 23 and 24 list nansen login --human twice with contradictory comments ("interactive prompt; saves to ~/.nansen/config.json" and "also saves NANSEN_API_KEY when already set"), which reads like a botched search and replace of the old plain-login line. Third, README option 2 still promises that the CLI "signs Payment-Signature headers automatically on 402 responses" without the anonymous-only qualifier that A4 establishes.
Evidence:
src/doctor.js:263 emits the fix string 'Run: nansen login (re-saves the default)'. Real CLI with HOME seeded as {"apiKey":"k","baseUrl":"https://api.example.com"}:
$ nansen login
{"version":1,"event":"error","code":"AUTH_ORIGIN_UNSUPPORTED","message":"Browser sessions require the Nansen production or staging API origin. Check NANSEN_BASE_URL.","cleanup":[]}
sed -n '22,25p' README.md:
nansen login --human # interactive prompt; saves to ~/.nansen/config.json
nansen login --human # also saves NANSEN_API_KEY when already set
nansen logout # remove saved key
Checks
-
npm ci in a clean worktree: 278 packages, both optional bindings resolved (@napi-rs/keyring-darwin-arm64 and fs-native-extensions load and expose Entry / tryLock).
-
npm test: 81 files, 3240 passed, 9 skipped, 123 s. Matches the writer's claim exactly.
-
npm run lint (eslint .): clean, no output.
-
Live CI inspected at the exact frozen head rather than taken from the handoff snapshot. Run 35426431220, headSha b907aff, status completed, conclusion success. Jobs: lint success, test (20) success, test (22) success, test (24) success, sync-clawhub skipped, publish skipped. Root's api506-initial-ci-status.json captured only lint and test (20)/(22) before test (24) finished; the live result confirms it green.
-
Full delta reviewed file by file, including the truncated middle of the api.js and cli.js hunks read directly from the working tree rather than from diff output.
-
Credential selection traced through the real resolver: explicit override, then NANSEN_API_KEY (including the empty-string case, which is now an error rather than a silent fall-through), then the single active saved credential. Session precedes legacy apiKey;
auth.active.kind === 'none'blocks the dev config.json key from reappearing after logout; a config with noauthpointer still resolves the legacy key, so pre-upgrade installs keep working. -
Real callers of the replaced paths checked: trading.js:2513
loadConfig().apiKeyis null under a session, so verifySwapOutcome and swap-simulation send no credential; mcp-verify.js:226 reports no key; commands/mcp.js:358 raises API_KEY_REQUIRED with a session-specific message. Confirmed no path exports the browser Bearer to MCP, trading or RPC. -
SSE and cache paths inspected, not just ordinary requests. commands/agent.js
buildHeadersnow awaitsrequestCredentials()so the agent stream carries the session Bearer and is subject to the same MIXED_CREDENTIALS guard; the anonymous guard replaced the old!apiKeyguard. The response cache key mixesthis.selection.generationintobaseUrland the credential headers into the identity digest, so a session and a key cannot share a cache entry. -
Real macOS keychain custody proved independently through the shipped worker and chunk/manifest layer, with disposable UUID accounts: 1695-byte bundle (1 chunk) write and read back OK; 11695-byte bundle (6 chunks) write and read back OK, which closes the writer's sandbox-denied 11580-byte probe on this platform; 17831-byte bundle refused with AUTH_BUNDLE_TOO_LARGE before any write; tampered manifest digest refused with AUTH_STORE_UNAVAILABLE. All entries fully removed, and
security find-generic-password -s nansen-cli-api-auth-v1afterwards reports no such item. -
@napi-rs/keyring missing-entry semantics verified directly, since
remove()walks a fixed eight-chunk namespace unconditionally: getSecret returns null and deleteCredential returns false rather than throwing, so cleaning a partially written generation does not spuriously report deletion incomplete. -
Fault-barrier and race coverage read in src/tests/auth-process.test.js: real forked children, real native locks, SIGKILL at chunk-0, manifest, stored, before-flush, before-rename, after-rename and committed; reverse-completing logins; logout and legacy-key save racing a pending approval; a SIGSTOP-paused owner retaining the lock and SIGKILL releasing it. The synthetic file-backed store there is a legitimate substitution for fault injection, and the native store is covered separately by the probe above.
-
Epoch and journal logic reasoned through by hand and confirmed against those tests:
beginsnapshots selectionEpoch under the global lock,installre-checks it under the same lock before writing any secret, an earlier pending login cannot overwrite a later commit or a logout, a failed install retires only the candidate and never the still-active old generation, and an uncertain pointer commit setsattempt.uncertainso the possibly live candidate is not revoked. -
Lock ownership checked for age-based theft:
recover()only unlinks an attempt lock file whose journal is absent and whose lock it can itself acquire non-blockingly, so a paused owner keeps its lock. No timer decides ownership anywhere. -
Secret redaction audited across config.json, journal files, machine events, human progress, errors and telemetry. Tokens and the private JWK exist only in keychain entries; journals carry operation and generation UUIDs;
X-Anonymous-Idtelemetry carries flag names but no option values; the worker receives no secret argv and a reduced environment allowlist that excludes NODE_OPTIONS. -
No production test or issuer override exists.
trustedIssueris a hard-coded two-entry map, the keychain service name is a literal in auth-store-worker.js, and the only injection points are function parameters. grep for process.env across the auth modules returns HOME, USERPROFILE, NANSEN_API_KEY, NANSEN_BASE_URL and the worker environment allowlist only. -
Browser launch reviewed: origin must equal the trusted issuer, path must be /device, no credentials, fragment or extra query parameters; shell disabled; the Windows URL travels over stdin to Start-Process rather than argv; a 5 s SIGKILL bound and an error handler that degrades to printing the link.
-
Device flow checked against the frozen authservice contract at 3e08f9e76a3fd785df9f641f640234e5a87e00a4:
verification_uri_completeconstruction matches byte for byte, DEVICE_POLL_INTERVAL_SECONDS 5 and DEVICE_CODE_TTL_SECONDS 600 fall inside the client bounds, the token response shape matches, and the revoke receipt fields refresh_family_revoked plus access_revocation {status: recorded, coverage: complete_family, propagation: pending, version: 1} matchretireSessionexactly, including the legacyaccess_tokens_revoked: falsebranch. -
API-507 refresh confirmed absent: no rotation engine, no dormant scheduler, no stay-signed-in claim. An expired session fails closed with SESSION_EXPIRED before any network call or cache read.
-
Help and destructive-command safety re-verified after login and logout moved into schema.json and out of UNSCHEMA_COMMANDS:
nansen logout --helprenders schema help and does not execute, and login and logout skip the update check and the cost-map refresh so they make no incidental network calls. -
Manual payment compatibility confirmed: an API key plus an explicit --x402-payment-signature still sends both headers, a session plus any of apikey, authorization or payment-signature is refused with MIXED_CREDENTIALS, and no selection other than anonymous can trigger automatic signing.
-
Offline diagnostics verified by running the CLI:
auth statusmakes no network call, separates effective_credential from saved_session, labels the session cached_unverified with storage_access not_checked_no_prompt, and does not open the native store.
Limitations
-
No staging or live walkthrough was performed or is available to me. No real browser approval, no real IdP round trip, no real GET /api/v1/account call, no measured revocation propagation, no remote-terminal or SSH login. Both server gates are default-off and absent from the captured dev render config, so end-to-end acceptance is unproven; see A5.
-
Native custody is proved only on this macOS arm64 host with the login keychain unlocked. Linux Secret Service remains unproven as a working store (root's linux-native-storage-preflight.log records a pinned refusal under an isolated bus without the service, which is a refusal proof, not a storage proof). Windows Credential Manager is entirely unproven, including the 2560-byte blob limit the chunking is designed around and the Enterprise-persistence roaming caveat the docs admit. No locked store, no cancelled OS prompt, no reboot, logoff or relogin persistence, and no network or synchronised home directory was tested.
-
Durability testing covers process death only. The SIGKILL barrier suite and my probes say nothing about filesystem power loss; POSIX file and directory fsync plus native readback do not establish all-OS power-loss durability, and the draft's own docs concede this.
-
The optional native platform workflow is retained outside this branch because the writer App token lacks workflow permission. That is an omission, not a passing job, so no CI matrix exercises native locks or the credential store on any OS; the green run 35426431220 covers lint and Node 20/22/24 unit tests only.
-
Cross-repo contract verification was reading frozen sources (auth 3e08f9e76a3fd785df9f641f640234e5a87e00a4, API 1e9bfcdf3af14a6a51459561a6a5e075b32e3840) plus the captured deployment config. I did not replay the signed producer/consumer fixture, and I take no position on its pass beyond what those sources show. No public gateway, account, ledger, cohort or platform acceptance is established by this review.
-
Concurrency and latency in A6 were measured in-process on a single host. I did not measure cross-process or cross-host contention, contention over a network filesystem, or behaviour with a slow or prompting keychain, so the AUTH_BUSY threshold is an extrapolation from the linear serialisation observed at 1, 4 and 16 concurrent reads.
-
Per assignment I did not read peer drafts, reports or events, and I did not inspect the three telemetry segments root captured. API-505 research billing (draft at 1099e580, awaiting its own reviews) and API-507 refresh are out of scope by design, so research-scope behaviour under a browser session is unverified here.
-
The optional-binding-absent probe removed and restored one self-installed node_modules package to observe the failure mode. It did not test a genuine platform without a prebuild, so A3's musl and unsupported-architecture reach is inferred from the shipped prebuilds list rather than executed.
-
I did not exercise the Windows code paths in auth-browser.js (powershell Start-Process over stdin) or auth-state.js (the win32 branches that skip POSIX mode, uid and directory fsync checks) on Windows; they were reviewed by reading only.
Dedicated security reviewer
Claude Code, claude-opus-5, requested high reasoning.
Intent: Dedicated security review of nansen-cli PR #657 (API-506 browser login) at frozen head b907aff against base cc4aa18, scoped to credential custody, trust boundaries, revocation, concurrency/cleanup, malicious input/redirects and secret exposure. Read AGENTS.md/CONTRIBUTING, the dead-code and blast-radius guidance, the pr-author security checklist, and RUN copies of API-503/API-506, API-506-plan-review.md, API-506-review-focus.md and API-506-implementation-handoff.md. Reviewed the complete delta and every changed production file, traced actual command callers and the replaced saveConfig/deleteConfig seams, and executed the real resolver, real auth-state/auth-store/auth-device/auth-login code and the real CLI binary with injected transports and isolated HOME directories rather than relying on helper mocks. No source edits, no GitHub writes, no agents or model changes, no access to existing user credentials or the real keychain.
Verdict: changes_requested.
S1 P2: src/auth-state.js:117
The journal ceiling in recover() is checked before any pruning and is reachable through the CLI's own printed recovery instruction, permanently disabling both nansen login and nansen logout.
When the OS credential store is unavailable (locked keychain, headless Linux with no Secret Service, Credential Manager blocked by policy, broken @napi-rs/keyring binding), store.remove fails on the fixed-namespace <generation>.probe delete, so cleanupGeneration returns local: 'incomplete', cleanJournal rewrites the journal instead of unlinking it (auth-state.js:88-100), and every failed nansen login leaves one auth-operations/<uuid>.json behind. begin() self-caps at 8 because of its own >= 8 guard (line 144), so login alone cannot cross the ceiling.
logout() does not have that guard: it writes its journal unconditionally at line 198 before calling recover() at line 204. Running nansen logout — exactly what the AUTH_CLEANUP_REQUIRED message tells the user to do — creates the 9th journal and only then hits files.length > 8, which throws. Because the count check precedes the cleanup loop, nothing ever prunes the directory again: begin() and logout() both throw immediately, and each further logout attempt appends another journal file. Unlocking the credential store does not help.
Effect: the user is permanently locked out of nansen login and, more importantly for this review, out of nansen logout — the supported control for clearing a saved credential, retiring a browser session family and finishing secure-store deletion. The only recovery is manually deleting ~/.nansen/auth-operations/, which no message, doc, or auth status/doctor check mentions; docs/browser-login.md instead says "Unlock the OS store and rerun logout." logout() also clears the config pointer before recover() throws, so the command reports failure while having already mutated saved state.
Suggested direction: prune (or classify and bound) stale journals before enforcing the ceiling, exempt logout() from creating a journal it cannot clean, and make the error name the actual path to clear.
Evidence:
Reproduced against the frozen head with the real createAuthState/createAuthStore and real fs-native-extensions locks, injecting only a store operation that throws AUTH_STORE_UNAVAILABLE (a locked store). Node v22.23.2, darwin arm64, isolated mkdtemp directory:
login#1..#8: err=AUTH_STORE_UNAVAILABLE journals=1..8
login#9: err=AUTH_CLEANUP_REQUIRED journals=8
--- user follows the error message: nansen logout ---
logout#1: err=AUTH_CLEANUP_REQUIRED journals=9
logout#2: err=AUTH_CLEANUP_REQUIRED journals=10
--- user unlocks the OS credential store, retries ---
login after unlock: err=AUTH_CLEANUP_REQUIRED msg="Too many unfinished authentication operations. Retry nansen logout after unlocking the credential store." journals=10
logout after unlock: err=AUTH_CLEANUP_REQUIRED journals=11
config: {"auth":{"version":1,"selectionEpoch":"76b74aa8-...","active":{"kind":"none"}}}
The per-attempt journal retention was independently confirmed by a separate end-to-end browserLogin probe: the "storage preflight fails before approval" scenario ends with journals=1 and store entries empty, while the three post-issuance failure scenarios (verify 401, verify 500, store write failure) all end with journals=0.
verdict: CONFIRMED
category: availability-of-credential-revocation
short_summary: Journal ceiling bricks login/logout; logout is the trigger
failure_scenario: OS credential store is locked/absent. User runs nansen login 8 times (each fails AUTH_STORE_UNAVAILABLE and leaves a journal), then follows the printed instruction and runs nansen logout. logout writes a 9th journal, then recover() throws AUTH_CLEANUP_REQUIRED. From then on both nansen login and nansen logout fail unconditionally — including after the store is unlocked — and every retry adds another journal file. Credential removal and session retirement become impossible without manually deleting ~/.nansen/auth-operations/.
Checks
-
Full delta reviewed: git diff cc4aa18..b907aff (36 files, +1766/-265), including every changed production file — src/auth-state.js, auth-store.js, auth-store-worker.js, auth-credentials.js, auth-device.js, auth-login.js, auth-browser.js, api.js, cli.js, doctor.js, commands/agent.js, commands/mcp.js, commands/completion.js, schema.json, package.json, vitest.config.js — plus README, docs/browser-login.md, skills and the major changeset.
-
npm ci then npm test at the frozen head: 81 test files passed, 3240 passed / 9 skipped (3249), exit 0. Matches the writer's reported 3240/9. Node v22.23.2, darwin arm64.
-
npm run lint (eslint .) passed with no output.
-
Real CLI binary, isolated HOME with a plaintext saved key:
nansen login --help,login -h,logout --help,logout -hall render schema help and leave config.json untouched (the removed simpleHelp['logout'] entry is correctly superseded by the new SCHEMA.commands.logout lookup, which runs earlier in runCLI).nansen auth statusis offline and separates effective_credential from saved_session. Realnansen logoutremoved the plaintext key, wrote the version-1 tombstone, preserved baseUrl, and used the real native lock on a real filesystem. -
End-to-end browserLogin probe driving the real pairDevice/createDeviceClient/createAuthState/createAuthStore with only fetch and the store operation injected: DPoP nonce challenge, poll interval, authorization_pending, issuance, validateSession and candidate verification all exercised. stdout/stderr and config.json contain none of device_code, refresh token, access token, private JWK
d, or the environment API key. NDJSON events on stdout, human progress on stderr, --no-browser honored (openBrowser never invoked). -
Candidate-B verification confirmed to bypass ambient credentials: the only header on GET {audience}/api/v1/account was
authorization: Bearer <candidate token>— no apikey, no env key A, no payment signature. Env-override wording emitted verbatim per API-503/506. -
Session replacement: installed session A, then a fresh browser login for B. POST /token/revoke carried A's own refresh_token and a DPoP proof signed with A's own key (public x matched key A, not key B); the new B generation and its store entries were untouched; A's store entries removed; config switched to a new generation and epoch.
-
Denied approval (access_denied): PAIRING_DENIED, config.json byte-identical to before, zero revocation calls, existing session store entries intact.
-
Post-issuance failure paths (candidate verify 401, candidate verify 500, secure-store write failure): prior saved API key preserved in every case, candidate best-effort revoked via retireSession, secure store left empty, journals cleaned to 0. Preflight failure before approval emits only the terminal error — no pending event, no approval requested.
-
api.js selection guards exercised against the real NansenAPI with a real saved session: session + 402 attempts no x402 payment and returns a redacted message with no server body in details; session + --x402-payment-signature throws MIXED_CREDENTIALS with zero network calls; session 401/500 do not echo the server body (planted secretEcho absent); expired session throws SESSION_EXPIRED before any request. mayAutoPay is gated to kind === 'anonymous', so a selected invalid/rejected key cannot trigger payment.
-
Central resolution verified by tracing every caller: resolveCredential/authConfigView is the single seam used by api.js, doctor.js, mcp-verify.js and trading.js; no new global key override exists for data commands (cli.js only sets Payment-Signature into defaultHeaders). saveConfig/deleteConfig are fully removed with no remaining references; no uncoordinated auth writer survives.
-
Key-only MCP/trading custody confirmed: commands/mcp.js install requires apiInstance.apiKey (null for sessions, error code API_KEY_REQUIRED), mcp-verify.js resolves auth.apiKey only, trading.js loadConfig().apiKey is null under a session selection, and loadConfig() overrides apiKey from the resolver so a stale config key cannot resurface.
-
Browser-open path reviewed: openAuthBrowser re-validates origin against trustedIssuer, rejects non-/device paths, userinfo, fragments and any param other than user_code; no shell, argv-safe on POSIX, URL passed over stdin to powershell on win32 with -NoProfile -NonInteractive; 5s SIGKILL bound. Device flow uses redirect: 'error' on authorize/poll/revoke/account, strict equality on verification_uri and verification_uri_complete, a 32 KiB incremental response cap, a 1 KiB nonce cap, bounded expires_in/interval/retry-after, and rejects control characters in the returned account identity.
-
Secure-store format reviewed and exercised: fixed 'nansen-cli-api-auth-v1' service, UUID-validated account names, 2048-byte chunks under the documented Windows 2560-byte blob limit, 16 KiB cap, manifest with count/bytes/SHA-256 validated on read, generation binding checked, fixed bounded delete namespace including interrupted writes, read-back verification after write. Worker receives no secret in argv, a scrubbed env allowlist, stderr discarded, private stdin/stdout pipes, 10s SIGKILL bound with close awaited before the caller's lock is released.
-
Concurrency/epoch model traced and cross-checked against the committed real-child tests (auth-process.test.js: reverse-completing logins, logout/key vs pending approval, SIGKILL at chunk-0/manifest/stored/before-flush/before-rename/after-rename/committed, killed legacy staging file, SIGSTOP paused owner). recover() never steals a live attempt lock by age (non-blocking acquire only), never cleans the active generation, and an uncertain pointer commit sets attempt.uncertain so a possibly-active candidate is not retired.
-
Dead-code check: no orphaned exports or unused helpers introduced; doctor.js readJsonDetailed/DEFAULT_BASE_URL/DEV_CONFIG_PATH/getConfigFilePath all still used; no dormant refresh/rotation engine present (API-507 is absent by design, as stated).
-
No production test-store or issuer override exists: the synthetic store is injected only through the code interface in test fixtures; trustedIssuer allows exactly api/idp.nansen.ai and api/idp.banansen.dev and otherwise throws AUTH_ORIGIN_UNSUPPORTED.
-
No tracked file was modified; git status clean at b907aff. All probe scripts were written to /tmp and deleted.
Limitations
-
Exercised only on local macOS arm64 (darwin 25.6.0) with Node v22.23.2. I did not run Windows, Linux, network/synchronized home directories, locked-store, reboot/logoff or SSH scenarios. Root's reported CI 35426431220 SUCCESS on Node 20/22/24 with lint (RUN/api506-initial-ci.log) and the committed native Mac helper/store 11416-byte synthetic bundle preflight/write/read/delete PASS (RUN/api506-native-bundle-result.json) stand on their own stated terms; I did not re-verify them and they do not extend to other platforms.
-
I deliberately did not touch the real OS keychain or any existing user credential, and created no native store entries of my own. All secure-store behavior I exercised used injected in-memory or file-backed operations plus the real createAuthStore chunk/manifest/validation logic. Native Keychain/Secret Service/Credential Manager persistence, locked-store prompting behavior and cross-login durability remain unverified by me. The writer's own native Mac 11580-byte bundle probe was sandbox-denied and is not store proof; prior external 2000-byte macOS keyring and macOS/Linux SIGSTOP/SIGKILL lock probes retain their documented narrow limits, as does linux-native-storage-preflight.log (a pinned refusal under an isolated bus with no Secret Service, not working Linux secure storage).
-
All device-flow, revocation and account-lookup behavior was exercised against injected transports. No real browser approval, no real authservice or public gateway call, no staging walkthrough from a local or remote terminal, and no zero-credit/zero-debit/ledger/cohort/platform acceptance evidence. Server pins auth 3e08f9e7 and API 1e9bfcdf were taken as given from the handoff; I did not independently re-verify those repositories.
-
The optional native workflow file was omitted from the PR because the App token lacked workflow permission; that is not a CI pass and I treated it as absent.
-
Native locking depends on fs-native-extensions prebuilds (android/darwin/ios/linux-x64/linux-arm64/win32 present; no musl build). On a musl/Alpine container or any platform without a prebuild and toolchain,
nansen login --api-key/--humanandnansen logoutfail closed with AUTH_LOCK_UNAVAILABLE even though only a plaintext config file is involved. This is an explicit, documented design choice (no plaintext fallback, help and environment/explicit key use stay available, which I verified via the packaged --omit=optional smoke test in package.test.js), so I did not raise it as a defect — but musl/Alpine is not named in docs/browser-login.md's supported-environment list and should be settled before cohort selection. -
Lower-confidence observations I did not raise as findings, having found no reproducible way to trigger them: recover() parses journal JSON at auth-state.js:124 without catching SyntaxError, and a malformed journal raises AUTH_STATE_INVALID whose message points at config.json rather than ~/.nansen/auth-operations/; a successful login leaves one empty .lock file that the next recover() removes; a 401 under a session selection has its tailored message overwritten by the generic 'Not logged in. Run: nansen login' at api.js:876; and pressing Ctrl-C during login aborts the attempt but suppresses default SIGINT termination for the duration of best-effort remote/local cleanup.
-
The change that a selected API key no longer triggers x402 auto-payment on a 402 (and no longer surfaces paymentRequirements) is a real user-visible behavior change that the major changeset does not mention, though docs/browser-login.md does. I treated it as intended no-hidden-fallback hardening rather than a defect; flagging it only so release notes can be reconciled.
-
Per assignment I did not read peer reviewer drafts, reports or event files in RUN, and did not run a third general-purpose correctness pass — coverage outside credential custody, trust, revocation, concurrency/cleanup, malicious input and secret exposure is intentionally incomplete.
Additional orchestrator reproduction
The controlled joint CLI/API probe independently found that session-selected credit and plan failures lose structured error codes. At CLI b907aff and API 1099e58, zero-credit session requests return FORBIDDEN where the same account with a key returns CREDITS_EXHAUSTED; premium-plan denial similarly loses plan_upgrade_required. Preserve an explicit allowlist of safe error codes without exposing upstream bodies or arbitrary code strings. Ten public-command cases also verified credential selection and controlled debit attribution. This used real CLI child processes and API middleware with synthetic issuance, injected transport and controlled storage; it does not prove deployed gateway behavior or PostgreSQL ledger results.
|
Repairs to the combined A/security review and current bot feedback, at 83e2f0d. This is the writer's repair report, not a review or approval.
Root's 11,416-byte and A's 11,695-byte synthetic native bundle roundtrips supplement the earlier unlocked-Mac evidence only. Writer sandbox native storage remains denied. Windows/Linux secure storage, locked/cancelled stores, reboot/logoff/remote terminal, deployed gateway, staging and live-ledger acceptance remain unverified. API507 refresh, API505 billing enablement, merge and publication are unchanged. |
|
Custody repair at 83d6f5d, preserving the prior consolidated repairs. This is a writer report, not a review or approval. Root's actual-owner probe at b907aff proved that a parent SIGKILL could let logout delete the last journal before an orphan helper recreated a synthetic secret chunk. The injected component was the native backend; this was not a reproduction against a live OS credential store. The helper now acquires its own stable This extends the existing auth-state/store owner, without a second auth mutation protocol. The gate file is never unlinked. The helper independently opens and locks its descriptor, avoiding inherited-handle assumptions across macOS flock, Linux OFD locks and Windows LockFileEx. API507 should keep using this same boundary and must not directly write keyring entries. Real-child tests use actual dispatcher/guardian/state/store source and replace only Current bot feedback at 83e2f0d:
New Linux native evidence remains old-head b907aff: Debian12/x64 Node22.23.2, temporary isolated GNOME Secret Service42.1; 11,455 stored bytes passed preflight/write/read, daemon-restart read and bounded deletion. Real collection locking refused preflight in 62 ms, unlock/restart preserved the bundle, and cleanup passed. These are not results for the repaired thread boundary; root's native rerun is pending. Neither they nor the synthetic process tests establish public login/staging, Windows, desktop prompt cancellation, OS login/PAM/reboot or power-loss behavior. Prior A6 latency measurements also predate the added thread startup and need remeasurement. No API507 implementation, agents, model changes, independent self-review, approval, merge, deployment, release or ticket closure. PR remains draft; body telemetry now includes the completed prior A-repair segment separately from this custody run. |
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Review B and bounded dedicated security repair verification at 83d6f5d1e851e9da1f46902e3df93dd523bddedd. Changes requested. Both completed independently. Full reports below retain all findings, evidence, checks and limitations.
Reviewer B
Factory Droid deepseek-v4-pro, requested max reasoning and high autonomy.
Intent: Independent final general reviewer B of nansen-cli PR 657 (API-506 browser login with secure session custody) at frozen head 83d6f5d against actual base cc4aa18. Read AGENTS.md, the pr-author security checklist, the dead-code and blast-radius skills, RUN API-503.md/API-506.md/API-506-review-focus.md/API-506-repair-handoff.md/API-506-orphan-handoff.md/API-506-orphan-helper-finding.md, the completed initial A report api506-cli/workers/reviewerA-round1/result.txt, the b907 security report security/api506-b907aff/result.txt, the live PR body with all issue comments and the combined A+security review pullrequestreview-5254931768. Reviewed the whole base-to-head delta file by file at the exact head, including both repair commits, and re-read every changed production module in full. Executed the full test suite, lint, diff check, the changeset gate, the six committed guardian lifetime/timeout tests with real native locks, and four independent probes against the real auth-login/auth-state/auth-store/auth-device code, including the real pairDevice pipeline. No source edits, commits, GitHub writes, deployments, agents, or model changes.
Verdict: changes_requested.
Findings
B1 P2: src/auth-login.js:64
Cancelling login between token polls prints a false remote revocation warning. pairDevice durably clears the unissued journal marker before the first poll through markIssuancePossible, then only restores it for denial or expiry responses via provenUnissued at auth-device.js:138 and 142. A SIGINT that lands during a backoff wait carries no classification, so the catch at auth-login.js:64 leaves the marker cleared. cleanJournal has no not_needed outcome for that state at auth-state.js:98, cleanupGeneration finds no manifest in the store, and cleanupMessage prints Remote revocation unconfirmed. Review the old CLI device in your Nansen account security settings. This CLI is the only holder of the device code, and no poll was in flight when the user cancelled, so issuance was impossible. On a first login there is no old device at all. The cancelled NDJSON event also reports remote unconfirmed. This is the same false-warning class that A1 fixed on the denial and expiry paths, still live on the natural cancel path, and no committed test asserts the cleanup lines for it. Fix by having pairDevice attach provenUnissued when the abort lands outside an in-flight token request, mirroring that into attempt.journal.unissued here, and add a regression that cancels after one authorization_pending poll and asserts no revocation line.
Evidence:
Reproduced against the real pairDevice, deviceClient, and validateSession pipeline at 83d6f5d with only fetch stubbed and an in-memory store. Authorize returned 200 with the pinned verification URIs and interval 5. The token poll answered authorization_pending once. SIGINT then fired during the next backoff wait. Both human and machine mode printed Remote revocation unconfirmed. Review the old CLI device in your Nansen account security settings. and the terminal cancelled event carried cleanup [{remote:unconfirmed, local:removed}]. The probe logged polls=1, so no poll ever issued a token. The existing cancel test in auth-login.test.js fires SIGINT before the first poll, where the unissued marker still reads not_needed, which is why the suite passes. A second probe reproduced the same message with an injected pair, confirming the mechanism is the missing abort classification, not the transport.
B2 P3: src/api.js:656
A6 per-request serialized native reads remain at the final head, and the nansen-pr-reviewer bot's proposed fix does not remove their cost. The bot claimed each createAuthState instance has no shared queue state and suggested passing a single instance from runCLI. The queue is a module-level Map keyed by directory at auth-state.js:8 and used at lines 75 through 82, so every instance in one process already serializes strictly. api.js:656 still falls back to createAuthState() per request, cli.js:2283 constructs NansenAPI without authState, and each session-selected request spawns one guarded helper process per store chunk plus one for the manifest, each now also starting a worker thread inside the helper. A's 136, 607, and 2466 ms figures predate that thread boundary, and docs/browser-login.md correctly requires remeasurement at the final head. I accept the deferral for the gated prerelease and record the latency as the remaining cost of per-request epoch and generation revalidation, but it needs measured final-head numbers and a decision before any promotion beyond the prerelease, because nansen agent and batch workloads share this path and scale it linearly.
Evidence:
Probe: two independent createAuthState instances, same directory, concurrent begin() calls. The event log shows all four of instance A's preflight operations completing before instance B's first starts, zero overlapping starts. Source contradicts the bot on the queue point: auth-state.js:8 declares const queues = new Map() at module scope, and locked() keeps per-directory tails in it. Source supports the bot on the residual cost: api.js:656 reads (this.authState || createAuthState()).readSession(this.selection) and cli.js:2283 passes no authState, so API-key and anonymous requests carry zero native overhead while session requests pay helper spawn plus keyring read each time. The bot's suggested constructor change would only reuse an object, it would not remove the native I/O it describes.
B3 P3: src/auth-state.js:18
A3 compatibility disagreement preserved for the release record. Legacy key saving and logout hard-require the fs-native-extensions lock binding, so an install that omits optional dependencies, or a musl or unverified architecture with no prebuild, cannot replace or delete a saved plaintext key through the CLI. I accept the fail-closed design: a second lockfile protocol could not exclude a native-lock owner, the AUTH_LOCK_UNAVAILABLE message names the packaged offline recovery procedure, docs/browser-login.md excludes musl and unverified installs from cohort mutation support, and package.test.js proves environment and explicit API-key requests still work without the native binding. Reviewer A's compatibility reading stays on record as the minority position; my disposition is that the documentation and packaging obligations are met and the remaining behavior is deliberate.
Evidence:
auth-state.js:18 throws AUTH_LOCK_UNAVAILABLE from loadLocks, which every begin(), install(), logout(), and recover() route reaches through locked(). The packaged offline guide in docs/browser-login.md#offline-recovery-without-native-locking covers legacy-key-only removal and explicitly refuses session-generations. package.test.js installs the tarball with --omit=optional and asserts the key-auth-without-native-ok smoke result. fs-native-extensions 1.5.1 ships darwin/linux/mobile and win32 prebuilds only, no musl build.
B4 P3: README.md:29
README option 2 still claims, without a qualifier, that after funding an x402 wallet the CLI signs Payment-Signature headers automatically on 402 responses. Automatic signing is anonymous-only at this head: mayAutoPay requires this.selection.kind === 'anonymous', so a funded wallet does not trigger automatic payment when a saved API key or browser session is selected. The new prerelease section states the qualifier a few lines below, but option 2 is the first place a subscribed user with a funded wallet looks, and A4 explicitly asked for this README qualifier. Add the anonymous-only condition to line 29 or point the reader at the prerelease section.
Evidence:
README.md line 29 reads: 2. x402 micropayment (no key needed): nansen wallet create, fund with USDC on Base or Solana, or USDT0 on X Layer, then call any endpoint - the CLI signs Payment-Signature headers automatically on 402 responses. api.js gates mayAutoPay on this.selection.kind === 'anonymous', confirmed by the four-case selected-key test in auth-login.test.js and by the 83e2 repair reply, which claimed the README was updated. The changeset documents the change; only this README line is stale.
Checks
-
Worktree sits detached at the exact head 83d6f5d, clean, three commits. GitHub API readback confirms base cc4aa18, draft state, three issue comments, one combined CHANGES_REQUESTED review at b907aff, and no other reviews.
-
Full delta reviewed: 39 files, +2288/-273, including the 83e2f0d and 83d6f5d repair diffs. Every changed production module read in full, not only hunks: api.js, auth-browser, auth-credentials, auth-device, auth-login, auth-state, auth-store, auth-store-native, auth-store-worker, cli.js, agent, completion, mcp, doctor, schema.json, package.json, changeset, docs.
-
npm ci (278 packages, both optional native bindings present), then the full suite at the exact head on darwin arm64 Node 22.23.2: 83 test files passed, 3270 passed, 9 skipped, 123.2 s. Matches the writer's reported totals exactly.
-
npm run lint exits 0 with no output, git diff --check passes, and scripts/check-changeset.js passes. The major changeset is accurate on plain-login script break, anonymous-only auto-payment, and helper custody.
-
Re-ran the six committed guardian lifetime plus timeout tests independently with real fs-native-extensions locks on this Mac: all six pass. They drive the real dispatcher, worker, and state owner with only the synchronous executor backend replaced, covering parent SIGKILL with no late writer, paused helper retaining exclusion, watchdog kill of a blocked executor with no parent timer, lock contention refusing authority, parent loss after readiness before secrets, and bounded failure when close is never observed.
-
The new helper custody design holds up under the owner-interaction checks: the gate auth-store.lock is acquired by the helper before the parent sends any operation or secret, lives in the same directory as the state owner's locks, is never unlinked, and every read, write, and delete uses the same gate. The worker thread owns the blocking keyring call and the main helper thread kills the whole process on parent EOF or the ten-second deadline, so a blocked executor cannot outlive its guard. Success waits for executor exit and helper close. SIGSTOP cannot age-steal the gate, and an unobserved kill fails bounded within one second while retained exclusion makes cleanup report incomplete and keep the journal. No deadlock is possible between auth.lock in the state owner and auth-store.lock in the helper because only helpers ever take the latter, and the parent's auth-operations journal cannot be deleted while the gate is held.
-
Recovery boundedness verified in source and by probe: recover() drains at most eight journals per invocation, including pre-existing over-limit directories, logout commits its tombstone before cleanup and never bricks on the journal count, and repeated locked-store logouts held the journal count at one instead of growing per call. Unlocking then drains to zero in one logout. The strict journal parse validation caps generations at two entries.
-
The durable unissued marker works as advertised: it starts true, is durably cleared before the first poll, and SIGKILL before versus after the marker rename classifies remote cleanup as not_needed versus unconfirmed in the real-child test. The cancel window in B1 is the one classification gap I found in this mechanism.
-
Bot contradiction resolved by execution: two independent createAuthState instances on one directory serialize strictly through the module-level queues Map at auth-state.js:8 and 75-82. The bot's no-shared-queue premise is false, its suggested same-instance fix removes no native I/O, and the residual B2 cost stands.
-
The bot's attempt-null footgun at auth-login.js:61-63 is unreachable today: candidate is only assigned by onIssued inside pair(), which runs only after begin() has returned an attempt, and attempt is nulled only after the candidate-retire block or on the success path. The asymmetric guard is style debt, not a defect, matching the writer's disposition.
-
Session error mapping verified: kept 'The selected browser session was rejected. Run: nansen login.' for 401, insufficient_credits maps to CREDITS_EXHAUSTED and plan_upgrade_required stays distinct for both key and session publics in api.js and agent.js, arbitrary body strings and reflected secrets never enter session errors, and browserSessionResponseMeta relays only numeric credits and rate-limit metadata.
-
Payment boundary verified: mayAutoPay is anonymous-only, a selected key receiving 402 gets a top-up or manual-signature instruction, manual API-key signatures still send both headers, sessions with any apikey, authorization, or payment-signature header fail MIXED_CREDENTIALS before any network call, and no selection other than anonymous can trigger automatic signing.
-
Custody and secret hygiene re-checked at the final head: journals carry only operation and generation UUIDs, NDJSON events never contain device_code, tokens, the private JWK, or nonce, the helper receives no secret argv and an environment allowlist that excludes NANSEN_API_KEY and NODE_OPTIONS while forwarding DBUS_SESSION_BUS_ADDRESS and XDG_RUNTIME_DIR on Linux, the gate is a sibling of the single state owner, and the store format keeps the 2048-byte chunks, 16 KiB cap, SHA-256 manifest, generation binding, and bounded delete namespace intact.
-
Key-only seams stay key-only: commands/mcp.js install throws API_KEY_REQUIRED for a session selection, trading loadConfig().apiKey is null under a session, and mcp-verify reads only apiKey, so no path exports the browser Bearer to MCP, trading, or RPC.
-
Dead-code gate at the exact head: every new or changed export has a live caller, including browserSessionError and browserSessionResponseMeta in api.js and agent.js, defaultAuthState and cleanupMessage in cli.js and browserLogin, openAuthBrowser in browserLogin, trustedIssuer in device, browser, and credential modules, nativeStoreOperation in the createAuthStore default, and auth-store-native.js both instantiated by the guardian Worker and asserted shipped by package.test.js. The superseded saveConfig and deleteConfig writers are gone with no remaining references, the old one-frame stdin native executor is removed, no dormant refresh engine exists, and the login/logout entries moved from UNSCHEMA_COMMANDS into schema.json keep help safe.
-
CI at the exact head verified live through the GitHub API: run 35429538419, conclusion success, lint plus test on Node 20, 22, and 24. Root's Mac 11416-byte native bundle result, Linux Debian 12 Node 22 Secret Service 11455-byte roundtrip with locked-refusal and daemon-restart evidence, and the joint CLI/API 9190 ten-case parity result are accepted as controlled synthetic evidence, not live ledger or gateway proof.
-
No tracked file was modified during this review; all probes live under /private/tmp and use isolated temp directories with synthetic stores and stubbed transports.
Limitations
-
No real browser approval, live IdP, deployed gateway, or staging walkthrough. Session issuance, middleware, transport, and storage in the joint probe were controlled, not live Postgres ledger or staging accounts. The server-side gates SESSION_ACCESS_REVOCATION_ENABLED and BROWSER_SESSION_ACCOUNT_ENABLED remain default-off, so end-to-end acceptance stays unproven even though the CLI now diagnoses the missing-coverage case correctly.
-
Windows Credential Manager is unproven in execution: no 2560-byte blob-limit test, no Enterprise roaming observation, no desktop prompt cancellation, and no Windows lock-release timing verification. The optional platform workflow is still absent from the PR because the writer app token lacks workflow permission, so no CI matrix exercises native locks or stores on any OS.
-
Mac and Linux native proofs cover synthetic bundles on unlocked stores only. No locked-store prompt, cancelled OS prompt, reboot, logoff or OS login persistence, SSH or remote-terminal walkthrough, or synchronized home directory was exercised.
-
Power-loss durability remains unproven. POSIX file and directory fsync plus native readback do not establish all-OS power-loss behavior, and the docs concede this.
-
The A6 latency numbers predate the worker-thread boundary added at the final head. Final-head remeasurement is still owed before promotion, per the docs.
-
API507 refresh is absent by design, so expired-session re-login and refresh coordination with the shared owner have no runtime proof in this PR.
-
My probes used injected in-memory stores and stubbed fetch transports; I performed no real keychain writes and no funded calls. The frozen cross-repo pins 3e08f9e and 1e9bfcd were taken as given from prior evidence rather than re-verified against those repositories.
-
I did not read running peer reports or other mutable checkouts; the dedicated security re-review of this exact head remains the security pass's responsibility.
Dedicated security repair verification
Claude Code claude-opus-5, requested high reasoning. Bounded repair scope, not another general round.
Intent: Bounded dedicated SECURITY REPAIR verification of nansen-cli PR #657 at frozen head 83d6f5d against true base cc4aa18 — not a third general review. Scope: disposition of the initial dedicated-security finding S1, and adversarial verification of the security-relevant b907aff->83e2f0d->83d6f5d repair delta and its interactions: native guardian authority/lifetime, journal/recovery bounding, the durable unissued marker and ambiguity-safe cleanup classification, session error redaction/allowlisting, and the declared major compatibility changes. Read AGENTS.md, CONTRIBUTING, the pr-author security checklist, dead-code-prevention and blast-radius guidance, and RUN copies of API-503.md, API-506.md, API-506-review-focus.md, API-506-repair-handoff.md, API-506-orphan-handoff.md, API-506-orphan-helper-finding.md, API-506-repair-response.md, API-506-orphan-response.md, plus the completed initial reviewerA-round1 and security/api506-b907aff results. Fetched the current public PR body, comments, bot findings and exact-head check runs once, read-only. Ran the real committed source (real auth-state, auth-store dispatcher, auth-store-worker guardian, real fs-native-extensions locks, real child processes, real NansenAPI and real agent command) with only the native keyring executor (auth-store-native.js) and HTTP transports replaced by deterministic synthetic backends, in isolated temporary HOME directories. No source edits, no commits, no GitHub writes, no agents, no model change, no access to existing user credentials or the real OS keychain, no staging/production/paid calls, and no inspection of peer reports or other mutable checkouts.
Verdict: changes_requested.
Findings
R1 P3: src/auth-state.js:149
The S1 repair fixed the self-inflicted journal brick but left a predicate mismatch that reintroduces the same class from a foreign precondition. begin()'s admission counter (line 149) counts every file ending in .json, while recover()'s drain loop (line 119) only selects /^[a-f0-9-]{36}\.json$/. Any eight .json files in ~/.nansen/auth-operations that the drain regex does not match therefore saturate admission permanently: nansen login fails AUTH_CLEANUP_REQUIRED forever, and the remedy the error prints — nansen logout — can never clear them, because recover() does not see them. Logout now correctly commits its tombstone and reports {local:'pending'}, so it is truthful, but it is also permanently ineffective.
A second, related gap: JSON.parse at line 128 sits inside a try { } finally { } with no catch, so one unparseable <uuid>.json journal propagates a raw SyntaxError with code: undefined and the message Expected property name or '}' in JSON at position 2 straight to the user. That violates the AGENTS.md actionable-error rule, carries no error code for machine consumers, and again leaves login permanently bricked because the offending journal is never removed. (The initial security review listed this at b907aff as a lower-confidence observation and stated it raised AUTH_STATE_INVALID pointing at config.json; that description is not what the code does at either head — no mapping occurs.)
Neither case has a documented recovery. No message, auth status field, or doctor check names ~/.nansen/auth-operations, and docs/browser-login.md:68 is the only mention of the directory in the repository — it instructs the user to not delete it ("Do not delete auth-operations, lock files or OS credential entries").
Reachability requires files the CLI itself does not create, so this is not the self-inflicted P2 that S1 was. It is plausible on the environments this PR explicitly claims to support but has not tested: synchronized/network home directories (a conflicted-copy <uuid> (conflicted copy).json or a half-synced journal), backup/restore tooling, or an older CLI version sharing ~/.nansen — a combination docs/browser-login.md itself calls unsupported but does not prevent.
Suggested direction: make the admission counter and the drain filter use the same predicate, delete or quarantine (with a bounded rename) entries the drain loop rejects, wrap the journal parse so a corrupt file yields an AuthError naming ~/.nansen/auth-operations, and stop telling users not to remove journals in the one case where removing them is the only fix.
Evidence:
Real createAuthState/createAuthStore source, real fs-native-extensions locks, isolated mkdtemp HOME, injected store only. Node v22.23.2, darwin arm64, head 83d6f5d.
Case 1 — eight foreign .json files (leftover-0.json .. leftover-7.json):
baseline login OK, journals= 0
login: AUTH_CLEANUP_REQUIRED "Unlock the credential store and run nansen logout to finish cleanup." journals=8
logout: removed=false cleanup=[{"local":"pending","remote":"not_attempted"}] journals=8
login: AUTH_CLEANUP_REQUIRED "Unlock the credential store and run nansen logout to finish cleanup." journals=8
Case 2 — one corrupt <uuid>.json containing { not json:
corrupt/login: code=undefined "Expected property name or '}' in JSON at position 2 (line 1 column 3)" journals=1
corrupt/logout: removed=false cleanup=[{"local":"incomplete","remote":"unconfirmed"}] journals=1
corrupt/login: code=undefined "Expected property name or '}' in JSON at position 2 (line 1 column 3)" journals=1
Repository-wide search for auth-operations outside src/tests returns exactly src/auth-state.js:22 and docs/browser-login.md:68 (the "Do not delete" instruction). Probe scripts were written under /tmp and removed; the checkout stayed clean at 83d6f5d.
verdict: CONFIRMED
category: availability-of-credential-cleanup
short_summary: Undrainable journal files brick login with no documented fix
failure_scenario: A synchronized or restored ~/.nansen leaves eight non-UUID .json files (or one unparseable <uuid>.json) in ~/.nansen/auth-operations. Every subsequent nansen login fails — AUTH_CLEANUP_REQUIRED in the first case, a raw SyntaxError with code: undefined in the second. nansen logout, the remedy the CLI prints, commits its tombstone but can never drain those files, and the only document that names the directory tells the user not to delete it. Browser pairing and session cleanup stay unavailable until the user guesses to remove the directory by hand.
R2 P3: src/auth-state.js:167
markIssuancePossible is wired to onBeforePoll (auth-login.js:41) and therefore runs before every device-token poll, not just the first. Its cheap early exit — if (!attempt.journal.unissued) return; — is placed inside locked() (line 168), so each poll still acquires the process queue and the cross-process native auth.lock even after the durable marker has already been cleared and there is provably nothing to write.
Uncontended this is negligible (30 no-op calls in 2 ms). Under cross-process contention it is a new abort path for a healthy, in-progress browser login: acquire() spins to its 15-second deadline and throws AUTH_BUSY out of pairDevice, killing the attempt while the user is still at the browser consent screen. A concurrent nansen logout against a slow or unresponsive credential store holds auth.lock for far longer than 15 s — store.remove alone issues 16 gated native operations, each with a 10-second helper deadline (measured at 1.33 s per generation even with a zero-cost executor, see R3) — so this is reachable without any adversary.
The outcome is fail-safe (nothing was issued, the previous credential is untouched, the marker is already durable, and aborting before the poll means the device code is never redeemed), which is why this is P3 and not higher. But it is a robustness regression introduced by the A1 repair: before it, polling took no global lock and the attempt only contended once, at install(). Hoisting the unissued check outside locked() removes the entire path.
Evidence:
Real createAuthState, real fs-native-extensions lock, isolated mkdtemp directory, Node v22.23.2 darwin arm64, head 83d6f5d:
journal unissued before=true after=false <- durable marker cleared before polling (A1 repair confirmed)
30 no-op markIssuancePossible calls: 2 ms <- early exit works, but still enters locked()
holder: locked <- separate real process holds
no-op mark under contention: THREW AUTH_BUSY after 15011 ms
The holder was an independent node -e process taking the same auth.lock via fs-native-extensions tryLock; the marker had already been cleared, so the acquisition that failed was performing no work.
verdict: CONFIRMED
category: robustness-regression
short_summary: Every poll re-locks auth.lock; AUTH_BUSY aborts live login
failure_scenario: User starts nansen login; the marker is cleared on the first poll. In another terminal a nansen logout runs against a slow or locked OS credential store and holds ~/.nansen/auth.lock for more than 15 seconds. The next poll's no-op markIssuancePossible blocks 15 s and throws AUTH_BUSY, aborting the login while the user is mid-approval in the browser, even though the call had nothing to write.
R3 P3: src/auth-store-worker.js:30
Requested independent assessment of the deferred A6 concern at the current head, which docs/browser-login.md itself flags as needing remeasurement ("Those numbers precede the helper-thread repair, which adds thread startup work; final-head latency needs remeasurement").
Measured on this head with a zero-cost synthetic executor — no keyring work at all, so this is pure custody overhead: 69 ms per gated native operation, 474 ms for one store.read of a realistic 11,134-byte / 6-chunk session bundle, 942 ms for store.write, 1,332 ms for store.remove (16 gated operations). Every session-selected API request performs exactly one store.read through readSession, and all of it serializes on the single cross-process auth.lock, whose acquire() gives up at 15 s with AUTH_BUSY.
The custody repair made this worse, not better: each operation now pays a child-process spawn plus a worker-thread instantiation (line 30) and an auth-store.lock acquisition, where b907aff paid only the spawn. At ~0.5 s of serialized overhead per session request before any Keychain/Secret Service time, roughly 30 concurrent session-selected commands are enough to start failing on AUTH_BUSY with nothing wrong with the credential — an availability ceiling on the authenticated path that API-507's refresh work will inherit.
I agree with the writer that no token cache should be bolted on without preserving logout/replacement invalidation, and that the latest bot's suggested fix does not help: the bot's premise that createAuthState() instances have "no shared queue state" is wrong — queues is a module-level Map keyed by directory (auth-state.js:8, used at 75-82), so same-process serialization already holds, and passing a shared authState would not remove a single native read. The cost is the per-request gated store I/O itself. Recording the current-head numbers so the deferral is made on measured rather than stale evidence.
Evidence:
Real createAuthStore + real auth-store.js dispatcher + real auth-store-worker.js guardian + real fs-native-extensions gate; only auth-store-native.js replaced with a synthetic filesystem executor (per the orphan handoff's stated reproduction procedure). Isolated HOME, Node v22.23.2, darwin arm64, head 83d6f5d:
{"bundleBytes":11134,"chunks":6,"writeMs":942,"readMsAvg":474,"removeMs":1332,"singleOpMsAvg":69}
readMsAvg is the mean of 5 sequential full-bundle reads; singleOpMsAvg the mean of 5 single get operations. Real keyring latency is additive on top of every one of these numbers. For comparison the reviewer-A figures retained in docs/browser-login.md (136 ms single, 607 ms four concurrent, 2,466 ms sixteen concurrent) were taken at b907aff before the worker-thread boundary existed.
verdict: CONFIRMED
category: efficiency
short_summary: ~474 ms custody overhead per session API request at this head
failure_scenario: An agent workload issues session-selected Nansen calls concurrently. Each one serializes on auth.lock and spends ~474 ms of pure process/thread/lock overhead in store.read before any keyring work. Past roughly 30 concurrent commands the 15-second acquire() deadline is exceeded and commands fail with AUTH_BUSY despite a valid, unexpired session.
Checks
-
Initial dedicated-security S1 (P2, journal ceiling bricks login and logout, logout is the trigger) — CONFIRMED REPAIRED. Reproduced the original scenario against real createAuthState with only a store that throws AUTH_STORE_UNAVAILABLE: 8 failed logins leave 8 journals, login 9/10 fail AUTH_CLEANUP_REQUIRED without growth, and three consecutive
logoutcalls now return normally (removed=false, 8 x {remote:'not_needed',local:'incomplete'} plus {local:'pending',remote:'not_attempted'}) instead of throwing and appending a 9th, 10th, 11th journal. A planted pre-existing over-limit directory (20 journals) drains 8 per invocation once the store unlocks (20 -> 12 -> 4 -> 0) and login then succeeds. Config ends at the correct version-1 tombstone. -
Logout truthfulness — VERIFIED. With an active session and a locked store, only the first logout reports removed=true; repeats report removed=false with {remote:'unconfirmed',local:'incomplete'} plus the pending marker, and journal count stays at 1 across six repeats. Twelve further locked logins cap at 8 journals. After unlock, one logout drains everything, retire is invoked exactly once for the displaced generation, and zero store entries remain. A journal is created only for a displaced session, and the tombstone is committed before cleanup (recover() wrapped in try/catch so a cleanup failure cannot un-commit the logout).
-
A1 durable unissued marker and ambiguity-safe classification — CONFIRMED REPAIRED, four end-to-end browserLogin runs on real pairDevice/createDeviceClient/createAuthState with injected fetch. (a) denial on the first poll: PAIRING_DENIED, cleanup [{remote:'not_needed',local:'removed'}], NO spurious 'Remote revocation unconfirmed' message. (b) transport failure then expired_token: provenUnissued correctly false, cleanup {remote:'unconfirmed'} and the warning is retained. (c) issuance then account-verify 500: candidate retirement result recorded into journal.candidateRemote and reported once, no contradictory second warning. (d) preflight failure before approval: terminal error only, no pending event, no journal, no store entry. Verified directly that the marker write precedes any token request: journal on disk shows unissued=true before markIssuancePossible and unissued=false after, and pairDevice awaits onBeforePoll before every client.request('/auth/device/token').
-
Unissued-marker soundness traced for the false-negative direction (the one that could leave a live upstream session silently deleted locally): the only writers of unissued=false are markIssuancePossible (before the first poll, atomic+fsync) and install() (before store.write); the only writer of unissued=true is begin() and the provenUnissued branch, which requires failures===0, i.e. no network error, no 429/5xx and no slow_down ever observed. Every ambiguous outcome increments failures and falls back to the conservative retire+warn path. In-memory candidateRemote/unissued values that are lost to a crash degrade to the conservative path, never the permissive one. recover() validates unissued as boolean and candidateRemote against the ['unconfirmed','recorded_pending','refresh_only'] allowlist before use; 'not_needed' cannot be forged through a journal file.
-
Native guardian authority/lifetime — CONFIRMED REPAIRED with real children. Real auth-store.js dispatcher + real auth-store-worker.js guardian + real fs-native-extensions locks, only auth-store-native.js replaced by a synthetic executor that blocks its thread on a filesystem barrier. (1) Roundtrip works and
/auth-store.lock is created. (2) Helper argv is exactlynode .../auth-store-worker.js— no secret in argv, and the env allowlist excludes NODE_OPTIONS and all API-key variables. (3) A second store operation attempted while a helper holds the gate is REJECTED with AUTH_STORE_UNAVAILABLE, never resolved — authority is withheld, not queued. (4) Parent SIGKILL while the executor thread is blocked: the helper is GONE within 1 s (main-thread EOF watchdog SIGKILLs the whole process despite the blocked thread), a fresh recovery process then reports 'removed', and releasing the barrier afterwards recreates nothing. (5) SIGSTOP the helper, then SIGKILL the parent: the helper stays alive in state T, retains exclusion, and a fresh recovery fails closed with AUTH_STORE_UNAVAILABLE rather than claiming deletion; after SIGCONT the orphan does complete its late write of chunk .0, and because recovery had already failed closed and retained its journal, the next recovery removes it and ends at zero entries. This is precisely the contract in the orphan handoff, verified independently. -
Store-gate interleaving analysed and found closed: the gate is acquired with a non-blocking tryLock before readiness and held for the entire single operation (phase transitions guard/operation/running admit exactly one operation per process), so a helper that loses the race dies rather than waiting to write after recovery has cleaned.
auth-store.locklives in the auth directory, matches none of recover()'s tmp/lock cleanup patterns, and is never unlinked; the committed lifetime test additionally asserts inode stability. No inherited-handle assumption: each helper opens and locks its own descriptor. No deadlock: the parent never waits on auth-store.lock, and all store access is reached under auth.lock. -
Helper hardening reviewed: directory argument must be absolute and passes a symlink/ownership/group-and-other-write check before mkdir 0700 and open 0600; account names are re-validated server-side against /^[a-f0-9-]{36}.(probe|manifest|[0-7])$/; the worker thread is created with stdout:true/stderr:true and both are drained, so an addon writing to the JS stdout cannot corrupt the NDJSON protocol or leak into the parent; a 40 KB input cap, a 10 s self-deadline, and stdin end/error plus stdout error all route to whole-process SIGKILL. The parent resolves only on an actual
closewith code 0, areadyhandshake, exactly onevaluemessage and no trailing bytes; every other path rejects with AUTH_STORE_UNAVAILABLE, and an unobserved kill becomes a bounded 1-second failure that unrefs local handles without claiming termination. -
A2 and the joint credit/plan error finding — CONFIRMED REPAIRED at both call sites. Real NansenAPI with a session selection and hostile bodies (planted secret in message/code/detail/x-request-id plus attacker paymentRequirements): 401 -> UNAUTHORIZED with the session wording 'The selected browser session was rejected. Run: nansen login.' (no longer overwritten by 'Not logged in'), 403 insufficient_credits -> CREDITS_EXHAUSTED, 403 plan_upgrade_required -> plan_upgrade_required, 403 with an unallowlisted code -> generic FORBIDDEN, 402/500/429 -> fixed generic text. details reduced to {attempt, retryAfterMs} in every case; no server body, no request id, no notice string, no paymentRequirements, no access/refresh token and no private JWK 'd' in message or details. Real
nansen agentcommand path gives the same five categories with details {} and identical redaction. browserSessionError reads the raw body only to compare against two literals and calls statusToErrorCode with no data, so server-controlled codes cannot pass through on the session path. -
A4 selected-key 402 no-autopay major compatibility — VERIFIED. api-key selection + 402 produces the explicit top-up / --x402-payment-signature message with no wallet import attempted; anonymous + 402 still enters the full autopay chain; session + Payment-Signature header throws MIXED_CREDENTIALS with zero fetch calls. The change is now stated in .changeset/browser-session-login.md (major) and README.md option 2, closing the initial review's release-note reconciliation note.
-
A5 revocation-coverage setup diagnostic — VERIFIED. validateSession now raises BROWSER_SESSION_SETUP_REQUIRED naming SESSION_ACCESS_REVOCATION_ENABLED and BROWSER_SESSION_ACCOUNT_ENABLED instead of an unbounded re-pairing loop, without asserting the server's actual configuration. retireSession still accepts only the pinned recorded/complete_family/pending/version-1 receipt or the legacy access_tokens_revoked:false shape; any other or future shape, including a hypothetical access_tokens_revoked:true, stays 'unconfirmed'. I agree with the writer's conservative disposition of the bot's medium on this point.
-
A7 / origin and doctor wording — VERIFIED corrected: the duplicate README --human example is gone, doctor no longer claims plain login re-saves the default base URL, and AUTH_ORIGIN_UNSUPPORTED now distinguishes an env override from config.json baseUrl via baseUrlSource. docs/browser-login.md gained an honest 'Offline recovery without native locking' section; package.json now ships docs/browser-login.md and src/auth-store-native.js, and src/tests/package.test.js:56-57 asserts both are packaged, so the error message's documentation target actually exists in an installed copy.
-
cli.js isTTY/stdinTTY split reviewed for credential-echo risk and found safe: prompt() gates hidden masking on input.isTTY (stdin) and writes the prompt and mask characters to stderr, so allowing --human when stdout is redirected cannot echo an API key to a captured stdout; browser login output mode remains machine mode independently.
-
Selection, MCP and trading boundaries re-checked for repair-delta interaction: resolveCredential/authConfigView remain the single seam; the delta touches no selection precedence; src/commands/mcp.js, src/mcp-verify.js and src/trading.js are unchanged between b907aff and 83d6f5d, so the initial review's key-only custody finding still holds. doctor.js and
auth statusremain offline — they read config only, never open the secure store, and continue to label session metadata cached/unverified. -
Full suite at the exact head after a clean
npm ci: 83 test files passed, 3,270 passed / 9 skipped (3,279), exit 0, 121.45 s — matches the orphan handoff exactly.npm run lint(eslint .) clean. Working tree clean at 83d6f5d throughout; no tracked file modified, no stray helper process left behind, all probe directories removed. -
src/tests/auth-store-lifetime.test.js read in full and judged substantive, not vacuous: five real-child cases covering parent SIGKILL with successful recovery and inode stability, a paused helper retaining exclusion with retained journals then draining after actual death, the helper watchdog killing a synchronously blocked executor with no parent-side timer, refusal of write authority under gate contention (asserting empty stdout, i.e. no readiness), and parent loss after readiness but before any credential. My independent probes reproduce the same conclusions against the same production source.
-
Public PR state read once, read-only: PR #657 open, draft=true, author app/hulk-linus, base main, headRefOid 83d6f5d, reviewDecision CHANGES_REQUESTED (the standing combined review at b907aff). Exact-head check runs: lint SUCCESS, test (20)/(22)/(24) SUCCESS, pr-reviewer success, publish/sync-clawhub/Aikido skipped. Three issue comments only; no new human feedback since the custody repair reply.
-
Latest bot medium independently assessed and rejected on its stated premise:
queuesis a module-level Map keyed by directory (auth-state.js:8, read/written at 75-82), so separate createAuthState instances in one process do share the serialization queue; the bot's claim that they do not, and its suggested same-instance fix, are both wrong as stated. The residual per-request native I/O cost it gestures at is real and is quantified in R3 instead. Latest bot low (attempt-null footgun at auth-login.js:61-63) independently confirmed unreachable at this head:candidateis assigned only by the onIssued callback inside pair(), which runs strictly afterattemptis assigned, and the catch block nullsattemptonly on its way out. Maintainability nit, not a defect; not raised as a finding.
Limitations
-
Retained disagreements are preserved, not resolved. A3 (mandatory native locking for legacy key writes and logout) remains an open compatibility disagreement between reviewer A and the dedicated security position: I continue to read fail-closed as intentional and correct for credential mutation, and I agree with the writer that an independent O_EXCL fallback would not exclude a native-lock owner, so it would create two uncoordinated writer protocols rather than one. The repair's mitigation is documentation plus a packaged legacy-key-only offline guide, not a code fallback; A's position that losing the ability to delete a saved key on an --omit=optional/musl/no-prebuild install is a compatibility defect is untouched by anything I verified. A6 is deferred by the writer; R3 supplies the current-head measurement the writer's own documentation says is needed, and does not by itself settle the deferral.
-
All native credential-store behaviour I exercised used a synthetic executor substituted for auth-store-native.js, exactly as the orphan handoff prescribes for reproduction. I deliberately did not touch the real macOS Keychain, created no native entries, and read no existing user credential. Real OS-store persistence, locked-store prompting, desktop prompt cancellation, and cross-login durability remain unverified by me. Root's Mac 11,416-byte bundle result and the Debian 12 / GNOME Secret Service 42.1 11,455-byte result both stand on their own stated terms and both predate the repaired thread/gate boundary (they were taken at b907aff); I did not re-verify them and they do not transfer to this head.
-
One residual in the same class as the original orphan finding is beyond synthetic reproduction and stays open: the guardian's SIGKILL terminates the client process, but a keyring write already dispatched to an out-of-process credential daemon (D-Bus/gnome-keyring on Linux, securityd on macOS) can still be applied server-side after the client dies and after the gate is released. In that window a recovery that legitimately observes a free gate could report 'removed', unlink the journal, and have the daemon land a chunk afterwards, leaving unreadable but real secret bytes in the OS store after a logout that reported completion. The bundle would be unusable (manifest validation fails) and re-login would clear it, but I could not test this without a real credential store and it is not covered by the committed tests or by either external native result.
-
No real browser approval, no real authservice or public gateway call, no staging walkthrough from a local or remote terminal, no live ledger, no zero-credit/zero-debit evidence, no cohort or rollout acceptance. Server pins auth 3e08f9e7 and API 1e9bfcd were taken as given; I did not re-verify those repositories. Windows (LockFileEx release semantics after termination), Linux at this head, musl/Alpine, network or synchronized home directories, reboot/logoff/PAM and power loss all remain unverified — R1's plausible trigger sits squarely in the untested synchronized-home-directory case.
-
One unintended outbound request: in the issued-then-verify-failure probe, browserLogin's candidate retirement calls the module-level retireSession, which is not injectable through browserLogin's options, so a single POST to https://idp.nansen.ai/token/revoke carrying a synthetic refresh token ('RT-SECRET') left this host. It involved no real user credential, no account and no funded operation, and the call's failure is what produced the 'unconfirmed' classification I recorded. Noting it because it is a real network call, and because the non-injectability means candidate retirement itself cannot be exercised offline.
-
Per the assignment this was a bounded repair verification, not a third general review: I did not re-derive the parent architecture, re-review unchanged modules outside the delta's interaction surface, or run a general-purpose correctness pass. Coverage outside credential custody, trust boundaries, revocation, concurrency/cleanup, error redaction and the declared compatibility changes is intentionally incomplete. I did not read peer reviewer drafts, running peer reports, or any other checkout.
-
Two lower-value observations recorded without raising them as findings. (1)
state.storeis returned by createAuthState (auth-state.js:145) with no production consumer anywhere in src/ — a dead export today, and a surface through which API-507 could reach the native store without holding auth.lock; the handoff's instruction that API-507 must not bypass the boundary is documentation, not enforcement. (2) begin()'s failure path calls cleanJournal insidetry { } finally { }with no catch, so if atomic() fails before the journal exists, the resulting ENOENT from fs.unlinkSync replaces the original error; same non-mapped-error family as R1's second half but I could not construct a realistic trigger.
Orchestrator evidence and report qualifications
A separate public-command telemetry probe at this exact head captured login SYNTHETIC-USER-CODE --chain synthetic-private-host.example as path /login/SYNTHETIC-USER-CODE and properties.chain with that hostname. Pairing failure was injected; real parsing and telemetry serialization ran. Fetch was captured locally, no outgoing analytics, no real secrets or existing user metadata. Fixed auth paths, allowed flag names and error categories must prevent arbitrary positional/options/code data from entering telemetry, including failure reporting, while preserving both opt-outs and offline diagnostics. This is an additional repair item.
Current-head real native evidence supplied to both reviewers is newer than the old b907 evidence described in one security limitation. Root independently ran the actual83d6 guardian/thread/store on macOS (11,416-byte owned synthetic bundle) and isolated Debian12/Node22/GNOME42.1 SecretService (11,455 stored bytes). Both roundtrips and deletion passed; Linux locked-collection preflight refused in128ms, restart/unlock preserved the bundle, then deletion passed. These do not prove Windows, desktop prompts, OSlogin/reboot, public browser ceremony or daemon-in-flight cancellation. Preserve the daemon-side outstanding-write limitation explicitly; terminating the client process is not by itself proof that an external credential daemon cannot complete work later.
Root actual83d6 state/store/native reads on one unlocked Mac, 11,416-byte bundle, separate owner instances in the same process:1read798ms;4concurrent2956ms;16concurrent11764ms; all results equal and owned entries removed. This measures current absolute cost, not a controlled before/after slowdown ratio because A6's earlier bundle size was unspecified. B2/R3 remain measured release decisions; object reuse does not remove native I/O. The reviewers' concurrency timeout projections are not a measured fleet or cross-process throughput result.
B4 is supported by the literal README option2 text: it still lacks the anonymous-only qualifier. The security report's statement that option2 was corrected is inaccurate; its other payment boundary checks stand. Also, slow_down does not increment the current failures counter; it is a known no-token response. B1 repair must preserve ambiguity from any earlier transport/5xx/unknown response and must never infer no issuance merely because no request is currently in flight.
R1's suggested deletion/quarantine must not discard possible cleanup authority. Use consistent recognized-journal predicates and actionable bounded corruption handling; retain corrupt/unknown material and explicit operator recovery rather than blindly deleting credential journals. Supported-environment restrictions do not substitute for safe diagnostics.
Security disclosed one unintended POST to the production revoke endpoint with a synthetic refresh token in a verification-failure probe. It used no real credential or funded operation. That execution limit is preserved above. Subsequent repair tests must fail on outbound traffic and intercept candidate retirement as well as issuance.
Required exact-head CI35429538419 passed lint and Node20/22/24 (3277pass,2skip each). Root six guardian fault tests and ten actual CLI/API selection/billing/error cases also passed. Joint issuance/gateway/storage are controlled fixtures, not live PostgreSQL ledger or staging proof. Remaining operator, native-platform and staging gates prevent rollout acceptance or final approval.
|
Final B/security/root repairs are in 294a511, based on
All unit tests now have a default global outbound-fetch block. Candidate retirement has a narrow injectable function in Native evidence correction: root's Mac and Debian12/Node22/GNOME42.1 proofs are at The ten joint synthetic cases bind API API507 rebase contract: keep one Validation at the repaired head
Exact-head CI run 35431763708 passed at AI execution metadata
|
|
Disposition of the edited bot comment at frozen head
The PR body's Root's bounded final repair verification passed 43 tests and 12 public telemetry cases at this head. The existing full suite and lint/Node20/22/24 CI remain passing evidence; no source changed or test rerun was needed for this administration. This is writer feedback disposition, not self-review or approval. The native daemon outstanding-write gap, platform acceptance, latency/cohort decisions, retained A minority compatibility concern and live staging/gateway/ledger limits remain. No new review request, commit, workflow change, merge, release or ticket closure. |
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Review A at 35fb7d46fc14bf90d1f654aa54e355c9ccf92500. Factory Kimi K3, high reasoning.
Changes requested for the anonymous hosted-simulation regression. The native-binding observation is explicitly nonblocking in the reviewer report.
A657-1 (P2) src/swap-simulation.js:226
The new if (hosted && auth.api && [401, 403].includes(res.status)) throw SIMULATION_ACCESS_DENIED check fires for every NansenAPI instance passed to hosted simulation, including selection.kind === 'anonymous' (x402 / no-credential traders), because auth.api is truthy regardless of selection kind and requestCredentials() returns {} for anonymous. At the parent commit, an anonymous call to the hosted endpoint that got a 401 fell into the pre-existing !ok branch as SIM_RPC_ERROR, which verifySwapOutcome degrades (warn + proceed). At this head the AuthError bypasses the degrade list (trading.js ~1165) and returns proceed:false, so every candidate quote is skipped and trade execute fails outright for a user with no selected account. The error text ('The selected account cannot access hosted simulation') is also wrong when no account is selected. The parity suite covers api-key and session kinds only; the anonymous kind against a hosted 401/403 is untested. Suggested fix: gate the 401/403 block on selection.kind !== 'anonymous' (or map anonymous 401/403 to SIM_RPC_ERROR) and add a parity test for the anonymous kind. The intended hardening for selected api-key/session credentials (block instead of degrade past an auth failure) is preserved by that gate.
Evidence: Local probe /tmp/probe-anon-sim.mjs at head 35fb7d4 with stubbed fetch returning 401 and an anonymous NansenAPI: selection kind: anonymous; simulateAssetChanges threw AuthError SIMULATION_ACCESS_DENIED; verifySwapOutcome returned {"proceed":false,"reason":"The selected account cannot access hosted simulation..."}. Code: src/swap-simulation.js:214-226 (hosted && auth.api branch), src/swap-simulation.js:483 (redaction wrap only for session kind), src/trading.js:1160-1170 (degrade list covers only SwapSimulationError NO_SIM_RPC/NOT_SIM_CAPABLE/SIM_RPC_ERROR). Tests in src/tests/auth-permission-parity.test.js iterate only ['api-key','session'].
A657-2 (P3) src/auth-state.js:20
Explicitly assessed as requested: loadLocks() makes the fs-native-extensions optional binding mandatory for every saved-credential mutation, so without the binding nansen login --human (legacy API-key save via cli.js authState.begin at cli.js:1233), browser login, and logout all fail with AUTH_LOCK_UNAVAILABLE. This is inherited byte-identical from parent 294a511 (git diff 294a511..35fb7d4 touches no state/store/helper/login source), is not introduced by the permission-parity head, and is mitigated: NANSEN_API_KEY keeps working, the error message is actionable and names the offline recovery path, and docs/browser-login.md plus the PR body disclose it with reviewer A's compatibility objection still open. I classify it as a documented rollout/compatibility gate, explicitly nonblocking, not a merge-blocking defect of this head. Storage limits verified in src/auth-store.js: 2048-byte chunks, 16 KiB MAX_BUNDLE_BYTES with fail-closed AUTH_BUNDLE_TOO_LARGE that leaves the previous credential unchanged, manifest digest verification, bounded 8-chunk removal namespace; refresh token capped at 4096 chars and issuance responses capped at 32 KiB in auth-device.js readResponse/validateSession. No unbounded secret growth path found.
Evidence: src/auth-state.js:20-23 (loadLocks throws AUTH_LOCK_UNAVAILABLE naming legacy-key impact), src/auth-store.js:8-9,76-110 (CHUNK_BYTES/MAX_BUNDLE_BYTES, manifest digest, bounded remove), src/auth-device.js:81 (refreshToken.length > 4096 rejected), src/auth-device.js:20-37 (32 KiB readResponse cap), cli.js:1233 (legacy key save goes through authState.begin), git diff 294a511..35fb7d4 --stat confirms no store/state/login changes at this head.
Reviewer checks
-
Verified checkout head is exactly 35fb7d4; working tree clean before and after review; no tracked edits made.
-
npm ci from lockfile, then
npm test -- --maxWorkers=2: 86 files passed, 3314 passed, 9 skipped, 3323 total, exit 0, 161s (matches the writer's claimed 3314/9; the writer's exit-137 run was not reproduced and is not counted). -
npm run lint passed; git diff --check cc4aa18..HEAD clean; changeset present and major-scoped (.changeset/browser-session-login.md).
-
Scope validation traced in src/auth-device.js: pairDevice requests scope 'nansen:api'; validateSession requires bundle.scope and JWT claims.scope exactly 'nansen:api', exact iss/aud via trustedIssuer (fixed production/staging map), ES256/P-256 alg pin, cnf.jkt binding to the stored private key, exp bounds, and session_access_revocation_version === 1; old nansen:read bundles and claim-level downgrades are rejected (test 'rejects old read grants rather than broadening their permissions'). No code path upgrades nansen:read; OAuth/MCP read grants untouched.
-
Smart-alert CRUD and all account endpoints share the single NansenAPI.request -> requestCredentials resolver (api.js:652, 2013-2045); no endpoint- or auth_method-keyed dispatch that could hide a parity gap. Session 401/403/429/503 errors are replaced with fixed strings and data={} (browserSessionError), so reflected secrets cannot echo into output; agent.js SSE path applies the same sanitizer and redirect:'error'.
-
Hosted simulation seam traced across all three production EVM call sites (trading.js:2936, 3319, 3639): all pass api: apiInstance; the old loadConfig().apiKey fallback is deleted and loadConfig is no longer imported by trading.js. requestCredentials enforces epoch/generation/account/audience match and MIXED_CREDENTIALS on extra auth headers; x402 mayAutoPay requires selection.kind === 'anonymous', so a session never falls back to payment.
-
Arbitrary-RPC proof: isNansenHostedUrl now requires exact origin match against {https://api.nansen.ai, https://api.banansen.dev} with userinfo rejected; non-hosted URLs skip requestCredentials entirely (no custody read, verified by the readSession spy test) and send only Content-Type; credential-bearing calls use redirect:'error'. Parity tests cover evil subdomain, http, alternate port, userinfo, origin mismatch before any fetch, logout invalidation (AUTH_SELECTION_CHANGED), and no token/private-key material in any request.
-
Wallet/service separation: trade quote/broadcast (getQuote/executeTransaction) send no account credentials (test asserts no Authorization/apikey); limit orders keep wallet-challenge JWT plus optional env NANSEN_API_KEY X-API-Key with redirect:'error', and no browser token is forwarded (limit-order.js:112-119, parity test); MCP install refuses to export a browser session and requires a real API key (commands/mcp.js). No API-key parity gap found in dispatch.
-
Store invariants reviewed at this head (byte-identical to parent): per-directory queue plus native tryLock with 15s bound, epoch/generation compare on install and readSession, journalized replacement with conservative unissued/candidateRemote handling, logout preserves wallets and retires only the session generation, no plaintext fallback and no secrets on argv or in helper env.
-
Local probe (/tmp/probe-anon-sim.mjs, stubbed fetch, no network) reproduced finding A657-1 at the exact head.
Reviewer limitations:
-
CI run 35475906734 green on Node 20/22/24 is corroborated by multiple run-state artifacts (cli657-result.md, continuation.md, writer-done-657.json, cli658-result.md) but I did not independently re-fetch it from GitHub; no GitHub reads or writes were performed.
-
Server-side acceptance of nansen:api with API-key-equivalent entitlement/plan/credit behavior, real consent flow, gateway/proxy, live-ledger billing, and revocation propagation are cross-service rollout gates outside this repository; they were not and could not be verified here, and they do not block this PR's merge safety as a gated prerelease.
-
No live auth, staging, gateway, funded-trade, or real-credential probes were run (prohibited); all verification used the repo's intercepted-fetch guard (test-isolation.js blocks all outbound fetch) plus one local stubbed probe.
-
Native secure-store runtime behavior (keyring roundtrip, latency, Windows/Enterprise limits) was not re-proven on this machine; the store/helper sources are byte-identical to parent 294a511 per git diff, so prior native evidence applies only to that unchanged code.
-
Solana simulation (simulateSolanaAssetChanges) carries no account credential by design and was reviewed by reading only; the Solana path is unaffected by the credential-seam change.
-
Child PR 658 adds renewal; this parent correctly does not claim renewal (validateSession throws SESSION_EXPIRED with 'Automatic renewal is not available in this prerelease'), so expired-session UX depends on the child landing.
-
Finding A657-1's real-world impact assumes the hosted simulate-swap endpoint returns 401/403 to anonymous callers, consistent with the code comments that it authenticates with the user's Nansen credential; I did not probe the live endpoint.
-
Severity labels in this resubmission are mapped to the required P0/P1/P2/P3 scale (A657-1 as P2, A657-2 as P3); finding content, evidence, verdict, and checks are unchanged from the original report.
|
Repairs at
Validation: Full AI execution metadataAI execution metadataProvider OpenAI; model
|
|
Assessed the three claims in the edited bot comment5739920044 at unchanged head
All nine feedback IDs in the current workflow snapshot have explicit retained dispositions. The historical repair reports and reviews remain evidence for their named heads. A657-1 remains fixed at2d4; A657-2 remains explicitly nonblocking, with the earlier A native-binding compatibility objection preserved for B. No alternative lock fallback is added. The prior repair reply and its limitations remain in force. No code, tests or packaged docs changed, so no redundant test run or commit. Existing exact-head CI35477865293 remains successful: lint and Node20/22/24 tests, each87files/3332passed/2skipped. Local validation remains87files/3325passed/9skipped. Native platform, daemon deletion, latency/cohort and live staging/gateway/ledger gates remain open. Root owns propagation to658/659; neither checkout was touched. This is writer feedback disposition, not review, approval or thread resolution. |
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Reviewer B: clean at 2d4bf29e8d1c8be74e920ae279fb8f06fc766a13. Limitations retained; not approval.
Independent reviewer B of nansen-cli PR 657 at head 2d4bf29 against base cc4aa18. Verified nansen:api account permission parity for browser login, the anonymous hosted-simulation repair, the safe home fix, the three current bot claims, custody and retirement boundaries, and that the native-binding, latency, and daemon acceptance limits stay preserved.
Checks
-
Worktree detached at 2d4bf29 with merge base and origin/main equal to cc4aa18. Commit chain is b907aff, 83e2f0d, 83d6f5d, 294a511, 35fb7d4, 2d4bf29. The 2d4 repair delta touches only swap-simulation.js, auth-credentials.js, two test files, docs, and the changeset, 75 insertions and 5 deletions. No state, store, worker, helper, or login source line changed at this head.
-
A657-1 is fixed in source and in tests. The hosted 401/403 denial guard in postSim requires auth.api.selection.kind !== 'anonymous'. Anonymous selections fall into the existing non-2xx branch as SIM_RPC_ERROR and verifySwapOutcome degrades with warn and proceed. Selected api-key and session selections return proceed:false with the fixed access message. The six-case matrix in auth-permission-parity.test.js asserts one fetch, exact headers per kind, redirect error for selected kinds, no payment attempt, no custody read for non-session kinds, and no token or private JWK material anywhere in the result or logs.
-
No credential can reach an arbitrary RPC. postSim attaches credentials only when isNansenHostedUrl matches one of the two fixed origins with userinfo rejected. Non-hosted endpoints skip requestCredentials entirely, which the custom-RPC test proves by spying on readSession and asserting bare Content-Type headers. A session requires the audience to equal its baseUrl or the call throws AUTH_ORIGIN_MISMATCH before fetch. Any extra apikey, authorization, or payment-signature header throws MIXED_CREDENTIALS before any request.
-
Payment boundary holds. mayAutoPay requires selection.kind anonymous plus no extra auth headers, so api-key and session selections can never trigger automatic signing. Session 402 responses return a fixed message stating no payment was attempted. The session 401 wording survives the generic UNAUTHORIZED rewrite because safeSessionError is truthy at that branch.
-
Scope rules hold. pairDevice requests nansen:api, and validateSession requires bundle.scope and claims.scope exactly nansen:api, ES256 with P-256, exact iss and aud from the fixed trustedIssuer map, cnf.jkt binding to the stored private key, bounded expiry, and revocation version 1. The old nansen:read bundles and downgraded claims are rejected by a dedicated test. No renewal engine exists in this parent; child 658 adds rotation.
-
Wallet and service separation holds. getQuote and executeTransaction send no account credential. Limit orders carry only the wallet challenge JWT plus an optional environment key with redirect error and no browser token. MCP install refuses to export a session and requires a real API key.
-
Custody and retirement boundaries hold for the bot claims. requestCredentials only calls readSession. The readSession callback checks epoch and generation, then reads the store. Neither locked() nor store.read performs recovery, cleanup, deletion, or retirement. recover() runs only from begin() and logout(). Production auth mutations inject defaultAuthState with retireSession. The store read validates the manifest, chunk sizes, SHA-256 digest, and generation binding; preflight, write, and remove carry the 16 KiB cap and the bounded eight-chunk namespace.
-
Bot TTY claim is an injection-only observation. The stdinTTY fallback dep.isTTY controls both injection defaults, but the production entry point passes no deps, so isTTY and stdinTTY default from their own process streams independently. buildCommands destructures stdinTTY = deps.isTTY ?? process.stdin.isTTY and runCLI passes the original deps object through. A regression supplies isTTY:false with stdinTTY:true and checks both prompting and browser output mode. auth-login.test.js has 220 lines, so the quoted line 1340 does not exist. The alias is intentional compatibility for existing injected callers; changing it would break them without fixing a production path. RunCLI constructs NansenAPI without authState, so the bot's suggested session assertion would break normal saved-session requests.
-
Bot claim about fallback readSession triggering silent recovery retirement describes a path that does not exist in this code. I traced every function on the alleged route and confirmed the writer's disposition. The residue is that direct createAuthState consumers must supply a real retire adapter for the mutations they add themselves; NansenAPI performs no such mutation.
-
Bot environment cleanup claim is speculative. auth-permission-parity.test.js has no vi.stubEnv or process.env mutation, passes an isolated env object to the resolver, and its afterEach restores globals, SIMULATION_RPCS.base, and temporary directories. Nothing was changed, correctly.
-
Double finish is not reproducible in source. In browserLogin, a rejection from state.finish inside the catch is swallowed locally and attempt is then nulled. An earlier retire throw exits before that block, leaving the finally to finish exactly once. The mixed key guard rejects empty string and zero because both are neither null nor undefined, which preserves the explicit-credential rejection.
-
Safe home fix verified. authDirectory prefers HOME then USERPROFILE, falls back to os.homedir(), and throws AUTH_HOME_UNAVAILABLE with a fixed actionable message before any file access when the result is missing, relative, non-string, or the lookup throws. Synthetic probes confirmed env precedence, the OS home fallback, refused relative paths, and no existsSync call before the throw. The five isolated tests in auth-home.test.js all pass.
-
One consequence of the home fix is on record. readAuthConfig resolves the home eagerly, so an environment key selection in an environment with no resolvable absolute home now throws AUTH_HOME_UNAVAILABLE, where the prior head fell back to a relative .nansen path and the key won. My synthetic probe reproduced the throw. This matches the documented refusal policy in the changeset, fails before any file access with an actionable instruction, and os.homedir() resolves on any provisioned platform, so the affected cohort is a home-less container without a passwd entry. It stays an observation for the rollout, not a merge blocker.
-
Focused bounded run: npm ci installed 278 packages, then five auth suites ran with maxWorkers 2, auth-permission-parity, auth-home, auth-login, auth-state, auth-device, 81 of 81 tests passing in 4.4 seconds. The repo's default test isolation was active, blocking all outbound fetch and disabling telemetry; every auth path uses synthetic stores and stub transports. I did not run the full 87-file suite.
-
CI claim independently read back from GitHub, read-only. Run 35477865293 concluded success at exactly head_sha 2d4bf29 with lint and Node 20, 22, 24 test jobs all success and publish plus sync-clawhub skipped. The retained log shows each test job reporting 87 files, 3332 passed, 2 skipped, 3334 total, matching the writer report. The writer's local 3325 passed and 9 skipped was not rerun here; the CI totals agree with it.
-
No tracked file was modified during this review. No live auth, trade, keyring, credential, deployment, GitHub write, agent, or model change occurred. All probes used synthetic environment values or test-isolated temporary directories under the blocked-fetch harness.
Limitations
-
No live server acceptance. Browser pairing, hosted simulation statuses, revocation propagation, gateway, staging, and live ledger behavior were not probed. The anonymous 401/403 warn-and-proceed behavior assumes the hosted endpoint returns those statuses to anonymous callers, as the code comments state.
-
Native platform cohorts remain unverified: Windows LockFileEx release semantics, the 2560-byte credential blob limit, Enterprise roaming, desktop prompt cancellation, reboot and logoff persistence, and power-loss durability. Physical deletion stays best effort, and an out-of-process credential daemon could still complete a dispatched write after client death. The retained evidence does not close that gap.
-
Per-request session reads keep the measured custody overhead, serialized on the module-level queue. The documented 83d6 measurements stand; latency and cohort acceptance is a prerelease gate, not resolved by this head and not resolved by object reuse.
-
The reviewer A minority objection stays preserved: legacy key writes and logout still hard-require the fs-native-extensions binding, and the packaged offline guide covers legacy-key-only removal. Unsupported platform mutation stays fail closed with no second lock protocol.
-
This parent has no renewal. Child 658 adds rotation and 659 adds onboarding, so expired-session behavior depends on those children. This review is not a final stack proof.
-
Cross-repo pins, authservice 3e08f9e and API 1e9bfcd, were taken as given from the frozen contract sources and not re-verified against those repositories.
-
Local validation was bounded to the five focused auth suites. The whole 87-file matrix was covered by the exact-head CI run on Node 20, 22, and 24, not by me locally.
|
B follow-up complete at unchanged parent head B passed 81 focused tests in five auth suites with two workers and independently verified exact-head CI35477865293: lint and Node20/22/24, each87files/3332passed/2skipped. B did not rerun the full local suite. The writer’s local3325passed/9skipped remains separate evidence. B upheld the three current bot dispositions: injected TTY compatibility, no recovery/retirement on readSession, and no environment stubs to restore. B’s home-less-container observation remains explicit: home resolution happens before credential selection, so even an environment key receives AUTH_HOME_UNAVAILABLE when neither the environment nor OS provides an absolute home. The fixed error directs the user to set HOME or USERPROFILE; no relative credential path is accepted. This is a nonblocking runtime limitation, not a newly fixed behavior. The earlier A native-binding minority disagreement also remains for the final handoff, along with the packaged legacy-key-only recovery guide and single lock protocol. The PR description now distinguishes this parent from root’s current assembled-stack evidence:
These results supersede the outdated pending synthetic cross-repository check. Real middleware/handlers/signing run with controlled stores, identity, services and gateway metadata. The process network guard is not OS egress isolation. The initial joint fixture failure was retained, then corrected without a repository change. There is no live consent, deployed durability, gateway, ledger, native custody, full trading-provider or revocation-propagation acceptance claim. Native/Windows/Enterprise/desktop/reboot, daemon outstanding writes, best-effort physical deletion and latency/cohort limits remain open. All11 current feedback IDs retain explicit dispositions: issues5739920044,5740154309,5740226414,5740472098,5740504876,5746309780,5746396512 and reviews5254931768,5255066999,5258408124,5258586917. Historical repairs and disagreements remain attached to their original evidence; A657-1 stays fixed and A657-2 stays nonblocking. The standalone237-word PR introduction remains followed by collapsed technical limits and actual writer telemetry. No human thread was resolved, review/approval submitted, other checkout edited, or merge/deployment/release performed. |
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Independent review of 1d38498b2ed916bc6de07b6532974e53bfcedcd1, using Anthropic Claude Opus 5 (claude-opus-5, high effort) in a separate checkout from the OpenAI writer.
No merge-blocking code defects found. The final review verified the native-binding error repair and the blank-environment-key diagnostics, including preserved rejection without falling back to another account. Earlier repairs cover stdin/stdout behavior, selected-key payment challenge details and removal of the duplicate simulation credential path. The review also checked the current-main trade, sanctions and credential-routing integration.
The three medium findings in the automated review are fixed and independently verified: the main API client receives injected auth state; failed-login telemetry tests no longer depend on native locking; doctor reports unsupported auth config formats. The request test verifies logout still invalidates the selected session, and malformed-config diagnostics do not print the saved key. The low PR-link observation requires no source change.
One low-priority observation remains at src/auth-login.js:62: the machine-readable saved event reports effective_source: "env" for both blank and nonblank environment keys. That value is accurate but does not tell a machine consumer that a blank override will make the next command fail. Human diagnostics warn about it, and commands fail immediately with INVALID_API_KEY. This is nonblocking; do not silently select another saved account to work around it. A future schema change can distinguish credential source from usability.
Final independent checks: 1,123 focused tests and lint passed. The writer's full local run passed 3,944 tests with 9 skips. Current PR CI passed lint and 3,956 tests with 2 skips on each of Node 20, 22 and 24. The earlier independent broader review passed 1,598 focused tests.
Main merge remains on hold. This COMMENT does not replace the release decision with an approval. API508 must establish the prerelease delivery path before this changeset reaches ordinary latest distribution. Native platform/storage and per-request native-read latency acceptance remain open. The PR stays draft; no npm publication or production enablement is authorized. These are release and acceptance limits, separate from the code verdict. The saved event observation remains recorded rather than being labelled fixed.
Final head author, committer and HTTPS push actor are verified as hulk-linus[bot]; this review is posted as Codier. Earlier SSH push attribution was corrected and disclosed in the PR metadata. No merge performed.
|
Disposition of the current pr-reviewer comment at
These are writer dispositions, not an independent approval. Factory Droid with Anthropic Opus 5 is reviewing all three current heads. The packaged macOS dev walkthrough and narrower Linux native checks are now recorded on API-503. Publication and production enablement remain held. |
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Independent review of 1d38498b2ed916bc6de07b6532974e53bfcedcd1 through Factory Droid, underlying model Anthropic claude-opus-5, high reasoning. The writer/coordinator is OpenAI Codex. This COMMENT forwards the independent report; it is not merge or publication approval.
No actionable code defects found on this head.
Checks:
- Verified the checkout head equals 1d38498 and matches nansen-cli-657.json headRefOid; base main, merge-base 1faade8.
- Read the complete PR diff against the actual parent (55 files, +2964/-363), not only the two repair commits.
- Read src/auth-credentials.js, src/auth-state.js, src/auth-store.js, src/auth-store-worker.js, src/auth-store-native.js, src/auth-device.js, src/auth-login.js, src/auth-browser.js in full, plus the api.js, cli.js, swap-simulation.js, rpc-urls.js, trading.js and telemetry.js diffs.
- Credential precedence: resolveCredential checks explicit key, then NANSEN_API_KEY, then config parse/format errors, then a saved session. An environment key still wins over a saved session, and a broken config.json does not silently downgrade to anonymous.
- Fresh login verification: createDeviceClient.verify sends only Authorization: Bearer with the freshly issued access token against audience/api/v1/account, with redirect: 'error', a 15s abort, a 32 KiB bounded body read and user_id character/length validation. No environment key participates.
- Preservation on error: auth-state install compares selectionEpoch and throws AUTH_SELECTION_CHANGED, writes the journal before the config pointer, and treats a committed pointer as authoritative even when fsync or cleanup fails. Walked the crash windows between store.write, journal rewrite and config rename; in each one recover() keeps the generation the live config points at and cleans the other.
- No billing fallback: mayAutoPay requires this.allowPayment && selection.kind === 'anonymous' && no apikey/authorization header. A session or API-key 402 cannot reach the x402 signing block.
- Logout: state.logout rewrites only config.json and auth-operations, deletes the legacy apiKey field, and journals the outgoing generation before switching the pointer so recovery can retire it. auth-state.test.js line 35 asserts ~/.nansen/wallets content is untouched.
- Hosted simulation: postSim attaches credentials only when isNansenHostedUrl matches the exact origin allowlist and the sim origin equals the selected api baseUrl, sets redirect: 'error' whenever a credential is attached, and raises SIMULATION_ACCESS_DENIED on 401/403 for a non-anonymous selection. verifySwapOutcome degrades only for SwapSimulationError codes NO_SIM_RPC/NOT_SIM_CAPABLE/SIM_RPC_ERROR, so an AuthError returns proceed: false and blocks the swap.
- Session error redaction: on a session selection the response body is replaced with {} before message construction, network error text is generic, and browserSessionResponseMeta echoes only credits and rateLimit.
- Cache isolation: getCacheKey mixes baseUrl + '#' + selection.generation and computeIdentityDigest over apikey/authorization/payment-signature, so an anonymous, key and session caller cannot share a cache entry.
- Verified the repair commit 1d38498 wires deps.authState into the runCLI NansenAPI construction and that the new auth-login.test.js case asserts against the production path: it installs a real session through createAuthState, spies on the real readSession, and asserts the outgoing Authorization header equals the stored access token. The other NansenAPI construction (cli.js:1491) is the explicit-key verification path and correctly needs no auth state.
- Verified auth-store-lifetime.test.js copies the production auth-state.js, auth-store.js, auth-store-worker.js and auth-credentials.js into a temp root and replaces only auth-store-native.js, so the dispatcher, guardian, lock acquisition and journal recovery under test are real code and real fs-native-extensions locks.
- Confirmed no test reaches a real OS credential store: @napi-rs/keyring is vi.doMock'd in auth-store-native.test.js, node:child_process spawn is mocked in auth-store-timeout.test.js, and the lifetime harness substitutes a file-backed native stub.
- Ran focused mocked suites: 13 files, 205 tests passed (auth-login, auth-state, auth-device, auth-permission-parity, auth-final-repair, auth-store-lifetime, auth-store-native, auth-store-timeout, auth-telemetry, auth-home, auth-browser, auth-process, doctor).
- Ran integration-point suites: 8 files, 1154 tests passed (api, swap-simulation, telemetry-tracking, cli.internal, package, execute-guard, agent, mcp). Total 1359 local tests passed, all under the repo's isolated temp HOME with globalThis.fetch replaced by a throwing stub.
- Wrote and ran an independent 11-check probe (probe-657.mjs) outside the repo test suite covering env-key precedence over a saved session, blank env key reporting, session selection, fail-closed behaviour on an unreadable stored bundle, origin/account mismatch, MIXED_CREDENTIALS, api-key 402 with no auto-pay and exactly one outbound request, corrupt config resolving to invalid, and a version-2 auth block refusing to fall back to the saved legacy key. 11/11 passed.
- Inspected .github/workflows/ci.yml and the changeset state instead of assuming publication behaviour. See limitations.
Merge limitations recorded by this review:
- Merging this branch arms an npm publish of a gated-prerelease feature. .github/workflows/ci.yml runs changesets/action on every push to main with both version: and publish: configured. main already carries 20 pending changeset files, and git history confirms the repo releases by merging a changeset-release/main pull request (commit 62dd378 'Version Packages', merge #655). Merging 657 adds .changeset/browser-session-login.md at major, which folds browser login into that standing release pull request and turns the next release into 2.0.0. Whoever merges that release pull request publishes browser login to npm and triggers sync-clawhub, with no separate gate distinguishing a prerelease cohort from a normal release. No evidence in this run shows a defined prerelease channel, a hold on the release pull request, or a dist-tag plan. This is the reason mergeReady is false.
- Windows native credential-store qualification is absent. auth-store.js commits the CLI to 2048-byte chunks, a 16 KiB bundle cap and the Windows Credential Manager through @napi-rs/keyring, and auth-state.js requires fs-native-extensions locking for every login, logout and session-authenticated request. The live evidence in cli-native-dev-validation-20260923 covers macOS only, with a synthetic Linux exact-package check. No Windows run, no Enterprise roaming behaviour and no desktop cancellation result exists.
Requirements before distribution:
- Define and enforce the prerelease/main publication boundary before this branch reaches main. A written release plan plus a mechanical hold, for example a changeset pre-enter prerelease tag, a paused changeset-release/main pull request, or a dist-tag other than latest, so that merging 657 cannot reach the npm latest tag through a routine release merge. Owner: CLI release owner with npm Trusted Publishing access for nansen-ai/nansen-cli. Proof: The workflow or changeset configuration showing the hold, plus a dry run of changeset:version on a main-with-657 tree showing the resulting version and dist-tag.
- Qualify Windows native credential storage and locking. A live Windows run of login, process restart, a session-authenticated request, logout and store cleanup on the exact packaged artifact, including a locked-store refusal and a roaming-profile case. Owner: CLI platform owner. Proof: An evidence file in the same shape as cli-native-dev-validation-20260923/renewal-evidence.md, recording stored byte counts, read/write/delete latencies, the locked refusal and confirmed absence of both credential generations after logout.
- Publish a positive supported-platform list once a cohort is accepted. The shipped docs/browser-login.md already discloses the negative side honestly: lines 34, 40, 56, 58 and 68 state that Windows needs native verification, that Windows Credential Manager may roam under Enterprise policy, that power-loss durability is unproved, and that musl/Alpine and architectures without a verified binding are not admitted. What is missing is the positive statement of which operating systems, architectures, Node versions and terminal types are qualified, which cannot exist until a cohort is accepted. Correction to an earlier draft of this report: this is a rollout item, not a documentation gap, because a user is not misled today. Owner: CLI documentation owner together with the rollout owner. Proof: A named supported-platform section in docs/browser-login.md, cross-checked against the platform evidence files so no unqualified platform is listed.
- Accept the per-request session read latency for parallel agent use, or reduce it. auth-state readSession takes the exclusive auth.lock and spawns a native helper for every session-authenticated request, with a 15s AUTH_BUSY deadline. The PR body already records 798/2956/11764 ms for one, four and sixteen same-process macOS reads, so sixteen parallel commands sit close to that deadline. Decide whether AUTH_BUSY under agent fan-out is acceptable or add in-process caching of the bundle for the command's lifetime. Owner: CLI performance owner. Proof: A measured cross-process fan-out run at the intended concurrency showing either no AUTH_BUSY or an accepted, documented failure rate with actionable recovery text.
- Define the rollback distribution path. A documented way for a cohort user on the browser-login version to return to a working API-key-only build, given that the changeset is major and the auth block format is version 1 with no downgrade reader. Owner: CLI release owner. Proof: A rehearsed downgrade on a machine that has completed a browser login, showing that the previous version still authenticates from NANSEN_API_KEY and that the leftover auth block and secure-store entries cause no failure or credential loss.
The coordinator is repairing the release boundary and documenting preview scope locally. GitHub rejected the new writer push because the hulk-linus App lacks Workflows permission. Those new commits are not on this PR yet and require final independent verification. The 658 cooldown-message repair is also being carried through the stack. This report stays tied to the remote head named above. Publication and production feature enablement remain held.
|
Repair closeout at CI passed lint and 3,975 tests on each of Node 20, 22 and 24, with two skips. Factory Droid with Anthropic The automated-review URL finding is fixed: explicit key login verifies and saves the same resolved API URL. Six regressions cover both key-login modes and environment/config/default selection. The main-branch integration also now passes the correct stderr callback. The earlier full local run had ten failures; all affected tests passed unchanged in isolated reruns, then the complete suite passed at lower concurrency. No timeout or assertion was weakened. The workflow-policy finding remains open for teammate review. Windows browser opening and native-store acceptance remain release gates; Codier pushed under Di's authorized exception; all commits retain hulk-linus author/committer attribution. Another teammate must review the workflow policy, supply final approval and close out the old changes-requested reviews. No review state has been dismissed or approved here. The exact-head hosted run linked above is green. The original report is preserved below, including any CI observation taken while jobs were still running. Publication hold must be rechecked at merge time; platform, recovery and rollout requirements remain. Disposition of the updated automated review at
API resource requests use Bearer tokens. DPoP applies to pairing and the companion renewal flow, not every API request. Independent final repair report, preserved verbatim{
"pr": 657,
"head": {
"sha": "bfd0fa7e05039cd766c497c94d5653eae0122d5d",
"previous": "32dbb1909fd413fb5ecda118f31b7dc3a6a5065c",
"checkout": "/Users/dipeng/Nansen/worktrees/cli-657-factory-final-20260924",
"verified": "git rev-parse HEAD matched; only untracked node_modules present. bfd0fa7 is a merge of 32dbb19 with origin/main 6b1e707, authored and committed as hulk-linus[bot] per 657-final-push-receipt.json, pushed by Codier."
},
"scope": "New delta since verified-review-657.json only. No re-audit of the browser-login feature or of upstream main.",
"verdict": "clean",
"verdictScope": "Source code at this head. Hosted CI, the workflow-policy human review and release acceptance are separate and remain open.",
"findings": [],
"checks": [
{
"id": "D657-1",
"what": "errorOutput binding repair and dead-reference sweep",
"command": "rg -n '_errorOutput' --glob '!node_modules' ; rg -c 'errorOutput|authConfigView|saveConfig|deleteConfig' src/cli.js",
"result": "Zero hits for _errorOutput anywhere in the repository. buildCommands now destructures errorOutput without the unused-prefix rename, and cli.js references it 13 times, including the browserLoginFn call and the new screener stderr note. saveConfig and deleteConfig are no longer imported or referenced in cli.js, so the removed import left no dangling caller. authConfigView is imported once and used twice. eslint reports clean at this head (657-low-concurrency-stack-lint.log), which is what would have caught an unused binding."
},
{
"id": "D657-2",
"what": "API-key login origin repair",
"command": "git diff 32dbb19 HEAD -- src/cli.js",
"result": "The login handler resolves const baseUrl = authConfigView(env).baseUrl once, then passes it both to the verification constructor new NansenAPIClass(apiKey.trim(), baseUrl, ...) and to authState.install(attempt, { apiKey, baseUrl }). The previous hardcoded 'https://api.nansen.ai' on the install call is gone, so a dev or staging origin used for verification is the origin that gets saved. Browser-login origin restrictions are untouched by this change."
},
{
"id": "D657-3",
"what": "Focused offline regression for the origin repair",
"command": "TMPH=$(mktemp -d); env -u NANSEN_API_KEY -u NANSEN_BASE_URL -u NANSEN_WALLET_PASSWORD -u GH_TOKEN -u GITHUB_TOKEN -u FACTORY_API_KEY -u PASEO_* HOME=$TMPH npx vitest run src/__tests__/auth-login.test.js -t 'API origin'",
"result": "1 file, 6 passed, 36 skipped, 2.27s, exit 0. Temp HOME removed. The six cases cross human and api-key login with an env, config and default origin, and each asserts both the constructor argument and the saved authConfigView baseUrl. The API class is a local stub, so no request left the process."
},
{
"id": "D657-4",
"what": "Focused offline regression for the stderr path that the errorOutput repair feeds",
"command": "same isolated env; npx vitest run src/__tests__/cli.internal.test.js -t search",
"result": "1 file, 67 passed, 652 skipped, 2.21s, exit 0. Covers the screener search-window marker set, including the incomplete-window note on stderr, the note staying off stdout under --fields, server pagination metadata taking precedence over row count, and no marker without --search."
},
{
"id": "D657-5",
"what": "New pull_request base filter exposes no publication permission",
"command": "sed -n '1,40p' .github/workflows/ci.yml ; grep -n 'CLI_PUBLICATION_ENABLED|id-token|contents: write|pull-requests: write' .github/workflows/*.yml",
"result": "pull_request now names main, hulk/api-506-browser-login and hulk/api-507-session-refresh. push is still main only. The workflow keeps top-level permissions: {}, and the lint and test jobs declare only contents: read. contents: write, pull-requests: write and id-token: write exist solely on the publish job, which still requires github.event_name == 'push' && github.ref == refs/heads/main && vars.CLI_PUBLICATION_ENABLED == 'true'. A pull_request event on a hulk base can therefore reach neither the OIDC token nor any write scope. sync-clawhub.yml keeps its job-level gate."
},
{
"id": "D657-6",
"what": "Coordinator test logs read including failures",
"command": "read 657-low-concurrency-stack-tests.log, 657-remaining-stack-tests.log, test-run-dispositions.md, leaf-node20.log, node20-auth-process-rerun.log",
"result": "The complete low-concurrency run at this tree is 99 files, 3968 passed, 9 skipped, 309s, and lint is clean. I did not rerun it. The earlier maxWorkers=4 run on the same unchanged tree reported 10 failures; the six affected files then passed in bounded reruns totalling 484 tests, with no timeout or assertion edits. I record the failed run rather than presenting only the green one. Separately, the older d4e38d4 Node 20 full run had one timeout in auth-process logout pending approval and an isolated rerun of that whole suite passed 16 of 16 on unchanged source; the cause was not established and I am not inferring one. The old Node 24 run was stopped and is not counted as a pass."
}
],
"limitations": [
{
"id": "M657-1",
"limitation": "Hosted CI on bfd0fa7 is unknown to me. The new base filter means the Node 20/22/24 matrix can now run on stacked branches, and the coordinator states the matrix triggers on all stacked bases, but I observed no result and assume none. An earlier hosted run on the previous head failed on the undefined _errorOutput reference, which is exactly what this delta repairs; that history is why the hosted result matters rather than being a formality.",
"status": "open"
},
{
"id": "M657-2",
"limitation": "The pr-reviewer workflow policy requires human review of any .github/workflows change. This delta edits ci.yml, so that review is required and still open. Nothing here bypasses, weakens or removes the policy, and I am not claiming it is satisfied.",
"status": "open"
},
{
"id": "M657-3",
"limitation": "Final approval must come from a teammate who is not Codier, since Codier pushed these heads and the commits are authored by hulk-linus[bot]. I posted no review, comment or any other GitHub mutation; this file is evidence for relay only.",
"status": "open"
},
{
"id": "M657-4",
"limitation": "Publication default-off was observed absent at repository and inherited organization level at 2026-09-24T01:56:49Z, with Version Packages still open and no auto-merge request. That is a point-in-time read taken before the latest pushes and must be repeated immediately before any merge.",
"status": "open, recheck required"
},
{
"id": "M657-5",
"limitation": "Carried forward unchanged from verified-review-657.json: the gate does not bind older workflow revisions, queued runs or a manual npm publish; enforcement depends on one repository variable staying absent and on the Integrations team review bypass staying unused; the macOS arm64 preview claim still needs a live walkthrough on a real artifact; Windows stays excluded and Linux remote qualification is pending.",
"status": "open"
}
],
"rolloutRequirements": [
{
"item": "Confirm the publication hold and no active ungated run immediately before merge.",
"gate": "Re-read repository and inherited organization CLI_PUBLICATION_ENABLED and list in-flight ci.yml and sync-clawhub.yml runs at merge time.",
"owner": "CLI maintainer with Actions access",
"evidence": "A fresh read replacing the 01:56:49Z observation, plus a run listing showing zero active ungated publication runs."
},
{
"item": "Obtain the workflow-policy human review for the ci.yml change.",
"gate": "A named human reviewer, not Codier, reviews the pull_request base filter and the publication condition.",
"owner": "Nansen reviewer other than Codier",
"evidence": "The recorded review on PR 657 covering the workflow diff."
},
{
"item": "Keep distribution separate from this source merge.",
"gate": "Release owner picks version, channel and cohort, enables the variable for a bounded window, then clears it.",
"owner": "CLI release owner",
"evidence": "A recorded release decision plus variable audit history."
},
{
"item": "Qualify the preview platform claim and publish a v2-compatible recovery package.",
"gate": "Live macOS arm64 walkthrough on the exact artifact a cohort installs, Linux remote either walked through or explicitly excluded, Windows excluded, and a rehearsed downgrade.",
"owner": "CLI platform owner and release operator",
"evidence": "An evidence file naming the archive SHA256, and the recovery version with a clean and an interrupted downgrade rehearsal."
}
]
} |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| return issuer; | ||
| } | ||
| export function readAuthConfig(env = process.env, devConfigPath = DEV_CONFIG) { | ||
| const userPath = path.join(authDirectory(env), 'config.json'); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: The joined path has no caller-supplied segment. readAuthConfig builds authDirectory(env) + the literal 'config.json'; the only variable is HOME/USERPROFILE, which authDirectory rejects unless it is an absolute string. No CLI argument, API response, JSON field or IPC message reaches this join, so there is no traversal input to sanitize.
Verification: src/auth-credentials.js:11-16,25-27. Offline probe with HOME='../../etc' threw AUTH_HOME_UNAVAILABLE. Grep over src (excluding tests) shows every readAuthConfig/authConfigView caller passes only env plus an optional devConfigPath that is a module-relative constant (auth-login.js:30, doctor.js:89, api.js:593,705, cli.js:1528, mcp-verify.js:222).
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
The joined path has no caller-supplied segment. readAuthConfig builds authDirectory(env) + the literal 'config.json'; the only variable is HOME/USERPROFILE, which authDirectory rejects unless it is an absolute string. No CLI argument, API response, JSON field or IPC message reaches this join, so there is no traversal input to sanitize.
Verification: src/auth-credentials.js:11-16,25-27. Offline probe with HOME='../../etc' threw AUTH_HOME_UNAVAILABLE. Grep over src (excluding tests) shows every readAuthConfig/authConfigView caller passes only env plus an optional devConfigPath that is a module-relative constant (auth-login.js:30, doctor.js:89, api.js:593,705, cli.js:1528, mcp-verify.js:222).
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
| try { signal?.throwIfAborted(); prepare(); release = await acquire(path.join(directory, 'auth.lock'), signal); return await fn(); } | ||
| finally { release?.(); finish(); if (queues.get(directory) === tail) queues.delete(directory); } | ||
| } | ||
| const journalPath = id => path.join(journalDir, `${id}.json`); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: journalPath joins journalDir with ${id}.json where id is either crypto.randomUUID() or a directory entry already matched against /^[a-f0-9-]{36}.json$/. Values that do come from the config file (config.auth.active.generation, old.generation) are never joined into a path: they are compared, or passed to store.read/store.remove, where createAuthStore.name re-validates /^[a-f0-9-]{36}$/ and throws AUTH_STORE_UNAVAILABLE otherwise.
Verification: src/auth-state.js:10,86,95-104,131-133,183-190,235-241; src/auth-store.js:71-74. journal shape is validated at auth-state.js:143 (generations array, max 2, each a 36-char hex/dash string).
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
journalPath joins journalDir with
${id}.jsonwhere id is either crypto.randomUUID() or a directory entry already matched against /^[a-f0-9-]{36}.json$/. Values that do come from the config file (config.auth.active.generation, old.generation) are never joined into a path: they are compared, or passed to store.read/store.remove, where createAuthStore.name re-validates /^[a-f0-9-]{36}$/ and throws AUTH_STORE_UNAVAILABLE otherwise.Verification: src/auth-state.js:10,86,95-104,131-133,183-190,235-241; src/auth-store.js:71-74. journal shape is validated at auth-state.js:143 (generations array, max 2, each a 36-char hex/dash string).
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
| fs.mkdirSync(directory, { recursive: true, mode: 0o700 }); | ||
| const file = path.join(directory, 'auth-store.lock'); | ||
| safe(file); | ||
| fd = fs.openSync(file, 'a+', 0o600); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: The opened path is <directory>/auth-store.lock with a literal basename. The only writer to this process's stdin is its own parent in nativeStoreOperation, over a pipe, with no socket or listener, so the 'directory' field is not a lower-privilege input. It must be an absolute string, is lstat-checked by safe(), and the 'a+' open neither reads nor writes content; it only provides a descriptor for tryLock.
Verification: src/auth-store-worker.js:21-33; src/auth-store.js:12-19,69 (parent writes the single directory line, child env is a fixed allowlist). Offline probe: a relative directory is rejected before any filesystem work (SIGKILL, no stdout); a traversal account value such as '../../../../etc/passwd' is rejected by the /^[a-f0-9-]{36}.(probe|manifest|[0-7])$/ check and the process dies before spawning the native worker or touching the keyring.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
The opened path is
<directory>/auth-store.lockwith a literal basename. The only writer to this process's stdin is its own parent in nativeStoreOperation, over a pipe, with no socket or listener, so the 'directory' field is not a lower-privilege input. It must be an absolute string, is lstat-checked by safe(), and the 'a+' open neither reads nor writes content; it only provides a descriptor for tryLock.Verification: src/auth-store-worker.js:21-33; src/auth-store.js:12-19,69 (parent writes the single directory line, child env is a fixed allowlist). Offline probe: a relative directory is rejected before any filesystem work (SIGKILL, no stdout); a traversal account value such as '../../../../etc/passwd' is rejected by the /^[a-f0-9-]{36}.(probe|manifest|[0-7])$/ check and the process dies before spawning the native worker or touching the keyring.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
| let configError = null; | ||
| if (configPath) { | ||
| try { | ||
| config = JSON.parse(fs.readFileSync(configPath, 'utf8')); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: Same fixed path as line 26, and the read cannot disclose file content: the bytes are JSON.parsed and discarded on failure, with configError set to the fixed tokens 'parse' or 'unreadable'. Nothing echoes the raw body; doctor prints only configPath, a masked key and the error token. There is no attacker-controlled path or content channel here, so the scanner's file-inclusion claim does not hold. A separate hardening gap on this same line is recorded under hardening H-1; it is not the reported issue and does not require a patch to clear this finding.
Verification: src/auth-credentials.js:30-37; src/doctor.js:190,197,263,269-283 (maskKey, error token only). Offline probe: a symlinked ~/.nansen/config.json is read, but only its parsed JSON fields are used and no content is emitted.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed. The separate local config-permission hardening gap H-1 remains open; this disposition does not mark it fixed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
Same fixed path as line 26, and the read cannot disclose file content: the bytes are JSON.parsed and discarded on failure, with configError set to the fixed tokens 'parse' or 'unreadable'. Nothing echoes the raw body; doctor prints only configPath, a masked key and the error token. There is no attacker-controlled path or content channel here, so the scanner's file-inclusion claim does not hold. A separate hardening gap on this same line is recorded under hardening H-1; it is not the reported issue and does not require a patch to clear this finding.
Verification: src/auth-credentials.js:30-37; src/doctor.js:190,197,263,269-283 (maskKey, error token only). Offline probe: a symlinked ~/.nansen/config.json is read, but only its parsed JSON fields are used and no content is emitted.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed. The separate local config-permission hardening gap H-1 remains open; this disposition does not mark it fixed.
| const temp = `${file}.${randomUUID()}.tmp`; | ||
| let fd; | ||
| try { | ||
| fd = fs.openSync(temp, 'wx', 0o600); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: This is a create-exclusive write, not a read. fs.openSync(temp, 'wx', 0o600) is O_CREAT|O_EXCL, which fails if the path exists at all, including an existing symlink, so it cannot be redirected onto another file. The name is ${file}.${randomUUID()}.tmp inside the 0700 auth directory, and the target file passed safePath immediately above.
Verification: src/auth-state.js:40-60. Temp names are also the only thing recover() will unlink, and only when they match /^config.json.[a-f0-9-]{36}.tmp$/ or the journal equivalent (auth-state.js:108-117).
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
This is a create-exclusive write, not a read. fs.openSync(temp, 'wx', 0o600) is O_CREAT|O_EXCL, which fails if the path exists at all, including an existing symlink, so it cannot be redirected onto another file. The name is
${file}.${randomUUID()}.tmpinside the 0700 auth directory, and the target file passed safePath immediately above.Verification: src/auth-state.js:40-60. Temp names are also the only thing recover() will unlink, and only when they match /^config.json.[a-f0-9-]{36}.tmp$/ or the journal equivalent (auth-state.js:108-117).
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
| fs.renameSync(temp, file); | ||
| await barrier('after-rename', path.basename(file)); | ||
| if (process.platform !== 'win32') { | ||
| const dirFd = fs.openSync(path.dirname(file), 'r'); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: fs.openSync(path.dirname(file), 'r') opens a directory purely so the following fsync durably commits the rename. No bytes are read and the descriptor is closed in a finally block. The directory is the fixed auth or journal directory derived from authDirectory, not from any caller input.
Verification: src/auth-state.js:51-56,23-24.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
fs.openSync(path.dirname(file), 'r') opens a directory purely so the following fsync durably commits the rename. No bytes are read and the descriptor is closed in a finally block. The directory is the fixed auth or journal directory derived from authDirectory, not from any caller input.
Verification: src/auth-state.js:51-56,23-24.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
| if (typeof directory !== 'string' || !path.isAbsolute(directory)) throw new Error(); | ||
| safe(directory, true); | ||
| fs.mkdirSync(directory, { recursive: true, mode: 0o700 }); | ||
| const file = path.join(directory, 'auth-store.lock'); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: Same construction as 4089135130: path.join of a parent-supplied absolute directory with the literal 'auth-store.lock'. No user, network or argument data contributes a path segment.
Verification: src/auth-store-worker.js:24-29; probe results above.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
Same construction as 4089135130: path.join of a parent-supplied absolute directory with the literal 'auth-store.lock'. No user, network or argument data contributes a path segment.
Verification: src/auth-store-worker.js:24-29; probe results above.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
| expect(fetch).toHaveBeenCalledTimes(2); | ||
| for (const [i, event] of events.entries()) { | ||
| expect(event.path).toBe(`/${command}`); | ||
| expect(event.properties.flags.sort()).toEqual(['--api-key', '--no-browser']); |
There was a problem hiding this comment.
Exposed secret in src/tests/auth-telemetry.test.js - low severity
Detected a secret as command line option
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More Info
There was a problem hiding this comment.
@AikidoSec ignore: The flagged value is the synthetic literal 'SYNTHETIC-PRIVATE-CODE', defined in the test itself as a probe token and asserted to be absent from every serialized telemetry event. It is not a credential for any system, it is the sole occurrence in the repository, and the test performs no network I/O: global fetch is replaced with vi.stubGlobal and the endpoint string is asserted inside the stub.
Verification: src/tests/auth-telemetry.test.js:3,10-14,21-32; rg over the repo excluding node_modules shows one file containing the literal. Offline run with scrubbed env and a temp HOME: 1 file, 5 tests passed, 2.59s, exit 0, stubbed fetch call counts asserted.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
The flagged value is the synthetic literal 'SYNTHETIC-PRIVATE-CODE', defined in the test itself as a probe token and asserted to be absent from every serialized telemetry event. It is not a credential for any system, it is the sole occurrence in the repository, and the test performs no network I/O: global fetch is replaced with vi.stubGlobal and the endpoint string is asserted inside the stub.
Verification: src/tests/auth-telemetry.test.js:3,10-14,21-32; rg over the repo excluding node_modules shows one file containing the literal. Offline run with scrubbed env and a temp HOME: 1 file, 5 tests passed, 2.59s, exit 0, stubbed fetch call counts asserted.
Independent Factory Droid review using Anthropic claude-opus-5, high reasoning, at
bfd0fa7e05039cd766c497c94d5653eae0122d5d. This disposition applies only to this finding. No scanner rule or path exclusion is being changed.
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Approved for source integration at bfd0fa7e05039cd766c497c94d5653eae0122d5d based on the independent Factory Droid review, Anthropic claude-opus-5, high reasoning, and its final repair verification on this exact head. Earlier change requests are closed by the recorded repairs and final review. This does not approve publication or production enablement.
Hosted CI passed lint and 3,975 tests on each of Node 20, 22 and 24, with two skips per run. Aikido check code is green. Publication and ClawHub sync were skipped.
Di explicitly authorized Codier approval despite Codier having pushed these commits. Commit author and committer remain hulk-linus[bot]. Independent code review was performed by Factory, not by the writer. This supersedes the earlier requirement for another GitHub account to approve.
Di also explicitly chose to keep the stack and accept the workflow-policy-only red check on #657. Factory inspected the workflow permissions and default-off publication conditions. The unconditional pr-reviewer policy remains unchanged and its failed check is not represented as green.
Merge in order #657, #658, #659; verify each child's base and any changed head before merging. Recheck the publication variable and active runs at merge time. Keep publication held until the exact release artifact, supported-platform walkthroughs, renewal and rollback acceptance are complete. Historical dev evidence does not establish acceptance of these new archives.
Independent security triage session 81019ce8-cd5e-4dd5-82d6-860bb1d04677 verified all eight Aikido findings as false positives. Per-finding replies contain the input traces and offline evidence; Aikido subsequently reported success. No path exclusion or scanner rule was weakened.
Nonblocking follow-up H-1 remains open: readAuthConfig accepts a symlinked or writable config file that auth-state mutation rejects. Exploitation requires local filesystem write access; a misconfigured shared home can allow API-key destination tampering. Apply consistent file ownership, type and permission checks and add regressions in a follow-up. This approval does not claim that hardening is implemented. The guards also do not establish complete ancestor-symlink or same-user race protection.
Additional triage runtime, separate from the previous shared review: 30 fresh input, 89615 cache creation, 879196 cache read, 25601 output and 4223 thinking tokens, as reported by Factory. Provider lock Anthropic; model claude-opus-5; reasoning high. Counters are retained separately rather than summed into an inferred total.
Plain
nansen loginrequests fresh browser approval, verifies the selected account and saves the session in the OS credential store. Explicit API-key setup remains available. Authentication failures never switch accounts or trigger payment.The final integration repair preserves the API URL used to verify an explicit key and uses the correct stderr callback after merging current main. Six regressions cover environment, saved and default URLs for both key-login modes.
This PR also adds
CLI_PUBLICATION_ENABLED, default off, to the Changesets job and manual/reusable ClawHub sync. CI now runs on the two stacked PR bases. Old workflow revisions and publication outside these workflows are outside the gate.Validation at
bfd0fa7e05039cd766c497c94d5653eae0122d5d: GitHub CI passed lint and 3,975 tests on each of Node 20, 22 and 24, with two skipped per run. Publish and ClawHub sync were skipped. The independent repair review found no code defects on this exact head. Its findings and remaining limitations are attached in the closeout comment.Review and release status
Codier approved this head after independent Factory review and passing CI. Di explicitly authorized Codier approval despite the Codier push and accepted #657's workflow-policy-only red check. Aikido is green. The policy remains unchanged. Approval covers source integration, not publication or production enablement. Merge order is #657, #658, then #659. After each parent merge, verify the child's base and rerun affected checks if its commit changes.
Keep
CLI_PUBLICATION_ENABLEDabsent or false and verify no ungated publication run immediately before any future merge. Do not merge the Version Packages PR or publish as part of source-review closeout. Preview-platform acceptance, the recovery artifact and production enablement remain separate work.AI execution metadata
Implementation and coordination: OpenAI Codex. Exact model identifier, effort and token counters are not exposed in this continuation; none are inferred. Earlier telemetry is retained below.
Independent final repair verification: Factory Droid, Anthropic
claude-opus-5, high reasoning. Sessione3542c92-362a-416a-9945-91fa5c7de492; runtime model and Anthropic provider lock verified at start and end. These cumulative session counters cover the prior verification and final repair check across all three PRs, not a per-PR total: 90 fresh input, 4567281 cache read, 292845 cache creation, 58740 output, 3787 thinking. Counters are reported as supplied by Factory and are not summed.Commit author and committer:
hulk-linus[bot]. Push actor: Codier, verified in GitHub activity under Di's explicit exception. Di subsequently authorized Codier's approval, now posted. This supersedes the earlier identity restriction for this stack only.Additional independent Aikido triage: Factory Droid, Anthropic
claude-opus-5, high reasoning; session81019ce8-cd5e-4dd5-82d6-860bb1d04677. Runtime provider/model verified. Separate triage cumulative counters: 30 fresh input, 879196 cache read, 89615 cache creation, 25601 output, 4223 thinking. Categories are not summed into an inferred total. Eight findings were individually verified as false positives and Aikido is green. No source change or broad scanner exclusion was made in this triage.Open nonblocking hardening H-1: config reads should enforce the ownership/type/permission checks already used for auth-state mutations. This concerns local filesystem write access, including misconfigured shared homes. It is not fixed by dismissing unrelated file-inclusion findings. Details and required regressions are in the approval review.
Earlier implementation and review evidence, tied to historical revisions
The current-head status above supersedes earlier draft/readiness statements. Earlier limitations remain unless the current closeout explicitly resolves them.
Plain
nansen loginrequests fresh browser approval, validates the new account independently, and saves its session in the OS credential store.--humanand--api-keypreserve explicit key setup. Failed pairing keeps the previous credential; environment keys still take precedence for ordinary commands.Requests use one selected credential. Browser authentication failures never switch accounts or trigger wallet payment. Logout retires the selected session while preserving wallet files. Offline status and recovery diagnostics avoid printing credentials. Automatic session renewal remains in #658.
Reconciled with current
main, preserving trade confirmation, dry runs, sanctions checks, cache commands and response parsing. Hosted trade simulation uses the selected account only on its matching Nansen origin, including the new sign-free preview path. Session plan errors now match API-key error codes.Validation and release
1d38498, local tests passed 3,944 with 9 skips. CI passed lint and 3,956 tests with 2 skips on each of Node 20, 22 and 24.Current review and execution
Writer/coordinator: OpenAI Codex. This session does not expose reliable exact model, effort or cumulative token counters, so those are not inferred. The earlier writer/model records are retained below.
Independent review: Anthropic
claude-opus-5, high effort, separate checkout, runtime verified. Final review at1d38498found no merge-blocking code defects and passed 1,123 focused checks. One low-priority observation remains: the machine-readable saved event identifies the environment override but does not distinguish a blank value. Human diagnostics warn about it; commands reject it without switching accounts. Native acceptance, latency and the API508 prerelease/main-merge hold remain open. This PR stays draft. The subsequent automated-review repairs pass the auth-state injection into API requests, isolate the telemetry test from native locking, and add unsupported-format diagnostics. The independent final review verified these repairs.Historical implementation and review evidence
The material below describes earlier revisions and their checks. Current-head validation above supersedes old readiness statements; remaining platform and rollout limitations still apply.
Follow-up to Shaun’s review, head
9b0508fc6d7deeb330f99c13e23c0ddcb9052ee9: added a regression test for issuance returning 500 after approval is consumed, followed by an expired-code response. The CLI asks for a fresh login, does not restart pairing automatically, and does not classify issuance as certainly absent. Documented that existing recovery path. Production code is unchanged. Full local suite: 3,326 passed, 9 skipped; lint passed. New-head CI and approval are separate from the historical evidence below.Follow-up AI execution metadata
Provider: OpenAI, Codex. Model identifier and token counters are unavailable from this turn’s exposed runtime; no values inferred. Thinking: default, no explicit override. Attribution: this test/documentation follow-up and publication, separate from all prior writer and reviewer snapshots. Writer identity read back as hulk-linus[bot]. No approval, merge or deployment.
This PR changes plain
nansen loginto open a browser, ask the user to approve access and save the resulting session in the OS credential store. Users can sign in without finding and pasting an API key.The session requests
nansen:api, giving it the same account API permissions as a pasted key. Smart-alert requests and hosted trade simulation use the selected account credential. Trading still needs its existing wallet setup, signatures and confirmations.Login verifies the new account before saving it. It never falls back to another account's key or automatically buys credits. Credentials only go to the intended Nansen API; custom RPCs receive none. Logout clears saved selection and attempts session retirement and secure-store cleanup while preserving wallets. Remote terminals can use
--no-browser.There are two compatibility changes: scripts that want API-key login must explicitly use
--humanor--api-key, and old read-scoped browser sessions need a fresh login. MCP installation still needs a separate integration API key because a browser session is not an exportable API key.#658 adds automatic session renewal. #659 finishes onboarding and packaged guidance. The assembled stack passed synthetic cross-service checks; deployment acceptance remains required.
Validation: 3,325 local tests passed with 9 skips. CI passed lint and tests on Node 20, 22 and 24. Tests cover alert CRUD, anonymous/key/session simulation routing, storage and logout. Independent B passed 81 focused tests with no findings. Live rollout and the platform-storage limitations below remain open.
Implementation details, validation and rollout limits
Browser login should give the same account API permissions as pasting an API key. This draft adds fresh browser approval with first-party
nansen:apisessions, including smart-alert CRUD and existing trade API operations under the same account, plan and endpoint checks.Tracks API-506 under API-503. Base
cc4aa180763ce76a5a1e46df10537d5b32d3ce16; head2d4bf29e8d1c8be74e920ae279fb8f06fc766a13.Flow
nansen loginpreflights OS storage, creates a fresh ES256 key and runs bounded device approval with same-key DPoP/nonce handling.--no-browserand non-TTY/JSON events support remote terminals. Candidate account verification uses only the new session, independent of an environment key.Boundaries and compatibility
nansen:readremains a read grant for existing OAuth/MCP clients. It is not upgraded locally; old read-scoped CLI sessions require fresh login. Server admission ofnansen:apiwith API-key-equivalent account permissions is covered by the synthetic stack evidence below; deployment acceptance remains separate. This adds no service/admin authority or wallet signing rights. Wallet configuration, transaction validation and signing remain separate.Trading quote/broadcast/bridge-status use their existing separate service transport. Limit orders keep wallet-signed challenge/JWT authorization and the existing optional environment key header. Browser tokens are not sent to that audience or arbitrary RPC URLs. MCP installation separately exports an integration API key into another client; the browser session is not an exportable API key. That setup requirement does not reduce its account API permissions.
Selected credentials never fall back to another account or automatic payment. Anonymous x402 and explicit manual API-key payment retain their behavior; mixed browser/key/payment credentials are rejected. Plain login no longer persists an environment key: scripts must use explicit
--humanor--api-key. Both changes are in the major changeset. API507 renewal and API508 publication remain separate.Validation
npm test -- --maxWorkers=2passed at this source:npm run lint, changeset and diff checks passed. The full suite includes schema/help, packaged installation without optional bindings, custody/concurrency fault tests and secret-output checks. The six-case HTTP401/403 matrix runs actual verifySwapOutcome for anonymous, key and session selections, asserting credential routing, no payment/retry, anonymous warning/proceed and selected-account refusal. Five home-directory regressions cover fallback and refusal before file access. Targeted parity coverage includes key/browser alert CRUD, hosted simulation, matching staging origin, rejected old read grants, mixed credentials, logout invalidation, reflected errors, third-party RPC exclusion and separate wallet JWTs.Exact-head CI35477865293 passed lint and tests on Node20/22/24 at
2d4bf29e8d1c8be74e920ae279fb8f06fc766a13. Each Node job passed 87 files, 3,332 tests with 2 skips. Publish and sync-clawhub were skipped. Independent B passed 81 focused tests across five auth suites with two workers in 4.4 seconds and independently read back this CI. B did not rerun the full local suite or verify external repository pins.All authentication, alert, simulation and trade requests in tests are synthetic and intercepted. No real login, funded operation or production probe was performed. Missing HOME/USERPROFILE uses Node’s OS home lookup; absent or relative results fail before credential file access. Because home resolution is eager, a container without a resolvable absolute home can reject even environment-key access with
AUTH_HOME_UNAVAILABLE. Set an absolute HOME or USERPROFILE. B reproduced this and retained it as a nonblocking rollout observation. Earlier reviews remain historical; the current B report is a COMMENT, not approval.Synthetic stack evidence
Root’s current synthetic stack evidence uses API
1d7bcc9efad02277554f563837450023b2c2a7f5and CLI leaf93fd21beb71632b10560f1988d3513298beee8b9. All 16 fresh-process cases passed: independent candidate verification, zero-credit free account and smart-alert CRUD, account selection and debit isolation, key/session credit and plan error parity, and revoked-session refusal. The leaf includes renewal: same-key ES256 DPoP nonce signatures were verified, tampering rejected, and one rotation persisted across commands. This is assembled-leaf evidence, not a renewal test of parent2d4bf29.A separate producer-to-API proof used auth
52618e420dab99ec47b1dea9f1d9104b14cfa7c9with API1d7bcc9. Original and rotated session-A tokens changed from 200 to 401 after retirement; same-account/key replacement-B tokens stayed 200. Signature, issuer, audience, expiry and not-before negatives rejected. These current synthetic results supersede the older pins as stack evidence; they do not reattribute B’s historical external-pin assumptions or this parent’s tests.These proofs use real handlers, middleware, CLI transport and signing with controlled identity, storage, upstream services and gateway metadata. The joint run blocks Python sockets and Node network entry points before application imports, which is a process guard, not OS egress isolation. Producer stores are in memory; no deployed durability, real balance, full trading-provider integration or live trade is proved. Real consent/staging, deployed gateway/proxy, live-ledger billing and revocation propagation acceptance remain open. Retirement receipts still mean recorded with propagation pending, under server receipt/source-age limits.
Remaining platform and rollout gates
Native evidence at unchanged store head
83d6covers synthetic Mac 11,416-byte roundtrip/delete and isolated Debian12/Node22/GNOME42.1 11,455 stored bytes, restart/read/delete and locked refusal in 128 ms. It does not establish Windows, Enterprise roaming behavior, desktop cancellation, OS login/PAM/reboot, power loss or remote-terminal acceptance. Client termination does not prove cancellation of outstanding daemon writes; physical deletion remains best effort. Windows storage uses 2,048-byte chunks, a 16 KiB bundle cap and default Enterprise persistence, not a device-local-only promise. Optional platform CI remains absent because the writer token cannot create workflows.Latency/cohort acceptance is still required: native Mac 11,416-byte reads measured 798/2,956/11,764 ms for one/four/sixteen same-process reads; the separate 11,134-byte synthetic executor measured read474/write942/remove1332/single-op69 ms. These are different backends/sizes, with no before/after ratio or measured cross-process threshold. No token cache or batching redesign is included. Mandatory native locking still prevents saved-key mutation without the binding; the packaged legacy-key-only recovery guide mitigates this, with the earlier A minority compatibility objection retained. A657-2 explicitly classifies this as a nonblocking compatibility/rollout limit; no insecure lock fallback is included.
Delivery remains a gated prerelease with no temporary login flag. No merge, deployment, publication or ticket closure is requested. Packaged guidance covers custody, recovery and cohort requirements.
AI execution metadata
AI execution metadata
Provider OpenAI; model
gpt-6-astra; thinking levelmedium. Original API506 writer session01a0b825-ca2b-7d03-b6b3-b7a5044dbfa1. No agents/model changes. Retained runner segments are separate and are not summed. Segments1–7 cover earlier writer work;8 is completed permission parity;9 is completed A repair;10 is completed A feedback administration;11 is this no-code B follow-up snapshot before final GitHub/handoff work. Independent-review usage is excluded. Cached input is included in input; reasoning is included in output.September 23 independent review telemetry
Anthropic
claude-opus-5, high effort, separate checkout/sessiona1ba8940-1cd7-45c5-a6c7-64217e79a7d2. These are separate runtime records, not a claimed lifecycle total. Thinking is included in output. Runtime auxiliary Haiku calls are not additional reviews.Initial SSH push actor was Codier despite hulk commit authorship. Subsequent HTTPS App-authenticated final-head pushes are verified as hulk-linus in GitHub activity; author and committer are also hulk-linus. No history was rewritten.