Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-25 00:07:47 +01:00
Code Issues Projects Releases Wiki Activity

fix: validate patched data with schema (#7616)

Browse Source 
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)
This commit is contained in:
Nuno Góis 2024-07-18 13:26:50 +01:00 committed by GitHub
parent f15bcdc2a6
commit 0d3dee0e96
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 148 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
src/lib/error/bad-data-error.ts
View File

@ -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(

99
src/lib/error/unleash-error.test.ts
View File

@ -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/);

5
src/lib/features/feature-toggle/feature-toggle-controller.ts
View File

@ -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,

2
src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts
View File

@ -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(

25
src/lib/openapi/validate.test.ts
View File

@ -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.',
);
});

15
src/lib/openapi/validate.ts
View File

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