Skip to content

API-320: login key hygiene — no echoes, no history, right URL - #534

Open
gulshngill wants to merge 1 commit into
mainfrom
fix/api-320-login-key-hygiene
Open

API-320: login key hygiene — no echoes, no history, right URL#534
gulshngill wants to merge 1 commit into
mainfrom
fix/api-320-login-key-hygiene

Conversation

@gulshngill

Copy link
Copy Markdown
Contributor

Why

First slice of API-320, re-landed from closed #507 without its MCP-stack coupling (see the close-out rationale on #507). Three login hygiene fixes, all on main today:

  1. The key could be echoed back to the user. VERIFICATION_FAILED relayed upstream error.message verbatim, and upstream errors can contain the key (echoed request headers, undici header-validation errors). Now redacted — raw and trimmed forms — before any user-visible surface, including the structured data payload.
  2. The invalid-key error sent users somewhere that mints keys. /auth/agent-setup auto-fires POST /api-keys on load and is plan-capped (Free = 1) — a user whose key was just rejected could land on a 403. Now points at the key management view app.nansen.ai/api?tab=api, matching the API-390 standardisation already merged in nansen-ra#3452 and nansen-api#1832. The no-key path keeps agent-setup — there, minting is the point.
  3. Help text led with the history-leaking form. --api-key <key> puts the literal key in shell history. Help now leads with --human and command substitution, and states explicitly which forms are recorded.

redactSecret lives in cli.js — deliberately not commands/mcp.js — so nothing here depends on the in-flight MCP stack (#487/#508); this merges independently of both.

Verification

  • 4 new tests: redaction of raw + trimmed key from message and data, resolution URL pinned (agent-setup excluded), help ordering. The redaction tests were run against the unfixed code and fail — they are real pins, not decorations.
  • Full suite: 2291 passed, 2 skipped; lint clean.

Remaining API-320 work (not this PR)

Checklist

  • Tests pass (2291 passed, 2 skipped; lint clean)
  • src/schema.json — n/a, no new commands or flags
  • README.md — n/a, help text is in-CLI
  • Changeset added (patch)

🤖 Generated with Claude Code

Three hygiene fixes to `nansen login`, re-landed from closed #507 without its
MCP-stack coupling:

- VERIFICATION_FAILED relayed upstream error text verbatim, and that text can
  contain the key (echoed request headers/bodies). Redact the key (raw and
  trimmed) before it reaches any user-visible surface. redactSecret lives in
  cli.js -- not commands/mcp.js -- so this carries no MCP dependency.
- The invalid-key resolution sent users to /auth/agent-setup, which auto-mints
  a key on load and is plan-capped (Free = 1), so a user whose key was
  rejected could hit a 403 on arrival. Point it at the key management view
  (API-390 decision, matching nansen-ra and the Kong 401).
- Help text led with `--api-key <key>`, which lands the literal key in shell
  history. Lead with --human and command substitution, and say explicitly
  which forms are recorded.

Tests: redaction (raw + trimmed, message and data payload), resolution URL
pinned with agent-setup excluded, help ordering. The redaction tests were
verified to fail against the unfixed code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nansen-pr-reviewer

Copy link
Copy Markdown

pr-reviewer Summary for #b1813a1

📝 1 finding

Review completed. Please address the findings below.

Findings by Severity

Severity Count
🟡 Medium 1

Review effort: 2/5 (Simple)

Summary

This PR ships three well-scoped security and UX fixes to nansen login: key redaction from relayed upstream errors, corrected resolution URL for invalid keys, and improved help text ordering. The implementation is sound — redactSecret avoids regex pitfalls, the double-call order is correct (raw key first, trimmed second), and all new tests are real failure pins. Changeset is present with the right package and bump level. One medium finding below.

Findings

src/cli.js — medium

Help text uses a macOS-specific command in the cross-platform command-substitution example

Line 1007 shows:

NANSEN_API_KEY=$(security find-generic-password -s nansen-api-key -w) nansen login

security find-generic-password is macOS Keychain CLI only. On Linux/Windows this command fails silently or errors out, potentially setting NANSEN_API_KEY to an empty string (causing a confusing API_KEY_REQUIRED error) or to an error message from security.

This is the example shown to users as the recommended safe path in --help, so it risks leading users on non-macOS systems into a broken flow.

Suggested fix: Either generalize the example to something cross-platform (e.g. NANSEN_API_KEY=$(cat ~/.nansen/key) nansen login) or annotate it as macOS-specific so users on other platforms aren't confused:

  NANSEN_API_KEY=$(security find-generic-password -s nansen-api-key -w) nansen login   # macOS Keychain

Token usage: 1,046 input, 3,294 output, 240,490 cache read, 28,757 cache write | Usage Guide

New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment @nansen-pr-reviewer re-review.

@nansen-pr-reviewer nansen-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved

This PR was automatically approved because:

  • Claude recommends approval
  • Claude assessed this as a moderate effort change
  • The effort level is within the auto-approval threshold of 2
  • No high or critical issues were detected
  • Review comment contains non-blocking feedback

If you have any concerns, please request a manual review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant