fix(onboard): prevent API key plaintext exposure in retry prompt#1313
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a credential paste-guard to onboarding recovery: detect likely API-key pastes (prefixes like Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant CLI as "Onboard CLI\n(bin/lib/onboard.js)"
participant Store as "Credential Store\n(replaceNamedCredential)"
participant Validator as "NVIDIA Validator\n(conditional)"
User->>CLI: Enter response at recovery prompt
CLI->>CLI: normalize choice (trim/toLowerCase)
CLI-->>CLI: check paste-guard (prefixes / no-space+len / regex)
alt looks like credential
CLI->>User: log warning, request secure credential re-entry
CLI->>Store: replaceNamedCredential(..., validator)
Store-->>CLI: ack -> returns "credential" flow
else other option
CLI->>CLI: handle back / exit / retry as before
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
693-715:⚠️ Potential issue | 🟠 MajorTreat all non-command input as secure credential retry.
At Line 693, the guard is heuristic-based; unrecognized but shorter inputs still fall through to Line 713 (
"selection"). In this credential-recovery branch, it’s safer to route any non-navigation input toreplaceNamedCredential(...)so mistakes always recover through hidden input.🔧 Proposed fix
- // Guard against the user accidentally pasting an API key at this prompt. - if (choice.startsWith("nvapi-") || choice.startsWith("ghp_") || choice.length > 40) { - console.log(" ⚠️ That looks like an API key — do not paste credentials here."); - console.log(" Treating as 'retry'. You will be prompted to enter the key securely."); - const validator = credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null; - await replaceNamedCredential(credentialEnv, `${label} API key`, helpUrl, validator); - return "credential"; - } + const isCommand = + choice === "" || choice === "retry" || choice === "back" || choice === "exit" || choice === "quit"; + if (!isCommand) { + console.log(" ⚠️ Unrecognized option. For safety, entering credential flow."); + const validator = credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null; + await replaceNamedCredential(credentialEnv, `${label} API key`, helpUrl, validator); + return "credential"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 693 - 715, The current handling of `choice` lets short/unrecognized inputs fall through to the selection branch; instead treat any non-navigation input as a secure-credential retry by calling `replaceNamedCredential(...)` and returning "credential". Concretely: keep the existing special-case for API-key-looking inputs (the `choice.startsWith("nvapi-") || choice.startsWith("ghp_") || choice.length > 40` branch) and keep the navigation branches (`choice === "back"`, `choice === "exit" || choice === "quit"` calling `exitOnboardFromPrompt()`), but replace the final fallback so that any other `choice` value (i.e., not back/exit/quit) invokes `replaceNamedCredential(credentialEnv, \`\${label} API key\`, helpUrl, credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null)` and then returns "credential" instead of returning "selection" or printing the retry prompt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 693-715: The current handling of `choice` lets short/unrecognized
inputs fall through to the selection branch; instead treat any non-navigation
input as a secure-credential retry by calling `replaceNamedCredential(...)` and
returning "credential". Concretely: keep the existing special-case for
API-key-looking inputs (the `choice.startsWith("nvapi-") ||
choice.startsWith("ghp_") || choice.length > 40` branch) and keep the navigation
branches (`choice === "back"`, `choice === "exit" || choice === "quit"` calling
`exitOnboardFromPrompt()`), but replace the final fallback so that any other
`choice` value (i.e., not back/exit/quit) invokes
`replaceNamedCredential(credentialEnv, \`\${label} API key\`, helpUrl,
credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null)` and then
returns "credential" instead of returning "selection" or printing the retry
prompt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f0a5a6d-1673-4e5b-96d9-a57765bfe212
📒 Files selected for processing (1)
bin/lib/onboard.js
prekshivyas
left a comment
There was a problem hiding this comment.
Thanks @BenediktSchackenberg — this addresses a real security/UX gap from #1251. The warning text and redirect to replaceNamedCredential are the right approach.
Two items to address:
1. choice.length > 40 false-positive on typed sentences
The heuristic catches pasted tokens, but it will also trigger on normal typed input like:
"I don't understand what to do here can you help me" (52 chars)
That would silently redirect the user into the credential re-entry flow, which is confusing. Suggestion — add a no-spaces check to distinguish a pasted token from a typed sentence:
if (choice.startsWith("nvapi-") || choice.startsWith("ghp_") || (!choice.includes(" ") && choice.length > 40)) {Tokens don't contain spaces; human sentences do. The nvapi- and ghp_ prefix checks can stay as-is since those are unambiguous.
2. Missing test coverage
The existing codebase has tests for promptValidationRecovery flows. Could you add at least one test that exercises the paste-guard detection path? Something like: input "nvapi-fake-key-value" → verify it returns "credential" and calls replaceNamedCredential.
Per prekshivyas review (NVIDIA#1313): 1. Add !choice.includes(' ') to the length-based heuristic to avoid false-positives when users type long help sentences (e.g. 52-char questions). Tokens don't contain spaces; human sentences do. 2. Add a static test in credential-exposure.test.js verifying: - nvapi- and ghp_ prefix checks are present - The length check uses the space-aware form - A bare choice.length > 40 without the space guard is absent Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
Both addressed:
|
|
@prekshivyas — both items from your review are addressed in the latest commit:
Ready for re-review! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
703-717: Hoist shared validator selection to a single definition.
validatoris resolved twice with the same expression (Lines 703 and 716). Hoisting it once above the guard reduces duplication and avoids drift.♻️ Proposed cleanup
+ const validator = credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null; if ( choice.startsWith("nvapi-") || choice.startsWith("ghp_") || (!choice.includes(" ") && choice.length > 40) ) { console.log(" ⚠️ That looks like an API key — do not paste credentials here."); console.log(" Treating as 'retry'. You will be prompted to enter the key securely."); - const validator = credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null; await replaceNamedCredential(credentialEnv, `${label} API key`, helpUrl, validator); return "credential"; } @@ if (choice === "" || choice === "retry") { - const validator = credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null; await replaceNamedCredential(credentialEnv, `${label} API key`, helpUrl, validator); return "credential"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 703 - 717, The code computes the same validator twice; move the validator resolution into a single definition before the choice guards so both the "credential" and the ""/"retry" branches use the same value. Specifically, compute const validator = credentialEnv === "NVIDIA_API_KEY" ? validateNvidiaApiKeyValue : null once (above the if (choice === ...) blocks) and then call await replaceNamedCredential(credentialEnv, `${label} API key`, helpUrl, validator) in both branches, removing the duplicated assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 696-700: The current heuristic in the prompt uses only
choice.startsWith("nvapi-") / choice.startsWith("ghp_") and length>40, which
misses shorter provider keys; update the check that examines the variable choice
(in onboard.js) to use a maintainable list of known API key prefixes (e.g.,
include nvapi-, ghp_, gcm-, sk-, gpt-, gemini- etc.) and replace the single
startsWith checks with a started-by-any-prefix test, and additionally add a
regex fallback (e.g., a URL/base64-safe token pattern with a lower length
threshold) so keys without spaces that match common token formats are caught
even if shorter than 40; ensure references to the variable name choice and the
new PREFIX list (e.g., API_KEY_PREFIXES) are used so the logic is easy to
extend.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 703-717: The code computes the same validator twice; move the
validator resolution into a single definition before the choice guards so both
the "credential" and the ""/"retry" branches use the same value. Specifically,
compute const validator = credentialEnv === "NVIDIA_API_KEY" ?
validateNvidiaApiKeyValue : null once (above the if (choice === ...) blocks) and
then call await replaceNamedCredential(credentialEnv, `${label} API key`,
helpUrl, validator) in both branches, removing the duplicated assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 677dd069-1692-46b7-8ba6-d7cf0385ac41
📒 Files selected for processing (1)
bin/lib/onboard.js
|
Addressed both CodeRabbit comments:
Test updated accordingly. |
0edf541 to
2252212
Compare
Per prekshivyas review (NVIDIA#1313): 1. Add !choice.includes(' ') to the length-based heuristic to avoid false-positives when users type long help sentences (e.g. 52-char questions). Tokens don't contain spaces; human sentences do. 2. Add a static test in credential-exposure.test.js verifying: - nvapi- and ghp_ prefix checks are present - The length check uses the space-aware form - A bare choice.length > 40 without the space guard is absent Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/credential-exposure.test.js`:
- Around line 107-113: Tighten the guards: require the validator declaration to
be present exactly once by changing the validatorCount check from <= 1 to === 1
(reference: validatorCount and the src.match call that searches for 'const
validator = credentialEnv === "NVIDIA_API_KEY"'), make the regex-fallback
assertion more specific by replacing the overly-permissive /\/\^\[A-Za-z0-9/
with a stricter token-regex that anchors and requires a realistic token
character class and minimum length (reference: the expect(src).toMatch(...)
line), and explicitly assert that the looksLikeToken identifier exists in the
generated source (reference: looksLikeToken) so the test fails if the variable
is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31f69f57-fcfb-4aea-aa4a-cb9bde19ffa5
📒 Files selected for processing (2)
bin/lib/onboard.jstest/credential-exposure.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/onboard.js
|
Tightened both assertions per CodeRabbit:
|
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
Per prekshivyas review (NVIDIA#1313): 1. Add !choice.includes(' ') to the length-based heuristic to avoid false-positives when users type long help sentences (e.g. 52-char questions). Tokens don't contain spaces; human sentences do. 2. Add a static test in credential-exposure.test.js verifying: - nvapi- and ghp_ prefix checks are present - The length check uses the space-aware form - A bare choice.length > 40 without the space guard is absent Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
…allback
Per CodeRabbit review:
1. Replace bare startsWith checks with an extensible API_KEY_PREFIXES array
(nvapi-, ghp_, gcm-, sk-, gpt-, gemini-, nvcf-)
2. Add regex fallback for base64-safe token patterns (/^[A-Za-z0-9_\-.]{20,}/)
to catch shorter keys without known prefixes
3. Hoist validator const above the guard to avoid duplication
4. Update test to verify prefix list, looksLikeToken var, and hoisted validator
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
Per CodeRabbit: two assertions were too permissive for a security regression test: - validatorCount <= 1 → === 1 (missing validator would no longer pass) - regex-fallback assertion now matches the full pattern shape including minimum length and test() call Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
30e2af6 to
6a831e8
Compare
Superseded: the no-spaces guard is in place, the paste-guard path now has dedicated test coverage, and the full PR workflow is green.
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed: paste-guard heuristics are sound — prefix list, length+no-space, regex fallback. Low false-positive risk on the happy path. Tests cover detection and secure re-prompt flow.
…DIA#1313) ## Summary Fixes NVIDIA#1251. After an invalid API key during NVIDIA Endpoints onboarding, the CLI showed: ``` Type 'retry', 'back', or 'exit' [retry]: ``` Users interpreted this as a place to re-enter their key and pasted it in plaintext (visible in terminal history). ## Changes 1. **Visible warning** — adds `⚠️ Do NOT paste your API key here` before the prompt 2. **Clearer prompt text** — `Options: retry (re-enter key), back (change provider), exit` 3. **Accidental paste guard** — detects if the user pastes a credential-shaped string (`nvapi-`, `ghp_`, or >40 chars), warns them, and redirects to the secure `replaceNamedCredential` flow ## Before / After **Before:** ``` NVIDIA authorization failed. Re-enter the API key or choose a different provider/model. Type 'retry', 'back', or 'exit' [retry]: nvapi-abc123... ← key pasted here in plaintext ``` **After:** ``` NVIDIA authorization failed. Re-enter the API key or choose a different provider/model.⚠️ Do NOT paste your API key here — use the options below: Options: retry (re-enter key), back (change provider), exit [retry]: ``` Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Clarified the credential-recovery prompt wording for clearer option presentation. * Improved detection of accidental API-key pastes (checks common prefixes, long space-free inputs, and token-like patterns) and forces secure credential re-entry when triggered. * **Tests** * Added a regression test to ensure the paste-detection heuristic is space-aware, prefix-aware, and stable across branches. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
…DIA#1313) ## Summary Fixes NVIDIA#1251. After an invalid API key during NVIDIA Endpoints onboarding, the CLI showed: ``` Type 'retry', 'back', or 'exit' [retry]: ``` Users interpreted this as a place to re-enter their key and pasted it in plaintext (visible in terminal history). ## Changes 1. **Visible warning** — adds `⚠️ Do NOT paste your API key here` before the prompt 2. **Clearer prompt text** — `Options: retry (re-enter key), back (change provider), exit` 3. **Accidental paste guard** — detects if the user pastes a credential-shaped string (`nvapi-`, `ghp_`, or >40 chars), warns them, and redirects to the secure `replaceNamedCredential` flow ## Before / After **Before:** ``` NVIDIA authorization failed. Re-enter the API key or choose a different provider/model. Type 'retry', 'back', or 'exit' [retry]: nvapi-abc123... ← key pasted here in plaintext ``` **After:** ``` NVIDIA authorization failed. Re-enter the API key or choose a different provider/model.⚠️ Do NOT paste your API key here — use the options below: Options: retry (re-enter key), back (change provider), exit [retry]: ``` Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Clarified the credential-recovery prompt wording for clearer option presentation. * Improved detection of accidental API-key pastes (checks common prefixes, long space-free inputs, and token-like patterns) and forces secure credential re-entry when triggered. * **Tests** * Added a regression test to ensure the paste-detection heuristic is space-aware, prefix-aware, and stable across branches. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: ColinM-sys <cmcdonough@50words.com>
Resolves a merge conflict in test/onboard-selection.test.ts, where main
had retyped the lambda parameter to (line: string) while this PR
clarified the assertion text to "Invalid NVIDIA API key. Must start
with nvapi-". Combined: typed parameter + the new assertion string.
Also updates test/credential-exposure.test.ts:114 ("api-key paste-guard
uses extensible prefix list and regex fallback") to match the rewritten
validator line in src/lib/onboard.ts. The original test (added by PR
NVIDIA#1313) asserted the source contained `const validator = credentialEnv
=== "NVIDIA_API_KEY"`; this PR refactors that to a one-line provider-
aware delegate `(key) => validateNvidiaApiKeyValue(key, credentialEnv)`,
so the regression test's regex needs to match the new shape. The
underlying single-definition invariant from NVIDIA#1313 is preserved.
Pushed by maintainer (Allow edits by maintainers is enabled on this PR).
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Summary
Fixes #1251.
After an invalid API key during NVIDIA Endpoints onboarding, the CLI showed:
Users interpreted this as a place to re-enter their key and pasted it in plaintext (visible in terminal history).
Changes
⚠️ Do NOT paste your API key herebefore the promptOptions: retry (re-enter key), back (change provider), exitnvapi-,ghp_, or >40 chars), warns them, and redirects to the securereplaceNamedCredentialflowBefore / After
Before:
After:
Signed-off-by: Benedikt Schackenberg 6381261+BenediktSchackenberg@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests