Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: revert slash validation and change validation message #343

Merged
merged 4 commits into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion node/feature_flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ const defaultFlags = {
edge_functions_correct_order: false,
edge_functions_fail_unsupported_regex: false,
edge_functions_invalid_config_throw: false,
edge_functions_manifest_validate_slash: false,
}

type FeatureFlag = keyof typeof defaultFlags
Expand Down
17 changes: 2 additions & 15 deletions node/validation/manifest/__snapshots__/index.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -122,25 +122,12 @@ ADDTIONAL PROPERTY must NOT have additional properties

exports[`route > should throw on invalid pattern 1`] = `
"Validation of Edge Functions manifest failed
FORMAT pattern needs to be a regex that starts with ^ followed by / and ends with $ without any additional slashes before and afterwards
FORMAT pattern must be a regex that starts with ^ and ends with $ (e.g. ^/blog/[d]{4}$)

10 | \\"name\\": \\"name\\",
11 | \\"function\\": \\"hello\\",
> 12 | \\"pattern\\": \\"/^/hello/?$/\\",
| ^^^^^^^^^^^^^^ 👈🏽 format pattern needs to be a regex that starts with ^ followed by / and ends with $ without any additional slashes before and afterwards
13 | \\"generator\\": \\"@netlify/fake-plugin@1.0.0\\"
14 | }
15 | ],"
`;

exports[`route > should throw on missing beginning slash with FF 1`] = `
"Validation of Edge Functions manifest failed
FORMAT pattern needs to be a regex that starts with ^ followed by / and ends with $ without any additional slashes before and afterwards

10 | \\"name\\": \\"name\\",
11 | \\"function\\": \\"hello\\",
> 12 | \\"pattern\\": \\"^hello/?$\\",
| ^^^^^^^^^^^ 👈🏽 format pattern needs to be a regex that starts with ^ followed by / and ends with $ without any additional slashes before and afterwards
| ^^^^^^^^^^^^^^ 👈🏽 format pattern must be a regex that starts with ^ and ends with $ (e.g. ^/blog/[d]{4}$)
13 | \\"generator\\": \\"@netlify/fake-plugin@1.0.0\\"
14 | }
15 | ],"
Expand Down
35 changes: 5 additions & 30 deletions node/validation/manifest/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import chalk from 'chalk'
import { test, expect, describe, beforeEach, vi } from 'vitest'
import { test, expect, describe } from 'vitest'

import { validateManifest, ManifestValidationError } from './index.js'

Expand Down Expand Up @@ -112,57 +112,32 @@ describe('bundle', () => {
})

describe('route', () => {
let freshValidateManifest: typeof validateManifest

beforeEach(async () => {
// reset all modules, to get a fresh AJV validator for FF changes
vi.resetModules()
const indexImport = await import('./index.js')
freshValidateManifest = indexImport.validateManifest
})

test('should throw on additional property', () => {
const manifest = getBaseManifest()
manifest.routes[0].foo = 'bar'

expect(() => freshValidateManifest(manifest)).toThrowErrorMatchingSnapshot()
expect(() => validateManifest(manifest)).toThrowErrorMatchingSnapshot()
})

test('should throw on invalid pattern', () => {
const manifest = getBaseManifest()
manifest.routes[0].pattern = '/^/hello/?$/'

expect(() => freshValidateManifest(manifest)).toThrowErrorMatchingSnapshot()
})

test('should not throw on missing beginning slash without FF', () => {
const manifest = getBaseManifest()
manifest.routes[0].pattern = '^hello/?$'

expect(() => freshValidateManifest(manifest, { edge_functions_manifest_validate_slash: false })).not.toThrowError()
})

test('should throw on missing beginning slash with FF', () => {
const manifest = getBaseManifest()
manifest.routes[0].pattern = '^hello/?$'

expect(() =>
freshValidateManifest(manifest, { edge_functions_manifest_validate_slash: true }),
).toThrowErrorMatchingSnapshot()
expect(() => validateManifest(manifest)).toThrowErrorMatchingSnapshot()
})

test('should throw on missing function', () => {
const manifest = getBaseManifest()
delete manifest.routes[0].function

expect(() => freshValidateManifest(manifest)).toThrowErrorMatchingSnapshot()
expect(() => validateManifest(manifest)).toThrowErrorMatchingSnapshot()
})

test('should throw on missing pattern', () => {
const manifest = getBaseManifest()
delete manifest.routes[0].pattern

expect(() => freshValidateManifest(manifest)).toThrowErrorMatchingSnapshot()
expect(() => validateManifest(manifest)).toThrowErrorMatchingSnapshot()
})
})

Expand Down
9 changes: 5 additions & 4 deletions node/validation/manifest/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ import edgeManifestSchema from './schema.js'

let manifestValidator: ValidateFunction<Manifest>

const initializeValidator = (featureFlags: FeatureFlags) => {
const initializeValidator = () => {
if (manifestValidator === undefined) {
const ajv = new Ajv({ allErrors: true })
ajvErrors(ajv)

// regex pattern for manifest route pattern
// checks if the pattern string starts with ^ and ends with $
const normalizedPatternRegex = featureFlags.edge_functions_manifest_validate_slash ? /^\^\/.*\$$/ : /^\^.*\$$/
const normalizedPatternRegex = /^\^.*\$$/
ajv.addFormat('regexPattern', {
validate: (data: string) => normalizedPatternRegex.test(data),
})
Expand All @@ -30,8 +30,9 @@ const initializeValidator = (featureFlags: FeatureFlags) => {
}

// throws on validation error
export const validateManifest = (manifestData: unknown, featureFlags: FeatureFlags = {}): void => {
const validate = initializeValidator(featureFlags)
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export const validateManifest = (manifestData: unknown, _featureFlags: FeatureFlags = {}): void => {
const validate = initializeValidator()

const valid = validate(manifestData)

Expand Down
5 changes: 2 additions & 3 deletions node/validation/manifest/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ const routesSchema = {
pattern: {
type: 'string',
format: 'regexPattern',
errorMessage:
'pattern needs to be a regex that starts with ^ followed by / and ends with $ without any additional slashes before and afterwards',
errorMessage: 'pattern must be a regex that starts with ^ and ends with $ (e.g. ^/blog/[d]{4}$)',
},
generator: { type: 'string' },
},
Expand All @@ -35,7 +34,7 @@ const functionConfigSchema = {
type: 'string',
format: 'regexPattern',
errorMessage:
'excluded_patterns needs to be an array of regex that starts with ^ followed by / and ends with $ without any additional slashes before and afterwards',
'excluded_patterns must be an array of regex that starts with ^ and ends with $ (e.g. ^/blog/[d]{4}$)',
},
},
on_error: { type: 'string' },
Expand Down