snoopdave commented on code in PR #248:
URL: https://github.com/apache/roller/pull/248#discussion_r4571035520


##########
app/src/main/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilter.java:
##########
@@ -32,11 +32,17 @@
  */
 public class LoadSaltFilter implements Filter {
 
+    static final String RENDERING_PATH = "/roller-ui/rendering/";
+
     @Override
     public void doFilter(ServletRequest request, ServletResponse response, 
FilterChain chain)
         throws IOException, ServletException {
 
         HttpServletRequest httpReq = (HttpServletRequest) request;
+        if (isRenderingRequest(httpReq)) {

Review Comment:
   🐞Claude Issue: **Important:** The change fixes the cookie, but I could not 
tell from the diff or tests what it does to a visitor's comment POST, and it 
should be checked and covered.
   
   `ValidateSaltFilter` is mapped to `/roller-ui/*` (REQUEST), so a POST to 
`/roller-ui/rendering/comment/*` goes through 
`SaltValidator.consumeSubmittedSalt`. That method returns true when 
`RollerSession.getRollerSession()` is null, which is the case when the request 
has no session (`getSession(false)`). Otherwise it needs a `salt` parameter. I 
found no `salt` reference in `WEB-INF/velocity`, `themes/` or the rendering 
Java, so the public comment form submits none.
   
   Before this PR, a visitor who viewed a page first had a session. The comment 
POST then looked like it would fail with "Security Violation". After this PR 
the same visitor has no session and the POST passes. A signed-in Roller user 
viewing a public page still has a session, so I'd expect that user's comment to 
be rejected.
   
   I did not run this, so please confirm it. Please check these against the 
built WAR and add a test that covers the comment POST:
   
   1. an anonymous visitor views a post, then posts a comment (expect: accepted)
   2. a signed-in user does the same (expect: decide what should happen and say 
so in the PR)
   
   If case 2 fails, it is an existing bug and belongs in a separate PR. In that 
case, note it in this PR so it is not mistaken for a regression.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to