Add functionality to override http methods (issue #1046) #2989
Conversation
🦋 Changeset detectedLatest commit: e3cdab5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
pnpm-lock.yaml
Outdated
| @@ -30,7 +30,7 @@ importers: | |||
| '@sveltejs/eslint-config': github.com/sveltejs/eslint-config/9a7d728e03ac433e5856a6e06775c17ee986d641_d9525e585486a10ae6317f3f61e9597f | |||
There was a problem hiding this comment.
you shouldn't need to modify the lockfile. there's a merge conflict since this file is being changed, so it'd be nice to drop it from the PR
There was a problem hiding this comment.
Reverted to the previous lockfile. It appears the build is failing due to the new config options.
Unexpected option config.kit.methodOverride
I'm assuming Netlify is building the create-svelte apps using the version of sveltekit provided by npm, and not this version of the kit? How is this typically handled?
documentation/docs/01-routing.md
Outdated
| Given that the only valid `<form>` methods are GET and POST, functionality is provided to override this limitation and allow the use of other HTTP verbs. This is particularly helpful when ensuring your application works even when javascript fails or is disabled. | ||
|
|
||
| - Disabled by default to prevent unintended behavior for those who don't need it | ||
| - For security purposes, the original method on the form must be `POST` and cannot be overridden with `GET` |
There was a problem hiding this comment.
this is a bit vague. it'd be better if we can say something more specific
| - For security purposes, the original method on the form must be `POST` and cannot be overridden with `GET` | |
| - To protect against CSRF attacks, the original method on the form must be `POST` and cannot be overridden with `GET` |
There was a problem hiding this comment.
Agreed. Although I'm not sure if this is for CSRF protection or for protection against other attack vectors (cache poisoning was the example given: #1046 (comment)).
Unfortunately I lack expertise in this field. Maybe @Prinzhorn could approve your suggestion or provide a more detailed/accurate statement?
There was a problem hiding this comment.
I don't think we need to be that specific and it's also not just a security thing. GET has vastly different semantics from POST, PUT etc. We should focus on why the feature exist, what problem it solves. I don't see any problem being solved by changing the method from/to GET.
I'd shorten the paragraph and the two bullet points to something like:
In contrast to fetch the only valid methods for a <form> are GET and POST. Svelte allows you to override the <form> method to workaround this limitation if you need to. That way your application can transparently work when JavaScript fails or is disabled by using fetch and <form> interchangeably with the same endpoint.
There was a problem hiding this comment.
I like your version of why this feature exists but I'm unsure about removing those 2 bullet points. It should be pointed out that the feature needs to be enabled by the user. The defaults are noted later in the configuration section but they might not notice that. And explaining that the form method must be POST and cannot be overridden with GET will prevent users from using this feature incorrectly and forcing them to figure out what is wrong when they receive build errors or their app doesn't function as expected.
There was a problem hiding this comment.
I agree about documenting everything. But for me docs are a last resort (the actual text, anything beyond skimming through code examples) when things already went wrong and I need to figure out why. But we can do much better. During dev when we see _method and methodOverride is disabled we can tell the developer that it needs to be enabled. Same if we see _method with something other than POST. Same for all other cases that are currently silently ignored. If the method is not in allowedMethods arguable it should even return a 400 in production. Or at the very least during dev it should tell you. We have the knowledge, let's not make the user run into unexpected behavior (e.g. silently ignoring _method for GET). Instead let's fail as loud as possible so they don't need to open their browser to actually read the docs or search though /issues. Let them stay in the zone and be like "oh, I need to set enabled: true, gotcha, thanks friendly error message".
There was a problem hiding this comment.
Agreed. Can anyone tell me how compile/build warnings are generated in Sveltekit?
And I'm on the fence regarding returning a 400 if the method isn't in allowed methods, but I agree silently ignoring it is probably not the correct action. I'd like to handle it similar to other scenarios in Sveltekit.
documentation/docs/01-routing.md
Outdated
| Given that the only valid `<form>` methods are GET and POST, functionality is provided to override this limitation and allow the use of other HTTP verbs. This is particularly helpful when ensuring your application works even when javascript fails or is disabled. | ||
|
|
||
| - Disabled by default to prevent unintended behavior for those who don't need it | ||
| - For security purposes, the original method on the form must be `POST` and cannot be overridden with `GET` |
There was a problem hiding this comment.
I don't think we need to be that specific and it's also not just a security thing. GET has vastly different semantics from POST, PUT etc. We should focus on why the feature exist, what problem it solves. I don't see any problem being solved by changing the method from/to GET.
I'd shorten the paragraph and the two bullet points to something like:
In contrast to fetch the only valid methods for a <form> are GET and POST. Svelte allows you to override the <form> method to workaround this limitation if you need to. That way your application can transparently work when JavaScript fails or is disabled by using fetch and <form> interchangeably with the same endpoint.
|
❌ Deploy Preview for kit-demo failed. 🔨 Explore the source changes: 4c3b054 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61db386281b2450007c38c32 |
There was a problem hiding this comment.
Thanks, this is great.
It's always a good idea to avoid things like 'both', since it's not future-proof — if we decided we wanted to support method overrides via headers, for example (which is sometimes used to get around firewalls that disallow non-GET/POST requests), then 'both' would need to become 'all', which would be a breaking change.
If we implement #70 (comment), we'll no longer be able to read form data before selecting a handler, which means that the hidden input method won't work in future.
What I'd suggest, therefore, is something like the following defaults...
methodOverride: {
allowed: [],
header: null,
parameter: '_method'
}...which would typically be used like so:
methodOverride: {
allowed: ['put', 'delete']
}
// or
methodOverride: {
allowed: ['put', 'delete'],
header: 'x-http-method-override'
}enabled is unused here because it's arguably redundant — it's enabled if you specify allowed methods, disabled otherwise.
|
❌ Deploy Preview for kit-demo failed. 🔨 Explore the source changes: e3cdab5 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61dd01a8c38474000773170b |
|
Wonderful, thank you! |
|
I see that this change has been made in the repo, but they are not present when Getting these errors still |
|
@Rich-Harris @benmccann @ignatiusmb Edit: Thanks @geoffrich, probably related to the
|
|
@am283721 Thanks for the report, I don't think this is related to the changeset. See the issue I opened above. |
See: #1046
In short: Given that the only valid
<form>methods are GET and POST, functionality is provided to override this limitation and allow the use of other HTTP verbs. This is particularly helpful when ensuring your application works even when javascript fails or is disabled.I could see this potentially needing to be an RFC due to either the impact of the change or the differences in opinion about how it should be implemented or what the default config values should be. Let me know if this is the case.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpx changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0