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

URLSession and AHC transports double-encode path parameters #251

Closed
sliemeobn opened this issue Sep 7, 2023 · 4 comments · Fixed by apple/swift-openapi-urlsession#14 or swift-server/swift-openapi-async-http-client#15
Assignees
Labels
area/transport Affects: Client or server transport library. kind/bug Feature doesn't work as expected. size/S Small task. (A couple of hours of work.)
Milestone

Comments

@sliemeobn
Copy link

When using String path parameters, the generated client (correctly) URL-encodes the values to be included in the path.
However, the server handler just passes the the "raw" string value in the input.path structure.

Eg:
using the generated client:

        let response = try await client.createRole(.init(
            path: .init(roleId: "A role"),
            ...
        ))

server code

    func createRole(_ input: Operations.createRole.Input) async throws -> 
      //input.path.roleId is "A%20role"
}

I would expect this to be symmetric and arrive decoded in the handler.

@sliemeobn sliemeobn changed the title Path parameters arrive URL-encoded in the server handlers Path parameters arrive URL-encoded in server handlers Sep 7, 2023
@czechboy0 czechboy0 added area/generator Affects: plugin, CLI, config file. kind/bug Feature doesn't work as expected. labels Sep 7, 2023
@czechboy0 czechboy0 self-assigned this Sep 7, 2023
@czechboy0 czechboy0 added this to the 1.0 milestone Sep 7, 2023
@czechboy0
Copy link
Collaborator

Thanks for filing, yes that looks like a bug.

@czechboy0
Copy link
Collaborator

I tried to reproduce in a unit test, but seems to be behaving correctly: apple/swift-openapi-runtime#49

Possibly this could be caused by the client using 0.2 and server 0.1? I could see that leaving the client as escaped, but not being unescaped on the server.

@czechboy0 czechboy0 added status/triage Collecting information required to triage the issue. and removed area/generator Affects: plugin, CLI, config file. kind/bug Feature doesn't work as expected. labels Sep 7, 2023
@sliemeobn
Copy link
Author

found it, it is actually a bug in the URLSessionTransport's initializer of URLRequest

The URLComponents.url adds another round of percent encoding, basically sending an double-encoded path parameter to the server.

@czechboy0 czechboy0 changed the title Path parameters arrive URL-encoded in server handlers URLSession transport double-encodes path parameters Sep 7, 2023
@czechboy0 czechboy0 changed the title URLSession transport double-encodes path parameters URLSession and AHC transports double-encode path parameters Sep 7, 2023
@czechboy0 czechboy0 added area/transport Affects: Client or server transport library. kind/bug Feature doesn't work as expected. size/S Small task. (A couple of hours of work.) and removed status/triage Collecting information required to triage the issue. labels Sep 7, 2023
czechboy0 added a commit to swift-server/swift-openapi-async-http-client that referenced this issue Sep 7, 2023
Fix double encoding of path parameters

### Motivation

Fixes apple/swift-openapi-generator#251.

### Modifications

Use the already escaped path setter on `URLComponents` to avoid the second encoding pass.

### Result

Path parameters that needed escaping are only escaped once, not twice.

### Test Plan

Adapted the existing unit test to cover a path item that needs escaping.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#15
czechboy0 added a commit to apple/swift-openapi-urlsession that referenced this issue Sep 7, 2023
Fix double encoding of path parameters

### Motivation

Fixes apple/swift-openapi-generator#251.

### Modifications

Use the already escaped path setter on `URLComponents` to avoid the second encoding pass.

### Result

Path parameters that needed escaping are only escaped once, not twice.

### Test Plan

Adapted the existing unit test to cover a path item that needs escaping.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#14
@czechboy0
Copy link
Collaborator

czechboy0 commented Sep 7, 2023

Ok both the URLSession and AHC transports have been released with this fix in 0.2.2.

Thank you @sliemeobn for doing the heavy lifting on this one! 🙏

czechboy0 added a commit to apple/swift-openapi-runtime that referenced this issue Sep 7, 2023
### Motivation

While investigating
apple/swift-openapi-generator#251 I noticed we
don't have a test case for an escaped path component.

### Modifications

Added an extra test case.

### Result

Unescaping path params has better test coverage.

### Test Plan

This whole PR is about improving the tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/transport Affects: Client or server transport library. kind/bug Feature doesn't work as expected. size/S Small task. (A couple of hours of work.)
Projects
None yet
2 participants