-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Escaping URL strings when running get do;end blocks. #5
Comments
I think URL escaping these strings makes sense, but it is somewhat complicated by the route parameters feature. For example: get "/foo/:id" do
do_request :id => 1 # requests "/foo/1" or "/foo/%2Fid"?
end I'm not sure how to handle this. Escaping inside http_spec is easier for the user, but doesn't lend itself to a clean implementation unless route parameters change. Ideas? |
A tricky thing indeed! The only idea I have right now is to escape parameters part of request string after the |
Actually, when thinking about http_spec DSL, at the moment, I don't see any other cases, where escaping could have to be required. |
How about this? get "/api/v1/places" do
# requests "/api/v1/places?bounds=46.28,30.53%3B46.59,31.73"
do_request :query => { :bounds => "46.28,30.53;46.59,31.73" }
end The idea here is we escape values before they become part of the URL. |
Actually, CGI.escape produces |
It works perfectly, that's why I'm talking about CGI-escaping params part only. |
It is up to you to decide, but I think params could just be CGI escaped - much more cleaner and stealth than adding additional options like |
One more important addition: Only values for the keys are to be encoded, not the whole resulting params string. I've tested this behaviour just now - if I cgi-encode So my conslusion: if ever implement cgi-encoding, than it should be done on params values one by one. |
I've done a bit of research on this, and I think that the best solution for now is to do nothing. Until I can get some advice from an expert on this, I'm going to continue to assume that URLs are properly escaped. I want to revisit this, so let me jot down the results from my research here: RFC 3986 §2 rack-test HTML5 Form Submission URI.encode_www_form and friends TL;DR: Closing because more feedback is required. Current workaround is to encode all strings manually. |
It depends on what you want. First of all, both '/api/v1/places?bounds=46.28,30.53%3B46.59,31.73' and '/api/v1/places?bounds=46.28,30.53;B46.59,31.73' are valid URIs. They just give you different things. See,
So. If it's data, it gets escaped, and if it's a delimiter, it doesn't.
All Ruby web stuff expects the
And httpbis:
|
Seems like it's impossible to automatically escape the string in @stanislaw's example because it's ambiguous whether the Ruby web stuff (and PHP too) expect the URI.encode_www_form([["foo", "foo1"], ["foo", "foo2"]]) # => foo=foo1&foo=foo2 I agree that it's silly to use a body with a GET request, but it is used In The Wild™. This is probably Wrong, but it bums me out to make things impossible. I'm willing to make a trade-off here if there is enough support for it. I will listen to use cases, implementation suggestions, and 👍s for this, because I don't feel comfortable making any trade-offs without more use cases. |
Some good insights here too: https://github.com/technoweenie/faraday/issues/78 Preferences between these? # foo%5B%5D=bar&foo%5B%5D=baz
do_request :query => [["foo[]", "bar"], ["foo[]", "baz"]]
# foo=bar&baz=quux
do_request :query => { "foo" => "bar", "baz" => "quux" } vs # default?
HTTPSpec.query_encoder = BracketizedQueryEncoder
# foo%5B%5D=bar&foo%5B%5D=baz
do_request :query => { "foo" => ["bar", "baz"] } I am leaning toward the first (less glamorous) option and leveraging |
I wonder, if http_spec does not escape request strings intentionally or this could be added as a feature?
For example if I write this request with semicolon escaped
then Sinatra (it serves to mock server api I run http_spec against) populates
params
hash correctly and vice versa if I write semicolons as is.Thanks!
The text was updated successfully, but these errors were encountered: