Correctly URI-encode HTTP Label values by bpholt · Pull Request #1668 · disneystreaming/smithy4s

bpholt · 2025-03-26T16:31:13Z

I was a little surprised to find that I didn't need to modify toSmithy4sHttpUri to make this test pass, so I wanted to get some feedback before I get too much further into modifying AwsClient and SimpleRestJson as laid out in #1663.

Needs disneystreaming/alloy#210

CLAassistant · 2025-03-26T16:31:19Z

All committers have signed the CLA.

CLAassistant · 2025-03-26T16:31:20Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

bpholt · 2025-04-02T21:40:56Z

modules/aws-http4s/src/smithy4s/aws/internals/AwsSigning.scala

+          else {
+            // Other services double encode and normalize
+
+            if (preparedRequest.uri.path.segments.exists(s => dotSegments.contains(s.encoded))) {


FYI, I think it would also be valid to simply use java.net.URI#normalize regardless of whether the path contains any dot segments. (The tests pass if I make that change.) I can switch to that if you'd prefer.

(That would also remove the reference to org.http4s.internal.CharPredicate.AlphaNum on line 163, which may be problematic since it appears to be an internal http4s API?)

yeah let's move 👍, thank you for raising the concern

bpholt · 2025-04-02T21:58:08Z

@kubukoz @Baccata I think this is ready for review (CLA aside), at least for the portions that are specific to AWS. I think we agreed in Discord that the signing changes don't need to be configurable from Smithy traits, so I didn't make any changes in that direction.

I wasn't sure if the changes to fromSmithy4sHttpUri and fromSmithy4sHttpRequest should be propagated out or not. Right now, I think @simpleRestJson would not use the new encoding rules (although it probably should), because SimpleRestJsonCodecs#makeClientCodecs doesn't set encodeReservedCharacters to true, but requests going to AWS will use the new rules since AwsClient does set that flag.

bpholt · 2025-04-03T23:45:57Z

modules/aws-http4s/test/src-jvm/smithy4s/aws/internals/AwsSignatureTest.scala

+    val genPathSegment: Gen[String] =
+      Gen
+        .frequency(
+          1 -> Gen.oneOf("..", "."),


Path normalization should remove these when it's enabled, so these dot segments are sometimes added to the path to make sure that happens.

bpholt · 2025-04-03T23:47:09Z

modules/aws-http4s/test/src-jvm/smithy4s/aws/internals/AwsSignatureTest.scala

+                ),
+                1 -> Gen.oneOf(
+                  // this list from https://github.com/http4s/http4s/blob/ac5c5cc40f732df238ef38c8ef8f571b4e66bfe3/core/shared/src/main/scala/org/http4s/Uri.scala#L1069
+                  "!$&'()*+,;=:/?@".toIndexedSeq


These are characters that should get encoded if they're present in a path segment and encoding is enabled.

bpholt · 2025-04-03T23:50:11Z

modules/aws-http4s/test/src-jvm/smithy4s/aws/internals/AwsSignatureTest.scala

      httpMethod <- Gen.oneOf(SdkHttpMethod.values().toList)
      host <- Gen.identifier
-      path <- Gen.listOfN(3, Gen.identifier).map(_.mkString("/"))
+      path <- Gen


When I was working on this and trying to address some edge cases, I commented out this generator and used a variety of generators specific to the edge case I was working on. For example, things like Gen.const("foo/../."), Gen.const("this:might:be:an:aws:arn"), etc. Unfortunately I didn't save the specifics, but they should be encoded in this generator.

bpholt · 2025-04-04T23:06:58Z

Unfortunately I think this implementation escapes too much. After running the protocol compliance suite from Alloy and Smithy itself, I noticed that the spec actually says only values inserted into the URI via the httpLabel trait should actually be escaped in this way. Notably, literal values that were already in the URI should not be escaped.

(Or at least, that's how I'm now reading the spec: "Patterns with no labels will match only requests containing the exact literal characters declared in the pattern", and then "Every label … MUST have the httpLabel trait". Then, the serialization rules that describe escaping only apply to the httpLabel trait.)

I don't think we have enough information in fromSmithy4sHttpUri to make that distinction, so I'm taking a step back. My current thought is to try to change smithy4s.http.internals.SchemaVisitorPathEncoder and maybe smithy4s.internals.SchemaVisitorPatternEncoder to escape labelled values, using the escaping algorithm used by Smithy.

bpholt · 2025-04-10T01:58:43Z

FWIW, I think we need to merge disneystreaming/alloy#210 before this will be fully tested by the Alloy compliance tests, but when I ran that locally before rebasing, it passed. (For some reason I can't get it to run now after pulling the latest updates; I get errors like "Unable to resolve trait aws.protocols#ec2QueryName" when executing http4s / Test / managedResources and http4s / Test / envVars. Maybe I'm not publishing Alloy correctly locally? I'm not very familiar with Mill.)

bpholt · 2025-04-10T02:02:06Z

smithy4s/modules/aws-http4s/src/smithy4s/aws/AwsClient.scala

Lines 91 to 108 in 87897b8

    
           case AwsProtocol.AWS_EC2_QUERY(_) => 
        
             AwsEcsQueryCodecs.make[F](version = service.version) 
        
           case AwsProtocol.AWS_JSON_1_0(_) => 
        
             AwsJsonCodecs.make[F]("application/x-amz-json-1.0") 
        
           case AwsProtocol.AWS_JSON_1_1(_) => 
        
             AwsJsonCodecs.make[F]("application/x-amz-json-1.1") 
        
           case AwsProtocol.AWS_QUERY(_) => 
        
             // TODO retain user encoding when applying compression 
        
             AwsQueryCodecs.make[F](version = service.version) 
        
           case AwsProtocol.AWS_REST_JSON_1(_) => 
        
             AwsRestJsonCodecs.make[F]("application/json") 
        
           case AwsProtocol.AWS_REST_XML(_) => 
        
             AwsRestXmlCodecs.make[F]()

Other than AWS_REST_JSON_1, should these codec constructors use the HTTP label encoding changes I introduced, or should I leave the old behavior in place?

modules/core/src/smithy4s/internals/StructurePatternRefinementProvider.scala

Baccata · 2025-04-11T12:06:39Z

modules/core/src/smithy4s/http/internals/PathEncode.scala

+  def encode(a: A, urlEncode: Boolean): List[String]
+  def encodeGreedy(a: A, urlEncode: Boolean): List[String]


I'm not sure whether it makes sense to do it at this level to be honest. I'd rather it was a constructor parameter (in the SchemaVisitorPathEncode construct, for instance).

I'm not sure how to implement it at the instance level and also maintain the distinction between the old path and the new encodedPath methods in the implementation of HttpEndpoint[I].

I guess I could change SchemaVisitorPathEncoder from an object to a class, and then construct two PathEncoder[I]s in HttpEndpoint.cast, where HttpEndpoint[I]s are constructed, in case downstream code calls the old path method? Is that what you meant?

In addition to not disturbing the public API (most of this work has been to internal APIs, but HttpEndpoint is not marked as internal), we also need to maintain the distinction in behavior in order to continue the existing behavior in the AWS codecs other than AWS_REST_JSON_1, as requested.

Addressed in fd284bd.

Thank you, that is indeed what I meant 👍

modules/core/src/smithy4s/http/internals/PathEncode.scala

modules/core/src/smithy4s/http/internals/SchemaVisitorPathEncoder.scala

modules/http4s/test/src/smithy4s/http4s/SimpleRestJsonComplianceSuite.scala

Baccata · 2025-04-11T12:12:43Z

Other than AWS_REST_JSON_1, should these codec constructors use the HTTP label encoding changes I introduced, or should I leave the old behavior in place?

Let's leave it unchanged for now and revisit on a case-by-case basis (unless we find some evidence by analysis the spec that some protocols are inherently and unilateraly abiding by one rule)

Baccata · 2025-04-11T12:12:59Z

A few comments, but great work overall !

bpholt · 2025-04-16T23:12:02Z

Other than AWS_REST_JSON_1, should these codec constructors use the HTTP label encoding changes I introduced, or should I leave the old behavior in place?

Let's leave it unchanged for now and revisit on a case-by-case basis (unless we find some evidence by analysis the spec that some protocols are inherently and unilateraly abiding by one rule)

FYI, it looks like AWS_REST_XML also requires encoding; without it, some of the compliance tests fail.

kubukoz

please add an entry in the changelog file, it'll be 0.18.40 at this point

modules/core/src/smithy4s/http/HttpEndpoint.scala

…ts not being sent to S3

…e request in the PR (the imperative code was converted from Java to Scala by IntelliJ)

…ciding based on method parameter values

…hen signing AWS requests

bpholt · 2025-07-18T18:43:18Z

modules/core/src/smithy4s/http/HttpEndpoint.scala

+  )
  def path(input: I): List[String]

+  def encodedPath(input: I): List[String]


I'm a little surprised this new method didn't trigger a MiMa error, but I didn't see anything that would have suppressed it, so I'm hoping it's ok.

I don't get it either, tbh. At least the hope is that nobody creates HttpEndpoint manually (as in, not via its unapply and cast)

btw. maybe we should seal this trait.

I tried a similar case and it emits ReversedMissingMethod. We actually have a filter for all of these (ProblemFilters.exclude[ReversedMissingMethodProblem]("smithy4s.*") in project/MimaVersionPlugin.scala) because it's a forward-compatibility problem.

personally I don't buy it, it's a binary risk like any other. What saves us is that nobody else should be implementing HttpEndpoint, as per the comment above.

Yep, makes sense. I missed MimaVersionPlugin.

personally I don't buy it, it's a binary risk like any other. What saves us is that nobody else should be implementing HttpEndpoint, as per the comment above.

Are you suggesting I change something so that it wouldn't emit a ReversedMissingMethod error if they weren't being suppressed? I'm not sure what the best way to do that is; none of the options I can think of are very good. (e.g. use ???, or delegate to path)

I wasn't suggesting any particular action in this case, just a more general wondering...ment for the other maintainers (@Baccata I'm looking at you) :)

in this case I think we're pretty safe because in practice this is "kinda sealed"

Ok. I made the changes necessary to seal HttpEndpoint, but I'm happy to revert them if that's not the path we want to take.

oh I wasn't suggesting to actually seal it yet.

I would probably keep it open - the reasoning is, if someone has a custom implementation, their experience will be:

runtime linkage error

they add the missing implementation, whatever it is

no more error

whereas if we seal it now, they may be unable to build with the new smithy4s at all.

Not super realistic, but who knows. I'd leave this kind of change for 0.19

Ok, works for me. I unsealed it.

build.sbt

project/Dependencies.scala

kubukoz

just waiting for https://github.com/disneystreaming/alloy/actions/runs/16455931297 now so that we can bump alloy here

bpholt force-pushed the uri-encoding branch from a0ae6bc to b9f949c Compare April 2, 2025 21:14

bpholt marked this pull request as ready for review April 2, 2025 21:31

bpholt commented Apr 2, 2025

View reviewed changes

bpholt force-pushed the uri-encoding branch from b9f949c to 6b6aede Compare April 3, 2025 23:44

bpholt commented Apr 3, 2025

View reviewed changes

bpholt marked this pull request as draft April 4, 2025 23:07

bpholt force-pushed the uri-encoding branch 2 times, most recently from cc8b59e to 37607ab Compare April 8, 2025 21:16

bpholt mentioned this pull request Apr 9, 2025

Add an encoded character to the GetMenu request tests disneystreaming/alloy#210

Merged

bpholt force-pushed the uri-encoding branch from 37607ab to f5ccf1e Compare April 9, 2025 23:48

bpholt changed the title ~~Add option to URI-encode all reserved characters~~ Correctly URI-encode HTTP Label values Apr 9, 2025

bpholt force-pushed the uri-encoding branch from f5ccf1e to a0aad90 Compare April 10, 2025 01:15

bpholt marked this pull request as ready for review April 10, 2025 01:48

Baccata reviewed Apr 11, 2025

View reviewed changes

modules/core/src/smithy4s/internals/StructurePatternRefinementProvider.scala Outdated Show resolved Hide resolved

Baccata reviewed Apr 11, 2025

View reviewed changes

modules/core/src/smithy4s/http/internals/PathEncode.scala Outdated Show resolved Hide resolved

Baccata reviewed Apr 11, 2025

View reviewed changes

modules/core/src/smithy4s/http/internals/SchemaVisitorPathEncoder.scala Outdated Show resolved Hide resolved

Baccata reviewed Apr 11, 2025

View reviewed changes

modules/http4s/test/src/smithy4s/http4s/SimpleRestJsonComplianceSuite.scala Outdated Show resolved Hide resolved

bpholt mentioned this pull request Apr 12, 2025

Scala codegen not working with local Alloy snapshots, partially breaks bloopInstall #1692

Closed

kubukoz reviewed Jul 11, 2025

View reviewed changes

modules/core/src/smithy4s/http/HttpEndpoint.scala Outdated Show resolved Hide resolved

modules/core/src/smithy4s/http/HttpEndpoint.scala Outdated Show resolved Hide resolved

bpholt force-pushed the uri-encoding branch from f47e165 to 73baa44 Compare July 17, 2025 19:48

bpholt added 5 commits July 18, 2025 11:25

When signing requests for AWS, double-encode URI paths for any reques…

de87c1b

…ts not being sent to S3

URI-encode HTTP label values inserted as URI path segments

d96c69f

exclude most AWS codecs from using encoded HTTP label values

3d242db

use imperative style to implement PathEncode.encodeUnreserved, per th…

f105769

…e request in the PR (the imperative code was converted from Java to Scala by IntelliJ)

address minor PR comments

0667da6

bpholt force-pushed the uri-encoding branch 2 times, most recently from 7f913d6 to 6faed70 Compare July 18, 2025 17:49

bpholt added 2 commits July 18, 2025 13:08

move HTTP label URL encoding decision to instance level instead of de…

f35750a

…ciding based on method parameter values

use java.net.URI#normalize to do most of the URI path normalization w…

5f0b0f5

…hen signing AWS requests

bpholt force-pushed the uri-encoding branch from 6faed70 to 17a56ee Compare July 18, 2025 18:09

bpholt commented Jul 18, 2025

View reviewed changes

build.sbt Show resolved Hide resolved

kubukoz reviewed Jul 18, 2025

View reviewed changes

project/Dependencies.scala Outdated Show resolved Hide resolved

bpholt force-pushed the uri-encoding branch 2 times, most recently from 4f9bffc to 42e38ff Compare July 22, 2025 20:38

kubukoz approved these changes Jul 22, 2025

View reviewed changes

bpholt force-pushed the uri-encoding branch from 42e38ff to 0b8c1d7 Compare July 22, 2025 21:42

bpholt added 4 commits July 22, 2025 19:26

update changelog with the changes in this PR for v0.18.40

b3852c8

Update smithytranslate-proto to 0.5.10

eff7b13

Update alloy to 0.3.29 for testing with its updated compliance tests

c3d9115

prepare to seal HttpEndpoint to avoid forward compatibility issues

bffe9be

bpholt force-pushed the uri-encoding branch from 0b8c1d7 to bffe9be Compare July 23, 2025 00:52

kubukoz approved these changes Jul 23, 2025

View reviewed changes

Merge branch 'series/0.18' into uri-encoding

5fc6608

kubukoz merged commit 53ce7d4 into disneystreaming:series/0.18 Jul 23, 2025
11 checks passed

kubukoz mentioned this pull request Jul 23, 2025

Adjust URL-encoding rules #1663

Closed

bpholt deleted the uri-encoding branch July 23, 2025 20:34

yisraelU mentioned this pull request Jul 29, 2025

Default behavior of http clients changed affecting third party clients #1792

Closed

		def encode(a: A, urlEncode: Boolean): List[String]
		def encodeGreedy(a: A, urlEncode: Boolean): List[String]

Conversation

bpholt commented Mar 26, 2025 • edited by kubukoz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Mar 26, 2025

Uh oh!

bpholt Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpholt commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpholt commented Apr 4, 2025

Uh oh!

bpholt commented Apr 10, 2025

Uh oh!

bpholt commented Apr 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Baccata commented Apr 11, 2025

Uh oh!

Baccata commented Apr 11, 2025

Uh oh!

bpholt commented Apr 16, 2025

Uh oh!

kubukoz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kubukoz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

bpholt commented Mar 26, 2025 •

edited by kubukoz

Loading

CLAassistant commented Mar 26, 2025 •

edited

Loading

bpholt Apr 2, 2025 •

edited

Loading