[MAGNOLIA-8679] Avoid duplication of headers Created: 23/Mar/18 Updated: 17/Jan/23 Resolved: 17/Jan/23 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | None |
| Affects Version/s: | 6.2.27 |
| Fix Version/s: | 6.3.0, 6.2.28 |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Mercedes Iruela | Assignee: | Canh Nguyen |
| Resolution: | Fixed | Votes: | 3 |
| Labels: | maintenance | ||
| Σ Remaining Estimate: | Not Specified | Remaining Estimate: | Not Specified |
| Σ Time Spent: | 2d 5h | Time Spent: | 2d 5h |
| Σ Original Estimate: | Not Specified | Original Estimate: | Not Specified |
| Attachments: |
|
|||||||||||||||||||||||||
| Issue Links: |
|
|||||||||||||||||||||||||
| Sub-Tasks: |
|
|||||||||||||||||||||||||
| Template: | ||||||||||||||||||||||||||
| Acceptance criteria: |
Empty
|
|||||||||||||||||||||||||
| Task DoD: |
[X]*
Doc/release notes changes? Comment present?
[X]*
Downstream builds green?
[X]*
Solution information and context easily available?
[X]*
Tests
[X]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
|||||||||||||||||||||||||
| Bug DoR: |
[X]*
Steps to reproduce, expected, and actual results filled
[X]*
Affected version filled
|
|||||||||||||||||||||||||
| Date of First Response: | ||||||||||||||||||||||||||
| Epic Link: | Support | |||||||||||||||||||||||||
| Sprint: | DevX 28 | |||||||||||||||||||||||||
| Story Points: | 2 | |||||||||||||||||||||||||
| Team: | ||||||||||||||||||||||||||
| Work Started: | ||||||||||||||||||||||||||
| Description |
|
When there are multiple requests, headers are duplicated in info.magnolia.cms.filters.AddHeadersFilter because org.apache.http.HttpMessage.addHeader(String, String) is used to add headers. Another case is info.magnolia.enterprise.registration.RegistrationFilter that includes "X-Magnolia-Registration", after that, here, the header is duplicated due to info.magnolia.module.cache.filter.CacheResponseWrapper.replayHeadersAndStatus(HttpServletResponse target), here info.magnolia.cms.util.RequestHeaderUtil.setHeader(HttpServletResponse, String, Object) where the header is added when it was already added to the request. An easy way to reproduce it is installing Sitemesh, after installing this module, after request will have duplicate headers Although, the protocol defines: Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded. It may be a good practice to check if a header already exists with the same value. There is a utility class to work with headers that might help: info.magnolia.cms.util.RequestHeaderUtil Workaround: |
| Comments |
| Comment by Quach Hao Thien [ 29/Nov/22 ] |
DiscoveryStack trace: appendHeader:353, CacheResponseWrapper (info.magnolia.module.cache.filter) addHeader:324, CacheResponseWrapper (info.magnolia.module.cache.filter) addHeader:132, GZipFilter$GZipCacheResponseWrapper (info.magnolia.module.cache.filter) addHeader:174, HttpServletResponseWrapper (javax.servlet.http) addHeader:202, HttpServletResponseBuffer (org.sitemesh.webapp.contentfilter) addHeader:174, HttpServletResponseWrapper (javax.servlet.http) doFilter:55, AddHeadersFilter (info.magnolia.cms.filters) Magnolia is using Cache Core module to cache for the server Response, and so are the headers of response, in CacheResponseWrapper. The headers is declared as a MultiValueMap (see headers), which accepts a key could hold multiple values, When the appendHeader(String, String) is called, hence the headers.put(key, value) will blindly put the key-value to a MultiValueMap no matter the value is already existing, see The duplicating value in header is what we are going to avoid. Propose solution:Check for duplicating value before put to the MultiValueMap |
| Comment by Roman Kovařík [ 29/Nov/22 ] |
The behaviour of the cache wrapper seems to be correct for #addHeader. The behaviour you described would be correct for #putHeader. I see the possible issues in AddHeadersFilter which can't be configured to put instead of add headers. Also the ticket mentions couple of different filters (addHeaders, registration, sitemesh) which should be split in multiple tickets if needed so the tickets can be estimated accordingly. The tickets created from support could describe one scenario at time with clear steps to reproduce e.g. :
|
| Comment by Roman Kovařík [ 30/Nov/22 ] |
|
For the record: |
| Comment by Quach Hao Thien [ 30/Nov/22 ] |
Discovery (Editited)Steps to reproduce:
Expected resultsHeaders are not duplicated. Actual resultsHeaders are duplicated. Stack trace from debugging:appendHeader:353, CacheResponseWrapper (info.magnolia.module.cache.filter) addHeader:324, CacheResponseWrapper (info.magnolia.module.cache.filter) addHeader:132, GZipFilter$GZipCacheResponseWrapper (info.magnolia.module.cache.filter) addHeader:174, HttpServletResponseWrapper (javax.servlet.http) addHeader:202, HttpServletResponseBuffer (org.sitemesh.webapp.contentfilter) addHeader:174, HttpServletResponseWrapper (javax.servlet.http) doFilter:55, AddHeadersFilter (info.magnolia.cms.filters) Magnolia is using Cache module to cache for the server Response, and so are the headers of response, in CacheResponseWrapper. The headers is declared as a MultiValueMap (see headers), which accepts a key could hold multiple values, When the response.addHeader() is called in the filter, the header will be put via headers.put(key, value) will blindly put the key-value to a MultiValueMap no matter the value is already existing, see The duplicating value in header is what we are going to avoid. Propose solution:Check for duplicating value before put to the MultiValueMap This ticket should belong to MGNLCACHE |
| Comment by Jaroslav Simak [ 12/Dec/22 ] |
|
To Discover: |
| Comment by Canh Nguyen [ 13/Dec/22 ] |
|
We can know if a request is forwarded by checking request.getDispatcherType().name()
A forwarded request could not be filtered by the current filter because it could be forwarded before reaching the current filter. Override the header could be the right way to do. |
| Comment by Roman Kovařík [ 13/Dec/22 ] |
|
thien.quach Don't you think the latest comments side tract from the actual problem? Do I get it right that the problem is:
But of course
|
| Comment by Canh Nguyen [ 14/Dec/22 ] |
|
Headers can have multiple pairs with same key name but different values. AddHeadersFilter should always add new headers, so we could make a new class like SetHeadersFilter if users need to set a single header instead of adding multiple values. We could also make a MergeHeadersFilter to merge some configured headers. If the headers have unwanted duplicated values, then it would be config errors, not the code. AddHeadersFilter should check if a request is forwarded and if the request is already added headers, to do proper actions for the request. |
| Comment by Quach Hao Thien [ 14/Dec/22 ] |
We could check the value if it's already there in the map before put to the MultiValueMap , not so clever but it could help: if (!((MultiValueMap<String, Object>) headers).containsValue(key, value)) { headers.put(key, value); }
The multivaluemap accepts duplicating in value, hence a: [a, a] would be considered as 2 differences.
yes, the AddHeaderFilter allows to add value to the same header, and as you said the
But introduce a new filter would not fixed the issue of current filter. re: SetHeadersFilter - as I understand you want to use setHeader instead of addHeader, this may override the current header with new values, but there would also an usecase where user may want to add new value to existing header. re: MergeHeadersFilter - as I understand you want to merge the existing header's values with the new one, you may need to check for duplication before merging |
| Comment by Canh Nguyen [ 14/Dec/22 ] |
|
I've just had a quick check followed by the steps to reproduce with below code:
Result:
We can see that TEST headers are not duplicated. (X-Magnolia-Registration: Registered-demo is not added by AddHeadersFilter is still duplicated) |
| Comment by Quach Hao Thien [ 14/Dec/22 ] |
|
canh.nguyen in the case of checking duplication for every filter, checking the cache would be more efficiently |
| Comment by Canh Nguyen [ 14/Dec/22 ] |
|
I think there is still more a config problem. Why the request is forwarded after a series of filters that modified the response? IMO the forward event should happen before any changes (except sometime changes could be required for some filters to work and forward the request). I believe that with the correct filter chain, this issue should not be happened. |
| Comment by Roman Kovařík [ 14/Dec/22 ] |
|
Could you try to disable the cache filter to see if there is no wrong behavior of the cache as thien.quach suggested? You can extend ~OncePerRequestFilter or what's the name so the request is executed only once per request. That filter is already implementing your custom check canh.nguyen . |
| Comment by Chuong Doan Huy [ 22/Dec/22 ] |
|
My findings :
|
| Comment by Roman Kovařík [ 22/Dec/22 ] |
The enabled mechanism is implemented at filter chain level, so should work for any filter. Any chance you modified a different (author vs. public instance)?
Exactly, simemesh forwards the request to render the personalized component on the homepage.
Perfect summary, works for me |
| Comment by Canh Nguyen [ 05/Jan/23 ] |
|
IMO the suggestion to fix the issue in this ticket does not solve the issue completely (fixing in the cache module). Because if we remove the cache module then the issue still exists, or AddHeaderFilter is added after the cache filter. WDYT if we fix this issue in all filters that add headers to the request response? Checking if the request is forwarded or the headers need to be added are already exist? |