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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: enforce leading slash in path and pattern #339

Merged
merged 1 commit into from
Mar 17, 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
21 changes: 11 additions & 10 deletions node/bundler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@ import { runESZIP, useFixture } from '../test/util.js'

import { BundleError } from './bundle_error.js'
import { bundle, BundleOptions } from './bundler.js'
import { Declaration } from './declaration.js'
import { isFileNotFoundError } from './utils/error.js'
import { validateManifest } from './validation/manifest/index.js'

test('Produces an ESZIP bundle', async () => {
const { basePath, cleanup, distPath } = await useFixture('with_import_maps')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -59,7 +60,7 @@ test('Produces an ESZIP bundle', async () => {

test('Uses the vendored eszip module instead of fetching it from deno.land', async () => {
const { basePath, cleanup, distPath } = await useFixture('with_import_maps')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -91,7 +92,7 @@ test('Adds a custom error property to user errors during bundling', async () =>

const { basePath, cleanup, distPath } = await useFixture('invalid_functions')
const sourceDirectory = join(basePath, 'functions')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -119,7 +120,7 @@ test('Prints a nice error message when user tries importing NPM module', async (

const { basePath, cleanup, distPath } = await useFixture('imports_npm_module')
const sourceDirectory = join(basePath, 'functions')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand All @@ -143,7 +144,7 @@ test('Prints a nice error message when user tries importing NPM module with npm:

const { basePath, cleanup, distPath } = await useFixture('imports_npm_module_scheme')
const sourceDirectory = join(basePath, 'functions')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -179,7 +180,7 @@ test('Uses the cache directory as the `DENO_DIR` value', async () => {
const { basePath, cleanup, distPath } = await useFixture('with_import_maps')
const sourceDirectory = join(basePath, 'functions')
const cacheDir = await tmp.dir()
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -207,7 +208,7 @@ test('Uses the cache directory as the `DENO_DIR` value', async () => {
test('Supports import maps with relative paths', async () => {
const { basePath, cleanup, distPath } = await useFixture('with_import_maps')
const sourceDirectory = join(basePath, 'functions')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -236,7 +237,7 @@ test('Supports import maps with relative paths', async () => {
test('Ignores any user-defined `deno.json` files', async () => {
const { basePath, cleanup, distPath } = await useFixture('with_import_maps')
const sourceDirectory = join(basePath, 'functions')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -291,7 +292,7 @@ test('Ignores any user-defined `deno.json` files', async () => {
test('Processes a function that imports a custom layer', async () => {
const { basePath, cleanup, distPath } = await useFixture('with_layers')
const sourceDirectory = join(basePath, 'functions')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down Expand Up @@ -322,7 +323,7 @@ test('Processes a function that imports a custom layer', async () => {

test('Loads declarations and import maps from the deploy configuration', async () => {
const { basePath, cleanup, distPath } = await useFixture('with_deploy_config')
const declarations = [
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
Expand Down
6 changes: 4 additions & 2 deletions node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export const enum Cache {
Manual = 'manual',
}

export type Path = `/${string}`

export type OnError = 'fail' | 'bypass' | `/${string}`

export const isValidOnError = (value: unknown): value is OnError => {
Expand All @@ -37,8 +39,8 @@ export const isValidOnError = (value: unknown): value is OnError => {

export interface FunctionConfig {
cache?: Cache
path?: string | string[]
excludedPath?: string | string[]
path?: Path | Path[]
excludedPath?: Path | Path[]
onError?: OnError
}

Expand Down
16 changes: 8 additions & 8 deletions node/declaration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Declaration, mergeDeclarations } from './declaration.js'
const deployConfigDeclarations: Declaration[] = []

test('In-source config takes precedence over netlify.toml config', () => {
const tomlConfig = [
const tomlConfig: Declaration[] = [
{ function: 'geolocation', path: '/geo', cache: 'off' },
{ function: 'json', path: '/json', cache: 'manual' },
]
Expand All @@ -29,7 +29,7 @@ test('In-source config takes precedence over netlify.toml config', () => {
})

test("Declarations don't break if no in-source config is provided", () => {
const tomlConfig = [
const tomlConfig: Declaration[] = [
{ function: 'geolocation', path: '/geo', cache: 'off' },
{ function: 'json', path: '/json', cache: 'manual' },
]
Expand All @@ -50,7 +50,7 @@ test("Declarations don't break if no in-source config is provided", () => {
})

test('In-source config works independent of the netlify.toml file if a path is defined and otherwise if no path is set', () => {
const tomlConfig = [{ function: 'geolocation', path: '/geo', cache: 'off' }]
const tomlConfig: Declaration[] = [{ function: 'geolocation', path: '/geo', cache: 'off' }]

const funcConfigWithPath = {
json: { path: ['/json', '/json-isc'], cache: 'off' },
Expand All @@ -76,7 +76,7 @@ test('In-source config works independent of the netlify.toml file if a path is d
})

test('In-source config works if only the cache config property is set', () => {
const tomlConfig = [{ function: 'geolocation', path: '/geo', cache: 'off' }]
const tomlConfig: Declaration[] = [{ function: 'geolocation', path: '/geo', cache: 'off' }]

const funcConfig = {
geolocation: { cache: 'manual' },
Expand All @@ -88,7 +88,7 @@ test('In-source config works if only the cache config property is set', () => {
})

test("In-source config path property works if it's not an array", () => {
const tomlConfig = [{ function: 'json', path: '/json-toml', cache: 'off' }]
const tomlConfig: Declaration[] = [{ function: 'json', path: '/json-toml', cache: 'off' }]

const funcConfig = {
json: { path: '/json', cache: 'manual' },
Expand All @@ -100,7 +100,7 @@ test("In-source config path property works if it's not an array", () => {
})

test("In-source config path property works if it's not an array and it's not present in toml or deploy config", () => {
const tomlConfig = [{ function: 'geolocation', path: '/geo', cache: 'off' }]
const tomlConfig: Declaration[] = [{ function: 'geolocation', path: '/geo', cache: 'off' }]
const funcConfig = {
json: { path: '/json-isc', cache: 'manual' },
} as Record<string, FunctionConfig>
Expand All @@ -114,7 +114,7 @@ test("In-source config path property works if it's not an array and it's not pre
})

test('In-source config works if path property is an empty array with cache value specified', () => {
const tomlConfig = [{ function: 'json', path: '/json-toml', cache: 'off' }]
const tomlConfig: Declaration[] = [{ function: 'json', path: '/json-toml', cache: 'off' }]

const funcConfig = {
json: { path: [], cache: 'manual' },
Expand All @@ -126,7 +126,7 @@ test('In-source config works if path property is an empty array with cache value
})

test('netlify.toml-defined excludedPath are respected', () => {
const tomlConfig = [{ function: 'geolocation', path: '/geo/*', excludedPath: '/geo/exclude' }]
const tomlConfig: Declaration[] = [{ function: 'geolocation', path: '/geo/*', excludedPath: '/geo/exclude' }]

const funcConfig = {}

Expand Down
6 changes: 3 additions & 3 deletions node/declaration.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import regexpAST from 'regexp-tree'

import { FunctionConfig } from './config.js'
import { FunctionConfig, Path } from './config.js'

interface BaseDeclaration {
cache?: string
Expand All @@ -10,8 +10,8 @@ interface BaseDeclaration {
}

type DeclarationWithPath = BaseDeclaration & {
path: string
excludedPath?: string
path: Path
excludedPath?: Path
}

type DeclarationWithPattern = BaseDeclaration & {
Expand Down
1 change: 1 addition & 0 deletions node/feature_flags.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const defaultFlags = {
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
16 changes: 8 additions & 8 deletions node/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ test('Generates a manifest with different bundles', () => {
hash: '654321',
}
const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
const declarations = [{ function: 'func-1', path: '/f1' }]
const declarations: Declaration[] = [{ function: 'func-1', path: '/f1' }]
const manifest = generateManifest({ bundles: [bundle1, bundle2], declarations, functions })

const expectedBundles = [
Expand All @@ -38,7 +38,7 @@ test('Generates a manifest with display names', () => {
{ name: 'func-1', path: '/path/to/func-1.ts' },
{ name: 'func-2', path: '/path/to/func-2.ts' },
]
const declarations = [
const declarations: Declaration[] = [
{ function: 'func-1', name: 'Display Name', path: '/f1/*' },
{ function: 'func-2', path: '/f2/*' },
]
Expand All @@ -60,7 +60,7 @@ test('Generates a manifest with a generator field', () => {
{ name: 'func-3', path: '/path/to/func-3.ts' },
]

const declarations = [
const declarations: Declaration[] = [
{ function: 'func-1', generator: '@netlify/fake-plugin@1.0.0', path: '/f1/*' },
{ function: 'func-2', path: '/f2/*' },
{ function: 'func-3', generator: '@netlify/fake-plugin@1.0.0', cache: 'manual', path: '/f3' },
Expand Down Expand Up @@ -132,7 +132,7 @@ test('Excludes functions for which there are function files but no matching conf
{ name: 'func-1', path: '/path/to/func-1.ts' },
{ name: 'func-2', path: '/path/to/func-2.ts' },
]
const declarations = [{ function: 'func-1', path: '/f1' }]
const declarations: Declaration[] = [{ function: 'func-1', path: '/f1' }]
const manifest = generateManifest({ bundles: [bundle1], declarations, functions })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$' }]
Expand All @@ -147,7 +147,7 @@ test('Excludes functions for which there are config declarations but no matching
hash: '123456',
}
const functions = [{ name: 'func-2', path: '/path/to/func-2.ts' }]
const declarations = [
const declarations: Declaration[] = [
{ function: 'func-1', path: '/f1' },
{ function: 'func-2', path: '/f2' },
]
Expand All @@ -160,7 +160,7 @@ test('Excludes functions for which there are config declarations but no matching

test('Generates a manifest without bundles', () => {
const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
const declarations = [{ function: 'func-1', path: '/f1' }]
const declarations: Declaration[] = [{ function: 'func-1', path: '/f1' }]
const manifest = generateManifest({ bundles: [], declarations, functions })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$' }]
Expand All @@ -186,7 +186,7 @@ test('Generates a manifest with pre and post-cache routes', () => {
{ name: 'func-2', path: '/path/to/func-2.ts' },
{ name: 'func-3', path: '/path/to/func-3.ts' },
]
const declarations = [
const declarations: Declaration[] = [
{ function: 'func-1', path: '/f1' },
{ function: 'func-2', cache: 'not_a_supported_value', path: '/f2' },
{ function: 'func-3', cache: 'manual', path: '/f3' },
Expand Down Expand Up @@ -214,7 +214,7 @@ test('Generates a manifest with layers', () => {
{ name: 'func-1', path: '/path/to/func-1.ts' },
{ name: 'func-2', path: '/path/to/func-2.ts' },
]
const declarations = [
const declarations: Declaration[] = [
{ function: 'func-1', path: '/f1/*' },
{ function: 'func-2', path: '/f2/*' },
]
Expand Down
17 changes: 15 additions & 2 deletions node/validation/manifest/__snapshots__/index.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,25 @@ 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 ^ and ends with $ without any additional slashes before and afterwards
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 ^ and ends with $ without any additional slashes before and afterwards
| ^^^^^^^^^^^^^^ 馃憟馃徑 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
13 | \\"generator\\": \\"@netlify/fake-plugin@1.0.0\\"
14 | }
15 | ],"
Expand Down
35 changes: 30 additions & 5 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 } from 'vitest'
import { test, expect, describe, beforeEach, vi } from 'vitest'

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll be able to get rid of this once we remove the feature flag, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is only because the code only initializes ajv once, but with the FF changing during the tests we have to reset the modules.


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(() => validateManifest(manifest)).toThrowErrorMatchingSnapshot()
expect(() => freshValidateManifest(manifest)).toThrowErrorMatchingSnapshot()
})

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

expect(() => validateManifest(manifest)).toThrowErrorMatchingSnapshot()
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()
})

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

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

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

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

Expand Down