Fix: Only add auth token when registry url matches pathname url (fixes #3907) by soda0289 · Pull Request #3987 · yarnpkg/yarn

soda0289 · 2017-07-21T20:36:03Z

Summary
Fixes issue: #3907

In yarn version 0.27.4 a patch (5ff6922) introduced a change that caused an authorization header to be added to requests that were not being sent to the registry.

This commit brings back some older logic to ensure we only add the authorization header when we are sending requests to the registry.

Test plan

To test this change you need a private repository using a repository server such, as Verdaccio configured, to only allow authenticated calls to be made for both download and publish commands. This configuration requires that yarn has the always-auth option enabled.

If we install additional packages from github and save them to the yarn.lock file it should not send the authorization header for these requests.

I started to add some unit tests. This is my first time writing mocks with Jest and any suggestions are welcome. I can add more tests for additional cases as well.

BYK

Awesome, thank you!

I've made some comments and it'd be great if you can address them. But more importantly, I think we are missing a crucial test case: "to non-registry with always-auth: true", right?

BYK · 2017-07-23T13:58:23Z

  }

+  getRequestUrl(registry: string, pathname: string): string {
+    const isPathnameUrl = pathname.match(/^https?:/);


You probably want /^https?:/.test(pathname) here which is faster and returns a boolean instead of a full match object.

Also, what is a "path name URL"? Can't we just say isHTTP or something for clarity?

The .match string method is used throughtout this class and I'm guessing those should be repalced as well. I can fix this in another PR.

I will rename the variable to isUrl. I think pathname can also be a path, /foo/bar/, as well as a URL.

.test is not part of the string prototype only the Regex prototype.

BYK · 2017-07-23T13:59:52Z

+    const requestUrl = this.getRequestUrl(registry, pathname);
+
    const alwaysAuth = this.getRegistryOrGlobalOption(registry, 'always-auth');
+    const customHostSuffix = this.getRegistryOrGlobalOption(registry, 'custom-host-suffix');


May be this should be part of the isRequestToRegistry function itself? This looks like a leaky abstraction to me.

I will have to move the isRequestToRegistry inside the NPMRegistry class since it will needs access to the config to read the 'custom-host-suffix`.

BYK · 2017-07-23T14:01:16Z

+    const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
+    const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter);
+
+    const pathname = 'http://github.com/yarnpkg/yarn.tgz';


This is a URL not a path name. Variable names are important ;)

BYK · 2017-07-23T14:02:45Z

+
+    const requestParams = mockRequestManager.request.mock.calls[0][0];
+
+    expect(requestParams.url).toBe(pathname);


You should probably use toHaveBeenLastCalledWith instead.

Is there a way to use .toHaveBeenLastCalledWith and only match one property of a JSON object? Sinon has matchers that can be used but I don't see anything similar in the documentation of Jest.

BYK · 2017-07-23T14:03:05Z

+
+    const requestParams = mockRequestManager.request.mock.calls[0][0];
+
+    expect(requestParams.headers.authorization).toBe(undefined);


Same as above: use toHaveBeenCalledWith or toHaveBeenLastCalledWith

BYK · 2017-07-23T14:03:22Z

+    };
+  }
+
+  test('should call requestManager.request with pathname url', () => {


Just url nor pathname url

BYK · 2017-07-23T14:04:37Z

+    const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
+    const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter);
+
+    const pathname = 'http://github.com/yarnpkg/yarn.tgz';


Use HTTPS URLs, please. Even for tests ;)

BYK · 2017-07-23T14:05:28Z

+
+    const requestParams = mockRequestManager.request.mock.calls[0][0];
+
+    expect(requestParams.headers.authorization).toBe(undefined);


Use toBeUndefined instead.

BYK · 2017-07-23T14:06:13Z

+    const pathname = 'https://registry.npmjs.org/yarnpkg/yarn.tgz';
+
+    npmRegistry.config = {
+      'always-auth': false,


I think this is the default value anyways, right?

I think the default might be undefined do you think I should have a test for that as well.

If you feel like it I wouldn't say "no" to more tests. That said if the default is false or "falsy" I think just omitting it from the config and adding a comment saying // The default is always-auth: false would be enough.

soda0289 · 2017-07-24T15:51:24Z

@BYK Thanks for the review!

I have made the following changes:
- Add test for non-registry and always-auth true
- Rename variables
- Use Regex.text instead of String.match
- Move isRequesteRegistry into NPMRegistry class
- Move customHostSuffix code inside isRequestToRegistry
- Use .toBeUndefined

I did not use .toHaveBeenLastCalledWith() since I was unsure how to only check one JSON property in the argument. I made all of the other changes.

- Add test for non-registry and always-auth true - Rename variables - Use Regex.text instead of String.match - Move isRequesteRegistry into NPMRegistry class - Move customHostSuffix code inside isRequestToRegistry - Use .toBeUndefined

BYK

Thanks for the additional test case! I think we have some code cleaning up and then we're good to merge!

BYK · 2017-07-25T11:49:39Z

+    const pathname = 'https://registry.npmjs.org/yarnpkg/yarn.tgz';
+
+    npmRegistry.config = {
+      'always-auth': false,


If you feel like it I wouldn't say "no" to more tests. That said if the default is false or "falsy" I think just omitting it from the config and adding a comment saying // The default is always-auth: false would be enough.

BYK · 2017-07-25T11:53:16Z

+    return port;
+  }
+
+  isRequestToRegistry(requestUrl: string, registry: string): boolean {


I don't see any reason for this or getPortOrDefaultPort methods to be "instance methods". Can you make them static?

BYK · 2017-07-25T12:00:27Z

    }
  }

+  getPortOrDefaultPort(port: ?string, protocol: ?string): ?string {


Just say getPort? Also you can simplify your function as follows:

const DEFAULT_PORTS = Object.freeze({ 'http:': '80', 'https:': '443', }); /** * Remove the port if it is the default for the protocol, fallback to emptry string if no port provided. */ static getPort(port: ?string, protocol: ?string): string { return !port || port === DEFAULT_PORTS[protocol] ? '' : port; }

BYK · 2017-07-25T12:02:08Z

+      requestPort === registryPort &&
+      (requestPath.startsWith(registryPath) ||
+        // For some registries, the package path does not prefix with the registry path
+        (typeof customHostSuffix === 'string' && customHostSuffix.length > 0 && requestHost.endsWith(customHostSuffix)))


Why can't we just do (customHostSuffix && requestHost.endsWith(customHostSuffix)?

I originally did have it like that but flow check was failing. I think the return value of getRegistryOrGlobalOption() is mixed and therefore would need to assert it is a string before performing string operations.

I can remove the .length check since that should not be necessary.

Oh, makes sense. Well, may be you can do

const customHostSuffix = String(this.getRegistryOrGlobalOption(registry, 'custom-host-suffix') || '');

So it is always a string? The only downside is if someone someone sets this to null or something, it will be converted into the string 'null'. Not too concerned about that since AFAIK typeof checks are expensive.

BYK · 2017-07-25T12:06:56Z

+      ),
+    ).toBe(true);
+
+    expect(npmRegistry.isRequestToRegistry('https://foo.bar:443/foo/bar/baz', 'https://foo.bar/foo/')).toBe(true);


How about this:

const validRegistryUrls = Object.feeze({ 'https://foo.bar:443/foo/bar/baz': 'https://foo.bar/foo/', 'https://foo.bar/foo/bar/baz': 'https://foo.bar:443/foo/', .... }); const invalidRegistryUrls = Object.freeze({ 'https://wrong.thing/foo/bar/baz': 'https://foo.bar/foo/', ... }); Object.keys(validRegistryUrls).forEach(url => expect(npmRegistry.isRequestToRegistry(url, validRegistryUrls[url]).toBeTruthy()); Object.keys(invalidRegistryUrls).forEach(url => expect(npmRegistry.isRequestToRegistry(url, validRegistryUrls[url]).toBeFalsy());

You may also wanna look at this library which does all this logic for you: https://yarnpkg.com/en/package/normalize-url

I used the normalize-url library cleans up the getPort() logic

I used the normalize-url library cleans up the getPort() logic

Did you push these changes? I don't see them yet.

I had to use arrays instead of Objects since some tests had duplicate keys.

soda0289 · 2017-07-25T15:11:08Z

@BYK

I was able to remove the getPort() function and use the library normalize-url to handle that logic. Also normalizes unicode urls which might not have been checked before. I cleaned up the tests by creating an array of the tests cases. Let me know if anything else can be improved.

- Commented out default value for always-auth - Added library to normalize urls - Removed defaultPort code and use library instead - Removed length check on customHostSuffix

BYK

Looks great, thanks a lot!

BYK · 2017-07-26T10:29:04Z

  }

+  getRequestUrl(registry: string, pathname: string): string {
+    const isUrl = /^https?:/.test(pathname);


Nitpick: technically git://xxx.com/yyy is also a URL so you'd want to name this as isHTTPUrl or something.

This will break protocol relative URLs, which are generated by default in .npmrc

Oh. Issue filed somewhere?

BYK · 2017-07-26T10:33:13Z

+  isRequestToRegistry(requestUrl: string, registryUrl: string): boolean {
+    const normalizedRequestUrl = normalizeUrl(requestUrl);
+    const normalizedRegistryUrl = normalizeUrl(registryUrl);
+    const requestParsed = url.parse(normalizedRequestUrl);


Simplification:

const {requestHost, requestPath} = url.parse(normalizeUrl(requestUrl)); const {registryHost, registryPath} = url.parse(normalizeUrl(registryUrl));

Also, registryUrl should not change frequently so we may consider memoizing parsing that in a future patch.

BYK · 2017-07-26T10:35:45Z

+
+    npmRegistry.request(url);
+
+    const requestParams = mockRequestManager.request.mock.calls[0][0];


toHaveBeenCalledWith()

BYK · 2017-07-26T10:44:16Z

+    expect(requestParams.url).toBe(url);
+  });
+
+  test('should not add authorization header if pathname not to registry', () => {


s/pathname/request

**Summary** Fixes matching protocol-relative registry URLs from config. Reported here: #3987 (comment) **Test plan** Added one new test.

**Summary** Fixes matching protocol-relative registry URLs from config. Reported here: yarnpkg#3987 (comment) **Test plan** Added one new test.

Fix: Only add auth token when registry url matches pathname url

ab1d919

soda0289 force-pushed the fix-registry-auth-token branch from a38a003 to ab1d919 Compare July 21, 2017 20:48

soda0289 changed the title ~~Fix: Only add auth token when registry url matches pathname url (fixes 3907)~~ Fix: Only add auth token when registry url matches pathname url (fixes #3907) Jul 21, 2017

BYK requested a review from arcanis July 23, 2017 13:53

BYK assigned arcanis Jul 23, 2017

BYK requested changes Jul 23, 2017

View reviewed changes

soda0289 force-pushed the fix-registry-auth-token branch from 5234a99 to e5c0a49 Compare July 24, 2017 15:55

Add test and clean up variable names

429bbea

- Add test for non-registry and always-auth true - Rename variables - Use Regex.text instead of String.match - Move isRequesteRegistry into NPMRegistry class - Move customHostSuffix code inside isRequestToRegistry - Use .toBeUndefined

soda0289 force-pushed the fix-registry-auth-token branch from e5c0a49 to 429bbea Compare July 24, 2017 16:40

BYK requested changes Jul 25, 2017

View reviewed changes

Cleanup isRequestToRegistry and tests

d4307df

- Commented out default value for always-auth - Added library to normalize urls - Removed defaultPort code and use library instead - Removed length check on customHostSuffix

soda0289 force-pushed the fix-registry-auth-token branch from 3c71e7a to d4307df Compare July 25, 2017 16:24

BYK approved these changes Jul 26, 2017

View reviewed changes

BYK merged commit 050815d into yarnpkg:master Jul 26, 2017

This was referenced Jul 26, 2017

Only send authentication header for the correct registries #3407

Closed

Scoped packages 403 on yarn install with empty cache #4016

Closed

plmrry mentioned this pull request Sep 7, 2017

[email protected] consistently 404s on private packages #3765

Closed

BYK added a commit that referenced this pull request Sep 8, 2017

Fix: support protocol-relative registry URLs

b888f8d

**Summary** Fixes matching protocol-relative registry URLs from config. Reported here: #3987 (comment) **Test plan** Added one new test.

BYK mentioned this pull request Sep 8, 2017

Fix: support protocol-relative registry URLs #4347

Merged

BYK added a commit that referenced this pull request Sep 8, 2017

Fix: support protocol-relative registry URLs (#4347)

161f97e

**Summary** Fixes matching protocol-relative registry URLs from config. Reported here: #3987 (comment) **Test plan** Added one new test.

bestander mentioned this pull request Jan 8, 2018

Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

Closed


		const requestParams = mockRequestManager.request.mock.calls[0][0];

		expect(requestParams.url).toBe(pathname);


		const requestParams = mockRequestManager.request.mock.calls[0][0];

		expect(requestParams.headers.authorization).toBe(undefined);


		npmRegistry.request(url);

		const requestParams = mockRequestManager.request.mock.calls[0][0];

Conversation

soda0289 commented Jul 21, 2017 • edited by BYK Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BYK left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soda0289 Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soda0289 commented Jul 24, 2017

Uh oh!

BYK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soda0289 commented Jul 25, 2017

Uh oh!

BYK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BYK Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

soda0289 commented Jul 21, 2017 •

edited by BYK

Loading

BYK left a comment •

edited

Loading

soda0289 Jul 24, 2017 •

edited

Loading

BYK Sep 8, 2017 •

edited

Loading