Method overrides - Githubissues

Rich-Harris commented 3 years ago

I could have sworn we'd already talked about this at some point, but I guess not.

Is your feature request related to a problem? Please describe. Bewilderingly, the only valid <form> methods are GET and POST — in other words <form method="put"> is invalid, and will result in form.method === 'get'.

A popular workaround is to use a method override query string parameter, like so:

<form method="post" action="/todos/{id}?_method=put">
  <!-- form elements -->
</form>

(As evidence for the validity of this approach, there's an official Express middleware for it: https://www.npmjs.com/package/method-override).

It would be nice if such a thing existed in SvelteKit as well, since we want to encourage people to build apps that work with JavaScript disabled, and that's only really possible if you can use PUT and DELETE alongside GET and POST without having to resort to fetch.

Describe the solution you'd like Since the ?_method=put cowpath already exists, I propose that we bake it into SvelteKit, while making it configurable:

// svelte.config.cjs
module.exports = {
  kit: {
    methodOverride: {
      key: '_method',
      allowed: ['post', 'put', 'delete']
    }
  }
};

Then, when any request that comes in where query.has(override), we change the request method before passing it to handle.

How important is this feature to you? It would be possible to do in userland...

export function handle({ request, render }) {
  return render({
    ...request,
    method: request.query.get('_method') || request.method
  });
}

...but I think this is the sort of thing that belongs in the framework.

Conduitry commented 3 years ago

Looking at the readme of that middleware, are they actually blessing that particular query parameter name? The only thing I saw blessed was a particular HTTP header. Or are you saying this would be off by default and people would need to explicitly opt in and say what query parameter they want to use?

Rich-Harris commented 3 years ago

They're blessing it in the sense that it's in the README. I've seen it used in a couple of different places

I'm suggesting that _method would be the default, but you could change it to whatever:

// svelte.config.cjs
module.exports = {
  kit: {
    methodOverride: {
      key: 'httpverb',
      allowed: ['patch']
    }
  }
};

Conduitry commented 3 years ago

I think not having this be opt-in would just bother me, no matter what name we used or what level of blessed this was in that middleware. It could be that I just wasn't looking, but I've certainly not gotten the impression that this has been at all an established de facto standard.

Rich-Harris commented 3 years ago

I don't know if I'd go as far as saying it's a de facto standard, just that it's a cowpath. Can you articulate why it bothers you? The framework is already doing other frameworky things like generating 304s and parsing bodies and serving static assets, is this so different?

Conduitry commented 3 years ago

That a 304 means a redirect and that a given content-type means that this is form data or that this is JSON data are all established real standards. That ?_method=put means "pretend this is a PUT" is not a standard. It's ascribing some additional meaning to something that officially has no particular meaning.

If this is an appeal to "2.4 million downloads of this package per month can't be wrong" then I will remind you about other commonly done things that you have complained about vehemently in the past. And if you still think this is a good idea, I'm not going to keep arguing about it, because it doesn't seem that awful as a default.

Rich-Harris commented 3 years ago

The only reason I'm keen on it is that without method overrides there's basically no way to build a CRUD app that doesn't rely on JS, AFAICT, and if 'you can build robust apps that don't make anti-SPA-fanatics froth at the mouth' is one of the selling points of SvelteKit (which I think it is!) then maybe it warrants being part of the framework.

At the same time, I've rarely regretted listening to you in the past. I think we need some more opinions!

GrygrFlzr commented 3 years ago

For consideration - prior art using the _method hack:

Rich-Harris commented 3 years ago

Thanks, that's very useful context. I see a few of these:

<input type="hidden" name="_method" value="PUT">

Perhaps we should support both? In which case it probably would need to be in the framework, since that would be more finicky to do in userland

benmccann commented 3 years ago

I'm in favor of this proposal. It's something I don't want to have to think about implementing as a user and would be happy to have the framework handle. The only danger I see is if I had a form with an input named _method but that seems quite unlikely and the benefits far outweigh the risks in my opinion

Conduitry commented 3 years ago

I'm not at all against this being included in the framework; what I'm concerned about is this being non-standard behavior that is being sent to folks as a default.

lukeed commented 3 years ago

Gross 😅

Better to scaffold a server-side POST /items/:id/update endpoint imo. It can even be an alias to the same handler for PUT route.

Methods and status codes are fundamental building blocks. Paths can be reworked if need be. And there's a much deeper cow path for shuttling everything thru POSTs if needed.

Rich-Harris commented 3 years ago

Huh. I find it much grosser to update or delete a resource with a POST request. If HTTP methods mean anything at all, then POST /items/xyz/update means 'create a new resource of the type /items/xyz/update', which is incoherent.

kresnasatya commented 3 years ago

Thanks, that's very useful context. I see a few of these:
<input type="hidden" name="_method" value="PUT">
Perhaps we should support both? In which case it probably would need to be in the framework, since that would be more finicky to do in userland

I like this option supported in svelte kit. In Laravel Framework - method spoofing as mention by @GrygrFlzr, when do something about update or delete data like PUT or PATCH or DELETE they give two options:

<form action="{{ route('update-url') }}" method="POST">
<input type="hidden" name="_method" value="PUT">
<input type="hidden" name="_token" value="csrf_token()">
</form>

OR

<form action="{{ route('update-url') }}" method="POST">
@method('PUT')
@csrf
</form>

am283721 commented 3 years ago

I'd be interested in tackling this.

I think there would be value in providing both of these approaches for overriding:

<form method="post" action="/todos/{id}?_method=put">

AND

<input type="hidden" name="_method" value="PUT">

The question of whether or not it should be enabled by default is interesting. Enabling by default could create some unexpected and confusing behavior for anyone unaware of the functionality. But I have to imagine the odds of someone submitting a form with a _method parameter has to be slim to none.

I can start working on a solution based on the discussion here and we can go from there.

Prinzhorn commented 3 years ago

The question of whether or not it should be enabled by default is interesting. Enabling by default could create some unexpected and confusing behavior for anyone unaware of the functionality. But I have to imagine the odds of someone submitting a form with a _method parameter has to be slim to none.

An attacker would increase the odds of that parameter quite a bit ;). This is related to #72, because right now if someone is using DELETE they are safe from CSRF if they didn't completely mess up CORS. However, enabling _method overrides by default means any regular <form> can now be turned into DELETE. This also brings me to another point: under no circumstances should _method be able to upgrade to/from GET (this would open up a whole new can of attack surface such as cache poisoning for mutating requests because your reverse proxy doesn't know about _method). It should exclusively work for POST, because that's the use-case (upgrading <form>). It should only allow upgrading to an allow-list of methods.

Also the name _method should be configurable as to avoid any conflicts with existing parameter at its root. Maybe the strategy (url_parameter, form_data, both) should be configurable as well to make it as predictable as possible.

It would probably be best to look at some existing implementations, assuming they already figured out in all these things.

https://github.com/expressjs/method-override/blob/5b83d4f0dc3db414df6c7e4a5da93dec170153de/index.js#L52-L55 https://github.com/expressjs/method-override/blob/5b83d4f0dc3db414df6c7e4a5da93dec170153de/index.js#L136-L144 ^This is not strict enough for my liking. You can turn POST into GET with the default configuration of the method-override middleware. I haven't put too much thought into attack scenarios (it doesn't seem as obviously bad as GET -> DELETE). This could for example cause DoS with the right conditions. You make a POST to an endpoint. It is turned into GET. Since it's now GET it might trigger some cache header middleware and now your reverse proxy caches the POST response (which it would otherwise not do) which DoS this endpoint for other users. This is pretty hypothetical, but still GET/HEAD should never be thrown into the mix. I don't see a point in blindly allowing any method https://github.com/jshttp/methods/blob/master/index.js#L40-L69 , especially things like TRACE or CONNECT have no business in being used in a regular app. There are probably more fun attack scenarios in these.

Edit: if anyone wants to play with this

const express = require('express');
const methodOverride = require('method-override')
const app = express();

app.use(methodOverride('_method'))

app.use((req, res, next) => {
  console.log(req.method);
  next();
});

app.listen(1337);

curl -X POST http://localhost:1337/?_method=GET
curl -X POST http://localhost:1337/?_method=CONNECT

This will log GET and CONNECT from the middleware.

Prinzhorn commented 3 years ago

Also don't just take my word for it, these things are happening right now

However, the REST API allows simulating different request types. As such, we can perform a POST request with the “users” string in the body of the request, and tell the REST API to act like it’s received a GET request.

https://hackerone.com/reports/384782

All the API endpoints are vulnerable , even the endpoints that only accept (PUT , PATCH or DELETE) , you can submit requests with these methods using _method parameter.

https://hackerone.com/reports/195156

https://www.google.com/search?q=site%3Ahttps%3A%2F%2Fhackerone.com%2F+_method

am283721 commented 3 years ago

So from your points, here's a list of minimum requirements for this functionality (some of these were planned already, given discussions above and mirroring other solutions):

Overrides should be disabled by default
When overriding, the original request must be a POST
Customizable list of allowed methods, defaults to PUT, PATCH, DELETE
- Maybe not have DELETE in defaults?
- Never allow GET (What about TRACE or CONNECT?)
Default name is _method, but is configurable by the user
Default strategy is both, but can be configured to url_parameter or form_data

Prinzhorn commented 3 years ago

Customizable list of allowed methods, defaults to PUT, PATCH, DELETE

TIL that polka supports more methods than I expected (https://github.com/lukeed/trouter/blob/0692417c728a4f6aecbc350f714019baf3072bc7/index.mjs#L7-L16) and express actually exposes every single one that the HTTP parser supports (https://github.com/expressjs/express/blob/06d11755c99fe4c1cddf8b889a687448b568472d/lib/router/index.js#L506-L513).

But for SvelteKit I think it's safe to limit them to these three until someone complains? What methods do SvelteKit endpoints support? I somewhat doubt Cloudflare Workers, Netlify or Vercel support all of them and that their reverse proxies let all of them through. But maybe I'm just too paranoid. But I'd be really curious if someone actually has a use-case for TRACE or CONNECT inside SvelteKit, that would not be better off in a custom entryPoint in the node adapter. Same for all the WebDAV methods.

Maybe not have DELETE in defaults?

I don't think that's a problem, DELETE is not inherently worse than any other. Overwriting things via PUT has a similar impact as DELETE. Or PUTing your crypto address to override the victims address to receive their funds is arguable worse than just deleting theirs.

205g0 commented 3 years ago

FWIW, a user's perspective: Just stumbled over this ?_method... override in Kit's demo app and was gonna write an angry issue about this confusing code until I found this issue and the conditional rewrite in hooks.ts.

First, I wanted to adapt your solution but I decided just to use POST for any mutation and GET for all queries, and nothing else. I use on the storage-side anyway kind of a super-powered upsert for all mutations. This can create new objects, replace existing ones and/or patch existing ones (all paired with strict types if anyone cares, so it's not a mess, haha). None of the HTTP verbs matches this upsert behavior exactly (it's a blend of PUT and PATCH), so I thought, why all this fuzz, mental drain for nothing. Besides, I find almost all http verbs unintuitive, not reflecting what they do, except GET and DELETE. Not sure how I gonna deal with DELETEs yet, maybe just POST an empty object or an extra flag, IDK yet, latter ideas feel also hacky, so I might opt for the method override just for DELETEs.

However and in general, if this method override is done in prior frameworks and learned by the users, why not.

ignatiusmb commented 2 years ago

Resolved by #2989

sveltejs / kit

Method overrides #1046