Skip to content

Commit

Permalink
WW-5352 Implement auto-allowlisting for Iterator component
Browse files Browse the repository at this point in the history
  • Loading branch information
kusalk committed Jan 9, 2024
1 parent a57c288 commit 937bc77
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
*/
package org.apache.struts2.components;

import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.util.ValueStack;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.ognl.ThreadAllowlist;
import org.apache.struts2.util.MakeIterator;
import org.apache.struts2.views.annotations.StrutsTag;
import org.apache.struts2.views.annotations.StrutsTagAttribute;
Expand Down Expand Up @@ -188,8 +190,8 @@
* <!-- START SNIPPET: example6description -->
*
* <p>Another way to create a simple loop, similar to JSTL's
* &lt;c:forEach begin="..." end="..." ...&gt; is to use some
* OGNL magic, which provides some under-the-covers magic to
* &lt;c:forEach begin="..." end="..." ...&gt; is to use some
* OGNL magic, which provides some under-the-covers magic to
* make 0-n loops trivial. This example also loops five times.</p>
*
* <!-- END SNIPPET: example6description -->
Expand Down Expand Up @@ -237,11 +239,17 @@ public class IteratorComponent extends ContextBean {
protected Integer end;
protected String stepStr;
protected Integer step;
private ThreadAllowlist threadAllowlist;

public IteratorComponent(ValueStack stack) {
super(stack);
}

@Inject
public void setThreadAllowlist(ThreadAllowlist threadAllowlist) {
this.threadAllowlist = threadAllowlist;
}

public boolean start(Writer writer) {
//Create an iterator status if the status attribute was set.
if (statusAttr != null) {
Expand Down Expand Up @@ -298,6 +306,7 @@ public boolean start(Writer writer) {
if ((iterator != null) && iterator.hasNext()) {
Object currentValue = iterator.next();
stack.push(currentValue);
threadAllowlist.allowClass(currentValue.getClass());

String var = getVar();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,8 +719,6 @@ public void serviceAction(HttpServletRequest request, HttpServletResponse respon
} else {
throw new ServletException(e);
}
} finally {
threadAllowlist.clearAllowlist();
}
}

Expand Down Expand Up @@ -1051,6 +1049,7 @@ protected MultiPartRequest getMultiPartRequest() {
*/
public void cleanUpRequest(HttpServletRequest request) {
ContainerHolder.clear();
threadAllowlist.clearAllowlist();
if (!(request instanceof MultiPartRequestWrapper)) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,14 @@
*/
package org.apache.struts2.views.jsp;

import com.opensymphony.xwork2.inject.Container;
import com.opensymphony.xwork2.util.ValueStack;
import org.apache.struts2.components.Component;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.jsp.JspException;

import com.opensymphony.xwork2.ActionContext;
import org.apache.struts2.components.Component;

import com.opensymphony.xwork2.inject.Container;
import com.opensymphony.xwork2.util.ValueStack;

/**
*/
public abstract class ComponentTagSupport extends StrutsBodyTagSupport {
Expand All @@ -39,8 +37,7 @@ public abstract class ComponentTagSupport extends StrutsBodyTagSupport {
public int doEndTag() throws JspException {
component.end(pageContext.getOut(), getBody());
component = null; // Always clear component reference (since clearTagStateForTagPoolingServers() is conditional).
clearTagStateForTagPoolingServers();
return EVAL_PAGE;
return super.doEndTag();
}

@Override
Expand All @@ -49,7 +46,7 @@ public int doStartTag() throws JspException {
component = getBean(stack, (HttpServletRequest) pageContext.getRequest(), (HttpServletResponse) pageContext.getResponse());
Container container = stack.getActionContext().getContainer();
container.inject(component);

populateParams();
boolean evalBody = component.start(pageContext.getOut());

Expand All @@ -62,7 +59,7 @@ public int doStartTag() throws JspException {

/**
* Define method to populate component state based on the Tag parameters.
*
* <p>
* Descendants should override this method for custom behaviour, but should <em>always</em> call the ancestor method when doing so.
*/
protected void populateParams() {
Expand All @@ -71,7 +68,7 @@ protected void populateParams() {

/**
* Specialized method to populate the performClearTagStateForTagPoolingServers state of the Component to match the value set in the Tag.
*
* <p>
* Generally only unit tests would call this method directly, to avoid calling the whole populateParams() chain again after doStartTag()
* has been called. Doing that can break tag / component state behaviour, but unit tests still need a way to set the
* performClearTagStateForTagPoolingServers state for the component (which only comes into being after doStartTag() is called).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,33 @@
import com.opensymphony.xwork2.ActionContext;
import com.opensymphony.xwork2.util.ValueStack;
import org.apache.struts2.StrutsInternalTestCase;
import org.apache.struts2.ognl.ThreadAllowlist;

import java.io.StringWriter;
import java.util.Arrays;
import java.util.List;

public class IteratorComponentTest extends StrutsInternalTestCase {

private ValueStack stack;
private IteratorComponent ic;
private ThreadAllowlist threadAllowlist;

@Override
public void setUp() throws Exception {
super.setUp();
stack = ActionContext.getContext().getValueStack();
ic = new IteratorComponent(stack);
threadAllowlist = new ThreadAllowlist();
ic.setThreadAllowlist(threadAllowlist);
}

public void testIterator() throws Exception {
// given
final ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new FooAction());

StringWriter out = new StringWriter();

IteratorComponent ic = new IteratorComponent(stack);
ic.setValue("items");
ic.setVar("val");

Expand Down Expand Up @@ -64,12 +76,10 @@ public void testIterator() throws Exception {

public void testIteratorWithBegin() throws Exception {
// given
final ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new FooAction());

StringWriter out = new StringWriter();

IteratorComponent ic = new IteratorComponent(stack);
ic.setValue("items");
ic.setVar("val");
ic.setBegin("1");
Expand All @@ -96,7 +106,6 @@ public void testIteratorWithBegin() throws Exception {

public void testIteratorWithNulls() throws Exception {
// given
final ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(new FooAction() {
private List items = Arrays.asList("1", "2", null, "4");

Expand All @@ -107,7 +116,6 @@ public List getItems() {

StringWriter out = new StringWriter();

IteratorComponent ic = new IteratorComponent(stack);
ic.setValue("items");
ic.setVar("val");
Property prop = new Property(stack);
Expand Down

0 comments on commit 937bc77

Please sign in to comment.