Skip to content
This repository has been archived by the owner on Dec 6, 2024. It is now read-only.

Commit

Permalink
fix: fix input validation and escape html on popups (#1075)
Browse files Browse the repository at this point in the history
  • Loading branch information
maghirardelli authored Dec 6, 2022
1 parent 61200d0 commit bbe1e20
Show file tree
Hide file tree
Showing 8 changed files with 194 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ import React from 'react';
import { decorate, observable, runInAction } from 'mobx';
import { observer } from 'mobx-react';
import PropTypes from 'prop-types';
import toastr from 'toastr';

import { Segment, Header, Divider, Button, Icon } from 'semantic-ui-react';
import uuidv4 from 'uuid/v4';
import { displayWarning } from '@amzn/base-ui/dist/helpers/notification';

/**
* A reusable file input component.
Expand Down Expand Up @@ -61,7 +61,7 @@ class FileDropZone extends React.Component {
if (this.props.onSelectFiles) {
const fileList = event.currentTarget.files || [];
if (fileList.length > this.props.maximumUploadFilesLimit) {
toastr.warning(
displayWarning(
`There are currently ${fileList.length} files selected. Please select less than ${this.props.maximumUploadFilesLimit} files.`,
);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { Button, Grid, Header, Segment } from 'semantic-ui-react';

import { displayError, displaySuccess, displayWarning } from '@amzn/base-ui/dist/helpers/notification';

import toastr from 'toastr';
import StudyFileDropZone from './FileDropZone';
import FileUploadTable from './FileUploadTable';

Expand Down Expand Up @@ -69,7 +68,7 @@ const FileUpload = observer(
color="blue"
onClick={() => {
if (files.length > maximumUploadFilesLimit) {
toastr.warning(
displayWarning(
`There are currently ${files.length} files selected. Please select less than ${maximumUploadFilesLimit} files.`,
);
} else {
Expand Down
20 changes: 10 additions & 10 deletions addons/addon-base-ui/packages/base-ui/src/helpers/notification.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,21 @@ import _ from 'lodash';
import toastr from 'toastr';

function displayError(msg, error, timeOut = '20000') {
toastr.options.escapeHtml = true;
toastr.error(toMessage(msg, error), 'We have a problem!', { ...toasterErrorOptions, timeOut });
if (error) console.error(msg, error);
if (_.isError(msg)) console.error(msg);
}

function displayWarning(msg, error, timeOut = '20000') {
toastr.options.escapeHtml = true;
toastr.warning(toMessage(msg, error), 'Warning!', { ...toasterWarningOptions, timeOut });
if (error) console.error(msg, error);
if (_.isError(msg)) console.error(msg);
}

function displaySuccess(msg, title = 'Submitted!') {
toastr.options.escapeHtml = true;
toastr.success(toMessage(msg), title, toasterSuccessOptions);
}

Expand All @@ -47,37 +50,34 @@ function displayFormErrors(form) {

function toMessage(msg, error) {
if (_.isError(msg)) {
return `${msg.message || msg.friendly} <br/>&nbsp;`;
return `${msg.message || msg.friendly} \n;`;
}

if (_.isError(error)) {
return `${msg} - ${error.message} <br/>&nbsp;`;
return `${msg} - ${error.message} \n;`;
}

if (_.isArray(msg)) {
const messages = msg;
const size = _.size(messages);

if (size === 0) {
return 'Unknown error <br/>&nbsp;';
return 'Unknown error \n;';
}
if (size === 1) {
return `${messages[0]}<br/>&nbsp;`;
return `${messages[0]}\n;`;
}
const result = [];
result.push('<br/>');
result.push('<ul>');
_.forEach(messages, message => {
result.push(`<li style="margin-left: -20px;">${message}</li>`);
result.push(`${message}\n`);
});
result.push('</ul><br/>&nbsp;');

return result.join('');
}

if (_.isEmpty(msg)) return 'Unknown error <br/>&nbsp;';
if (_.isEmpty(msg)) return 'Unknown error \n;';

return `${msg} <br/>&nbsp;`;
return `${msg} \n;`;
}

// For details of options, see https://github.com/CodeSeven/toastr
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/
const ServicesContainer = require('@amzn/base-services-container/lib/services-container');
const JsonSchemaValidationService = require('@amzn/base-services/lib/json-schema-validation-service');

jest.mock('@amzn/base-services/lib/db-service');
const DbServiceMock = require('@amzn/base-services/lib/db-service');

jest.mock('@amzn/base-services/lib/audit/audit-writer-service');
const AuditServiceMock = require('@amzn/base-services/lib/audit/audit-writer-service');

jest.mock('@amzn/base-services/lib/authorization/authorization-service');
const AuthServiceMock = require('@amzn/base-services/lib/authorization/authorization-service');

jest.mock('@amzn/base-services/lib/settings/env-settings-service');
const SettingsServiceMock = require('@amzn/base-services/lib/settings/env-settings-service');

const KeyPairService = require('../key-pair-service');

describe('keyPairService', () => {
let service = null;
let dbService = null;
beforeEach(async () => {
// initialize services container and register dependencies
const container = new ServicesContainer();
container.register('jsonSchemaValidationService', new JsonSchemaValidationService());
container.register('dbService', new DbServiceMock());
container.register('authorizationService', new AuthServiceMock());
container.register('auditWriterService', new AuditServiceMock());
container.register('settings', new SettingsServiceMock());
container.register('keyPairService', new KeyPairService());
await container.initServices();

// Get instance of the service we are testing
service = await container.find('keyPairService');
dbService = await container.find('dbService');

// skip authorization
service.assertAuthorized = jest.fn();
service.isAuthorized = jest.fn();
});

describe('create', () => {
const requestContext = {
principalIdentifier: {
uid: 'UID',
},
};

describe('if the name is invalid', () => {
const keyPair = {
name: '<script>console.log("**hacker voice** I\'m in")</script>',
};

it('should fail', async () => {
// OPERATE and CHECK
await expect(service.create(requestContext, keyPair)).rejects.toThrow(
expect.objectContaining({
boom: true,
code: 'badRequest',
safe: true,
message: 'Input has validation errors',
}),
);
});
});

describe('if the name is valid', () => {
const keyPair = {
name: 'valid-key-name',
};

it('should pass', async () => {
// BUILD
service.audit = jest.fn();

// OPERATE
await service.create(requestContext, keyPair);

// CHECK
// expect(dbService.table.key).toHaveBeenCalledWith({ id: keyPair.id });
expect(dbService.table.update).toHaveBeenCalled();
expect(service.audit).toHaveBeenCalledWith(
requestContext,
expect.objectContaining({ action: 'create-key-pair' }),
);
});
});
});

describe('update', () => {
const requestContext = {
principalIdentifier: {
uid: 'UID',
},
};

describe('if the name is invalid', () => {
const keyPair = {
id: 'id',
name: '<script>console.log("**hacker voice** I\'m in")</script>',
};

it('should fail', async () => {
// OPERATE and CHECK
await expect(service.update(requestContext, keyPair)).rejects.toThrow(
expect.objectContaining({
boom: true,
code: 'badRequest',
safe: true,
message: 'Input has validation errors',
}),
);
});
});

describe('if the name is valid', () => {
const keyPair = {
id: 'valid-id',
name: 'valid-key-name',
rev: 0,
};

const existingKeyPair = {
principalIdentifier: requestContext.principalIdentifier,
};

it('should pass', async () => {
// BUILD
service.audit = jest.fn();
service.mustFind = jest.fn().mockReturnValue(existingKeyPair);

// OPERATE
await service.update(requestContext, keyPair);

// CHECK
expect(dbService.table.key).toHaveBeenCalledWith({ id: keyPair.id });
expect(dbService.table.update).toHaveBeenCalled();
expect(service.audit).toHaveBeenCalledWith(
requestContext,
expect.objectContaining({ action: 'update-key-pair' }),
);
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -190,20 +190,25 @@ class KeyPairService extends Service {
}

async update(requestContext, keyPair) {
const existingKeyPair = await this.mustFind(requestContext, { id: keyPair.id });
// Validate input
const [validationService] = await this.service(['jsonSchemaValidationService']);
await validationService.ensureValid(keyPair, updateKeyPairSchema);

const existingKeyPair = await this.mustFind(requestContext, {
id: keyPair.id,
});

// Make sure the user has permissions to update the key-pair
// By default, allow only active admin users or self
await this.assertAuthorized(
requestContext,
{ action: 'update', conditions: [allowIfActive, allowIfCurrentUserOrAdmin] },
{
action: 'update',
conditions: [allowIfActive, allowIfCurrentUserOrAdmin],
},
{ ...existingKeyPair, ...keyPair, ...existingKeyPair.principalIdentifier },
);

// Validate input
const [validationService] = await this.service(['jsonSchemaValidationService']);
await validationService.ensureValid(keyPair, updateKeyPairSchema);

const by = _.get(requestContext, 'principalIdentifier.uid');
const { id, rev } = keyPair;

Expand All @@ -225,7 +230,10 @@ class KeyPairService extends Service {
// 1 - The keyPair does not exist
// 2 - The "rev" does not match
// We will display the appropriate error message accordingly
const existing = await this.find(requestContext, { id, fields: ['id', 'updatedBy'] });
const existing = await this.find(requestContext, {
id,
fields: ['id', 'updatedBy'],
});
if (existing) {
throw this.boom.badRequest(
`key-pair information changed just before your request is processed, please try again`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const schema = {
type: 'string',
maxLength: 100,
minLength: 2,
pattern: '^[a-zA-Z0-9_\\-]*',
pattern: '^[A-Za-z0-9-_ ]+$',
},
// Description for this key-pair
desc: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const schema = {
type: 'string',
maxLength: 100,
minLength: 2,
pattern: '^[a-zA-Z0-9_\\-]*',
pattern: '^[A-Za-z0-9-_ ]+$',
},
// Description for this key-pair
desc: {
Expand Down
6 changes: 6 additions & 0 deletions openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2469,6 +2469,9 @@ components:
properties:
name:
type: string
maxLength: 100
minLength: 2
pattern: '^[A-Za-z0-9-_ ]+$'
desc:
type: string
example: {"name": "foo2-key", "desc": "Example key being created"}
Expand All @@ -2479,6 +2482,9 @@ components:
type: number
name:
type: string
maxLength: 100
minLength: 2
pattern: '^[A-Za-z0-9-_ ]+$'
desc:
type: string
example: { "rev": 0, "name": "foo2-key", "desc": "Example key being created" }
Expand Down

0 comments on commit bbe1e20

Please sign in to comment.