From 0d3dee0e969af1f2002d00adff4a328363547be9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 18 Jul 2024 13:26:50 +0100 Subject: [PATCH] fix: validate patched data with schema (#7616) https://linear.app/unleash/issue/2-2453/validate-patched-data-against-schema This adds schema validation to patched data, fixing potential issues of patching data to an invalid state. This can be easily reproduced by patching a strategy constraints to be an object (invalid), instead of an array (valid): ```sh curl -X 'PATCH' \ 'http://localhost:4242/api/admin/projects/default/features/test/environments/development/strategies/8cb3fec6-c40a-45f7-8be0-138c5aaa5263' \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '[ { "path": "/constraints", "op": "replace", "from": "/constraints", "value": {} } ]' ``` Unleash will accept this because there's no validation that the patched data actually looks like a proper strategy, and we'll start seeing Unleash errors due to the invalid state. This PR adapts some of our existing logic in the way we handle validation errors to support any dynamic object. This way we can perform schema validation with any object and still get the benefits of our existing validation error handling. This PR also takes the liberty to expose the full instancePath as propertyName, instead of only the path's last section. We believe this has more upsides than downsides, especially now that we support the validation of any type of object. ![image](https://github.com/user-attachments/assets/f6503261-f6b5-4e1d-9ec3-66547d0d061f) --- src/lib/error/bad-data-error.ts | 38 +++---- src/lib/error/unleash-error.test.ts | 99 ++++++++++++++++--- .../feature-toggle-controller.ts | 5 + .../tests/feature-toggles.e2e.test.ts | 2 +- src/lib/openapi/validate.test.ts | 25 ++++- src/lib/openapi/validate.ts | 15 +++ 6 files changed, 148 insertions(+), 36 deletions(-) diff --git a/src/lib/error/bad-data-error.ts b/src/lib/error/bad-data-error.ts index 9b4aeb6ee6e6..8ca2c1aa4fb8 100644 --- a/src/lib/error/bad-data-error.ts +++ b/src/lib/error/bad-data-error.ts @@ -7,6 +7,7 @@ type ValidationErrorDescription = { message: string; path?: string; }; + class BadDataError extends UnleashError { statusCode = 400; @@ -67,13 +68,11 @@ const additionalPropertiesMessage = ( }; const genericErrorMessage = ( - requestBody: object, propertyName: string, + propertyValue: object, errorMessage: string = 'is invalid', ) => { - const input = getProp(requestBody, propertyName.split('/')); - - const youSent = JSON.stringify(input); + const youSent = JSON.stringify(propertyValue); const message = `The \`${propertyName}\` property ${errorMessage}. You sent ${youSent}.`; return { message, @@ -117,24 +116,24 @@ const enumMessage = ( }; export const fromOpenApiValidationError = - (request: { body: object; query: object }) => + (data: object) => (validationError: ErrorObject): ValidationErrorDescription => { const { instancePath, params, message } = validationError; - const [errorSource, substringOffset] = instancePath.startsWith('/body') - ? [request.body, '/body/'.length] - : [request.query, '/query/'.length]; - const propertyName = instancePath.substring(substringOffset); + const propertyValue = getProp( + data, + instancePath.split('/').filter(Boolean), + ); switch (validationError.keyword) { case 'required': return missingRequiredPropertyMessage( - propertyName, + instancePath, params.missingProperty, ); case 'additionalProperties': return additionalPropertiesMessage( - propertyName, + instancePath, params.additionalProperty, ); case 'enum': @@ -142,25 +141,26 @@ export const fromOpenApiValidationError = instancePath.substring(instancePath.lastIndexOf('/') + 1), message, params.allowedValues, - getProp( - errorSource, - instancePath.substring(substringOffset).split('/'), - ), + propertyValue, ); case 'oneOf': - return oneOfMessage(propertyName, validationError.message); + return oneOfMessage(instancePath, validationError.message); default: - return genericErrorMessage(errorSource, propertyName, message); + return genericErrorMessage( + instancePath, + propertyValue, + message, + ); } }; export const fromOpenApiValidationErrors = ( - request: { body: object; query: object }, + data: object, validationErrors: [ErrorObject, ...ErrorObject[]], ): BadDataError => { const [firstDetail, ...remainingDetails] = validationErrors.map( - fromOpenApiValidationError(request), + fromOpenApiValidationError(data), ); return new BadDataError( diff --git a/src/lib/error/unleash-error.test.ts b/src/lib/error/unleash-error.test.ts index 0fde99573191..4b6d328b4b89 100644 --- a/src/lib/error/unleash-error.test.ts +++ b/src/lib/error/unleash-error.test.ts @@ -64,7 +64,7 @@ describe('OpenAPI error conversion', () => { message: // it tells the user that the property is required expect.stringContaining('required'), - path: 'enabled', + path: '/body/enabled', }); // it tells the user the name of the missing property @@ -95,7 +95,7 @@ describe('OpenAPI error conversion', () => { message: // it provides the message expect.stringContaining(error.message), - path: 'parameters', + path: '/body/parameters', }); // it tells the user what they provided @@ -128,17 +128,13 @@ describe('OpenAPI error conversion', () => { message: // it provides the message expect.stringContaining(error.message), - path: instancePath.substring('/body/'.length), + path: instancePath, }); // it tells the user what happened expect(result.message).toContain('matches more than one option'); // it tells the user what part of the request body this pertains to - expect(result.message).toContain( - instancePath === '/body' - ? 'root object' - : `"${instancePath.substring('/body/'.length)}" property`, - ); + expect(result.message).toContain(`"${instancePath}" property`); }, ); @@ -164,7 +160,7 @@ describe('OpenAPI error conversion', () => { expect(result).toMatchObject({ message: expect.stringContaining(error.params.pattern), - path: 'description', + path: '/body/description', }); expect(result.message).toContain('description'); }); @@ -317,6 +313,79 @@ describe('OpenAPI error conversion', () => { }); }); + it('Handles any data, not only requests', () => { + const errors: [ErrorObject, ...ErrorObject[]] = [ + { + keyword: 'maximum', + instancePath: '/newprop', + schemaPath: + '#/components/schemas/addonCreateUpdateSchema/properties/newprop/maximum', + params: { + comparison: '<=', + limit: 5, + exclusive: false, + }, + message: 'should be <= 5', + }, + { + keyword: 'required', + instancePath: '/', + schemaPath: + '#/components/schemas/addonCreateUpdateSchema/required', + params: { + missingProperty: 'enabled', + }, + message: "should have required property 'enabled'", + }, + ]; + + const serializedUnleashError: ApiErrorSchema = + fromOpenApiValidationErrors({ newprop: 7 }, errors).toJSON(); + + expect(serializedUnleashError).toMatchObject({ + name: 'BadDataError', + message: expect.stringContaining('`details`'), + details: [ + { + message: expect.stringContaining('newprop'), + }, + { + message: expect.stringContaining('enabled'), + }, + ], + }); + }); + + it('Handles invalid data gracefully', () => { + const errors: [ErrorObject, ...ErrorObject[]] = [ + { + keyword: 'maximum', + instancePath: '/body/newprop', + schemaPath: + '#/components/schemas/addonCreateUpdateSchema/properties/body/newprop/maximum', + params: { + comparison: '<=', + limit: 5, + exclusive: false, + }, + message: 'should be <= 5', + }, + ]; + + const serializedUnleashError: ApiErrorSchema = + fromOpenApiValidationErrors({}, errors).toJSON(); + + expect(serializedUnleashError).toMatchObject({ + name: 'BadDataError', + message: expect.stringContaining('`details`'), + details: [ + { + message: expect.stringContaining('newprop'), + }, + ], + }); + }); + describe('Disallowed additional properties', () => { it('gives useful messages for base-level properties', () => { const openApiError = { @@ -337,10 +406,10 @@ describe('OpenAPI error conversion', () => { message: expect.stringContaining( openApiError.params.additionalProperty, ), - path: 'bogus', + path: '/body/bogus', }); - expect(error.message).toMatch(/\broot\b/i); + expect(error.message).toMatch(/\bbody\b/i); expect(error.message).toMatch(/\badditional properties\b/i); }); @@ -365,8 +434,8 @@ describe('OpenAPI error conversion', () => { const error = fromOpenApiValidationError(request2)(openApiError); expect(error).toMatchObject({ - message: expect.stringContaining('nestedObject/nested2'), - path: 'nestedObject/nested2/extraPropertyName', + message: expect.stringContaining('/body/nestedObject/nested2'), + path: '/body/nestedObject/nested2/extraPropertyName', }); expect(error.message).toContain( @@ -395,7 +464,7 @@ describe('OpenAPI error conversion', () => { expect(result).toMatchObject({ message: expect.stringMatching(/\bnestedObject\/a\/b\b/), - path: 'nestedObject/a/b', + path: '/body/nestedObject/a/b', }); expect(result.message).toContain('[]'); @@ -420,7 +489,7 @@ describe('OpenAPI error conversion', () => { expect(result).toMatchObject({ message: expect.stringContaining(illegalValue), - path: 'nestedObject/a/b', + path: '/body/nestedObject/a/b', }); expect(result.message).toMatch(/\bnestedObject\/a\/b\b/); diff --git a/src/lib/features/feature-toggle/feature-toggle-controller.ts b/src/lib/features/feature-toggle/feature-toggle-controller.ts index b08c0ebc995e..eacd1dc82540 100644 --- a/src/lib/features/feature-toggle/feature-toggle-controller.ts +++ b/src/lib/features/feature-toggle/feature-toggle-controller.ts @@ -32,6 +32,7 @@ import { type FeatureSchema, featuresSchema, type FeaturesSchema, + featureStrategySchema, type FeatureStrategySchema, getStandardResponses, type ParametersSchema, @@ -54,6 +55,7 @@ import type { } from '../../db/transaction'; import { BadDataError } from '../../error'; import { anonymise } from '../../util'; +import { throwOnInvalidSchema } from '../../openapi/validate'; interface FeatureStrategyParams { projectId: string; @@ -1060,6 +1062,9 @@ export default class ProjectFeaturesController extends Controller { const strategy = await this.featureService.getStrategy(strategyId); const { newDocument } = applyPatch(strategy, patch); + + throwOnInvalidSchema(featureStrategySchema.$id, newDocument); + const updatedStrategy = await this.startTransaction(async (tx) => this.transactionalFeatureToggleService(tx).updateStrategy( strategyId, diff --git a/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts index ab40646391ab..491d31eeea1a 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts @@ -1500,7 +1500,7 @@ test('Can patch a strategy based on id', async () => { strategy!.id }`, ) - .send([{ op: 'replace', path: '/parameters/rollout', value: 42 }]) + .send([{ op: 'replace', path: '/parameters/rollout', value: '42' }]) .expect(200); await app.request .get( diff --git a/src/lib/openapi/validate.test.ts b/src/lib/openapi/validate.test.ts index 196394ff3dd8..58c5440d9c95 100644 --- a/src/lib/openapi/validate.test.ts +++ b/src/lib/openapi/validate.test.ts @@ -1,7 +1,30 @@ -import { validateSchema } from './validate'; +import { constraintSchema } from './spec'; +import { throwOnInvalidSchema, validateSchema } from './validate'; test('validateSchema', () => { expect(() => validateSchema('unknownSchemaId' as any, {})).toThrow( 'no schema with key or ref "unknownSchemaId"', ); }); + +test('throwOnInvalidSchema', () => { + expect(() => + throwOnInvalidSchema(constraintSchema.$id, { + contextName: 'a', + operator: 'NUM_LTE', + value: '1', + }), + ).not.toThrow(); +}); + +test('throwOnInvalidSchema', () => { + expect(() => + throwOnInvalidSchema(constraintSchema.$id, { + contextName: 'a', + operator: 'invalid-operator', + value: '1', + }), + ).toThrow( + 'Request validation failed: your request body or params contain invalid data. Refer to the `details` list for more information.', + ); +}); diff --git a/src/lib/openapi/validate.ts b/src/lib/openapi/validate.ts index a11472da6702..57920dc771b8 100644 --- a/src/lib/openapi/validate.ts +++ b/src/lib/openapi/validate.ts @@ -1,6 +1,7 @@ import Ajv, { type ErrorObject } from 'ajv'; import { type SchemaId, schemas } from './index'; import { omitKeys } from '../util/omit-keys'; +import { fromOpenApiValidationErrors } from '../error/bad-data-error'; export interface ISchemaValidationErrors { schema: S; @@ -38,3 +39,17 @@ export const validateSchema = ( }; } }; + +export const throwOnInvalidSchema = ( + schema: S, + data: object, +): void => { + const validationErrors = validateSchema(schema, data); + if (validationErrors) { + const [firstError, ...remainingErrors] = validationErrors.errors; + throw fromOpenApiValidationErrors(data, [ + firstError, + ...remainingErrors, + ]); + } +};