Allow stripping query parameters in the reverse proxy #1624
Conversation
|
CS issue unrelated, master is failing @leofeyer :) |
Toflar
force-pushed
the
feature/strip-query-params-subscriber
branch
from
28dde96
to
2c0ad69
Compare
| @@ -82,7 +94,7 @@ public function preHandle(CacheEvent $event): void | |||
| if (0 !== \count($this->whitelist)) { | |||
| $this->filterCookies($request, $this->whitelist, true); | |||
| } else { | |||
| $this->filterCookies($request, self::BLACKLIST); | |||
| $this->filterCookies($request, array_diff(self::BLACKLIST, $this->disabledFromBlacklist)); | |||
There was a problem hiding this comment.
Using array_diff here could lead to unexpected results I think. If someone wants to allow the _utma cookie and calls disableFromBlacklist(['_utma']) the cookie would still get stripped because the regex for this cookie is __utm.+.
I think we should extend the filterCookies() method and remove every name from $removeCookies that matches the disableFromBlacklist array.
There was a problem hiding this comment.
I actually wanted to explicitly disallow this use case. If you want to remove something from the blacklist, specify it correctly. You can still use the whitelist if you want to have full control.
There was a problem hiding this comment.
But what if we change one of the regexes in the blacklist in a future update of Contao? The disableFromBlacklist() would no longer work than for that regex and this would be BC break.
There was a problem hiding this comment.
I thought about that, would be easy to solve with an alias in the disableFromBlacklist() method 😉
There was a problem hiding this comment.
But what should I do then if I want to remove just the _utma (not all _utm*) cookie from the blacklist?
There was a problem hiding this comment.
That's not a use case imho. Either you need all of the _utm query parameters or none.
But you're free trying to optimize this in a later version.
There was a problem hiding this comment.
I could also think of something like a bimodal_transport cookie which is currently blacklisted by (.*)?modal(.*)?. But you are right, I will provide a separate pull request for this 👍
|
Thank you @Toflar. |
|
we should probably blacklist the very common UTM parameters as well? |
|
What do you mean? The regex |
|
🤦 haven't seen that |
Description ----------- As discussed in #1624 (comment) This pull request probably needs a reabase once #1798 got resolved. Commits ------- ba8a8f8 Improve disabled cookie deny list 52ca008 Improve disabled query parameter deny list 94178bc Merge branch 'master' into feature/improve-disable-deny-list
Description ----------- As discussed in contao/contao#1624 (comment) This pull request probably needs a reabase once #1798 got resolved. Commits ------- ba8a8f8c Improve disabled cookie deny list 52ca0082 Improve disabled query parameter deny list 94178bcc Merge branch 'master' into feature/improve-disable-deny-list
leofeyer
changed the title
Description ----------- | Q | A | -----------------| --- | Fixed issues | - | Docs PR or issue | TODO once merged This PR will allow to strip query parameters that are not required for the application but only for e.g. some tracking tools in the front end. It increases the cache hit rate and prevents these parameters from flooding the cache. While implementing this I thought there's also the use case where people trust the Contao developers to maintain a proper blacklist but only need one exception to that. Like e.g. you need the `utm_*` parameters in PHP but you don't want to whitelist all of the allowed query parameters. Same might apply for cookies so I unified that :) Commits ------- 2c0ad69 Allow to strip query parameters in reverse proxy 1d957d0 Merge branch 'master' into feature/strip-query-params-subscriber
Description ----------- As discussed in contao#1624 (comment) This pull request probably needs a reabase once contao#1798 got resolved. Commits ------- ba8a8f8 Improve disabled cookie deny list 52ca008 Improve disabled query parameter deny list 94178bc Merge branch 'master' into feature/improve-disable-deny-list
This PR will allow to strip query parameters that are not required for the application but only for e.g. some tracking tools in the front end. It increases the cache hit rate and prevents these parameters from flooding the cache.
While implementing this I thought there's also the use case where people trust the Contao developers to maintain a proper blacklist but only need one exception to that. Like e.g. you need the
utm_*parameters in PHP but you don't want to whitelist all of the allowed query parameters.Same might apply for cookies so I unified that :)