Skip to content

Fixes npm auth #3774

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 7, 2017
Merged

Fixes npm auth #3774

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f0a89d
Fixes npm auth
Jun 30, 2017
a241fb7
Fixes yarn install with private packages
arcanis Jun 30, 2017
d4dcfa7
Fixes #3831
Jul 6, 2017
52e3c8b
Uses constants
Jul 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 31 additions & 18 deletions src/registries/npm-registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type RequestManager from '../util/request-manager.js';
import type {RegistryRequestOptions, CheckOutdatedReturn} from './base-registry.js';
import type Config from '../config.js';
import type {ConfigRegistries} from './index.js';
import {YARN_REGISTRY} from '../constants.js';
import * as fs from '../util/fs.js';
import NpmResolver from '../resolvers/registries/npm-resolver.js';
import envReplace from '../util/env-replace.js';
Expand Down Expand Up @@ -76,15 +77,19 @@ export default class NpmRegistry extends Registry {
const registry = this.getRegistry(packageName || pathname);
const requestUrl = url.resolve(registry, pathname);
const alwaysAuth = this.getRegistryOrGlobalOption(registry, 'always-auth');
const customHostSuffix = this.getRegistryOrGlobalOption(registry, 'custom-host-suffix');

const headers = Object.assign(
{
Accept: 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*',
},
opts.headers,
);
if (this.token || (alwaysAuth && isRequestToRegistry(requestUrl, registry, customHostSuffix))) {

const packageIdent = packageName || pathname;
const isScoppedPackage = packageIdent.match(/^@|\/@/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the second part cover? (sorry, not very familiar with scoped packages except for the @user/package pattern.

Copy link
Member Author

@arcanis arcanis Jul 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases packageIdent will be @user/package, and in other cases it will be https://example.org/@user/package.tgz


// this.token must be checked to account for publish requests on non-scopped packages
if (this.token || alwaysAuth || isScoppedPackage) {
const authorization = this.getAuth(packageName || pathname);
if (authorization) {
headers.authorization = authorization;
Expand Down Expand Up @@ -208,26 +213,34 @@ export default class NpmRegistry extends Registry {
return this.token;
}

const registry = this.getRegistry(packageName);
const baseRegistry = this.getRegistry(packageName);
const registries = [baseRegistry];

// Check for bearer token.
const authToken = this.getRegistryOrGlobalOption(registry, '_authToken');
if (authToken) {
return `Bearer ${String(authToken)}`;
// If sending a request to the Yarn registry, we must also send it the auth token for the npm registry
if (baseRegistry === YARN_REGISTRY) {
registries.push(DEFAULT_REGISTRY);
}

// Check for basic auth token.
const auth = this.getRegistryOrGlobalOption(registry, '_auth');
if (auth) {
return `Basic ${String(auth)}`;
}
for (const registry of registries) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this trying to get an auth token for the first matching registry? If so, I find that a bit dangerous. Should we keep a mapping of registry: token pairs and use the appropriate one when communicating. This would also leak tokens to other registries which may be an important security threat.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hence the check to make sure that the npm fallback is only added when the registry is the Yarn registry. The token won't be sent for any other hostname (wich is another issue).

That being said, the whole "multi-registry" logic is flawed, since we only support a single registry implementation (and adding more of them wouldn't make much sense, since it would complexify the codebase for little gain). I'd like to rework it so that we only support a single registry implementation, the npm one, and then make possible to configure what needs to be generic (mostly the hostname). But that's a second step.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 to this until we do the refactor. The code can use a short comment explaining this safe-guard.

// Check for bearer token.
const authToken = this.getRegistryOrGlobalOption(registry, '_authToken');
if (authToken) {
return `Bearer ${String(authToken)}`;
}

// Check for basic username/password auth.
const username = this.getRegistryOrGlobalOption(registry, 'username');
const password = this.getRegistryOrGlobalOption(registry, '_password');
if (username && password) {
const pw = new Buffer(String(password), 'base64').toString();
return 'Basic ' + new Buffer(String(username) + ':' + pw).toString('base64');
// Check for basic auth token.
const auth = this.getRegistryOrGlobalOption(registry, '_auth');
if (auth) {
return `Basic ${String(auth)}`;
}

// Check for basic username/password auth.
const username = this.getRegistryOrGlobalOption(registry, 'username');
const password = this.getRegistryOrGlobalOption(registry, '_password');
if (username && password) {
const pw = new Buffer(String(password), 'base64').toString();
return 'Basic ' + new Buffer(String(username) + ':' + pw).toString('base64');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, no idea we did double base64 for the password part.

}
}

return '';
Expand Down