azure-sdk-for-java icon indicating copy to clipboard operation
azure-sdk-for-java copied to clipboard

[FEATURE REQ] Encode invalid URL character, even when encoded=true on PathParam or QueryParam

Open weidongxu-microsoft opened this issue 1 year ago • 3 comments

Is your feature request related to a problem? Please describe.

@PathParam(value = "<>", encoded = true) or @QueryParam(value = "<>", encoded = true).

If customer sends some value that containsinvalid character (e.g. "foo bar" which contains a space), the URL sent would include the space, e.g.

"HTTP request","method":"GET","url":"https://localhost:3000/contoso/v1/contoso/foo bar"

(I tested on Netty and OkHttp client)

This would be an invalid URL.

Describe the solution you'd like azure-core keeps the reserve character like / or %, but still percent-encoding the character that would be invalid in URL.

We need some investigation about what character is invalid in URL.

Some discussion here https://github.com/Azure/cadl-ranch/pull/673#discussion_r1715262451

Reply from Python, the percent-encoding happens in their underlying http client.

Describe alternatives you've considered Codegen may do the escaping. But it would be more consistent if in azure-core

Additional context Add any other context or screenshots about the feature request here.

Information Checklist Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

  • [x] Description Added
  • [x] Expected solution specified

weidongxu-microsoft avatar Aug 13 '24 09:08 weidongxu-microsoft

Thanks for reporting this ask Weidong.

I'm actually going to push back against this request from the Core perspective as in this case the path and query parameter annotations state the passed value is already encoded. Which if we try to encode again, we may run into double encoding issues which are just as nebulous, if not worse because they'll always be valid but would result in possible corruption / requests that aren't doing what is expected.

Or to state it more bluntly, I'd rather throw a very explicit error than try to work and maybe work correctly. But, I'd be happy to add in utility methods that could be called to handle these scenarios.

alzimmermsft avatar Aug 13 '24 15:08 alzimmermsft

Sure, we probably keep the issue for some time for more discussion.

Here the ask is only encode invalid character that never could happen to URL (it is hard for me to find the full list of such characters explicitly, but I guess "space" is one of them). So e.g. %20 won't be encoded again.

I am OK if we decide to do the encoding either in emitted code directly, or via calling a helper method.

One argument for doing this in core, is that if the completed URL (pieced together from RestProxy) have invalid character, the request guaranteed to fail. And some 3rd-party http client (e.g. Python's) does "fix" these invalid character. Of course, the alternative of "throw an Error" or "let the underlying Http Client handle it" also make sense.

weidongxu-microsoft avatar Aug 14 '24 01:08 weidongxu-microsoft

Note that there could be some difference (other than the invalid char) between allowReserved from x-ms-skip-url-encoding. https://github.com/Azure/cadl-ranch/pull/673#discussion_r1726178663

I think currently that discussion is still on going.

If eventually we do need to do some extra encoding for valid char (but not among "reserved" char), we may do this in generated code, and not touching the encoded = true logic in azure-core.

weidongxu-microsoft avatar Aug 22 '24 03:08 weidongxu-microsoft