From bbe1e2067c9297dc5f288307bd48ec8170fe8304 Mon Sep 17 00:00:00 2001 From: Marianna Ghirardelli <43092418+maghirardelli@users.noreply.github.com> Date: Tue, 6 Dec 2022 18:56:34 -0500 Subject: [PATCH] fix: fix input validation and escape html on popups (#1075) --- .../src/parts/files/FileDropZone.js | 4 +- .../src/parts/files/FileUpload.js | 3 +- .../base-ui/src/helpers/notification.js | 20 +-- .../__tests__/key-pair-service.test.js | 158 ++++++++++++++++++ .../lib/key-pair/key-pair-service.js | 22 ++- .../lib/key-pair/schema/create-key-pair.js | 2 +- .../lib/key-pair/schema/update-key-pair.js | 2 +- openapi.yaml | 6 + 8 files changed, 194 insertions(+), 23 deletions(-) create mode 100644 addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/__tests__/key-pair-service.test.js diff --git a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileDropZone.js b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileDropZone.js index 008bfcbd0a..9b0ccf6d8f 100644 --- a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileDropZone.js +++ b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileDropZone.js @@ -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. @@ -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 { diff --git a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileUpload.js b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileUpload.js index d7cf866538..bd518da3dd 100644 --- a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileUpload.js +++ b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/files/FileUpload.js @@ -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'; @@ -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 { diff --git a/addons/addon-base-ui/packages/base-ui/src/helpers/notification.js b/addons/addon-base-ui/packages/base-ui/src/helpers/notification.js index bbe1aa16a4..88c212a81f 100644 --- a/addons/addon-base-ui/packages/base-ui/src/helpers/notification.js +++ b/addons/addon-base-ui/packages/base-ui/src/helpers/notification.js @@ -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); } @@ -47,11 +50,11 @@ function displayFormErrors(form) { function toMessage(msg, error) { if (_.isError(msg)) { - return `${msg.message || msg.friendly}
 `; + return `${msg.message || msg.friendly} \n;`; } if (_.isError(error)) { - return `${msg} - ${error.message}
 `; + return `${msg} - ${error.message} \n;`; } if (_.isArray(msg)) { @@ -59,25 +62,22 @@ function toMessage(msg, error) { const size = _.size(messages); if (size === 0) { - return 'Unknown error
 '; + return 'Unknown error \n;'; } if (size === 1) { - return `${messages[0]}
 `; + return `${messages[0]}\n;`; } const result = []; - result.push('
'); - result.push('
 '); return result.join(''); } - if (_.isEmpty(msg)) return 'Unknown error
 '; + if (_.isEmpty(msg)) return 'Unknown error \n;'; - return `${msg}
 `; + return `${msg} \n;`; } // For details of options, see https://github.com/CodeSeven/toastr diff --git a/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/__tests__/key-pair-service.test.js b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/__tests__/key-pair-service.test.js new file mode 100644 index 0000000000..c71594431c --- /dev/null +++ b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/__tests__/key-pair-service.test.js @@ -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: '', + }; + + 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: '', + }; + + 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' }), + ); + }); + }); + }); +}); diff --git a/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/key-pair-service.js b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/key-pair-service.js index f5967b6d70..4e357f0aa6 100644 --- a/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/key-pair-service.js +++ b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/key-pair-service.js @@ -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; @@ -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`, diff --git a/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/create-key-pair.js b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/create-key-pair.js index afd893711d..1789827d2e 100644 --- a/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/create-key-pair.js +++ b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/create-key-pair.js @@ -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: { diff --git a/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/update-key-pair.js b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/update-key-pair.js index c6d4bb5546..4034e80c9a 100644 --- a/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/update-key-pair.js +++ b/addons/addon-key-pair-mgmt-api/packages/key-pair-mgmt-services/lib/key-pair/schema/update-key-pair.js @@ -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: { diff --git a/openapi.yaml b/openapi.yaml index 8bc04458bc..0c8e419a66 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -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"} @@ -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" }