audit: report any errors above 400 as potentially not supporting audit #128
Conversation
| @@ -189,8 +189,8 @@ function auditCmd (args, cb) { | |||
| }).then((auditReport) => { | |||
| return audit.submitForFullReport(auditReport) | |||
| }).catch((err) => { | |||
| if (err.statusCode === 404 || err.statusCode >= 500) { | |||
There was a problem hiding this comment.
401 needs to not do this, as the main registry is (or is about to) start returning 401s when your auth data is bad, regardless of endpoint.
There was a problem hiding this comment.
I would suggest that at the very least, we catch 404, 401 and then all other errors separately and tune the error messages to align better, eg:
401 - "Either your login credentials are invalid or your registry does not support audit."
404 - "Your registry does not support audit."
all other - what you have now
There was a problem hiding this comment.
I've changed this to follow your suggestion. You can re-review at your convenience.
zkat
force-pushed
the
zkat/better-audit-msg
branch
from
1c99673
to
f611415
Compare
|
Thank you all very much! Looking forward to this in an upcoming release. Appreciate you, as always! |
|
Thanks for this PR, I think this is an improvement as the current error message is very misleading for server errors when you have not configured any other npm registry. Would it make sense to maybe pass the actual http status code to the message, as this might be useful for investigating errors? Also, when will this be included in an npm release? |
|
I verified that the messaging in 6.6.0-next.1 successfully addresses this bug. Thank you, Kat, Rebecca, and Audrey!
|
Fixes: https://npm.community/t/npm-audit-fails-with-enoaudit-on-500-response/3629
Fixes: https://npm.community/t/npm-audit-error-messaging-update-for-401s/3983