You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by "kusalk (via GitHub)" <gi...@apache.org> on 2023/03/01 10:06:53 UTC
[GitHub] [struts] kusalk opened a new pull request, #663: WW-5267 Add option to generate ActionContext for excluded URLs
kusalk opened a new pull request, #663:
URL: https://github.com/apache/struts/pull/663
WW-5267
--
SiteMesh can be applied to URLs that are not Struts actions, in which case we need to still ensure the ActionContext exists.
--
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: dev-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] kusalk commented on pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #663:
URL: https://github.com/apache/struts/pull/663#issuecomment-1457625729
> User is "escaping" from Struts sandbox, yet having option to operate on `ActionContext` like `ActionContext.getContext().getContainer()`.
Ah, I see what you mean. Let me rethink this problem and see if I can come up with a solution that doesn't violate `struts.action.excludePattern`. Thanks for the feedback
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #663:
URL: https://github.com/apache/struts/pull/663#issuecomment-1457617483
> > right now it would be possible to access `ActionContext` out of action, directly from JSP?
>
> Not clear on what you mean, how so?
If the flag is "on" and the request matches excluded urls, the `ActionContext` will be available in non-Struts managed endpoints. By design all the requests should be handled by the actions first and then forwarded into view layer (like JSP or Freemarker) - this also involves the whole security mechanism embedded into _normal_ flow (interceptors).
With this change it is possible to overuse this functionality by having an excluded url and still accessing `ActionContext` out of action scope directly from JSP or Freemarker. User is "escaping" from Struts sandbox, yet having option to operate on `ActionContext` like `ActionContext.getContext().getContainer()`.
This raises security concerns tbh.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121748372
##########
core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java:
##########
@@ -73,6 +73,7 @@ public void testWrappedRequestCleanup() {
});
IntStream.range(0, mockedRecursions - 1).forEach(i -> prepare.cleanupWrappedRequest(req));
+ // Assert org.apache.struts2.dispatcher.Dispatcher#cleanUpRequest has not yet run
Review Comment:
This comment was meant to clarify the intent of line 77. This unit test is testing that if prepare#wrapRequest is run X times, then prepare#cleanupWrappedRequest must also be run X times before the request is cleaned up. We do not want to clean up too early (line 77) and we don't want to forget to clean up (line 79).
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121457278
##########
core/src/test/java/org/apache/struts2/dispatcher/TwoFilterIntegrationTest.java:
##########
@@ -19,87 +19,133 @@
package org.apache.struts2.dispatcher;
import com.opensymphony.xwork2.ActionContext;
-import junit.framework.TestCase;
-import org.apache.struts2.dispatcher.Dispatcher;
-import org.apache.struts2.dispatcher.PrepareOperations;
import org.apache.struts2.dispatcher.filter.StrutsExecuteFilter;
import org.apache.struts2.dispatcher.filter.StrutsPrepareFilter;
-import org.springframework.mock.web.*;
+import org.junit.Before;
+import org.junit.Test;
+import org.springframework.mock.web.MockFilterChain;
+import org.springframework.mock.web.MockFilterConfig;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
-import javax.servlet.*;
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
import java.io.IOException;
-import java.util.LinkedList;
import java.util.Arrays;
+import java.util.LinkedList;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
/**
* Integration tests for the filter
*/
-public class TwoFilterIntegrationTest extends TestCase {
- StrutsExecuteFilter filterExecute;
- StrutsPrepareFilter filterPrepare;
- Filter failFilter;
+public class TwoFilterIntegrationTest {
+ private StrutsExecuteFilter filterExecute;
+ private StrutsPrepareFilter filterPrepare;
+ private Filter failFilter;
private Filter stringFilter;
+ @Before
public void setUp() {
filterPrepare = new StrutsPrepareFilter();
filterExecute = new StrutsExecuteFilter();
- failFilter = new Filter() {
- public void init(FilterConfig filterConfig) throws ServletException {}
- public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
- fail("Should never get here");
- }
- public void destroy() {}
- };
- stringFilter = new Filter() {
- public void init(FilterConfig filterConfig) throws ServletException {}
- public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
- response.getWriter().write("content");
- assertNotNull(ActionContext.getContext());
- assertNotNull(Dispatcher.getInstance());
- }
- public void destroy() {}
- };
+ failFilter = newFilter((req, res, chain) -> fail("Should never get here"));
+ stringFilter = newFilter((req, res, chain) -> {
+ res.getWriter().write("content");
+ assertNotNull(ActionContext.getContext());
+ assertNotNull(Dispatcher.getInstance());
+ });
}
+ @Test
public void test404() throws ServletException, IOException {
MockHttpServletResponse response = run("/foo.action", filterPrepare, filterExecute, failFilter);
assertEquals(404, response.getStatus());
}
+ @Test
public void test200() throws ServletException, IOException {
MockHttpServletResponse response = run("/hello.action", filterPrepare, filterExecute, failFilter);
assertEquals(200, response.getStatus());
}
+ @Test
public void testStaticFallthrough() throws ServletException, IOException {
MockHttpServletResponse response = run("/foo.txt", filterPrepare, filterExecute, stringFilter);
assertEquals(200, response.getStatus());
assertEquals("content", response.getContentAsString());
}
+ @Test
public void testStaticExecute() throws ServletException, IOException {
MockHttpServletResponse response = run("/static/utils.js", filterPrepare, filterExecute, failFilter);
assertEquals(200, response.getStatus());
assertTrue(response.getContentAsString().contains("StrutsUtils"));
}
+ @Test
public void testFilterInMiddle() throws ServletException, IOException {
- Filter middle = new Filter() {
- public void init(FilterConfig filterConfig) throws ServletException {}
- public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
- assertNotNull(ActionContext.getContext());
- assertNotNull(Dispatcher.getInstance());
- assertNull(ActionContext.getContext().getActionInvocation());
- chain.doFilter(request, response);
- assertEquals("hello", ActionContext.getContext().getActionInvocation().getProxy().getActionName());
- }
- public void destroy() {}
- };
+ Filter middle = newFilter((req, res, chain) -> {
+ assertNotNull(ActionContext.getContext());
+ assertNotNull(Dispatcher.getInstance());
+ assertNull(ActionContext.getContext().getActionInvocation());
+ chain.doFilter(req, res);
+ assertEquals("hello", ActionContext.getContext().getActionInvocation().getProxy().getActionName());
+ });
MockHttpServletResponse response = run("/hello.action", filterPrepare, middle, filterExecute, failFilter);
assertEquals(200, response.getStatus());
}
+ /**
+ * It is possible for a Struts excluded URL to be forwarded to a Struts URL. If this happens, the ActionContext
+ * should not be cleared until the very first execution of the StrutsPrepareFilter, otherwise SiteMesh will malfunction.
+ */
+ @Test
+ public void testActionContextNotClearedUntilEndWhenForwardedFromExcludedUrl() throws ServletException, IOException {
Review Comment:
This integration test is actually for [WW-5270](https://issues.apache.org/jira/browse/WW-5270). (We already have acceptance test coverage for that fix.)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121748372
##########
core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java:
##########
@@ -73,6 +73,7 @@ public void testWrappedRequestCleanup() {
});
IntStream.range(0, mockedRecursions - 1).forEach(i -> prepare.cleanupWrappedRequest(req));
+ // Assert org.apache.struts2.dispatcher.Dispatcher#cleanUpRequest has not yet run
Review Comment:
This comment was meant to clarify the intent of line 77. This unit test is testing that if prepare#wrapRequest is run X times, then prepare#cleanupWrappedRequest must also be run X times before the request is cleaned up. We do not want to clean up too early (line 77) and we don't want to forget to clean up (line 79). This scenario only occurs when requests are forwarded and the StrutsPrepareFilter executes recursively.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #663:
URL: https://github.com/apache/struts/pull/663#issuecomment-1450178068
> right now it would be possible to access `ActionContext` out of action, directly from JSP?
Not clear on what you mean, how so?
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk closed pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk closed pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
URL: https://github.com/apache/struts/pull/663
--
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: dev-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121470998
##########
core/src/main/java/org/apache/struts2/dispatcher/filter/StrutsPrepareFilter.java:
##########
@@ -44,6 +46,12 @@ public class StrutsPrepareFilter implements StrutsStatics, Filter {
protected PrepareOperations prepare;
protected List<Pattern> excludedPatterns = null;
+ protected boolean alwaysCreateActionContext = false;
+
+ @Inject(value = StrutsConstants.STRUTS_ALWAYS_CREATE_ACTION_CONTEXT, required = false)
Review Comment:
Actually - we can't inject here can we? 😬
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121470998
##########
core/src/main/java/org/apache/struts2/dispatcher/filter/StrutsPrepareFilter.java:
##########
@@ -44,6 +46,12 @@ public class StrutsPrepareFilter implements StrutsStatics, Filter {
protected PrepareOperations prepare;
protected List<Pattern> excludedPatterns = null;
+ protected boolean alwaysCreateActionContext = false;
+
+ @Inject(value = StrutsConstants.STRUTS_ALWAYS_CREATE_ACTION_CONTEXT, required = false)
Review Comment:
Actually - we can't inject here can we? 😬
Let me think of a better solution
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121540419
##########
core/src/main/java/org/apache/struts2/dispatcher/filter/StrutsPrepareFilter.java:
##########
@@ -44,6 +46,12 @@ public class StrutsPrepareFilter implements StrutsStatics, Filter {
protected PrepareOperations prepare;
protected List<Pattern> excludedPatterns = null;
+ protected boolean alwaysCreateActionContext = false;
+
+ @Inject(value = StrutsConstants.STRUTS_ALWAYS_CREATE_ACTION_CONTEXT, required = false)
Review Comment:
All sorted, ready for review :)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121734243
##########
core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java:
##########
@@ -73,6 +73,7 @@ public void testWrappedRequestCleanup() {
});
IntStream.range(0, mockedRecursions - 1).forEach(i -> prepare.cleanupWrappedRequest(req));
+ // Assert org.apache.struts2.dispatcher.Dispatcher#cleanUpRequest has not yet run
Review Comment:
?
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] kusalk commented on a diff in pull request #663: WW-5267 Add option to generate ActionContext for excluded URLs
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #663:
URL: https://github.com/apache/struts/pull/663#discussion_r1121542340
##########
core/src/main/java/org/apache/struts2/dispatcher/filter/StrutsExecuteFilter.java:
##########
@@ -46,14 +51,43 @@ public void init(FilterConfig filterConfig) throws ServletException {
protected synchronized void lazyInit() {
if (execute == null) {
- InitOperations init = new InitOperations();
+ InitOperations init = createInitOperations();
Review Comment:
These changes are a port of your [prior changes](https://github.com/atlassian/struts/commit/6651089e823d2d2934f0597ab690ea9322f584e3) which sadly didn't make it to the two filter setup. They are very useful. 😁
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org