Adds notice support to the login command #4308
Conversation
| const autoOpen = notice.match(/open (https?:\/\/\S+)/i); | ||
| if (autoOpen) { | ||
| try { | ||
| await execUtils.execvp(`open`, [autoOpen[1]], {cwd: ppath.cwd()}); | ||
| } catch { | ||
| try { | ||
| await execUtils.execvp(`xdg-open`, [autoOpen[1]], {cwd: ppath.cwd()}); | ||
| } catch {} | ||
| } | ||
| } |
There was a problem hiding this comment.
Poor Windows users, we should just let https://yarnpkg.com/package/open handle this as it deals with the edge cases.
There was a problem hiding this comment.
Doesn't open spawn a bundled shell file, meaning that we can't bundle it?
There was a problem hiding this comment.
It can do that but it will check if it can use it first (which it can't) and fall back to the system one.
https://github.com/sindresorhus/open/blob/05ba9e150cc1a2629e518a9cc19b586c6ca3f269/index.js#L173-L182
There was a problem hiding this comment.
I think the ideal solution would be to not automatically open the link, this seems like a CVE waiting to happen.
There was a problem hiding this comment.
From a UX perspective I think the auto-open would still be very beneficial. What if we checked the hostname (or even the full url) to check it matches a known pattern? Various other tools have this kind of "open if possible" logic (AWS iirc?), so it should be possible to achieve it safely 🤔
There was a problem hiding this comment.
I've added a prompt; pressing <enter> will try to open the url, but people can use n (or ctrl-c).
There was a problem hiding this comment.
I still don't think we should if we can't do it consistently.
There was a problem hiding this comment.
Sorry, I saw your comment after merging - by consistently, do you refer to the Windows support? I can disable this altogether for win32, if you think that's better (I'm probably a little biased, but I'd like to avoid adding dependencies from this maintainer if I can avoid it ...).
There was a problem hiding this comment.
Yeah, I was referring to Windows support.
* Adds notice support to the login command * Opens the auth url automatically * Replaces tryPromise with try/catch * Updates snapshots * Adds test * Avoids replaceAll * Fixes lint * Loosens the regex * Fixes tests * Adds tests * Adds a prompt before opening the url * Versions * Renames TEST_ENV into YARN_IS_TEST_ENV * Fixes publish test * Fixes set/version test * Fixes windows test
What's the problem this PR addresses?
The npm registry is implementing some authentication features around tokens. For this to work they need us to display the
npm-noticeheader.Closes #5006
How did you fix it?
Prints the header before requesting the OTP token.
Checklist