Skip to content

Commit

Permalink
fix: enforce leading slash in path and pattern
Browse files Browse the repository at this point in the history
  • Loading branch information
danez committed Mar 14, 2023
1 parent 7d4e2f5 commit 17b9360
Show file tree
Hide file tree
Showing 10 changed files with 87 additions and 44 deletions.
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

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

0 comments on commit 17b9360

Please sign in to comment.