Recent patch releases are breaking in subtle ways

[tragiclifestories]

Hi there,

The 1.0.4-1.0.6 series of rack-cors adds path escaping to the middleware. This has caused us problems, because there are several scenarios that cause Rack::Utils.clean_path_info to throw an exception.

We have rack-cors quite high up our middleware stack to give back a quick OPTIONS response, but with the recent change, this means that exceptions unrelated to CORS will be thrown long before the request gets to our exception app, so we are unable to display our usual Bad Request error page without significant contortions to wrap rack-cors. Instead we give out a 500.

It's not entirely clear to me what the motivation for the 1.0.4 changes were (would be educational for me to find out!), but could that problem be dealt with more specifically? Should CORS middleware be concerned about whether there's a null byte in the URL path, for example (the known case where it breaks for us)?

[medexdev]

We are also getting the following error using 1.04.

undefined methodunescape_path' for Rack::Utils:Module)`

[typeoneerror]

Looks like it was fixed in 1.0.6?

https://github.com/cyu/rack-cors/commit/b3c06f15364ddd442947656e684117d60c5433cc

@cyu should you yank the versions that are broken and release a 1.1 or 2.0 since that is a breaking change?

[tragiclifestories]

FYI 1.0.4 has been yanked (the resulting broken builds brought me here).

[cyu]

@tragiclifestories This fix was done to address a vulnerability where specially formatted AJAX requests can access resources that would otherwise be unaccessible cross origin.

This devolved into a couple of mistakes on my part which is why we jump to version 1.0.6:

1.0.4 - I forgot I didn't specify a rack version prior to the change, and the methods I'm using are not in older of versions of rack.

1.0.5 - I set the rack version to >= 1.6.0, but mistakenly thought that unescape_path was available.

1.0.6 - Used unescape to get under rack 1.6.0 compatibility.

I yanked 1.0.4 because people could mistakenly run bundle update and it would break if they have an older version of rack. Having said that, this broke other people setups which makes me gun shy about yank 1.0.5. Worse case scenario there is you run bundle update again and get 1.0.6.

@typeoneerror My understanding about going up versions is if there are changes that might require changes to the library user's code. In this case, if you don't have rack 1.6.0 and couldn't resolve to it, bundle wouldn't update the gem.

It's unfortunate that clean_path_info throws exceptions. Looking at the code, it isn't raising anything specifically, so I'm guessing this is something that should be fixed in clean_path_info as well. I'm reluctant to write my own implementation of clean_path_info, because I don't want to presume I can do it better and also want to be consistent with rack in path resolution.

Can you give me some details of what URLs are causing exceptions? I'd like to understand inputs I'm dealing with. Also, what would you suggest would be the correct behavior on my part? Should I still try to run it against my rules, or should I consider those cases a CORS miss? Leaning towards the latter, but willing to hear other points of view.

[tragiclifestories]

Thanks a lot for the reply. I'm going to preface this by saying that I'm by no means a CORS expert, so I may be misunderstanding the problem.

There are a couple of test cases that went from green to red as a result of using clean_path_info here, all relating to invalid characters in the path.

  context "with an ID that contains non-UTF characters" do
    before { get "/exceptional_route_with_id/%28t%B3odei%29" }

    specify { expect(response.status).to eq(400) }
  end

  context "when query params contain an invalid byte sequence" do
    before { get "/exceptional_route?%28t%B3odei%29" }

    specify { expect(response.status).to be(400) }
  end

These are tests that exercise the class we've registered as the exception app in rails config; but as is clear, they're capybara style integration specs that hit the whole Rack stack. The result for the app, if you actually hit one of these cases, is that a 500 with no content is returned, because rack-cors in our case runs before Rails really kicks in, instead of the semantically-correct 400. The exception is ultimately raised by String#split, which blows up if there's non-UTF data and suchlike.

If you're just looking to use the feature of clean_path_info that resolves paths, it should be possible to call Rack::Utils.valid_path? on the unescaped path before passing it to clean_path_info. valid_path? returns true for Rack::Utils.unescape "/foo/bar/../baz", but false for Rack::Utils.unescape "/exceptional_route?%28t%B3odei%29". I will try to put together a PR if I get a moment and you think that would be acceptable.

[tragiclifestories]

I've done a sort of 10-minute version over here: https://github.com/cyu/rack-cors/pull/194. I'll tidy it up tomorrow if you're OK with that approach.

[cyu]

I pushed out a version 1.1.0, which will not attempt a clean_info_path if the path is invalid.

Oddly, I couldn't come up with a test case that would cause valid_path? to return false and clean_info_path to raise an exception. I tried porting @tthatragiclifestories test example, but couldn't get that path to fail.

[tragiclifestories]

Yep, looks like this fixes us. Thanks for your help!