Scaffold generated code gives a CSRF error on delete

[satb]

Description

Very new to Crystal and Amber. Was going through the guides and generated a basic project with one User model. The app started and everything was good but "delete" of the user isn't working. It fails with a "CSRF check failed." error upon delete.

Also delete musn't be a http get request but a http post. The network request on delete is a GET with out of the box scaffolding and the URL looks like this - "http://localhost:3000/users/1?_method=delete"

Steps to Reproduce

1. amber new myapp
2. amber g scaffold User first_name:string last_name:string
3. shards install (took me a while to figure out why amber was complaining that "require "amber"" is failing. Looks like crystal shards now install to a local lib folder
4. amber w
5. open http://localhost:3000
6. Create a new user and try to delete the user

Expected behavior: http post request with _method of delete and no CSRF error

Actual behavior: http get request with an error that says "CSRF check failed."

Reproduces how often: Everytime

Versions

❯ amber --version
Amber CMD (amberframework.org) - v0.3.0

Additional Information

Just an out of the box scaffolding

[faustinoaq]

I can reproduce it too, try commenting Amber::Pipe::CSRF on :web pipeline as workaround

Amber::Server.configure do |app|
  pipeline :web do
    plug Amber::Pipe::Error.new
    plug Amber::Pipe::Logger.new
    plug Amber::Pipe::Session.new
    plug Amber::Pipe::Flash.new
    # plug Amber::Pipe::CSRF.new
  end

  routes :web do
    resources "/posts", PostController
    get "/", HomeController, :index
  end
end

[elorest]

@faustinoaq Commenting out CSRF pipe will definitely work. But it doesn't explain why it stopped working. It might have something to do with #299

[elorest]

@drujensen @faustinoaq @eliasjpr This brings up this issue once again. Should DELETE have to be a POST like it is in rails?

Personally I don't like having to make it either a form or rely on a js library to make delete links work, so would vote for allowing DELETE through GET.

In the case that it's a GET though do we want to pass the CSRF through in a query param?

[satb]

@elorest - There are multiple reasons to not use an GET but do it using a form post instead. Firstly, in doing it via a POST, you avoid the back button problem. The browser will warn if someone hits the back button to a form POST. Secondly, you don't automatically delete stuff when spiders crawl the site. Thirdly, people don't expect it to be a GET having been used to best practices elsewhere.

[elorest]

@satb Those are valid points not necessarily conclusive.

1. Back to a deleted object will just give you a 404 not found since it's already been deleted.

2. If spiders can access delete buttons you already have an issue. Spiders CAN/DO still delete if it's a post, unless they're just being polite.

3. Most people from RAILS and other frameworks don't actually know it's not a GET until they accidentally mess up their JS and all of their delete links stop working.

[veelenga]

@elorest it is possible to pass csrf token through a query param using DELETE:

a href="/posts/#{ post.id }?_method=delete&_csrf=#{csrf_token}"

Technically, when we do _method=delete, it is a GET request from the browser to the middleware, and then Amber overrides request method to DELETE so it will be routed to the needed action and perform the deletion.

The real problem can happen when someone just disables CSRF as suggested above. So user can follow a link from someone https://myapp.com/posts/#{ post.id }?_method=delete and delete the object without any confirmation.

It stopped working, because this link has never been CSRF proof using amber generators. Before #299 request methods with lowercase names (i.e. _method=delete) were just ignored.

[satb]

@elorest - An http GET should be free from side effects. Deleting a record on a GET is not side effect free. It goes against the grain of the http protocol itself. The Http standard is clear on that - https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.1

[drujensen]

@satb We agree that GET should not modify data, that is why we are using an override.

We also think that you should not delete data using a POST, you should use the method DELETE. But since HTML spec and browsers don't support DELETE, we are using the override so we can use the Restful Verbs as they should be used.

See: https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.1 regarding what POST should be used for.

Question, Why should method override only apply to POST?

When you override POST to DELETE, Is that any different that overriding GET to DELETE?

[satb]

@drujensen - You are right that the lack of full http user agent support is the reason to pick a commonly supported http verb for rest'ish style resource management. The semantics of GET are clear. It is to get data about a particular resource. Similarly HEAD is also clear in its semantics. POST/PUT/DELETE on the other hand is expected to have side effects. From the commonly supported verbs across http user agents, only POST is the one that gives the user agent the hint that some side effect is intended. Hence its use is generally accepted with an overridden _method that picks the right http verb on the server.

I site other reasons above on why GET is also bad to delete a resource. Imagine an e-commerce store. You do not want spiders to crawl your site only to realize that it deleted all your products. On the other hand spiders don't submit forms. Could you create a form and override its method to a GET if you so choose so spider problems don't exist. Sure you could....but thats generally not how it is designed or intended to work.

[drujensen]

@satb thanks for your input. I agree that GET should not modify data and that it's semantics are clear on that.

I also think POST semantics are clear and you should not DELETE data using it. Would you agree with this?

I agree that it's best to override POST instead of GET when modifying data. I think your statement that the expectation is POST/PUT/DELETE will potentially modify data where HEAD/GET will not, may be a good enough one.

I guess the question is, is this a compelling enough reason to add a restriction in the framework that forces you to only use the method override on a POST.

I personally would like to leave it so you can override any method and leave it up to the developer to decide for their situation which is best, but that may be to libertarian of a view. I also want to legalize pot and eliminate the FDA so my worldview may have some impact on my thinking here. ;-)

[satb]

@drujensen

I also think POST semantics are clear and you should not DELETE data using it. Would you agree with this?

True. POST semantics are not meant to delete. But thats the best we have to support a broad array of http user agents. Its the closest thing we have to indicate a side effect on data. However, as user agents more broadly support other http methods, it is only natural that DELETE will be directly supported.

is this a compelling enough reason to add a restriction in the framework that forces you to only use the method override on a POST.

It is always on the frameworks shoulders to push the developers in the right direction so they avoid costly mistakes. Imagine someone new to amber builds their fantastic app, deploys to production only to find a web crawler got rid of their data. How would they feel?

Rather, the framework must ensure it protects against common and costly mistakes but if someone has a more arcane use case, the framework allows that without getting in the way too much.

[drujensen]

If a spider can hit a delete button, doesn't that mean that anyone can? I doubt a developer would allow anyone to just click the delete button without authenticating them but I may be missing something.

[elorest]

@satb @drujensen If someone leaves a delete button where a spider can hit it, it's not up to the framework to prevent that. We give them tools like auth to help them not do that. When it comes to spiders there is nothing that prevents them from clicking a POST link/button either. In most cases trusted spiders like google will error on the side of not POSTing, most other spider that crawl the web will not be so courteous.

Forcing a DELETE to only go through POST DOESN'T make you more secure. It may be better practice... but if a user is counting on POST to prevent a bot from deleting things they're already in big trouble.

[drujensen]

@elorest right, I agree. I doubt we can help someone if they leave a delete button open for a spider to click.

@satb Is there other "costly mistakes" related to allowing the override of GET that we should be aware of?

[satb]

@drujensen - You are probably right that delete is protected behind a login. But I don't think thats something the framework is entitled to assume. There are several other tools (apart from just spiders) examples browse plugins, testing tools etc that implicitly assume that fetching via GET is side effect free. The framework is violating http specifications and people build ecosystem tooling based on specifications and not based on some presumed behavior (like being logged in) on the part of the user.