Add option to retain request method on 301/302/303 redirects#887
Add option to retain request method on 301/302/303 redirects#887hamzahrmalik wants to merge 9 commits intoswift-server:mainfrom
Conversation
| /// - Parameters: | ||
| /// - max: The maximum number of allowed redirects. | ||
| /// - allowCycles: Whether cycles are allowed. | ||
| /// - convertToGet: Whether to convert POST requests into GET requests when following a 301, 302 or 303 redirect. This does not apply to 307 or 308. Those always retain the original method. |
There was a problem hiding this comment.
I think we should call out that this should be true except, if users have clear needs.
| /// Redirects are followed with a specified limit. | ||
| /// | ||
| /// - parameters | ||
| /// | ||
| /// - warning: Cycle detection will keep all visited URLs in memory which means a malicious server could use this as a denial-of-service vector. |
There was a problem hiding this comment.
i copied it from the existing one above
fabianfett
left a comment
fabianfett
left a comment
There was a problem hiding this comment.
Love the tests. Let the naming bike-shed begin.
| /// Whether to convert POST requests into GET requests when following a 301, 302 or 303 redirect. | ||
| /// This does not apply to 307 or 308. Those always retain the original method. | ||
| public var convertToGet: Bool |
There was a problem hiding this comment.
I like that curls specifically calls out 301, 302 and 303. I wonder if we should do the same here? convertToGet sounds too broad for me.
| } | ||
|
|
||
| /// Configuration for following redirects. | ||
| public struct FollowConfiguration: Hashable, Sendable { |
There was a problem hiding this comment.
I wonder if it is worth adding this new struct now? If I see this correctly, we don't pass it around at all. Lets just add another property on the enum for now.
There was a problem hiding this comment.
given that we're doing 3 properties now, imo we should keep the struct. We can actually take advantage of it by holding the struct inside the RedirectState
There was a problem hiding this comment.
see last commit
| /// - `retainHTTPMethodAndBodyOn301` (bool, optional, default: false): Retain the original HTTP method and body on 301 redirect when mode is "follow". This is contrary to the fetch specification, but is allowed by RFC 9110. | ||
| /// - `retainHTTPMethodAndBodyOn302` (bool, optional, default: false): Retain the original HTTP method and body on 302 redirect when mode is "follow". This is contrary to the fetch specification, but is allowed by RFC 9110. |
There was a problem hiding this comment.
Lets call out POST here explicitly. Because this is just about POST.
fabianfett
left a comment
fabianfett
left a comment
There was a problem hiding this comment.
This looks amazing! Thanks!
2 participants
Add a configuration option to retain the HTTP method and body receiving 301 or 302 responses.
Currently we automatically change the method to GET, and remove the body, before following a 301 or 302. This is compliant with the fetch specification: https://fetch.spec.whatwg.org/#http-redirect-fetch
However, it is useful to be able to override this behaviour and retain the method and body.
Changes
retainHTTPMethodAndBodyOn301andretainHTTPMethodAndBodyOn302. Defaults to false because thats the existing behaviour today