Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add functionality to override http methods (issue #1046) #2989
Add functionality to override http methods (issue #1046) #2989
Changes from 1 commit
5252335
d9cb88a
8b4b713
f9d55b9
4c3b054
7ac5c7f
2c357ae
63d9dcb
ea1f75e
6ae9e7a
e3cdab5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is a bit vague. it'd be better if we can say something more specific
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
areGET
andPOST
. 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 usingfetch
and<form>
interchangeably with the same endpoint.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andmethodOverride
is disabled we can tell the developer that it needs to be enabled. Same if we see_method
with something other thanPOST
. Same for all other cases that are currently silently ignored. If the method is not inallowedMethods
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
forGET
). 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 setenabled: true
, gotcha, thanks friendly error message".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.
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.