Skip to content

Commit

Permalink
fix: unpublish and add or remove star colision (#1434)
Browse files Browse the repository at this point in the history 
* fix: unpublish and add or remove star colision

The issue was the npm star use a similar payload, but we did not check properly the shape of the payload, this fix and allow unpublish correctly.

Improve unit testing for publishing and unpublishing
Add new code documentation for future changes.

* chore: update secrets baseline

* chore: add missing type

this will requires update types in the future
  • Loading branch information
@juanpicado
juanpicado committed Aug 10, 2019
1 parent 8d51856 commit c264f94
Show file tree
Hide file tree
Showing 22 changed files with 632 additions and 250 deletions.
27 changes: 11 additions & 16 deletions .secrets-baseline
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"files": null,
"lines": null
},
"generated_at": "2019-07-27T21:44:36Z",
"generated_at": "2019-08-10T11:09:03Z",
"plugins_used": [
{
"name": "AWSKeyDetector"
Expand Down Expand Up @@ -40,31 +40,26 @@
"src/lib/auth-utils.ts": [
{
"hashed_secret": "6947818ac409551f11fbaa78f0ea6391960aa5b8",
"line_number": 10,
"line_number": 12,
"type": "Secret Keyword"
},
{
"hashed_secret": "ecb252044b5ea0f679ee78ec1a12904739e2904d",
"line_number": 174,
"line_number": 187,
"type": "Secret Keyword"
},
{
"hashed_secret": "f35dd4c51c0a89bd055b5ad30c162c778981306d",
"line_number": 179,
"line_number": 192,
"type": "Secret Keyword"
},
{
"hashed_secret": "45c43fe97e3a06ab078b0eeff6fbe622cc417a25",
"line_number": 197,
"line_number": 210,
"type": "Secret Keyword"
}
],
"src/lib/auth.ts": [
{
"hashed_secret": "3812d6abc055424d0556b35f48774c7b0044eac2",
"line_number": 32,
"type": "Secret Keyword"
},
{
"hashed_secret": "6981afa9890d125c05133d13053201f32292ec9f",
"line_number": 38,
Expand All @@ -91,12 +86,12 @@
"src/lib/constants.ts": [
{
"hashed_secret": "f34fbc9a9769ba9eff5aff3d008a6b49f85c08b1",
"line_number": 14,
"line_number": 15,
"type": "Secret Keyword"
},
{
"hashed_secret": "b9343f1143ccb83555b450eb54dde96a05522ccc",
"line_number": 115,
"line_number": 116,
"type": "Secret Keyword"
}
],
Expand Down Expand Up @@ -270,12 +265,12 @@
"test/unit/modules/api/api.spec.ts": [
{
"hashed_secret": "97752a468368b0d6b192140d6a140c38fd0cbd8b",
"line_number": 293,
"line_number": 304,
"type": "Secret Keyword"
},
{
"hashed_secret": "364bdf2ed77a8544d3b711a03b69eeadcc63c9d7",
"line_number": 802,
"line_number": 828,
"type": "Secret Keyword"
}
],
Expand Down Expand Up @@ -304,12 +299,12 @@
"test/unit/modules/auth/jwt.spec.ts": [
{
"hashed_secret": "364bdf2ed77a8544d3b711a03b69eeadcc63c9d7",
"line_number": 121,
"line_number": 118,
"type": "Secret Keyword"
},
{
"hashed_secret": "eaacdf2d9ed66df2601c8b51ab4084db14336d11",
"line_number": 132,
"line_number": 129,
"type": "Secret Keyword"
}
],
Expand Down
126 changes: 110 additions & 16 deletions src/api/endpoint/api/publish.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import Path from 'path';
import mime from 'mime';

import { API_MESSAGE, HEADERS, DIST_TAGS, API_ERROR, HTTP_STATUS } from '../../../lib/constants';
import { validateMetadata, isObject, ErrorCode } from '../../../lib/utils';
import {validateMetadata, isObject, ErrorCode, hasDiffOneKey} from '../../../lib/utils';
import { media, expectJson, allow } from '../../middleware';
import { notify } from '../../../lib/notify';
import star from './star';
Expand All @@ -12,14 +12,80 @@ import { Router } from 'express';
import { Config, Callback, MergeTags, Version, Package } from '@verdaccio/types';
import { IAuth, $ResponseExtend, $RequestExtend, $NextFunctionVer, IStorageHandler } from '../../../../types';
import { logger } from '../../../lib/logger';
import {isPublishablePackage} from "../../../lib/storage-utils";

export default function publish(router: Router, auth: IAuth, storage: IStorageHandler, config: Config) {
const can = allow(auth);

// publishing a package
router.put('/:package/:_rev?/:revision?', can('publish'), media(mime.getType('json')), expectJson, publishPackage(storage, config));

// un-publishing an entire package
/**
* Publish a package / update package / un/start a package
*
* There are multiples scenarios here to considere:
*
* 1. Publish scenario
*
* Publish a package consist of at least 1 step (PUT) with a metadata payload.
* When a package is published, an _attachment property is present that contains the data
* of the tarball.
*
* Example flow of publish.
*
* npm http fetch PUT 201 http://localhost:4873/@scope%2ftest1 9627ms
npm info lifecycle @scope/test1@1.0.1~publish: @scope/test1@1.0.1
npm info lifecycle @scope/test1@1.0.1~postpublish: @scope/test1@1.0.1
+ @scope/test1@1.0.1
npm verb exit [ 0, true ]
*
*
* 2. Unpublish scenario
*
* Unpublish consist in 3 steps.
* 1. Try to fetch metadata -> if it fails, return 404
* 2. Compute metadata locally (client side) and send a mutate payload excluding the version to be unpublished
* eg: if metadata reflects 1.0.1, 1.0.2 and 1.0.3, the computed metadata won't include 1.0.3.
* 3. Once the second step has been succesfully finished, delete the tarball.
*
* All these steps are consecutive and required, there is no transacions here, if step 3 fails, metadata might
* get corrupted.
*
* Note the unpublish call will suffix in the url a /-rev/14-5d500cfce92f90fd revision number, this not
* used internally.
*
*
* Example flow of unpublish.
*
* npm http fetch GET 200 http://localhost:4873/@scope%2ftest1?write=true 1680ms
npm http fetch PUT 201 http://localhost:4873/@scope%2ftest1/-rev/14-5d500cfce92f90fd 956606ms attempt #2
npm http fetch GET 200 http://localhost:4873/@scope%2ftest1?write=true 1601ms
npm http fetch DELETE 201 http://localhost:4873/@scope%2ftest1/-/test1-1.0.3.tgz/-rev/16-e11c8db282b2d992 19ms
*
* 3. Star a package
*
* Permissions: start a package depends of the publish and unpublish permissions, there is no specific flag for star or un start.
* The URL for star is similar to the unpublish (change package format)
*
* npm has no enpoint for star a package, rather mutate the metadata and acts as, the difference is the
* users property which is part of the payload and the body only includes
*
* {
"_id": pkgName,
"_rev": "3-b0cdaefc9bdb77c8",
"users": {
[username]: boolean value (true, false)
}
}
*
*/
router.put('/:package/:_rev?/:revision?', can('publish'), media(mime.getType('json')), expectJson, publishPackage(storage, config, auth));

/**
* Un-publishing an entire package.
*
* This scenario happens when the first call detect there is only one version remaining
* in the metadata, then the client decides to DELETE the resource
* npm http fetch GET 304 http://localhost:4873/@scope%2ftest1?write=true 1076ms (from cache)
npm http fetch DELETE 201 http://localhost:4873/@scope%2ftest1/-rev/18-d8ebe3020bd4ac9c 22ms
*/
router.delete('/:package/-rev/*', can('unpublish'), unPublishPackage(storage));

// removing a tarball
Expand All @@ -35,10 +101,13 @@ export default function publish(router: Router, auth: IAuth, storage: IStorageHa
/**
* Publish a package
*/
export function publishPackage(storage: IStorageHandler, config: Config) {
export function publishPackage(storage: IStorageHandler, config: Config, auth: IAuth) {
const starApi = star(storage);
return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
const packageName = req.params.package;

logger.debug({packageName} , `publishing or updating a new version for @{packageName}`);

/**
* Write tarball of stream data from package clients.
*/
Expand Down Expand Up @@ -72,9 +141,10 @@ export function publishPackage(storage: IStorageHandler, config: Config) {

const afterChange = function(error, okMessage, metadata) {
const metadataCopy: Package = { ...metadata };

const { _attachments, versions } = metadataCopy;

// old npm behavior, if there is no attachments
// if the is no attachments, it is change, it is a new package.
if (_.isNil(_attachments)) {
if (error) {
return next(error);
Expand All @@ -89,18 +159,22 @@ export function publishPackage(storage: IStorageHandler, config: Config) {
// npm-registry-client 0.3+ embeds tarball into the json upload
// https://github.com/isaacs/npm-registry-client/commit/e9fbeb8b67f249394f735c74ef11fe4720d46ca0
// issue https://github.com/rlidwka/sinopia/issues/31, dealing with it here:
if (isObject(_attachments) === false || Object.keys(_attachments).length !== 1 || isObject(versions) === false || Object.keys(versions).length !== 1) {
const isInvalidBodyFormat = isObject(_attachments) === false || hasDiffOneKey(_attachments) ||
isObject(versions) === false || hasDiffOneKey(versions);

if (isInvalidBodyFormat) {
// npm is doing something strange again
// if this happens in normal circumstances, report it as a bug
return next(ErrorCode.getBadRequest('unsupported registry call'));
logger.info({ packageName }, `wrong package format on publish a package @{packageName}`);
return next(ErrorCode.getBadRequest(API_ERROR.UNSUPORTED_REGISTRY_CALL));
}

if (error && error.status !== HTTP_STATUS.CONFLICT) {
return next(error);
}

// at this point document is either created or existed before
const firstAttachmentKey = Object.keys(_attachments)[0];
const [firstAttachmentKey] = Object.keys(_attachments);

createTarball(Path.basename(firstAttachmentKey), _attachments[firstAttachmentKey], function(error) {
if (error) {
Expand Down Expand Up @@ -134,23 +208,34 @@ export function publishPackage(storage: IStorageHandler, config: Config) {
});
};

if (Object.prototype.hasOwnProperty.call(req.body, '_rev') && isObject(req.body.users)) {
if (isPublishablePackage(req.body) === false && isObject(req.body.users)) {
return starApi(req, res, next);
}

try {
const metadata = validateMetadata(req.body, packageName);
if (req.params._rev) {
storage.changePackage(packageName, metadata, req.params.revision, function(error) {
afterChange(error, API_MESSAGE.PKG_CHANGED, metadata);
logger.debug({packageName} , `updating a new version for @{packageName}`);
// we check unpublish permissions, an update is basically remove versions
const remote = req.remote_user;
auth.allow_unpublish({packageName}, remote, (error, allowed) => {
if (error) {
logger.debug({packageName} , `not allowed to unpublish a version for @{packageName}`);
return next(error);
}

storage.changePackage(packageName, metadata, req.params.revision, function(error) {
afterChange(error, API_MESSAGE.PKG_CHANGED, metadata);
});
});
} else {
logger.debug({packageName} , `adding a new version for @{packageName}`);
storage.addPackage(packageName, metadata, function(error) {
afterChange(error, API_MESSAGE.PKG_CREATED, metadata);
});
}
} catch (error) {
logger.error({error}, 'error on publish, bad package data @{error}');
logger.error({packageName}, 'error on publish, bad package data for @{packageName}');
return next(ErrorCode.getBadData(API_ERROR.BAD_PACKAGE_DATA));
}
};
Expand All @@ -161,7 +246,10 @@ export function publishPackage(storage: IStorageHandler, config: Config) {
*/
export function unPublishPackage(storage: IStorageHandler) {
return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
storage.removePackage(req.params.package, function(err) {
const packageName = req.params.package;

logger.debug({packageName} , `unpublishing @{packageName}`);
storage.removePackage(packageName, function(err) {
if (err) {
return next(err);
}
Expand All @@ -176,11 +264,17 @@ export function unPublishPackage(storage: IStorageHandler) {
*/
export function removeTarball(storage: IStorageHandler) {
return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
storage.removeTarball(req.params.package, req.params.filename, req.params.revision, function(err) {
const packageName = req.params.package;
const {filename, revision} = req.params;

logger.debug({packageName, filename, revision} , `removing a tarball for @{packageName}-@{tarballName}-@{revision}`);
storage.removeTarball(packageName, filename, revision, function(err) {
if (err) {
return next(err);
}
res.status(HTTP_STATUS.CREATED);

logger.debug({packageName, filename, revision} , `success remove tarball for @{packageName}-@{tarballName}-@{revision}`);
return next({ ok: API_MESSAGE.TARBALL_REMOVED });
});
};
Expand Down
2 changes: 2 additions & 0 deletions src/api/endpoint/api/star.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { USERS, HTTP_STATUS } from '../../../lib/constants';
import {Response} from 'express';
import {$RequestExtend, $NextFunctionVer, IStorageHandler} from '../../../../types';
import _ from 'lodash';
import { logger } from '../../../lib/logger';

export default function(storage: IStorageHandler) {
const validateInputs = (newUsers, localUsers, username, isStar) => {
Expand All @@ -21,6 +22,7 @@ export default function(storage: IStorageHandler) {

return (req: $RequestExtend, res: Response, next: $NextFunctionVer): void => {
const name = req.params.package;
logger.debug({name}, 'starring a package for @{name}');
const afterChangePackage = function(err?: Error) {
if (err) {
return next(err);
Expand Down
12 changes: 4 additions & 8 deletions src/api/middleware.ts
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
/**
* @prettier
* @flow
*/

import _ from 'lodash';

import { validateName as utilValidateName, validatePackage as utilValidatePackage, getVersionFromTarball, isObject, ErrorCode } from '../lib/utils';
import { API_ERROR, HEADER_TYPE, HEADERS, HTTP_STATUS, TOKEN_BASIC, TOKEN_BEARER } from '../lib/constants';
import { stringToMD5 } from '../lib/crypto-utils';
import { $ResponseExtend, $RequestExtend, $NextFunctionVer, IAuth } from '../../types';
import { Config, Package } from '@verdaccio/types';
import { Config, Package, RemoteUser } from '@verdaccio/types';
import { logger } from '../lib/logger';
import { VerdaccioError } from '@verdaccio/commons-api';

Expand Down Expand Up @@ -108,9 +103,10 @@ export function allow(auth: IAuth): Function {
req.pause();
const packageName = req.params.scope ? `@${req.params.scope}/${req.params.package}` : req.params.package;
const packageVersion = req.params.filename ? getVersionFromTarball(req.params.filename) : undefined;
const remote: RemoteUser = req.remote_user;
logger.trace({ action, user: remote.name }, `[middleware/allow][@{action}] allow for @{user}`);

// $FlowFixMe
auth['allow_' + action]({ packageName, packageVersion }, req.remote_user, function(error, allowed): void {
auth['allow_' + action]({ packageName, packageVersion }, remote, function(error, allowed): void {
req.resume();
if (error) {
next(error);
Expand Down
Loading

0 comments on commit c264f94

Please sign in to comment.