fix: throw a clear error when npm login runs without a TTY (#9860) #9878
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,3 +141,108 @@ t.test('does not prompt if stdin or stdout is not a tty', async (t) => { | |
| }, | ||
| }, fn), { message: 'nope' }, 'rejects with the original error') | ||
| }) | ||
|
|
||
| const setupLogin = async (t, { creds = {}, loginWeb, ...rest }, opts = {}) => { | ||
| const { login } = tmock(t, '{LIB}/utils/auth.js', { | ||
| '{LIB}/utils/read-user-info.js': { | ||
| username: async () => 'foo', | ||
| password: async () => 'bar', | ||
| }, | ||
| '{LIB}/utils/open-url.js': { | ||
| createOpener: () => () => {}, | ||
| }, | ||
| 'npm-profile': { | ||
| loginCouch: async () => ({ token: 'test-token' }), | ||
| ...(loginWeb ? { loginWeb } : {}), | ||
| }, | ||
| }) | ||
| const { npm } = await setupMockNpm(t, { | ||
| ...rest, | ||
| config: { 'auth-type': 'legacy', ...rest.config }, | ||
|
Author
There was a problem hiding this comment. Forcing It is not just the config: the 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 (For what it is worth, |
||
| }) | ||
| return login(npm, { creds, registry: 'https://registry.npmjs.org/', ...opts }) | ||
| } | ||
|
|
||
| t.test('login throws a clear error when stdin is not a tty', async (t) => { | ||
| await t.rejects(setupLogin(t, { | ||
| globals: { | ||
| 'process.stdin': { isTTY: false }, | ||
| 'process.stdout': { isTTY: true }, | ||
| }, | ||
| }), { | ||
| code: 'ENOTTYAUTH', | ||
| message: /requires an interactive terminal/, | ||
| }, 'rejects with a clear, actionable error instead of hanging') | ||
| }) | ||
|
|
||
| t.test('login throws a clear error when stdout is not a tty', async (t) => { | ||
| await t.rejects(setupLogin(t, { | ||
| globals: { | ||
| 'process.stdin': { isTTY: true }, | ||
| 'process.stdout': { isTTY: false }, | ||
| }, | ||
| }), { | ||
| code: 'ENOTTYAUTH', | ||
| message: /requires an interactive terminal/, | ||
| }, 'rejects with a clear, actionable error instead of hanging') | ||
| }) | ||
|
|
||
| t.test('login throws a clear error when neither stdin nor stdout is a tty', async (t) => { | ||
| await t.rejects(setupLogin(t, { | ||
| globals: { | ||
| 'process.stdin': { isTTY: false }, | ||
| 'process.stdout': { isTTY: false }, | ||
| }, | ||
| }), { | ||
| code: 'ENOTTYAUTH', | ||
| message: /requires an interactive terminal/, | ||
| }, 'rejects with a clear, actionable error instead of hanging') | ||
| }) | ||
|
|
||
| t.test('login succeeds with couch when stdin and stdout are ttys', async (t) => { | ||
|
Author
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 |
||
| const result = await setupLogin(t, { | ||
| globals: { | ||
| 'process.stdin': { isTTY: true }, | ||
| 'process.stdout': { isTTY: true }, | ||
| }, | ||
| }) | ||
|
|
||
| t.strictSame(result, { | ||
| message: 'Logged in on https://registry.npmjs.org/.', | ||
| newCreds: { token: 'test-token' }, | ||
| }) | ||
| }) | ||
|
|
||
| t.test('login throws a clear error for the web login ENYI fallback when not a tty', async (t) => { | ||
| await t.rejects(setupLogin(t, { | ||
| config: { 'auth-type': 'web' }, | ||
| loginWeb: async () => { | ||
| throw Object.assign(new Error('web login not supported'), { code: 'ENYI' }) | ||
| }, | ||
| globals: { | ||
| 'process.stdin': { isTTY: false }, | ||
| 'process.stdout': { isTTY: false }, | ||
| }, | ||
| }), { | ||
| code: 'ENOTTYAUTH', | ||
| message: /requires an interactive terminal/, | ||
| }, 'rejects with a clear, actionable error instead of hanging on the couch fallback') | ||
| }) | ||
|
|
||
| t.test('login falls back to couch after web login ENYI when a tty', async (t) => { | ||
| const result = await setupLogin(t, { | ||
| config: { 'auth-type': 'web' }, | ||
| loginWeb: async () => { | ||
| throw Object.assign(new Error('web login not supported'), { code: 'ENYI' }) | ||
| }, | ||
| globals: { | ||
| 'process.stdin': { isTTY: true }, | ||
| 'process.stdout': { isTTY: true }, | ||
| }, | ||
| }) | ||
|
|
||
| t.strictSame(result, { | ||
| message: 'Logged in on https://registry.npmjs.org/.', | ||
| newCreds: { token: 'test-token' }, | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
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 andopenUrlPrompt()inlib/utils/open-url.jsexactly, and the placement insideif (!res)is right — it covers both--auth-type=legacyand the web ->ENYI-> couch fallback, which are the two repros in #9860, while leaving--auth-type=webalone. No objection to the guard itself.The blocker is fallout:
test/lib/commands/login.jsmocksprocess.stdin/process.stdoutasstream.PassThroughwith{ replace: true }and never setsisTTY, so this guard fires inlegacy > basic login,legacy > scoped login default registry,legacy > scoped login scoped registryandweb > fallback. All four fail withcode: ENOTTYand 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 addisTTY: trueto the replaced stdin/stdout globals (a PassThrough happily takes the property), matching how theotpleasetests intest/lib/utils/auth.jsalready pass{ isTTY: true }. Please runtap test/lib/commands/login.jsas well astest/lib/utils/auth.jsbefore pushing.