Skip to content
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

Adjust URL-encoding rules #1663

Open
kubukoz opened this issue Mar 20, 2025 · 5 comments
Open

Adjust URL-encoding rules #1663

kubukoz opened this issue Mar 20, 2025 · 5 comments
Assignees
Labels
AWS support Issues related to AWS support

Comments

@kubukoz
Copy link
Member

kubukoz commented Mar 20, 2025

We're relying on http4s to do our URLencoding for us, in both query and path parameters. However, it looks like this isn't following the smithy spec:

Characters not defined as unreserved by RFC 3986 section 2.3 MUST be percent-encoded. That is, all characters except for alphanumerics and -._~.

https://smithy.io/2.0/spec/http-bindings.html#httplabel-serialization-rules

http4s skips escaping of more than that - e.g. :, which is most notably part of any AWS ARN, making it currently impossible to e.g. call Lambdas across accounts.

We should update this - do our own pathencoding, where we'll have control over which characters we escape.

Now, this can be an incompatible change that'll break some APIs if we just do it for everyone - would like to avoid that. I propose:

  1. We do this for AWS protocols always
  2. We enable it as opt-in in SimpleRestJson
  3. Later - we switch to opt-out in SimpleRestJson, with a deprecated method
  4. Even later - we remove the deprecated method

We could keep it forever, but since the spec says it's a MUST, I'd rather keep it consistent.


Bonus points - verify if our handling of queries is compliant.

@kubukoz kubukoz added the AWS support Issues related to AWS support label Mar 20, 2025
@bpholt
Copy link
Contributor

bpholt commented Mar 20, 2025

Thanks for writing this up, @kubukoz!

@bpholt
Copy link
Contributor

bpholt commented Mar 20, 2025

FYI, I'm going to attempt picking this up. @kubukoz suggested in Discord:

  • the code we need to update is toSmithy4sHttpUri and fromSmithy4sHttpUri in smithy4s.http4s.kernel - they're duals of each other, will need to apply the same uri encoding consistently. Needs to be parameterized with a boolean or something equivalent, so we probably need an overload
  • you'll need to hardcode said boolean in AwsClient's call to fromSmithy4sHttpRequest and fromSmithy4sHttpRequest
  • need to add a new builder method on SimpleRestJson, similar to withMaxArity and friends; sth like .withCompliantLabelEncoding(bool), and said bool needs to be propagated all the way through the SimpleRestJsonCodecs to toSmithy4sHttpRequest / fromSmithy4sHttpRequest again. For the server and client

@Baccata
Copy link
Contributor

Baccata commented Mar 21, 2025

@bpholt, FYI, I'm afraid I'm nowhere close to getting you the corporate CLA you've asked for :(

@kubukoz
Copy link
Member Author

kubukoz commented Mar 21, 2025

@Baccata thoughts on the SimpleRestJson part? (opt-in -> opt-out -> always enabled)

or should we make some sort of .routesV2 / .clientV2 change at some point, to not do breaking changes without clear indication via source breakage?

@Baccata
Copy link
Contributor

Baccata commented Mar 24, 2025

I agree with starting with opt-in, and possibly opt-out later. I disagree with making it always enabled, as it may prevent some usecases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWS support Issues related to AWS support
Projects
None yet
Development

No branches or pull requests

3 participants