Add Forwarded headers to the fetch request. #382
Conversation
To be cautious re: privacy/security, specify only host. Note that this changes behavior for anybody who was previously including "Forwarded" or "X-Forwarded-Host" in their ForwardedRequestHeaders in the config.
| @@ -183,6 +183,8 @@ func (this *SignerSuite) TestSimple() { | |||
| this.Assert().Equal(fakePath, this.lastRequest.URL.String()) | |||
| this.Assert().Equal(userAgent, this.lastRequest.Header.Get("User-Agent")) | |||
| this.Assert().Equal("1.1 amppkg", this.lastRequest.Header.Get("Via")) | |||
| this.Assert().Equal(`host="example.com"`, this.lastRequest.Header.Get("Forwarded")) | |||
There was a problem hiding this comment.
Could you add a test that has other parameters apart from host?
There was a problem hiding this comment.
There are no other parameters in the prod code:
amppackager/packager/signer/signer.go
Line 184 in 47085ca
I think the only thing I didn't add a test for is the concatenation if the upstream request has an XFH header:
amppackager/packager/signer/signer.go
Lines 186 to 188 in 47085ca
Is that what you want me to add a test for?
There was a problem hiding this comment.
You can add a test for that, too if you'd like. I was more looking at the comment:
// TODO(twifkak): Extract host from upstream Forwarded header
// and concatenate. (Do not include any other parameters, as
// they may lead to over-signing.)
If you can add a test where the over-signing doesn't happen.
There was a problem hiding this comment.
Ah, my mistake. Good idea. Done, PTAL.
To be cautious re: privacy/security, specify only host. Note that this
changes behavior for anybody who was previously including "Forwarded" or
"X-Forwarded-Host" in their ForwardedRequestHeaders in the config.
Fixes #260.