-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FUTURE: initial support for .npmrc file #23560
Conversation
Factored out from #23560 to make it easier to review.
cli/http_util.rs
Outdated
let mut builder = self.get_no_redirect(new_url.clone())?; | ||
if let Some((header_name, header_value)) = maybe_header.as_ref() { | ||
builder = builder.header(header_name, header_value); | ||
} | ||
let new_response = builder.send().await?; |
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.
You should only serve auth tokens after a redirect if the original and redirected URL have the same origin.
66708ec
to
e1a9606
Compare
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.
Nice! Looks really good. Just a few comments.
.unwrap(); | ||
let mut relative_url = registry_root_dir.make_relative(specifier)?; | ||
if relative_url.starts_with("../") { | ||
return None; |
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.
I think we could make this more optimal by storing root_url_to_safe_local_dirname
for each registry url in the constructor, then here getting the relative path from the self.root_dir_url
, then check the first component for a match, then continue below
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.
It appears --reload
flag is not working correctly if .npmrc
is present and custom registries are used.
This is a blocker to landing this PR. Opened #23953 to address in a follow up
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.
LGTM!
This commit adds initial support for ".npmrc" files.
Currently we only discover ".npmrc" files next to "package.json" files
and discovering these files in user home dir is left for a follow up.
This pass supports "_authToken" and "_auth" configuration
for providing authentication.
LSP support has been left for a follow up PR.
Towards #16105