[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: XML File config.server.filters.addTestHeaders.xml     PNG File image-2022-11-09-15-50-07-578.png     PNG File image-2022-11-09-15-55-55-799.png     PNG File screenshot-1.png     PNG File screenshot-2.png    
Issue Links:
Cloners
is cloned by MGNLEE-743 Avoid duplication of headers Closed
causality
dependency
relation
Sub-Tasks:
Key
Summary
Type
Status
Assignee
MAGNOLIA-8680 Implement Sub-task Completed Canh Nguyen  
MAGNOLIA-8681 Review Sub-task Completed Jaroslav Simak  
MAGNOLIA-8682 piQA Sub-task Completed Jaroslav Simak  
MAGNOLIA-8683 QA Sub-task Completed Jaroslav Simak  
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: DeveloperX
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:
Adding a bypass to the filter -> info.magnolia.voting.voters.ForwardVoter



 Comments   
Comment by Quach Hao Thien [ 29/Nov/22 ]

Discovery

Stack 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 ]

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 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. :

  • MGNLEE ticket: curl -I <URL> // you see the XX header is duplicated
  • MAGNOLIA ticket: configure X=Y to AddHeadersFilter (link to config in an instance), curl -I <URL> // you see the XX header is duplicated
  • SITEMESH ticket: install sitemesh module (link to git/docu/maven), curl -I <URL> // you see the XX header is duplicated
  • ...
Comment by Roman Kovařík [ 30/Nov/22 ]

For the record:
 
thienqh  8:28
but when I debug, all the issue endup can be fixed in this line https://git.magnolia-cms.com/projects/MODULES/repos/cache/browse/magnolia-cache-core/sr[…]nfo/magnolia/module/cache/filter/CacheResponseWrapper.java
8:30
before we call headers.put(...) we only need to check if the value's already there in the map, to avoid the duplication
 
roman  8:31 AM
ok, I probably get it now. Could you move the ticket to the cache module and change the description with the bug template (steps/behaviour) ?

Comment by Quach Hao Thien [ 30/Nov/22 ]

Discovery (Editited)

Steps to reproduce:

  1. Go to https://demoauthor.magnolia-cms.com/ and login.
  2. Open Configuration -> server -> filters
  3. Add a new filter using info.magnolia.cms.filters.AddHeadersFilter class or just import it from the attachment config.server.filters.addTestHeaders.xml file config.server.filters.addTestHeaders.xml. (I put it after uriSecurity filter, because it is important in my application, but probably ordering doesn't matter)
  1. Open https://demoauthor.magnolia-cms.com/travel in browser (I used Chrome)
  2. Open browser dev tools and go to the Network tab
  3. Observe that the same headers added multiple times. It causes unpredictable behavior on client side in some cases.

Expected results

Headers are not duplicated.

Actual results

Headers 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:
Can we get info abou request that it's forwarded? What to do then? Rewrite Headers? Skip writing header values?

Comment by Canh Nguyen [ 13/Dec/22 ]

We can know if a request is forwarded by checking request.getDispatcherType().name()

https://stackoverflow.com/questions/40133177/how-to-determine-request-is-forwarded-or-not-in-java-filters

 

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:

  • addHeader(a, a)
  • addHeader(a, a)
    => results in a: [a, a]
    but should be still a: a since the same value is already set.

But of course

  • addHeader(a, a)
  • addHeader(a, b)
    => results in a: [a, b]
    (this should work already)
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 ]

jsimak 

Can we get info abou request that it's forwarded? What to do then? Rewrite Headers? Skip writing header values?

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);
} 

 

rkovarik 

Do I get it right that the problem is:addHeader(a, a)addHeader(a, a)
=> results in a: [a, a]
but should be still a: a since the same value is already set.

The multivaluemap accepts duplicating in value, hence a: [a, a] would be considered as 2 differences.

 

canh.nguyen 

yes, the AddHeaderFilter allows to add value to the same header, and as you said the

Headers can have multiple pairs with same key name but different values.

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 :

  • Disabling the cache filter (set enabled flag to false) does not affect the duplicated problems. Maybe the cache filter is currently not considering that flag ??? (seem like it still running even with that flag)
  • But disabling the sitemesh filter (set enabled flag to false) completely resolve the problems (both duplicated created by AddHeadersFilter and RegistrationFilter gone). This can be explained that sitemesh trigger some decoration/replay for all the filters (including AddHeadersFilter and RegistrationFilter) which doesn't do check for header duplication before adding causing duplication.
  • But eventually, the CacheResponseWrapper wrapped the request and replay headers again. So here we can apply universal duplication check as Thien mentioned, but not at this line (check at this line only works for AddHeaderFilter, "X-Magnolia-Registration" is still duplicated.
  • I think it's better to check at this line instead, this is where all the traffic go through (tested and eliminated all duplications).
  • That line reference to this method, so the check will be inside that method.
    rkovarik and thien.quach if you are ok with this, i will proceed to implement and will add you as reviewers as well ?
Comment by Roman Kovařík [ 22/Dec/22 ]

Disabling the cache filter (set enabled flag to false) does not affect the duplicated problems. Maybe the cache filter is currently not considering that flag ??? (seem like it still running even with that flag)

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)?

But disabling the sitemesh filter (set enabled flag to false) completely resolve the problems (both duplicated created by AddHeadersFilter and RegistrationFilter gone). This can be explained that sitemesh trigger some decoration/replay for all the filters (including AddHeadersFilter and RegistrationFilter) which doesn't do check for header duplication before adding causing duplication.

Exactly, simemesh forwards the request to render the personalized component on the homepage.

But eventually, the CacheResponseWrapper wrapped the request and replay headers again. So here we can apply universal duplication check if you are ok with this...

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?

Generated at Mon Feb 12 04:34:50 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.