From e7e62b7822287bb9a4f97f1ddcb8373e0d9650ad Mon Sep 17 00:00:00 2001 From: Marco Gonzalez Date: Tue, 27 Aug 2024 15:16:24 -0400 Subject: [PATCH 1/2] feat: add ValidationException WHAT? Add `ValidationException` and throw it when `signRequest()` or `verifyRequest()` fail validation. WHY? When there's a validation error a `runtypes.ValidationError` is thrown. This is an internal implementation detail and as such, there is no supported way of detecting validation errors. Similar to [#692], so that developers can: ```ts import { ValidationException, verifyRequest } from '@contentful/node-apps-toolkit' try { verifyRequest(/* ...*/) } catch (e) { if (e instanceof ValidationException) { if (e.constraintName === 'SecretLength') { // this is unexpected, return 500 status code and alarm } else { // this is expected, should return 400 or 403 status code } } } ``` [#692]: https://github.com/contentful/node-apps-toolkit/pull/692 --- package.json | 4 +- src/index.ts | 8 +- src/requests/exceptions.ts | 12 +++ src/requests/index.ts | 2 +- src/requests/sign-request.spec.ts | 11 +-- src/requests/typings/validators.ts | 59 ------------- src/requests/typings/validators/index.ts | 78 ++++++++++++++++ .../validators/proxy-validation-error.spec.ts | 88 +++++++++++++++++++ .../validators/proxy-validation-error.ts | 37 ++++++++ src/requests/verify-request.spec.ts | 8 +- 10 files changed, 235 insertions(+), 72 deletions(-) delete mode 100644 src/requests/typings/validators.ts create mode 100644 src/requests/typings/validators/index.ts create mode 100644 src/requests/typings/validators/proxy-validation-error.spec.ts create mode 100644 src/requests/typings/validators/proxy-validation-error.ts diff --git a/package.json b/package.json index 9d7ff98e..6cae6b22 100644 --- a/package.json +++ b/package.json @@ -11,8 +11,8 @@ "lint": "eslint --ext .ts ./src", "lint:fix": "npm run lint -- --fix", "pretest": "echo ' 🔑 Creating valid keypair for testing' && sh test/make-private-keys.sh &> /dev/null", - "test:unit": "mocha -r dotenv/config -r ts-node/register ./src/**/*.spec.ts", - "test:integration": "mocha -r dotenv/config -r ts-node/register --timeout 10000 ./test/**/*.test.ts", + "test:unit": "mocha -r dotenv/config -r ts-node/register './src/**/*.spec.ts'", + "test:integration": "mocha -r dotenv/config -r ts-node/register --timeout 10000 './test/**/*.test.ts'", "test": "npm run test:unit && npm run test:integration", "build": "rm -rf lib && tsc", "build:docs": "typedoc --options .typedocrc.json src", diff --git a/src/index.ts b/src/index.ts index 8a7e3db5..7f3b89a3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,11 @@ export { getManagementToken } from './keys' -export { signRequest, verifyRequest, ContentfulHeader, ExpiredRequestException } from './requests' +export { + signRequest, + verifyRequest, + ContentfulHeader, + ExpiredRequestException, + ValidationException, +} from './requests' export type { AppActionCallContext, diff --git a/src/requests/exceptions.ts b/src/requests/exceptions.ts index 9d6a2f26..56fc9af4 100644 --- a/src/requests/exceptions.ts +++ b/src/requests/exceptions.ts @@ -8,3 +8,15 @@ export class ExpiredRequestException extends Error { this.message = `[${this.constructor.name}]: Requests are expected to be verified within ${this.ttl}s from their signature.` } } + +export class ValidationException extends Error { + constructor( + message: string, + /* eslint-disable no-unused-vars */ + readonly constraintName?: string, + readonly key?: string, + /* eslint-enable no-unused-vars */ + ) { + super(message) + } +} diff --git a/src/requests/index.ts b/src/requests/index.ts index 556b4e35..673c5114 100644 --- a/src/requests/index.ts +++ b/src/requests/index.ts @@ -1,4 +1,4 @@ -export { ExpiredRequestException } from './exceptions' +export { ExpiredRequestException, ValidationException } from './exceptions' export { signRequest } from './sign-request' export { verifyRequest } from './verify-request' export { ContentfulHeader, ContentfulContextHeader } from './typings' diff --git a/src/requests/sign-request.spec.ts b/src/requests/sign-request.spec.ts index 99e9cf68..2ae05139 100644 --- a/src/requests/sign-request.spec.ts +++ b/src/requests/sign-request.spec.ts @@ -1,4 +1,5 @@ import * as assert from 'assert' +import { ValidationException } from './exceptions' import { CanonicalRequest, Secret } from './typings' import { signRequest } from './sign-request' @@ -21,7 +22,7 @@ const assertThrowsForFieldInValues = (field: keyof CanonicalRequest, values: any }, VALID_TIMESTAMP, ) - }, `Did not throw for ${field.toString()}:${value}`) + }, ValidationException) } } @@ -59,9 +60,9 @@ describe('create-signature', () => { for (const secret of invalidSecrets) { assert.throws(() => { - // @ts-ignore + // @ts-expect-error signRequest(secret, VALID_REQUEST, VALID_TIMESTAMP) - }, `Did not throw for ${secret}`) + }, ValidationException) } }) it('does not throw if valid', () => { @@ -77,9 +78,9 @@ describe('create-signature', () => { for (const timestamp of invalidTimestamps) { assert.throws(() => { - // @ts-ignore + // @ts-expect-error signRequest(VALID_SECRET, VALID_REQUEST, timestamp) - }, `Did not throw for ${timestamp}`) + }, ValidationException) } }) it('does not throw if missing', () => { diff --git a/src/requests/typings/validators.ts b/src/requests/typings/validators.ts deleted file mode 100644 index 2a40b439..00000000 --- a/src/requests/typings/validators.ts +++ /dev/null @@ -1,59 +0,0 @@ -import * as runtypes from 'runtypes' - -const MethodValidator = runtypes.Union( - runtypes.Literal('GET'), - runtypes.Literal('PATCH'), - runtypes.Literal('HEAD'), - runtypes.Literal('POST'), - runtypes.Literal('DELETE'), - runtypes.Literal('OPTIONS'), - runtypes.Literal('PUT'), -) - -const PathValidator = runtypes.String.withConstraint((s) => s.startsWith('/'), { - name: 'CanonicalURI', -}) - -const SignatureValidator = runtypes.String.withConstraint((s) => s.length === 64, { - name: 'SignatureLength', -}) - -export const CanonicalRequestValidator = runtypes - .Record({ - method: MethodValidator, - path: PathValidator, - }) - .And( - runtypes.Partial({ - headers: runtypes.Dictionary(runtypes.String, 'string'), - body: runtypes.String, - }), - ) -export type CanonicalRequest = runtypes.Static - -export const SecretValidator = runtypes.String.withConstraint((s) => s.length === 64, { - name: 'SecretLength', -}) -export type Secret = runtypes.Static - -// Only dates after 01-01-2020 -export const TimestampValidator = runtypes.Number.withConstraint((n) => n > 1577836800000, { - name: 'TimestampAge', -}) -export type Timestamp = runtypes.Static - -const SignedHeadersValidator = runtypes - .Array(runtypes.String) - .withConstraint((l) => l.length >= 2, { name: 'MissingTimestampOrSignedHeaders' }) - -export const RequestMetadataValidator = runtypes.Record({ - signature: SignatureValidator, - timestamp: TimestampValidator, - signedHeaders: SignedHeadersValidator, -}) -export type RequestMetadata = runtypes.Static - -export const TimeToLiveValidator = runtypes.Number.withConstraint((n) => n >= 0, { - name: 'PositiveNumber', -}) -export type TimeToLive = runtypes.Static diff --git a/src/requests/typings/validators/index.ts b/src/requests/typings/validators/index.ts new file mode 100644 index 00000000..41f6e44b --- /dev/null +++ b/src/requests/typings/validators/index.ts @@ -0,0 +1,78 @@ +import * as runtypes from 'runtypes' +import { proxyValidationError } from './proxy-validation-error' + +const MethodValidator = proxyValidationError( + runtypes.Union( + runtypes.Literal('GET'), + runtypes.Literal('PATCH'), + runtypes.Literal('HEAD'), + runtypes.Literal('POST'), + runtypes.Literal('DELETE'), + runtypes.Literal('OPTIONS'), + runtypes.Literal('PUT'), + ), +) + +const PathValidator = proxyValidationError( + runtypes.String.withConstraint((s) => s.startsWith('/'), { + name: 'CanonicalURI', + }), +) + +const SignatureValidator = proxyValidationError( + runtypes.String.withConstraint((s) => s.length === 64, { + name: 'SignatureLength', + }), +) + +export const CanonicalRequestValidator = proxyValidationError( + runtypes + .Record({ + method: MethodValidator, + path: PathValidator, + }) + .And( + runtypes.Partial({ + headers: runtypes.Dictionary(runtypes.String, 'string'), + body: runtypes.String, + }), + ), +) +export type CanonicalRequest = runtypes.Static + +export const SecretValidator = proxyValidationError( + runtypes.String.withConstraint((s) => s.length === 64, { + name: 'SecretLength', + }), +) +export type Secret = runtypes.Static + +// Only dates after 01-01-2020 +export const TimestampValidator = proxyValidationError( + runtypes.Number.withConstraint((n) => n > 1577836800000, { + name: 'TimestampAge', + }), +) +export type Timestamp = runtypes.Static + +const SignedHeadersValidator = proxyValidationError( + runtypes + .Array(runtypes.String) + .withConstraint((l) => l.length >= 2, { name: 'MissingTimestampOrSignedHeaders' }), +) + +export const RequestMetadataValidator = proxyValidationError( + runtypes.Record({ + signature: SignatureValidator, + timestamp: TimestampValidator, + signedHeaders: SignedHeadersValidator, + }), +) +export type RequestMetadata = runtypes.Static + +export const TimeToLiveValidator = proxyValidationError( + runtypes.Number.withConstraint((n) => n >= 0, { + name: 'PositiveNumber', + }), +) +export type TimeToLive = runtypes.Static diff --git a/src/requests/typings/validators/proxy-validation-error.spec.ts b/src/requests/typings/validators/proxy-validation-error.spec.ts new file mode 100644 index 00000000..f5aaad66 --- /dev/null +++ b/src/requests/typings/validators/proxy-validation-error.spec.ts @@ -0,0 +1,88 @@ +import * as assert from 'assert' +import * as runtypes from 'runtypes' +import { ValidationException } from '../../exceptions' +import { proxyValidationError } from './proxy-validation-error' + +describe('proxyValidationError', () => { + describe('scalar without named constraint', () => { + it('converts to ValidationException', () => { + assert.throws(() => { + try { + proxyValidationError( + runtypes.Union(runtypes.Literal('val1'), runtypes.Literal('val2')), + ).check('invalid') + } catch (error) { + if (error instanceof ValidationException) { + assert.strictEqual(error.constraintName, undefined) + assert.strictEqual(error.key, undefined) + } + + throw error + } + }, ValidationException) + }) + }) + + describe('scalar with named constraint', () => { + it('converts to ValidationException without constraint name', () => { + assert.throws(() => { + try { + proxyValidationError( + runtypes.String.withConstraint((s) => s === 'value', { name: 'constraint-name' }), + ).check('invalid') + } catch (error) { + if (error instanceof ValidationException) { + assert.strictEqual(error.constraintName, 'constraint-name') + assert.strictEqual(error.key, undefined) + } + + throw error + } + }, ValidationException) + }) + }) + + describe('object without named constraint', () => { + it('converts to ValidationException with key', () => { + assert.throws(() => { + try { + proxyValidationError( + runtypes.Record({ + field: runtypes.Union(runtypes.Literal('val1'), runtypes.Literal('val2')), + }), + ).check({ field: 'invalid' }) + } catch (error) { + if (error instanceof ValidationException) { + assert.strictEqual(error.constraintName, undefined) + assert.strictEqual(error.key, 'field') + } + + throw error + } + }, ValidationException) + }) + }) + + describe('object with named constraint', () => { + it('converts to ValidationException with key and constraint name', () => { + assert.throws(() => { + try { + proxyValidationError( + runtypes.Record({ + field: runtypes.String.withConstraint((s) => s === 'value', { + name: 'constraint-name', + }), + }), + ).check({ field: 'invalid' }) + } catch (error) { + if (error instanceof ValidationException) { + assert.strictEqual(error.constraintName, 'constraint-name') + assert.strictEqual(error.key, 'field') + } + + throw error + } + }, ValidationException) + }) + }) +}) diff --git a/src/requests/typings/validators/proxy-validation-error.ts b/src/requests/typings/validators/proxy-validation-error.ts new file mode 100644 index 00000000..e111f272 --- /dev/null +++ b/src/requests/typings/validators/proxy-validation-error.ts @@ -0,0 +1,37 @@ +import * as runtypes from 'runtypes' +import { ValidationException } from '../../exceptions' + +const NAMED_CONSTRAINT_FAILURE_MSG = /^Failed (.+?) check/ + +// eslint-disable-next-line no-unused-vars +export function proxyValidationError(constraint: T): T { + // eslint-disable-next-line no-undef + return new Proxy(constraint, { + get(target, property) { + const value = target[property as keyof T] + if (typeof value !== 'function') { + return value + } + + return (...args: unknown[]) => { + try { + return value(...args) + } catch (error) { + if (error instanceof runtypes.ValidationError) { + let constraintName = undefined + if ('name' in constraint && typeof constraint.name === 'string') { + constraintName = constraint.name + } else { + const result = NAMED_CONSTRAINT_FAILURE_MSG.exec(error.message) + constraintName = result ? result[1] : undefined + } + + throw new ValidationException(error.message, constraintName, error.key) + } + + throw error + } + } + }, + }) +} diff --git a/src/requests/verify-request.spec.ts b/src/requests/verify-request.spec.ts index 56c50dad..5cb96eca 100644 --- a/src/requests/verify-request.spec.ts +++ b/src/requests/verify-request.spec.ts @@ -9,7 +9,7 @@ import { Context, } from './typings' import { signRequest } from './sign-request' -import { ExpiredRequestException } from './exceptions' +import { ExpiredRequestException, ValidationException } from './exceptions' const makeContextHeaders = (subject?: { appId: string } | { userId: string }) => { return subject @@ -203,14 +203,14 @@ describe('verifyRequest', () => { delete incomingRequest.headers[ContentfulHeader.Signature] - assert.throws(() => verifyRequest(VALID_SECRET, incomingRequest)) + assert.throws(() => verifyRequest(VALID_SECRET, incomingRequest), ValidationException) }) it('throws when missing timestamp', () => { const incomingRequest = makeIncomingRequest({}, makeContextHeaders(contextHeaders)) delete incomingRequest.headers[ContentfulHeader.Timestamp] - assert.throws(() => verifyRequest(VALID_SECRET, incomingRequest)) + assert.throws(() => verifyRequest(VALID_SECRET, incomingRequest), ValidationException) }) it('throws when missing signed headers', () => { const incomingRequest = makeIncomingRequest( @@ -222,7 +222,7 @@ describe('verifyRequest', () => { delete incomingRequest.headers[ContentfulHeader.SignedHeaders] - assert.throws(() => verifyRequest(VALID_SECRET, incomingRequest)) + assert.throws(() => verifyRequest(VALID_SECRET, incomingRequest), ValidationException) }) }) From ef63f99cb6d09d2455ce24bbc7886f068c031d17 Mon Sep 17 00:00:00 2001 From: Marco Gonzalez Date: Tue, 27 Aug 2024 15:21:23 -0400 Subject: [PATCH 2/2] chore: make proxy more strict --- .../typings/validators/proxy-validation-error.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/requests/typings/validators/proxy-validation-error.ts b/src/requests/typings/validators/proxy-validation-error.ts index e111f272..0e7ae1fd 100644 --- a/src/requests/typings/validators/proxy-validation-error.ts +++ b/src/requests/typings/validators/proxy-validation-error.ts @@ -4,18 +4,19 @@ import { ValidationException } from '../../exceptions' const NAMED_CONSTRAINT_FAILURE_MSG = /^Failed (.+?) check/ // eslint-disable-next-line no-unused-vars -export function proxyValidationError(constraint: T): T { +export function proxyValidationError unknown }>( + constraint: T, +): T { // eslint-disable-next-line no-undef return new Proxy(constraint, { get(target, property) { - const value = target[property as keyof T] - if (typeof value !== 'function') { - return value + if (property !== 'check') { + return target[property as keyof T] } return (...args: unknown[]) => { try { - return value(...args) + return target.check(...args) } catch (error) { if (error instanceof runtypes.ValidationError) { let constraintName = undefined