[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: |
|
||||||||||||
| Issue Links: |
|
||||||||||||
| 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 |
| 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) |
| 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
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. Cheers, Rico for the Magnolia Team |
| Comment by Federico Grilli [ 23/Aug/21 ] |
|
I ended up creating |