Skip to content

feat: validate values for cache-control and content-type headers in dev mode#13114

Merged
elliott-with-the-longest-name-on-github merged 7 commits intosveltejs:mainfrom
JR-G:12784-warn-on-invalid-headers
Jan 29, 2025
Merged

feat: validate values for cache-control and content-type headers in dev mode#13114
elliott-with-the-longest-name-on-github merged 7 commits intosveltejs:mainfrom
JR-G:12784-warn-on-invalid-headers

Conversation

@JR-G
Copy link
Copy Markdown
Contributor

@JR-G JR-G commented Dec 6, 2024

Fix for #12784

Adds validation for common HTTP headers in dev mode to help catch invalid values early:

  • Validates cache-control directives against standard values
  • Validates content-type format
  • Only active in dev mode

run pnpm dev from test/apps/basics and navigate to http://localhost:5173/headers/invalid. In your console you'll see:

[SvelteKit] Invalid cache-control directive "totally-invalid". Did you mean one of: max-age, public, private, no-cache, no-store, must-revalidate, proxy-revalidate, s-maxage, immutable, stale-while-revalidate, stale-if-error, no-transform, only-if-cached, max-stale, min-fresh
[SvelteKit] Invalid content-type value "not-a-real-type"

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Dec 6, 2024

🦋 Changeset detected

Latest commit: 15d9f74

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Copy Markdown
Member

preview: https://svelte-dev-git-preview-kit-13114-svelte.vercel.app/

this is an automated message

@JR-G JR-G force-pushed the 12784-warn-on-invalid-headers branch 2 times, most recently from 72f42fd to e651a53 Compare December 6, 2024 15:41
@JR-G JR-G marked this pull request as ready for review December 6, 2024 15:44
Comment thread .changeset/rich-pants-beam.md Outdated
@benmccann benmccann changed the title feat: warn on invalid headers feat: validate values for cache-control and content-type headers in dev mode Dec 6, 2024
@JR-G JR-G force-pushed the 12784-warn-on-invalid-headers branch from e651a53 to adc6077 Compare December 8, 2024 19:25
Comment thread packages/kit/src/runtime/server/respond.js Outdated
'cache-control': (value) => {
const directives = value
.split(',')
.map((directive) => directive.trim().split('=')[0].toLowerCase());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map((directive) => directive.trim().split('=')[0].toLowerCase());
.map((directive) => directive.trim().split('=').at(0)?.toLowerCase());

Is it possible for the string to end up with an empty entry after splitting on ','? Like cache-control: stale-while-revalidate,,no-transform or something? Obviously that's still wrong, but hopefully we'd catch that and show an error, not throw a hard-to-debug runtime error 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was a good point! I've introduced an additional check to catch the empty directives.

Comment thread packages/kit/src/runtime/server/validate-headers.js Outdated

beforeAll(() => {
console_warn = console.warn;
// @ts-expect-error
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same thing here, I think you can add a global declaration to the top and avoid using globalThis here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found success in the other declarations, but I didn't have much luck silencing it here, despite trying a couple of things 🤔.

Comment thread packages/kit/src/runtime/server/validate-headers.js Outdated
@JR-G JR-G force-pushed the 12784-warn-on-invalid-headers branch from adc6077 to 838fd8b Compare January 17, 2025 16:33
]);

const CONTENT_TYPE_PATTERN =
/^(application|audio|font|image|model|text|video|x-[a-z]+)\/[-+.\w]+$/i;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/^(application|audio|font|image|model|text|video|x-[a-z]+)\/[-+.\w]+$/i;
/^(application|audio|font|image|model|text|video|x-[a-z]+)\/[-+.\w]+$/i;

The IANA spec defines the following top-level types, some of which are not included here:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Updated to reflect this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can move this to the test-dev-only package -- that package only runs dev tests, which means you don't need to fake the globalThis.__SVELTEKIT_DEV__ thing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Shifted!

@teemingc teemingc added the feature / enhancement New feature or request label Jan 23, 2025
@JR-G JR-G force-pushed the 12784-warn-on-invalid-headers branch from 838fd8b to 4f7c4b9 Compare January 24, 2025 13:50
@elliott-with-the-longest-name-on-github
Copy link
Copy Markdown
Contributor

Thanks @JR-G! I pushed a commit with a few improvements to the tests (mainly just simplifying setup) and some additional information in the error messages. Great PR. Merging as soon as CI passes :)

@github-actions github-actions Bot mentioned this pull request Jan 27, 2025
@bluwy bluwy mentioned this pull request Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature / enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants