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, + ]); + } +};