fix: throw a clear error when npm login runs without a TTY (#9860) - #9878
fix: throw a clear error when npm login runs without a TTY (#9860)#9878wakqasahmed wants to merge 2 commits into
Conversation
wakqasahmed
left a comment
There was a problem hiding this comment.
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()reachescreateOpener()->openUrlPrompt()inlib/utils/open-url.js, which already hasif (browser === false || !process.stdin.isTTY || !process.stdout.isTTY) { return }. It prints the URL and returns instead of prompting, sonpm login --auth-type=webstill works non-interactively. This PR does not regress that.- Non-interactive CI auth via
NPM_TOKEN/_authTokenin.npmrcnever enterslib/utils/auth.js login()at all, so it is unaffected. read.username(msg, default)always prompts on the first call even whencreds.usernameis pre-populated (the early return is guarded byisRetry), 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.
|
|
||
| // auth type !== web or ENYI error w/ web login | ||
| if (!res) { | ||
| if (!process.stdin.isTTY || !process.stdout.isTTY) { |
There was a problem hiding this comment.
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.
| 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' + |
There was a problem hiding this comment.
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.
| 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.' |
There was a problem hiding this comment.
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:
ENOTTYis a real POSIX errno that Node emits for genuine ioctl failures, so overloading it slightly muddiesnpm error code ENOTTY.ENEEDAUTHalready exists inlib/utils/error-message.jsand is semantically adjacent; either reuse it or pick a clearly npm-specific code.- There is no
casefor this code inlib/utils/error-message.js, so all three lines fall into thedefaultbranch and land insummary— the user gets threenpm errorlines of equal weight. npm's convention (seeENEEDAUTH,EACCES,ENOSPC) is a one-linesummary.pushplus the remediation indetail.push. Adding a smallcasethere would make this read like the rest of npm's errors.
| }) | ||
| const { npm } = await setupMockNpm(t, { | ||
| ...rest, | ||
| config: { 'auth-type': 'legacy', ...rest.config }, |
There was a problem hiding this comment.
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.)
| }, 'rejects with a clear, actionable error instead of hanging') | ||
| }) | ||
|
|
||
| t.test('login succeeds with couch when stdin and stdout are ttys', async (t) => { |
There was a problem hiding this comment.
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
|
Thanks for the thorough review — addressed everything: Blocking: broken test suite — fixed. Web-login path test coverage gap — fixed. Extended Missing both-non-TTY test case — added ( Inaccurate error message — reworded to Circular Error code collision — renamed Re-ran both Left as-is, per your note: this remains a UX guard, not a fix for the underlying |
Fixes #9860
Summary
npm logincurrently prompts for username/password viaread, which readsfrom stdin. When stdin (or stdout) is not a TTY — e.g. running
npm logininCI, 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), mirroringthe existing TTY check already used in
otplease()in the same file. Whenprocess.stdin.isTTYorprocess.stdout.isTTYis falsy,npm loginnowthrows a clear, actionable error instead of silently failing:
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=webbehavior is unchanged.Tests
Added to
test/lib/utils/auth.js:login throws a clear error when stdin is not a ttylogin throws a clear error when stdout is not a ttylogin 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
eslinton both changed files with no errors.