Skip to content

fix: throw a clear error when npm login runs without a TTY (#9860) - #9878

Open
wakqasahmed wants to merge 3 commits into
npm:latestfrom
wakqasahmed:fix/login-tty-guard-9860
Open

fix: throw a clear error when npm login runs without a TTY (#9860)#9878
wakqasahmed wants to merge 3 commits into
npm:latestfrom
wakqasahmed:fix/login-tty-guard-9860

Conversation

@wakqasahmed

Copy link
Copy Markdown

Fixes #9860

Summary

npm login currently prompts for username/password via read, which reads
from stdin. When stdin (or stdout) is not a TTY — e.g. running npm login in
CI, a script, or with input piped in — the prompt has nothing to read from and
the command exits with code 1 and no error message, giving the user no idea
what went wrong.

This adds an explicit TTY guard in login() (lib/utils/auth.js), mirroring
the existing TTY check already used in otplease() in the same file. When
process.stdin.isTTY or process.stdout.isTTY is falsy, npm login now
throws a clear, actionable error instead of silently failing:

This command requires a TTY to prompt for a username and password.
Non-interactive auth is not supported for `npm login`.
Use `npm token create` or set an auth token in your .npmrc instead.

The error is thrown before any prompt is attempted, and only affects the
couch-login fallback path (i.e. after web login is skipped or not
applicable) — npm login --auth-type=web behavior is unchanged.

Tests

Added to test/lib/utils/auth.js:

  • login throws a clear error when stdin is not a tty
  • login throws a clear error when stdout is not a tty
  • login succeeds with couch when stdin and stdout are ttys (regression guard)

Ran locally: tap test/lib/utils/auth.js --no-coverage — all 10 tests pass.
Also ran eslint on both changed files with no errors.

@wakqasahmed
wakqasahmed requested review from a team as code owners August 15, 2026 21:30

@wakqasahmed wakqasahmed left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Cold-start review (no prior context on this change).

Verdict: changes required — this breaks the existing test suite

The TTY guard itself is placed correctly and matches the otplease() / openUrlPrompt() precedent exactly (!process.stdin.isTTY || !process.stdout.isTTY), and the web-login path is genuinely unaffected. But test/lib/commands/login.js fails completely on this branch — 4 pre-existing tests now throw ENOTTY, and the file aborts. This was not caught because only test/lib/utils/auth.js was run.

Reproduction

git clone --branch fix/login-tty-guard-9860 <fork> && npm i --ignore-scripts
./node_modules/.bin/tap test/lib/commands/login.js --no-coverage

On this branch:

not ok 1 - basic login
not ok 2 - scoped login default registry
not ok 3 - scoped login scoped registry
not ok 4 - fallback            (web -> ENYI -> couch)
not ok - test/lib/commands/login.js

Each fails with:

not ok 1 - This command requires a TTY to prompt for a username and password. ...
  code: ENOTTY

With the guard hunk reverted and nothing else changed, the same file is green (ok 1 - test/lib/commands/login.js). So the failures are caused by this PR, not by the environment.

Cause: mockLogin() in test/lib/commands/login.js does

mockGlobals(t, {
  'process.stdin': stdin,                          // stream.PassThrough
  'process.stdout': new stream.PassThrough(),
}, { replace: true })

Neither PassThrough has isTTY, so it is undefined and the new guard fires before read.username() is ever reached. (Even without replace, process.stdin.isTTY is falsy under tap/CI, so these would fail regardless of the mock.) Every existing test in that file that drives login through the couch prompt must be updated to set isTTY: true on the mocked globals, the same way the otplease tests in test/lib/utils/auth.js already do.

Note that web > fallback is in that list — that is exactly repro #2 from #9860, and it is the one existing test that covers the ENYI fallback. It has to keep passing with isTTY: true, not be deleted.

On the parent question: does the web-login path have the same untested bug?

No — I checked and the web path is safe, for a reason worth stating in the PR body since it is not obvious:

  • loginWeb() reaches createOpener() -> openUrlPrompt() in lib/utils/open-url.js, which already has if (browser === false || !process.stdin.isTTY || !process.stdout.isTTY) { return }. It prints the URL and returns instead of prompting, so npm login --auth-type=web still works non-interactively. This PR does not regress that.
  • Non-interactive CI auth via NPM_TOKEN / _authToken in .npmrc never enters lib/utils/auth.js login() at all, so it is unaffected.
  • read.username(msg, default) always prompts on the first call even when creds.username is pre-populated (the early return is guarded by isRetry), so there is no pre-seeded-credentials flow that used to work non-interactively and now breaks.

So the only real behavioural change for users is: an interactive stdin with a redirected stdout (npm login > log) now hard-fails. That is a change, but it is exactly what otplease() and openUrlPrompt() already do, so I think consistency wins here — just be aware of it.

Scope note

This is a UX guard, not a root-cause fix. read() still never settles on stdin EOF, so npm profile set password, npm profile enable-2fa and npm token create (the other read-user-info.js consumers) still hang the same way. That is fine for this PR's scope, but the upstream npm/read fix referenced in #9860 is still the durable fix and this should not be presented as superseding it.

Individual points inline.

Comment thread lib/utils/auth.js Outdated

// auth type !== web or ENYI error w/ web login
if (!res) {
if (!process.stdin.isTTY || !process.stdout.isTTY) {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The predicate matches the precedent in otplease() at line 10 and openUrlPrompt() in lib/utils/open-url.js exactly, and the placement inside if (!res) is right — it covers both --auth-type=legacy and the web -> ENYI -> couch fallback, which are the two repros in #9860, while leaving --auth-type=web alone. No objection to the guard itself.

The blocker is fallout: test/lib/commands/login.js mocks process.stdin/process.stdout as stream.PassThrough with { replace: true } and never sets isTTY, so this guard fires in legacy > basic login, legacy > scoped login default registry, legacy > scoped login scoped registry and web > fallback. All four fail with code: ENOTTY and the file aborts. Reverting just this hunk makes the file green again, so it is this change.

Fix is in the test fixture, not here: mockLogin() should add isTTY: true to the replaced stdin/stdout globals (a PassThrough happily takes the property), matching how the otplease tests in test/lib/utils/auth.js already pass { isTTY: true }. Please run tap test/lib/commands/login.js as well as test/lib/utils/auth.js before pushing.

Comment thread lib/utils/auth.js Outdated
if (!process.stdin.isTTY || !process.stdout.isTTY) {
throw Object.assign(new Error(
'This command requires a TTY to prompt for a username and password.\n' +
'Non-interactive auth is not supported for `npm login`.\n' +

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Wording is inaccurate in the case this guard most often fires. "Non-interactive auth is not supported for npm login" is not true: npm login --auth-type=web is usable without a TTY — openUrlPrompt() deliberately skips the prompt and prints the URL for you to open elsewhere. And the ENYI fallback means a user who ran the web flow can land on this exact error, at which point the message tells them something they just did successfully is unsupported.

Suggest scoping the claim to the thing that is actually unsupported, e.g.

`npm login` needs a TTY to prompt for a username and password.

and leaving the remedies to the detail lines.

Comment thread lib/utils/auth.js Outdated
throw Object.assign(new Error(
'This command requires a TTY to prompt for a username and password.\n' +
'Non-interactive auth is not supported for `npm login`.\n' +
'Use `npm token create` or set an auth token in your .npmrc instead.'

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

npm token create is circular advice here: it authenticates against the registry using credentials the user does not have, which is why they are running npm login in the first place. Someone in CI with no TTY and no token cannot act on this.

The actionable remedies are: create a granular access token on npmjs.com, then set //registry.npmjs.org/:_authToken=... in .npmrc or NPM_TOKEN in the environment. Worth naming the config key explicitly — it is the part people get wrong.

Two smaller conventions points on this block:

  1. ENOTTY is a real POSIX errno that Node emits for genuine ioctl failures, so overloading it slightly muddies npm error code ENOTTY. ENEEDAUTH already exists in lib/utils/error-message.js and is semantically adjacent; either reuse it or pick a clearly npm-specific code.
  2. There is no case for this code in lib/utils/error-message.js, so all three lines fall into the default branch and land in summary — the user gets three npm error lines of equal weight. npm's convention (see ENEEDAUTH, EACCES, ENOSPC) is a one-line summary.push plus the remediation in detail.push. Adding a small case there would make this read like the rest of npm's errors.

Comment thread test/lib/utils/auth.js
})
const { npm } = await setupMockNpm(t, {
...rest,
config: { 'auth-type': 'legacy', ...rest.config },

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Forcing 'auth-type': 'legacy' here is what makes the helper work, but it also means this helper structurally cannot test the branch #9860 actually reports as repro #2 — web login, registry returns 4xx, npm-profile maps it to ENYI, npm falls back to couch and hits the prompt.

It is not just the config: the npm-profile mock only stubs loginCouch, so loginWeb and webAuthOpener are undefined, and {LIB}/utils/open-url.js is not mocked at all. Set auth-type to web and you get loginWeb is not a function, not an ENYI fallback — so the guard's most important real-world entry point is silently untested and any future regression in the fallback branch would not be caught here.

Worth extending the helper rather than leaving the gap:

'{LIB}/utils/open-url.js': { createOpener: () => () => {} },
'npm-profile': {
  loginWeb: async () => { throw Object.assign(new Error('nyi'), { code: 'ENYI' }) },
  loginCouch: async () => ({ token: 'test-token' }),
},

plus a case with config: { 'auth-type': 'web' } and non-TTY globals asserting ENOTTY, and one asserting a non-ENYI loginWeb error still rethrows untouched rather than being swallowed into the TTY message.

(For what it is worth, test/lib/commands/login.js already has a web > fallback integration test covering that path — it is one of the four this PR breaks. Fixing its isTTY mocks is the higher-priority half of this.)

Comment thread test/lib/utils/auth.js Outdated
}, 'rejects with a clear, actionable error instead of hanging')
})

t.test('login succeeds with couch when stdin and stdout are ttys', async (t) => {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good that a positive-path regression guard is included — asserting that a real TTY still logs in is the thing that stops a guard like this from quietly disabling the feature.

One gap: there is no case for stdin and stdout both non-TTY. The two negative tests each flip only one flag, so a future refactor that changed || to && would still pass both of them. The existing does not prompt if stdin or stdout is not a tty test above sets both to false; mirroring that here would close it.

- fix mockLogin() in test/lib/commands/login.js to set isTTY: true on
  the mocked stdin/stdout PassThrough streams, which the new TTY guard
  was tripping on since PassThrough has no isTTY property; this had
  broken 4 pre-existing tests
- add auth.js unit test coverage for the web-login -> ENYI -> couch
  fallback path, mocking loginWeb and open-url.js so the guard is
  exercised on that path too, not just legacy couch login
- add a test where both stdin and stdout are non-TTY together
- reword the error message so it no longer claims web login is
  unsupported non-interactively (it is); scope the message to the
  couch/legacy prompt path where a real TTY read is required
- replace the circular 'npm token create' remediation with actionable
  steps: create a granular access token on npmjs.com, then set it via
  //registry.npmjs.org/:_authToken or NPM_TOKEN
- rename the error code from ENOTTY (a real POSIX errno Node already
  uses for ioctl failures) to ENOTTYAUTH, and add a case for it in
  error-message.js so it renders with npm's standard summary/detail
  format instead of falling into the generic default handler
@wakqasahmed

Copy link
Copy Markdown
Author

Thanks for the thorough review — addressed everything:

Blocking: broken test suite — fixed. mockLogin() in test/lib/commands/login.js now sets isTTY: true on both mocked stdin/stdout PassThrough streams (they had no isTTY at all, so the new guard was firing on tests that legitimately simulate an interactive session). legacy > basic login, legacy > scoped login default registry, legacy > scoped login scoped registry, and web > fallback all pass again — confirmed with tap test/lib/commands/login.js --no-coverage (13/13 green, no aborts).

Web-login path test coverage gap — fixed. Extended setupLogin() in test/lib/utils/auth.js to optionally mock loginWeb and open-url.js's createOpener, and added two new tests: the ENYI→couch fallback now hits ENOTTYAUTH when non-TTY, and still succeeds via couch when a real TTY is present. This exercises repro #2 from the issue, not just the legacy couch path.

Missing both-non-TTY test case — added (login throws a clear error when neither stdin nor stdout is a tty), so an ||&& regression in the guard would be caught.

Inaccurate error message — reworded to npm login requires an interactive terminal to prompt for credentials., scoped specifically to the couch/legacy prompt path (web login is unaffected and was never really the thing failing).

Circular npm token create advice — replaced with actionable steps: create a granular access token on npmjs.com, then set it via //registry.npmjs.org/:_authToken=<token> or the NPM_TOKEN env var.

Error code collision — renamed ENOTTY (a real Node/POSIX errno used elsewhere for ioctl failures) to ENOTTYAUTH, and added a proper case 'ENOTTYAUTH' in lib/utils/error-message.js with a summary/detail pair so it renders in npm's standard error format instead of falling into the generic default handler. Updated the snapshot test accordingly.

Re-ran both test/lib/utils/auth.js (17 tests) and test/lib/commands/login.js (13 tests) — all green, no regressions. Also ran test/lib/utils/error-message.js since I touched that file — all green. eslint clean on all changed files.

Left as-is, per your note: this remains a UX guard, not a fix for the underlying read()-never-settles-on-EOF issue — npm profile/npm token still hang the same way, and the durable fix is the separate upstream npm/read#157. Not claiming otherwise here.

@wakqasahmed

Copy link
Copy Markdown
Author

Hi @reggi @martinrrm — noticed this PR doesn't have a reviewer assigned yet — it's been about 4 days, CI is green and it's mergeable. Would you (or whoever's best placed) be able to take a look when you get a chance, or point me to who should? Thanks!

@martinrrm

Copy link
Copy Markdown
Contributor

@wakqasahmed I think this doesn't fix the linked issue #9860. The TTY guard changes existing behavior without fixing the issue and I’m concerned it introduces a compatibility regression.

stdin.isTTY === false does not mean that no input is available. npm login can currently consume credentials from a pipe or redirected stream, and this change rejects that input before reading it. It also rejects cases where stdout is redirected even though stdin is still interactive. That makes this a potentially breaking change.

Conversely, a TTY does not guarantee that input remains available: a user can press Ctrl-D at the prompt. In that case stdin.isTTY is still true, so the guard is bypassed and the read promise can still remain unsettled. The underlying problem is that read@6.0.0 does not settle its promise when readline emits close, rather than whether the stream is a TTY.

…m#9860)

Per @martinrrm's review: the blanket `if (!process.stdin.isTTY || !process.stdout.isTTY)`
guard was wrong in three ways — it rejected genuine piped/redirected
credentials that would have worked fine (isTTY false doesn't mean no input
is available), it wrongly gated on stdout's TTY-ness too (redirecting
output has nothing to do with whether input can be read), and it didn't
actually close the reported hole: a real TTY session ending via Ctrl-D
still has isTTY true, so the guard was bypassed and read() could still
hang forever.

Replaced the guard with readOrErrorOnStdinEnd(), which races each prompt
against stdin's own 'end' event. That's the actual failure condition
described in npm#9860 (readline emits 'close' on EOF with no handler, so
read()'s promise never settles) and it's symmetric across every case:
a real TTY session ending (Ctrl-D), a redirected empty file, or a pipe
that closes without writing all now reject with a clear ENOTTYAUTH error
instead of hanging. A pipe or TTY that produces a real line before EOF
is unaffected, since the prompt then wins the race and the listener is
a no-op — closing the regression concern directly rather than arguing
around it.

Test rewrite: the login() test suite's stdin mocks were bare
{isTTY: true/false} objects, incompatible with a fix that needs a real
.once()/.removeListener() — replaced with EventEmitter-based mocks.
Discovered along the way that mock-npm's own "globals" option merges
process.stdin/stdout without mock-globals' "{ replace: true }", which
silently fails to swap those two specifically (they're lazily-defined
getters, not plain properties) — worked around by calling mockGlobals
directly with that flag, matching the pattern test/lib/commands/login.js
already uses. Mocked username/password prompts now return a
never-resolving promise for the 'hangs' test cases, matching the real
bug's behavior more faithfully than a promise that resolves after some
number of microtask ticks, which also removes a timing dependency on
exactly how many ticks separate stdin.emit('end') from prompt resolution
across the legacy vs. web-login-ENYI-fallback code paths.

Verified: test/lib/utils/auth.js (13/13, all new/rewritten) and
test/lib/commands/login.js (10/10, including 'web > fallback' and
'legacy > basic login', which exercise the real read package against a
real PassThrough stream — not mocked around this change) all pass.
eslint clean on both changed files.
@wakqasahmed
wakqasahmed force-pushed the fix/login-tty-guard-9860 branch from ff7d923 to 830ac57 Compare August 26, 2026 00:27
@wakqasahmed

Copy link
Copy Markdown
Author

Thanks — all three points are right, and I want to flag that this was a real gap in the original approach, not a nitpick.

Verified the piped-input claim empirically first: stdin.isTTY === false really doesn't mean no input, and the blanket check really did reject it before even attempting a read.

Replaced the guard with something that addresses all three:

The fix: readOrErrorOnStdinEnd() races each prompt (read.username(), read.password()) against stdin's own end event, rather than checking isTTY up front.

  • Piped credentials work again. If stdin produces a real line before it ends, the prompt resolves and wins the race — the end listener is a no-op. No longer gated on isTTY at all.
  • stdout's TTY-ness is no longer checked. Redirecting output has nothing to do with whether input can be read, so it's dropped from the condition entirely.
  • Ctrl-D on a real interactive session now rejects instead of hanging too. process.stdin.isTTY stays true when a user presses Ctrl-D, but the stream still emits end — this fix catches that case explicitly, which the old TTY check couldn't (this was previously scoped out as "the same read#157 gap," but it turned out to be directly closable at this layer, no upstream dependency needed).

This is symmetric across every case that can actually make the promise hang: TTY-with-Ctrl-D, redirected-empty-file, and pipe-closes-without-writing all reject with a clear ENOTTYAUTH error now, and genuine piped or interactive input that produces a real answer is unaffected regardless of isTTY.

Rewrote the test suite to match — the old tests used bare { isTTY: true/false } objects for process.stdin, which can't simulate this (no .once()/.removeListener()). New tests use EventEmitter-based stdin mocks and cover: stdin-ends-before-answering (piped and TTY), and successful login on both a real TTY and with piped credentials. Also re-verified against the existing test/lib/commands/login.js integration tests, which exercise the real read package against a real PassThrough stream (not mocked around this change) — legacy > basic login and web > fallback both still pass, which is the strongest signal that real piped/typed credentials still work end to end.

test/lib/utils/auth.js (13/13) + test/lib/commands/login.js (10/10) passing, eslint clean.

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.

[BUG] npm login exits 1 with no error message when stdin is not interactive

2 participants