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

Query parameters with pipes (|) result in invalid calls #3393

Closed
GuiForget opened this Issue Jun 8, 2017 · 3 comments

Comments

2 participants
@GuiForget

GuiForget commented Jun 8, 2017

This is a follow up of the bugs initially reported here and here

The workaround provided in the latter works but I would think okhttp should be able to do that without having to write an interceptor.

I chased the bug all the way to this code:

if (includeAuthorityInRequestLine(request, proxyType)) {
result.append(request.url());
} else {
result.append(requestPath(request.url()));
}

This uses the URL set of characters, not the URI set of characters. I'm not entirely sure of the consequences of changing the code and why I didn't submit a pull request.

@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jun 10, 2017

Member

What action would you like for us to take? OkHttp’s treatment of the | character in a query is consistent with Chrome, Firefox, and Safari.

http://tinyurl.com/url-escaping

Member

swankjesse commented Jun 10, 2017

What action would you like for us to take? OkHttp’s treatment of the | character in a query is consistent with Chrome, Firefox, and Safari.

http://tinyurl.com/url-escaping

@GuiForget

This comment has been minimized.

Show comment
Hide comment
@GuiForget

GuiForget Jun 28, 2017

I would expect it to be escaped like you do already for some characters. Namely right now you already encode the characters present in okhttp3.HttpUrl.QUERY_COMPONENT_ENCODE_SET. But you could encode more aggressively and also encode the characters in okhttp3.HttpUrl.QUERY_COMPONENT_ENCODE_SET_URI

And if you don't provide it by default, at least make this an option so that client would want to use the more strict RFCs can.

GuiForget commented Jun 28, 2017

I would expect it to be escaped like you do already for some characters. Namely right now you already encode the characters present in okhttp3.HttpUrl.QUERY_COMPONENT_ENCODE_SET. But you could encode more aggressively and also encode the characters in okhttp3.HttpUrl.QUERY_COMPONENT_ENCODE_SET_URI

And if you don't provide it by default, at least make this an option so that client would want to use the more strict RFCs can.

@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jul 31, 2017

Member

Unfortunately, we can’t just encode | without breaking servers that expect it to not be encoded. So we choose to be consistent with the major web browsers which don’t encode |.

It’s frustrating that this keeps coming up. Unfortunately I think the best place to fix this is server-side.

Member

swankjesse commented Jul 31, 2017

Unfortunately, we can’t just encode | without breaking servers that expect it to not be encoded. So we choose to be consistent with the major web browsers which don’t encode |.

It’s frustrating that this keeps coming up. Unfortunately I think the best place to fix this is server-side.

@swankjesse swankjesse closed this Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment