Make switch npm registry and authentication configurable#269
Make switch npm registry and authentication configurable#269
Conversation
Confidence Score: 2/5
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/zpm-switch/src/http.rs
Line: 67-71
Comment:
**`auth_ident` base64 detection encodes on wrong condition**
The intent here appears to be: encode raw `username:password` strings, but pass through pre-encoded base64 tokens as-is. However, the condition is inverted for this intent — it base64-encodes when `:` is present (raw credentials) and passes through as-is when `:` is absent (presumably already encoded).
This happens to be correct behavior because RFC 7617 Basic auth requires base64-encoding of `user:password` (which contains `:`), and a pre-encoded base64 string will never contain `:` (base64 uses A–Z, a–z, 0–9, `+`, `/`, `=`). So mechanically the code works.
That said, the logic would be clearer and more intentional if the comment explained that `:` presence distinguishes raw credentials from pre-encoded tokens — to avoid a future reader interpreting this as a bug and "fixing" it by reversing the condition.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/zpm-switch/src/cache.rs
Line: 33
Comment:
**Cache key does not include registry URL**
The cache key (`CacheKey`) is hashed from `cache_version`, `version`, and `platform` only. It does not include the registry URL (`YARN_SWITCH_NPM_REGISTRY`). This means:
- If a user downloads Yarn from a private/custom registry (e.g., one serving a patched binary), the result is cached under the same key as the official package.
- On the next invocation — even if the user has unset `YARN_SWITCH_NPM_REGISTRY` or changed it back to the default — the cached binary from the private registry will be served.
Conversely, a clean cache populated from the default registry will be reused when a private registry is configured, completely bypassing the custom registry.
Since the registry can now serve genuinely different binaries at the same version number (the whole point of this PR), the registry URL should be part of the cache key to ensure cache entries are scoped to their source.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 5b68f86 |
c620476 to
5b68f86
Compare
| if let Ok(token) = std::env::var("YARN_SWITCH_NPM_AUTH_TOKEN") { | ||
| request = request.bearer_auth(token); | ||
| } else if let Ok(mut auth_ident) = std::env::var("YARN_SWITCH_NPM_AUTH_IDENT") { | ||
| if auth_ident.contains(':') { | ||
| auth_ident = base64::Engine::encode(&base64::engine::general_purpose::STANDARD, auth_ident); |
There was a problem hiding this comment.
auth_ident base64 detection encodes on wrong condition
The intent here appears to be: encode raw username:password strings, but pass through pre-encoded base64 tokens as-is. However, the condition is inverted for this intent — it base64-encodes when : is present (raw credentials) and passes through as-is when : is absent (presumably already encoded).
This happens to be correct behavior because RFC 7617 Basic auth requires base64-encoding of user:password (which contains :), and a pre-encoded base64 string will never contain : (base64 uses A–Z, a–z, 0–9, +, /, =). So mechanically the code works.
That said, the logic would be clearer and more intentional if the comment explained that : presence distinguishes raw credentials from pre-encoded tokens — to avoid a future reader interpreting this as a bug and "fixing" it by reversing the condition.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm-switch/src/http.rs
Line: 67-71
Comment:
**`auth_ident` base64 detection encodes on wrong condition**
The intent here appears to be: encode raw `username:password` strings, but pass through pre-encoded base64 tokens as-is. However, the condition is inverted for this intent — it base64-encodes when `:` is present (raw credentials) and passes through as-is when `:` is absent (presumably already encoded).
This happens to be correct behavior because RFC 7617 Basic auth requires base64-encoding of `user:password` (which contains `:`), and a pre-encoded base64 string will never contain `:` (base64 uses A–Z, a–z, 0–9, `+`, `/`, `=`). So mechanically the code works.
That said, the logic would be clearer and more intentional if the comment explained that `:` presence distinguishes raw credentials from pre-encoded tokens — to avoid a future reader interpreting this as a bug and "fixing" it by reversing the condition.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| if self.version >= first_npm_release { | ||
| return Some(format!("https://registry.npmjs.org/@yarnpkg/yarn-{}/-/yarn-{}-{}.tgz", self.platform, self.platform, self.version.to_file_string())); | ||
| return Some(format!("@yarnpkg/yarn-{}/-/yarn-{}-{}.tgz", self.platform, self.platform, self.version.to_file_string())); |
There was a problem hiding this comment.
Cache key does not include registry URL
The cache key (CacheKey) is hashed from cache_version, version, and platform only. It does not include the registry URL (YARN_SWITCH_NPM_REGISTRY). This means:
- If a user downloads Yarn from a private/custom registry (e.g., one serving a patched binary), the result is cached under the same key as the official package.
- On the next invocation — even if the user has unset
YARN_SWITCH_NPM_REGISTRYor changed it back to the default — the cached binary from the private registry will be served.
Conversely, a clean cache populated from the default registry will be reused when a private registry is configured, completely bypassing the custom registry.
Since the registry can now serve genuinely different binaries at the same version number (the whole point of this PR), the registry URL should be part of the cache key to ensure cache entries are scoped to their source.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm-switch/src/cache.rs
Line: 33
Comment:
**Cache key does not include registry URL**
The cache key (`CacheKey`) is hashed from `cache_version`, `version`, and `platform` only. It does not include the registry URL (`YARN_SWITCH_NPM_REGISTRY`). This means:
- If a user downloads Yarn from a private/custom registry (e.g., one serving a patched binary), the result is cached under the same key as the official package.
- On the next invocation — even if the user has unset `YARN_SWITCH_NPM_REGISTRY` or changed it back to the default — the cached binary from the private registry will be served.
Conversely, a clean cache populated from the default registry will be reused when a private registry is configured, completely bypassing the custom registry.
Since the registry can now serve genuinely different binaries at the same version number (the whole point of this PR), the registry URL should be part of the cache key to ensure cache entries are scoped to their source.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Since the registry can now serve genuinely different binaries at the same version number (the whole point of this PR), the registry URL should be part of the cache key to ensure cache entries are scoped to their source.
The point of this PR isn't that different registries can serve different results, it's that not everyone has open internet access from their device. The NPM registry is explicitly blocked on our corporate network.
There was a problem hiding this comment.
That makes sense for your use case. If the custom registry is purely a proxy/mirror serving identical binaries, then the cache key doesn't need to change.
However, the implementation doesn't enforce that constraint — it accepts any registry URL via the environment variable. If someone (intentionally or accidentally) points YARN_SWITCH_NPM_REGISTRY at a registry serving different packages, the cache would still serve binaries from the wrong source.
Two options:
- Accept the risk — document that custom registries should be mirrors/proxies only, and cache invalidation is the user's responsibility if they switch registries
- Add a safeguard — include the registry URL in the cache key to ensure cache entries are isolated by source, even if that's not the primary use case
Which approach fits your project's expectations better?
I'm not sure where & how best to add tests for this, would love some pointers!
Code works with a local verdaccio setup.