twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.28k stars 409 forks source link

Allow relative URI in Location response header #524

Closed vip4f closed 4 years ago

vip4f commented 4 years ago

Allow relative URL in Location response header. Actually please roll back changes from v18.7.0 release: "finatra-http: (BREAKING API CHANGE) Fix HttpResponseFilter to properly respect URI schema during location header overwritingPHAB_ID=D191448"

This article stated that HTTP standard has been relaxed and actually allows relative URL in Location header: https://en.wikipedia.org/wiki/HTTP_location

Expected behavior

Relative URL in Location header should not be extended with host:port which might be inaccurately discovered.

Actual behavior

Any relative URL in Location header is automatically converted to absolute URL. The resulting absolute URL can be totally misleading because sometimes application cannot determine its absolute URL if it works behind proxy.

Steps to reproduce the behavior

private val defaultResponse = { val response = Response(Version.Http11, Status.SeeOther) response.location = "/help" response.contentType = "text/plain" response.contentString = "" response } get("/") ((_:Request) => defaultResponse)

The actual response on the wire is expected to have: HTTP/1.1 303 See Other Location: /help

ryanoneill commented 4 years ago

Hi @vi-p4f, thanks for reporting this. We'll look into it and report back.

ryanoneill commented 4 years ago

Hi @vi-p4f, I wanted to let you know that I did look into this. This code was changed in response to an issue raised by an internal team. I'm attempting to understand the back story in order to know what prompted the behavior change. You're correct that a relative should be fine here.

cacoco commented 4 years ago

@vi-p4f just to update, we are testing a change to allow relative references again in Location response header values. We hope to have this in time for our May release. Thanks!

cacoco commented 4 years ago

Changes merged in b6453a4d0b047e965b5d43f319d028739b80d5d3. Should be in the 20.5.0 release happening shortly.

cacoco commented 4 years ago

Released in 20.5.0.