-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add skew protection to Frameworks API #6601
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
Changes from 3 commits
8fbe9ed
1b69150
96ad9ba
cadd32d
687a92c
a4f84c0
eba572b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { promises as fs } from 'node:fs' | ||
|
||
import { z } from 'zod' | ||
|
||
const deployIDSourceTypeSchema = z.enum(['cookie', 'header', 'query']) | ||
|
||
const deployIDSourceSchema = z.object({ | ||
type: deployIDSourceTypeSchema, | ||
name: z.string(), | ||
}) | ||
|
||
const skewProtectionConfigSchema = z.object({ | ||
patterns: z.array(z.string()), | ||
sources: z.array(deployIDSourceSchema), | ||
}) | ||
|
||
export type SkewProtectionConfig = z.infer<typeof skewProtectionConfigSchema> | ||
export type DeployIDSource = z.infer<typeof deployIDSourceSchema> | ||
export type DeployIDSourceType = z.infer<typeof deployIDSourceTypeSchema> | ||
|
||
const validateSkewProtectionConfig = (data: unknown): SkewProtectionConfig => { | ||
try { | ||
return skewProtectionConfigSchema.parse(data) | ||
} catch (error) { | ||
if (error instanceof z.ZodError) { | ||
throw new Error(`Invalid skew protection configuration: ${error.message}`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we extend the assertion on the test for this to ensure the message contains actionable information? I think we actually need to read There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in a4f84c0. |
||
} | ||
|
||
throw error | ||
} | ||
} | ||
|
||
export const loadSkewProtectionConfig = async (configPath: string) => { | ||
try { | ||
const data = await fs.readFile(configPath, 'utf8') | ||
const config = validateSkewProtectionConfig(JSON.parse(data)) | ||
|
||
return config | ||
} catch (err) { | ||
// If the file doesn't exist, this is a non-error. | ||
if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { | ||
throw err | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are four cases here:
We're testing 1, 2, and 4. Could we add a test for 3? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in a4f84c0. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,9 @@ export const FRAMEWORKS_API_CONFIG_ENDPOINT = `${FRAMEWORKS_API_ENDPOINT}/config | |
export const FRAMEWORKS_API_EDGE_FUNCTIONS_ENDPOINT = `${FRAMEWORKS_API_ENDPOINT}/edge-functions` | ||
export const FRAMEWORKS_API_EDGE_FUNCTIONS_IMPORT_MAP = 'import_map.json' | ||
export const FRAMEWORKS_API_FUNCTIONS_ENDPOINT = `${FRAMEWORKS_API_ENDPOINT}/functions` | ||
export const FRAMEWORKS_API_SKEW_PROTECTION_ENDPOINT = `${FRAMEWORKS_API_ENDPOINT}/skew-protection.json` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is existing naming, but I find endpoint to be a misleading term here (especially since many of these do have very similar URL endpoints). how about "path"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 687a92c. |
||
|
||
export const EDGE_REDIRECTS_DIST_PATH = '.netlify/deploy-config/edge-redirects.json' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't found any trace of this in Linear, Notion, or Slack, but it seems to have been around for years in nfserver? Is this dead code we're reviving from the partially implemented and partially reverted edge redirects project from 2023...? In any case, I'm not quite following what Skew Protection has to do with edge redirects. Would you mind explaining? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in cadd32d. |
||
|
||
type DirectoryTreeFiles = Map<string, string[]> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { mkdir, writeFile } from 'node:fs/promises' | ||
|
||
await mkdir('.netlify/v1', { recursive: true }) | ||
|
||
await writeFile('.netlify/v1/skew-protection.json', JSON.stringify({ | ||
patterns: ["api"], | ||
sources: [{ type: "invalid_type", name: "test" }] | ||
})) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[build] | ||
command = "node build.mjs" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
// No skew protection file created |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[build] | ||
command = "node build.mjs" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { mkdir, writeFile } from 'node:fs/promises' | ||
|
||
await mkdir('.netlify/v1', { recursive: true }) | ||
|
||
await writeFile('.netlify/v1/skew-protection.json', JSON.stringify({ | ||
patterns: ["/api/*", "/dashboard/*"], | ||
sources: [ | ||
{ type: "cookie", name: "nf_deploy_id" }, | ||
{ type: "header", name: "x-nf-deploy-id" }, | ||
{ type: "query", name: "deploy_id" } | ||
] | ||
})) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[build] | ||
command = "node build.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could avoid the try-catch and conditional by using
safeParse
instead: https://zod.dev/basics?id=handling-errorsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a4f84c0.