[MGNLCE-262] CsrfTokenSecurityFilter does not encode cookie path Created: 08/Feb/21  Updated: 23/Aug/21  Resolved: 19/Aug/21

Status: Closed
Project: Community Edition
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Neutral
Reporter: Samuli Saarinen Assignee: Unassigned
Resolution: Duplicate Votes: 2
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File encode_cookie_path.patch    
Issue Links:
duplicate
duplicates MAGNOLIA-7991 Invalid path for cookie with special ... Closed
duplicates MAGNOLIA-8142 Non ASCII characters in URIs interfer... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Steps to reproduce

  1.  create page with non-ascii chars in path e.g. ä
  2. Try to access page (with out being logged in to magnolia) 

Expected results

Page is visible

Actual results

HTTP Status 500 – Internal Server Error


Type Exception Report

Message An invalid path [/testiä] was specified for this cookie

Description The server encountered an unexpected condition that prevented it from fulfilling the request.

Exception

java.lang.IllegalArgumentException: An invalid path [/testiä] was specified for this cookie org.apache.tomcat.util.http.Rfc6265CookieProcessor.validatePath(Rfc6265CookieProcessor.java:241) org.apache.tomcat.util.http.Rfc6265CookieProcessor.generateHeader(Rfc6265CookieProcessor.java:160) org.apache.catalina.connector.Response.generateCookieString(Response.java:975) org.apache.catalina.connector.Response.addCookie(Response.java:927) org.apache.catalina.connector.ResponseFacade.addCookie(ResponseFacade.java:385) javax.servlet.http.HttpServletResponseWrapper.addCookie(HttpServletResponseWrapper.java:60) info.magnolia.cms.security.CsrfTokenSecurityFilter.unloggedRequestCheckPasses(CsrfTokenSecurityFilter.java:171) info.magnolia.cms.security.CsrfTokenSecurityFilter.csrfCheckPasses(CsrfTokenSecurityFilter.java:116) info.magnolia.cms.security.CsrfTokenSecurityFilter.doFilter(CsrfTokenSecurityFilter.java:106)

Workaround

Development notes

Magnolia CE 6.2.5, Tomcat 9.0.41



 Comments   
Comment by Samuli Saarinen [ 11/Feb/21 ]

Ended up patching the filter like so to fix the issue for us. (Magnolia 6.2.5)

encode_cookie_path.patch

Comment by Jonathan Ayala [ 11/Feb/21 ]

Hi Samuli,

We've been trying to apply your patch but we are getting the following error:

error: patch fragment without header at line 3: @@ -35,6 +35,7 @@

Could you confirm if the patch file is woking ok in your environment?

Comment by Samuli Saarinen [ 11/Feb/21 ]

Hi, could you just try to manually add the changes. It's like 5 changed lines and a couple of imports. Or I'll try to come up with working patch tomorrow.

Comment by Jonathan Ayala [ 12/Feb/21 ]

Hi Samuli,

ok, just to confirm there's nothing missing, it would be replacing line:

168. cookie.setPath(request.getServletPath()); 

with the new encoding operation:

// PATCH: encode cookie path
cookie.setPath(encodePath(request.getServletPath()));
// END PATCH

...

private String encodePath(String path) {
 return new URI(path).toASCIIString();
 }
Comment by Samuli Saarinen [ 12/Feb/21 ]

yes that is correct. It's essentially encoding the cookie path (page with path like `/mypage/ä-test` it gets encoded to `/mypage/%C3%A4-test`). What comes to encoding the path feel free to use what ever machinery Magnolia might already have in the matter. Using URI was just what I ended up using as

  1. it came up on the subject on SO
  2. did not want to use external libraries (e.g. Spring has some util to handle it as well)
  3. I'm not that well informed if Magnolia already does that (has similar util to Spring)

Also I'm wondering if you could just use  `/` as the path (root-path) but it might have some security implications.

Comment by Samuli Saarinen [ 08/Mar/21 ]

I just found out that my fix does not work if request.getServletPath() contains spaces because then URI(path) throws following:

Exception in thread "main" java.net.URISyntaxException: Illegal character in path at index 51: /dam/jcr:d0836006-76be-458d-baa0-fd03ee061e57/image name with spaces.jpg
	at java.base/java.net.URI$Parser.fail(URI.java:2913)
	at java.base/java.net.URI$Parser.checkChars(URI.java:3084)
	at java.base/java.net.URI$Parser.parseHierarchical(URI.java:3166)
	at java.base/java.net.URI$Parser.parse(URI.java:3125)
	at java.base/java.net.URI.<init>(URI.java:600)
	at Scratch.main(Scratch.java:14)

Again I could strip the filename from the path (maybe I should) but there could be spaces in the path I guess and maybe some other Illegal characters as well.

 

 

Comment by Samuli Saarinen [ 10/Aug/21 ]

Sorry for the delay on getting back at this but related to the above exception that is thrown if servletPath contains spaces.

It can be resolved using URI constructor as follows:

return new URI(null, null, path, null).toASCIIString(); // use that instead of new URI(path)...
Comment by Federico Grilli [ 19/Aug/21 ]

Hi ssaarinen, thank you vey much for your report and investigation! For some reason this ticket didn't make it to the maintenance backlog and a new one was tackled instead.
After some trials and errors luckily caught by integration tests I ended up using what you suggest but did not account for the possibility that sometimes request.getServletPath() can have spaces. I'll fix it and add a better test. 

Cheers,

Rico for the Magnolia Team

Comment by Federico Grilli [ 23/Aug/21 ]

I ended up creating MAGNOLIA-8162 

Generated at Mon Feb 12 00:07:42 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.