Amber: 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 https://github.com/amberframework/amber/pull/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.

[drujensen]

The framework is violating http specifications

I disagree with this assessment but I understand your argument for why we may want to prevent it. Something we will take into consideration. Thanks for your input!

[faustinoaq]

@eliasjpr Do exist some workaround for this issue before publish a new release?

Maybe we should publish a pre-release like crystal is doing with 0.24.0, WDYT?

[c910335]

In my opinion, DELETE should be protected by CSRF token, but it is inappropriate to pass CSRF token in query params, so I vote for DELETE via POST.

[elorest]

@faustinoaq @drujensen @satb An obvious solution is to add the CSRF token to delete links generated via scaffolding. However, having to remember to include that every time on manually generated links could be annoying.

Another solution would be to create a delete_link helper. This will allow us to leave it as a GET or even created a form with a POST in it for delete if it makes people happier. And it hopefully eliminates having to resort a JS hack like rails has been using for the last 5 years.

@c910335 "but it is inappropriate to pass CSRF token in query params" Why inappropriate? CSRF isn't secret per say. It's already being posted in the form params as well as existing in the html? With ssl Query params are also encrypted and if someone isn't using ssl post data can be stolen just as easily.

[c910335]

@elorest
Because users could copy the link with CSRF token and paste it to some other places.

[drujensen]

Yup, they can sniff the URL and copy it.

I think I'm convinced now we need to create a helper and convert this to a form post.

I think that is also a compelling reason to prevent method override for GET.

[elorest]

I've basically already conceded this point, due to popular demand. :( However in terms of security.

@drujensen If it's ssl they can't sniff query params. If it isn't ssl then they can sniff it from the form body just as easily.

@c910335 It's fine if they copy and paste it. They can already do that from their browser. The CSRF exists in plain text if they view source. In the case of clicking the link it it calls it and redirects. After it's used a new one is issued so having the one just sent wouldn't matter anyway.

CSRF is only meant to prevent websites running in other tabs/windows from being able to use your signed in cookies when using JS to make a call to your site. For instance a call to your bank to transfer all your money. It isn't to prevent someone from copying code from source or query strings, that isn't the exploit.

[drujensen]

@elorest You are right. For some reason I thought the params were available even on an SSL connection but it looks like they are encrypted. It's only the domain name that is clear text from DNS lookup.

[elorest]

Should we just opt for a simple fix for now? And add some helpers later? My vote would be to add csrf to the link for now and work on some helpers later. What's important is that generated code works and is secure. We can acheive this via link_to "delete", "/listing/1?_method=DELETE&csrf=#{CSRF.token}"