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)
Unleash · Jul 18, 2024 · 0d3dee0 · 0d3dee0
1 parent f15bcdc
commit 0d3dee0
Show file tree

Hide file tree

Showing 6 changed files with 148 additions and 36 deletions.
diff --git 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,50 +116,51 @@ 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':
                 return enumMessage(
                     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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<S = SchemaId> {
     schema: S;
@@ -38,3 +39,17 @@ export const validateSchema = <S = SchemaId>(
         };
     }
 };
+
+export const throwOnInvalidSchema = <S = SchemaId>(
+    schema: S,
+    data: object,
+): void => {
+    const validationErrors = validateSchema(schema, data);
+    if (validationErrors) {
+        const [firstError, ...remainingErrors] = validationErrors.errors;
+        throw fromOpenApiValidationErrors(data, [
+            firstError,
+            ...remainingErrors,
+        ]);
+    }
+};