-
Notifications
You must be signed in to change notification settings - Fork 122
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
Allowing disabling percent encoding for some HTTP header fields #529
Comments
Hi @zunda-pixel, just to confirm, you're writing a server here, right? You can return a completely arbitrary URL in the response's Location header, can you clarify why you say you can't redirect to a different domain? |
Yes, I am writing a server. Base url is keeping...
|
Hmm, |
This is sample repository that has issue I told. Please check. openapi: '3.1.0'
info:
title: LoginService
version: 1.0.0
servers:
- url: https://example.com/
description: Example service deployment.
paths:
/login:
get:
operationId: login
responses:
'303':
description: A success response Login
headers:
location:
schema:
type: string import OpenAPIRuntime
import OpenAPIVapor
import Vapor
struct Handler: APIProtocol {
func login(_ input: Operations.login.Input) async throws -> Operations.login.Output {
return .seeOther(.init(headers: .init(location: "https://apple.com")))
}
}
@main struct LoginServer {
static func main() async throws {
let app = Vapor.Application()
let transport = VaporTransport(routesBuilder: app)
let handler = Handler()
try handler.registerHandlers(on: transport)
try await app.execute()
}
} |
Can you clarify what the issue is? The code in the project all looks correct. What are the steps you're taking, what is the result you see, and what is the result you expect? That'll help us understand where the mismatch is. |
Current ResultExpecting Result |
Which HTTP client are you using? A web browser? curl? |
I use a web browser. |
Thank you @zunda-pixel, I was able to isolate the issue. The problem is that OpenAPI-defined headers are serialized according to the rules of RFC6570 (details here), which dictate that non-reserved characters need to be percent encoded. However, in the As a workaround, add a middleware that removes the percent encoding of the Location header: import OpenAPIRuntime
import OpenAPIVapor
import Vapor
import HTTPTypes
struct Handler: APIProtocol {
func login(_ input: Operations.login.Input) async throws -> Operations.login.Output {
return .seeOther(.init(headers: .init(location: "https://apple.com")))
}
}
@main struct LoginServer {
static func main() async throws {
let app = Vapor.Application()
let transport = VaporTransport(routesBuilder: app)
let handler = Handler()
try handler.registerHandlers(on: transport, middlewares: [
UnescapeLocationHeaderMiddleware()
])
try await app.execute()
}
}
struct UnescapeLocationHeaderMiddleware: ServerMiddleware {
func intercept(
_ request: HTTPRequest,
body: HTTPBody?,
metadata: ServerRequestMetadata,
operationID: String,
next: (HTTPRequest, HTTPBody?, ServerRequestMetadata) async throws -> (HTTPResponse, HTTPBody?)
) async throws -> (HTTPResponse, HTTPBody?) {
var (response, responseBody) = try await next(request, body, metadata)
guard let location = response.headerFields[.location] else {
return (response, responseBody)
}
response.headerFields[.location] = location.removingPercentEncoding
return (response, responseBody)
}
} Now, thanks for reporting this. It's a bit troubling, and I suspect we'll need some way to make this more compatible with clients that don't percent-decode header fields. If you don't mind, I'll repurpose this issue to track improving Swift OpenAPI Generator this way and rename it. |
Thank you for looking into this issue, and providing a workaround. |
👋 Is there any progress on this issue? We just started using In case it helps anyone else, here's the middleware we're using at the moment (to fix all the headers at once): private struct HeaderEncodingMiddleware: ClientMiddleware {
func intercept(
_ request: HTTPRequest, body: HTTPBody?, baseURL: URL, operationID: String,
next: (HTTPRequest, HTTPBody?, URL) async throws -> (HTTPResponse, HTTPBody?)
) async throws -> (HTTPResponse, HTTPBody?) {
var modifiedRequest = request
modifiedRequest.headerFields = HTTPFields(
request.headerFields.map { field in
HTTPField(name: field.name, value: field.value.removingPercentEncoding!)
})
return try await next(modifiedRequest, body, baseURL)
}
} |
percent-encoding of special characters in header fields is prescribed by OpenAPI, see this comment for details and links to the RFC: #529 (comment) This issue tracks some possible ways to opt out of the behavior, because some services write their OpenAPI docs after their initial implementation, and they don't correctly remove percent encoding. That's a bug on any service that does that, but it's common enough that we're open to helping our users handle that situation better to interop with such legacy services. If you'd like to see progress here, please propose a solution that we can discuss (how exactly should the opt out work?), and maybe you could ever open a PR for it later 🙂 |
@czechboy0 Thanks for following up and linking that doc. I stand corrected; however, we currently have no plans to change our server or our other clients to (unnecessarily IMHO) URL-encode header params. I would suggest adding a configuration flag that can be passed when initializing the generated Client, e.g. let client = Client(serverURL: foo, configuration: .init(urlEncodeHeaderParams: false), transport: bar) |
Our goal is to conform to the OpenAPI specification. For non-compliant implementations, there's the middleware solution from above. If we support some sort of opt-out, it's likely that it'd be specific to certain headers that historically haven't used percent encoding. But for any new headers, a compliant OpenAPI implementation should include percent encoding. Unilaterally diverging from the specification would break our compatibility with other OpenAPI tools. |
Question
I want to redirect to
domain2.com
fromdomain1.com
.I can redirect same domain.
ex: from
domain1.com/login
todomain1.com/account
.The text was updated successfully, but these errors were encountered: