feat: Allow token to be sent on download #739
Conversation
sources/httpUtils.ts
Outdated
| } | ||
|
|
||
| if (input.origin === (process.env.COREPACK_NPM_REGISTRY || DEFAULT_NPM_REGISTRY_URL) && process.env.COREPACK_NPM_TOKEN) { | ||
| if ((process.env.COREPACK_NPM_REGISTRY || DEFAULT_NPM_REGISTRY_URL).includes(input.origin) && process.env.COREPACK_NPM_TOKEN) { |
There was a problem hiding this comment.
This should probably by something like
new URL(process.env.COREPACK_NPM_REGISTRY || DEFAULT_NPM_REGISTRY_URL).origin === input.originAlso needs a new test to confirm the new behavior works 😄
There was a problem hiding this comment.
That makes sense. I've added a test as well.
Additionally while writing code I got confused because the token and basic auth are handled twice in the code. So I've now updated it to only do it once and updated the tests accordingly. Does that makes sense to do?
There was a problem hiding this comment.
Is there a scenario where COREPACK_NPM_REGISTRY and DEFAULT_NPM_REGISTRY_URL are both undefined?
In that case new URL() will throw.
There was a problem hiding this comment.
No, DEFAULT_NPM_REGISTRY_URL is set as a const in npmRegistryUtils.ts so that should be impossible.
sources/httpUtils.ts
Outdated
|
|
||
| if (username || password) { | ||
| headers = { | ||
| if (username && password) { |
There was a problem hiding this comment.
Now that both username and password are required to set the authorization, should we warn if one is set and not the other?
There was a problem hiding this comment.
Good catch!
Why this change? An empty username or password should be fine, no?
There was a problem hiding this comment.
Ah yes, I didn't explain my logic. In httpUtils it did || but in npmRegistryUtils it did &&. In the README the text is:
COREPACK_NPM_USERNAME and COREPACK_NPM_PASSWORD to set a Basic authorization header when connecting to a npm type registry. Note that both environment variables are required and as plain text. If you want to send an empty password, explicitly set COREPACK_NPM_PASSWORD to an empty string.
Since that is && I picked to do that. Though in reality now that I think about it I believe that it acted as || since even though npmRegistryUtils didn't set the authorization header because of the &&, the httpUtils would add it in anyway 🤔
There was a problem hiding this comment.
Oh, I am dumb, this if would not pass with an empty string of course 🤦♂️
|
CI is failing, can you take a look? |
|
I also created a fix for this problem in #743, which is a smaller change that only addresses the bug. This PR, on the other hand, also updates the way authentication is handled and adds some useful tests. Maybe we can first fix the bug and then follow up with the overall improvements from this PR, since I see it still needs some test fixes. It would be great if we could fix this bug quickly. After the Shai-Hulud attack, I expect more companies will move to registries that let them scan, block, and have more control over the packages being downloaded. These registries can also include paths. |
Should be fixed now! I guess I copied something wrong over. The tests connect to the urls of Yarn and NPM, but they are blocked on my work laptop after the Shai-Hulud attack. So I need to juggle a bit to get the code working and into the PR 😅 |
BobLuursema
force-pushed
the
use-token-for-download
branch
from
6c1b741 to
e4b8daa
Compare
When using Artifactory the registry is set to for example:
https://my-company.example.org/artifactory/api/npm/my-npm. Authentication is mandatory in our setup. But the token will only be sent if the hostname matches with the value in the environment variable. This blocks the download since the variable also includes the path portion.I am not sure how precise you want to be with this check. If needed I can also change it to parse out the hostname from the environment variable for a more secure check.
Fixes #733
In addition, I didn't see a test that checks this check. I can add it if you'd like.