test: add unit test for isUplinkValid #571

src/lib/up-storage.js

@@ -7,19 +7,17 @@ import _ from 'lodash';
 import request from 'request';
 import Stream from 'stream';
 import URL from 'url';
-import {parseInterval, isObject, ErrorCode} from './utils';
+import {parseInterval, is_object, ErrorCode} from './utils';
 import {ReadTarball} from '@verdaccio/streams';
 
 import type {
   IProxy,
   Config,
-  UpLinkConf,
   Callback,
-  Headers,
   Logger,
 } from '@verdaccio/types';
 
-// import type {IUploadTarball, IReadTarball} from '@verdaccio/streams';
+import type {IUploadTarball} from '@verdaccio/streams';
 
 const LoggerApi = require('./logger');
 const encode = function(thing) {
@@ -44,15 +42,15 @@ const setConfig = (config, key, def) => {
  * (same for storage.js, local-storage.js, up-storage.js)
  */
 class ProxyStorage implements IProxy {
-  config: UpLinkConf;
+  config: Config;
   failed_requests: number;
   userAgent: string;
   ca: string | void;
   logger: Logger;
   server_id: string;
   url: any;
-  maxage: number;
-  timeout: number;
+  maxage: string;
+  timeout: string;
   max_fails: number;
   fail_timeout: number;
   upname: string;
@@ -78,7 +76,7 @@ class ProxyStorage implements IProxy {
 
     this.config.url = this.config.url.replace(/\/$/, '');
 
-    if (this.config.timeout && Number(this.config.timeout) >= 1000) {
+    if (Number(this.config.timeout) >= 1000) {
       this.logger.warn(['Too big timeout value: ' + this.config.timeout,
                         'We changed time format to nginx-like one',
                         '(see http://nginx.org/en/docs/syntax.html)',
@@ -98,14 +96,14 @@ class ProxyStorage implements IProxy {
    * @param {*} cb
    * @return {Request}
    */
-  request(options: any, cb?: Callback) {
+  request(options: any, cb: Callback) {
     let json;
 
     if (this._statusCheck() === false) {
       let streamRead = new Stream.Readable();
 
       process.nextTick(function() {
-        if (cb) {
+        if (_.isFunction(cb)) {
           cb(ErrorCode.get500('uplink is offline'));
         }
         // $FlowFixMe
@@ -133,7 +131,7 @@ class ProxyStorage implements IProxy {
       uri: uri,
     }, 'making request: \'@{method} @{uri}\'');
 
-    if (isObject(options.json)) {
+    if (is_object(options.json)) {
       json = JSON.stringify(options.json);
       headers['Content-Type'] = headers['Content-Type'] || 'application/json';
     }
@@ -144,7 +142,6 @@ class ProxyStorage implements IProxy {
       // $FlowFixMe
       processBody(err, body);
       logActivity();
-      // $FlowFixMe
       cb(err, res, body);
 
       /**
@@ -167,7 +164,7 @@ class ProxyStorage implements IProxy {
           }
         }
 
-        if (!err && isObject(body)) {
+        if (!err && is_object(body)) {
           if (_.isString(body.error)) {
             error = body.error;
           }
@@ -264,9 +261,8 @@ class ProxyStorage implements IProxy {
    * @private
    */
   _setAuth(headers: any) {
-    const auth = this.config.auth;
 
-    if (typeof auth === 'undefined' || headers['authorization']) {
+    if (_.isNil(this.config.auth) || headers['authorization']) {
       return headers;
     }
 
@@ -277,12 +273,10 @@ class ProxyStorage implements IProxy {
     // get NPM_TOKEN http://blog.npmjs.org/post/118393368555/deploying-with-npm-private-modules
     // or get other variable export in env
     let token: any = process.env.NPM_TOKEN;
-
-    if (auth.token) {
-      token = auth.token;
-    } else if (auth.token_env ) {
-      // $FlowFixMe
-      token = process.env[auth.token_env];
+    if (this.config.auth.token) {
+      token = this.config.auth.token;
+    } else if (this.config.auth.token_env) {
+      token = process.env[this.config.auth.token_env];
     }
 
     if (_.isNil(token)) {
@@ -290,9 +284,8 @@ class ProxyStorage implements IProxy {
     }
 
     // define type Auth allow basic and bearer
-    const type = auth.type;
+    const type = this.config.auth.type;
     this._setHeaderAuthorization(headers, type, token);
-
     return headers;
   }
 
@@ -341,17 +334,14 @@ class ProxyStorage implements IProxy {
    * @param {Object} headers
    * @private
    */
-  _overrideWithUplinkConfigHeaders(headers: Headers) {
-    if (!this.config.headers) {
-      return headers;
-    }
-
+  _overrideWithUplinkConfigHeaders(headers: any) {
     // add/override headers specified in the config
-    /* eslint guard-for-in: 0 */
     for (let key in this.config.headers) {
+      if (Object.prototype.hasOwnProperty.call(this.config.headers, key)) {
         headers[key] = this.config.headers[key];
       }
     }
+  }
 
   /**
    * Determine whether can fetch from the provided URL
@@ -361,8 +351,10 @@ class ProxyStorage implements IProxy {
   isUplinkValid(url: string) {
       // $FlowFixMe
       url = URL.parse(url);
+      return (url.protocol === this.url.protocol) &&
+             (url.host === this.url.host) &&
              // $FlowFixMe
-    return url.protocol === this.url.protocol && url.host === this.url.host && url.path.indexOf(this.url.path) === 0;
+             (url.path.indexOf(this.url.path) === 0);
   }
 
   /**
@@ -457,8 +449,8 @@ class ProxyStorage implements IProxy {
    * @return {Stream}
    */
   search(options: any) {
-    const transformStream: any = new Stream.PassThrough({objectMode: true});
-    const requestStream: stream$Readable = this.request({
+    const transformStream: IUploadTarball = new Stream.PassThrough({objectMode: true});
+    const requestStream: IUploadTarball = this.request({
       uri: options.req.url,
       req: options.req,
       headers: {
@@ -467,7 +459,7 @@ class ProxyStorage implements IProxy {
     });
 
     let parsePackage = (pkg) => {
-      if (isObject(pkg)) {
+      if (is_object(pkg)) {
         transformStream.emit('data', pkg);
       }
     };
@@ -496,8 +488,6 @@ class ProxyStorage implements IProxy {
     });
 
     transformStream.abort = () => {
-      // FIXME: this is clearly a potential issue
-      // $FlowFixMe
       requestStream.abort();
       transformStream.emit('end');
     };

test/functional/sanity/nullstorage.js

@@ -8,26 +8,28 @@ function getBinary() {
 
 export default function (server, server2) {
 
+  describe('should check whether test-nullstorage is on server1', () => {
     test('trying to fetch non-existent package / null storage', () => {
       return server.getPackage('test-nullstorage-nonexist')
                .status(404)
                .body_error(/no such package/);
     });
+  });
 
-  describe('test-nullstorage on server2', () => {
+  describe('should check whether test-nullstorage is on server2', () => {
     beforeAll(function() {
       return server2.addPackage('test-nullstorage2');
     });
 
-    test('creating new package - server2', () => {/* test for before() */});
+    test('should creaate a new package on server2', () => {/* test for before() */});
 
-    test('downloading non-existent tarball', () => {
+    test('should fails on download a non existent tarball', () => {
       return server.getTarball('test-nullstorage2', 'blahblah')
                .status(404)
                .body_error(/no such file/);
     });
 
-    describe('tarball', () => {
+    describe('test and publish test-nullstorage2 package', () => {
       beforeAll(function() {
         return server2.putTarball('test-nullstorage2', 'blahblah', getBinary())
                  .status(201)
@@ -42,9 +44,9 @@ export default function (server, server2) {
                  .body_ok(/published/);
       });
 
-      test('uploading new tarball', () => {/* test for before() */});
+      test('should upload a new version for test-nullstorage2', () => {/* test for before() */});
 
-      test('downloading newly created tarball', () => {
+      test('should fetch the newly created published tarball for test-nullstorage2', () => {
         return server.getTarball('test-nullstorage2', 'blahblah')
                  .status(200)
                  .then(function(body) {
@@ -52,7 +54,7 @@ export default function (server, server2) {
                  });
       });
 
-      test('downloading newly created package', () => {
+      test('should check whether the metadata for test-nullstorage2 match', () => {
         return server.getPackage('test-nullstorage2')
                  .status(200)
                  .then(function(body) {

test/unit/up-storage.spec.js

@@ -5,26 +5,20 @@ import _ from 'lodash';
 // $FlowFixMe
 import configExample from './partials/config';
 import {setup} from '../../src/lib/logger';
-
-import type {UpLinkConf, Config} from '@verdaccio/types';
+import type {IProxy, Config} from '@verdaccio/types';
 
 setup([]);
 
 describe('UpStorge', () => {
 
-  const uplinkDefault: UpLinkConf = {
-    url: 'https://registry.npmjs.org/',
-    fail_timeout: '5m',
-    max_fails: 2,
-    maxage: '2m',
-    timeout: '1m',
+  const uplinkDefault = {
+    url: 'https://registry.npmjs.org/'
   };
-
-  let generateProxy = (config: UpLinkConf = uplinkDefault) => {
+  const generateProxy = (config: UpLinkConf = uplinkDefault): IProxy => {
     const appConfig: Config = new AppConfig(configExample);
 
     return new ProxyStorage(config, appConfig);
-  }
+  };
 
   test('should be defined', () => {
     const proxy = generateProxy();
@@ -143,4 +137,69 @@ describe('UpStorge', () => {
       });
     });
 
+  describe('UpStorge::isUplinkValid', () => {
+    const validateUpLink = (
+      url: string,
+      tarBallUrl?: string = `${url}/artifactory/api/npm/npm/pk1-juan/-/pk1-juan-1.0.7.tgz`) => {
+      const uplinkConf = { url };
+      const proxy: IProxy = generateProxy(uplinkConf);
+
+      return proxy.isUplinkValid(tarBallUrl);
+    }
+
+    test('should validate tarball path against uplink', () => {
+      expect(validateUpLink('https://artifactory.mydomain.com')).toBe(true);
+    });
+
+    test('should validate tarball path against uplink case#2', () => {
+      expect(validateUpLink('https://artifactory.mydomain.com:443')).toBe(true);
+    });
+
+    test('should validate tarball path against uplink case#3', () => {
+      expect(validateUpLink('http://localhost')).toBe(true);
+    });
+
+    test('should validate tarball path against uplink case#4', () => {
+      expect(validateUpLink('http://my.domain.test')).toBe(true);
+    });
+
+    test('should validate tarball path against uplink case#5', () => {
+      expect(validateUpLink('http://my.domain.test:3000')).toBe(true);
+    });
+
+    // corner case https://github.com/verdaccio/verdaccio/issues/571
+    test('should validate tarball path against uplink case#6', () => {
+      expect(validateUpLink('https://my.domain.test',
+      `https://my.domain.test:443/artifactory/api/npm/npm/pk1-juan/-/pk1-juan-1.0.7.tgz`)).toBe(false);
+    });
+
+    test('should fails on validate tarball path against uplink', () => {
+      const url = 'https://artifactory.mydomain.com';
+      const tarBallUrl = 'https://localhost/api/npm/npm/pk1-juan/-/pk1-juan-1.0.7.tgz';
+      const uplinkConf = { url };
+      const proxy: IProxy = generateProxy(uplinkConf);
+
+      expect(proxy.isUplinkValid(tarBallUrl)).toBe(false);
+    });
+
+    test('should fails on validate tarball path against uplink case#2', () => {
+      const url = 'https://localhost:5001';
+      const tarBallUrl = 'https://localhost/api/npm/npm/pk1-juan/-/pk1-juan-1.0.7.tgz';
+      const uplinkConf = { url };
+      const proxy: IProxy = generateProxy(uplinkConf);
+
+      expect(proxy.isUplinkValid(tarBallUrl)).toBe(false);
+    });
+
+    test('should fails on validate tarball path against uplink case#3', () => {
+      const url = 'http://localhost:5001';
+      const tarBallUrl = 'https://localhost/api/npm/npm/pk1-juan/-/pk1-juan-1.0.7.tgz';
+      const uplinkConf = { url };
+      const proxy: IProxy = generateProxy(uplinkConf);
+
+      expect(proxy.isUplinkValid(tarBallUrl)).toBe(false);
+    });
+
+  });
+
 });