[APT-2828] - Return 405 when Grape signals method not allowed

[jovan-appfolio]

Summary

• When a request targets a path that exists but with the wrong HTTP method (e.g. GET on a PATCH-only endpoint), Grape sets GRAPE_ALLOWED_METHODS in the env before invoking the endpoint
• The authorization before-filter was running before Grape could return 405, causing the request to fail with 403 Forbidden instead of 405 Method Not Allowed
• Fix checks GRAPE_ALLOWED_METHODS at the start of filter_access_filter and returns 405 when the request method is absent from the allowed list

Root cause

In Grape::Endpoint#run, before filters run on line 255 before the GRAPE_ALLOWED_METHODS check on line 257. The existing rescue block in filter_access_filter was intended to handle this, but it only catches cases where route access raises an exception. In practice, the Grape router's greedy match populates GRAPE_ROUTING_ARGS before calling the endpoint, so route is accessible and no exception is raised — authorization proceeds with the wrong method and returns 403.

🤖 Generated with Claude Code

[kermitapp]

Jira Issue: https://appfolio.atlassian.net/browse/APT-2828

[mike-mike-mike-mike-mike]

Based on this comment, why wasn't this already "working". Is this comment even correct? Looks to me like this route method just loads data from environment variables. In other words, I'm wondering if we should remove this begin/rescue and comment if it's not actually doing anything.

[jovan-appfolio]

I was thinking the same thing but was erring on the side of leaving it alone in case I misunderstood it or didn't trace it correctly. I'll take it out since you found the same thing.

[jovan-appfolio]

I did some digging with Claude and found this out, TLDR it is safe to remove.

The commit message explains everything:

  ▎ "the value of env[Grape::Env::GRAPE_ROUTING_ARGS] is nil when the http method requested is not allowed resulting in an exception that masks the 405"                                                             
                                                                                                                                                                                                                     
  So the history is:                                                                                                                                                                                                 
                                                                                                                                                                                                                     
  2019 (Grape ~1.6): call_with_allow_headers did not call prepare_env_from_route first. GRAPE_ROUTING_ARGS was nil on wrong-method requests. So route — which does env[Grape::Env::GRAPE_ROUTING_ARGS][:route_info] —
   would raise NoMethodError on nil. The begin/rescue caught it, returned early, and Grape returned 405. The test passed correctly.
                                                                                                                                                                                                                     
  Grape 2.x (current): call_with_allow_headers now calls prepare_env_from_route on line 154 before setting GRAPE_ALLOWED_METHODS. GRAPE_ROUTING_ARGS is always populated. route never raises. The begin/rescue became
   dead code, auth ran, returned 403 — and the test_method_not_allowed test would have been failing with 403 ≠ 405. This is precisely the bug APT-2828 describes.
                                                                                                                                                                                                                     
  Our fix: The GRAPE_ALLOWED_METHODS check correctly handles the Grape 2.x behavior. The existing test still covers the right scenario and should pass with our fix — no changes needed to it.                       
   
  So to directly answer the reviewer's question: yes, the begin/rescue and its comment are now dead code and should be removed. The original approach was valid for the Grape version it was written against; it just
   hasn't worked since Grape changed call_with_allow_headers to pre-populate GRAPE_ROUTING_ARGS.

[andrewdt23]

This raises some concerns for me from a security perspective. Lmk if I'm misunderstanding how this works, but it seems like this is essentially bypassing authorization and relying on the client application to deny the request (in this case with a 405). It's not necessarily guaranteed that a client app will do that, which leaves this vulnerable to authz rules being skipped over and the request going through successfully.

At the minimum, I think it's worth looping in someone from security to give this a thumbs up.

[jovan-appfolio]

You bring up a good point. It's not necessarily guaranteed that the client app will return a 405 - but this code path only executes for Grape endpoints and if allowed_methods is set (set by the Grape library), then the grape library which is what calls this authorization code is pretty much guaranteed to throw the 405 unless for some reason in the future, the grape library decides to set that environment variable and not throw a 405 and the code path exists and can be reached.

[andrewdt23]

That's fair and it does seem very low likelihood that we run into that scenario, but the implications are significant should it happen. I don't love that we would be relying on the internal implementation of a library.

Just to throw out another option - what if we just raise the 405 error here instead?

[jovan-appfolio]

I was originally hesitant to do that because that seems like the Grape libraries responsibility, but that might be worth it if it addresses a security concern. I'll make the update.

[jovan-appfolio]

Another idea I had was to add a test in declarative authorization to make sure our assumption of the environment variable and 405 response are upheld, but I realized that the grape version probably comes from property app, so it could be changed without ever re-testing declarative authorization.