[vcl] don't explicitly hash the host header #28928
Conversation
Hashing `req.http.host`/`client.ip` is already handled by the [built-in vcl](https://github.com/varnishcache/varnish-cache/blob/6.0/bin/varnishd/builtin.vcl#L86) so there's no need to repeat it explicitly. It's also a bit confusing as `req.url` is not explicitly handled, even though it's a more important hash input than the host. note: all versions have been changed for the sake of consistency but both the 4.x and 5.x series have been EOL'd a (long) while ago and users should be encouraged to upgraded as soon as possible.
|
Hi @gquintard. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. |
|
@magento run all tests |
|
Hi @lbajsarowicz, thank you for the review.
|
|
@gquintard thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository. |
|
@magento run all tests |
|
Automated Tests are not applicable, as the change is related to infrastructure configuration. |
|
@magento create issue |
|
Dev experience is required for testing of this PR. Please note that Manual testing has not been performed. |
|
✔️ QA Passed Hashing is already handled by the built-in vcl starting from varnish 4.0. |
|
Hi @gquintard, thank you for your contribution! |
Conditional hashing can lead to collisions and should be avoided. As an
example, this code:
``` vcl
sub vcl_hash {
if (req.http.a) {
hash_data(req.http.a);
}
if (req.http.b) {
hash_data(req.http.b);
}
}
```
will return the same hash for these two requests:
```
GET / HTTP/1.1
a: foo
```
and
```
GET / HTTP/1.1
b: foo
```
whereas
``` vcl
sub vcl_hash {
hash_data(req.http.a);
hash_data(req.http.b);
}
```
is correct and simpler.
Hashing
req.http.host/client.ipis already handled by the built-invcl
so there's no need to repeat it explicitly.
It's also a bit confusing as
req.urlis not explicitly handled, eventhough it's a more important hash input than the host.
note: all versions have been changed for the sake of consistency
but both the 4.x and 5.x series have been EOL'd a (long) while ago and users
should be encouraged to upgraded as soon as possible.
Contribution checklist (*)
Resolved issues: