Skip to content

Commit 2d6f1bc

Browse files
yasserzamaniyasserzamani
committed
add some improvements
1 parent 36e4e74 commit 2d6f1bc

File tree

9 files changed

+108
-15
lines changed

9 files changed

+108
-15
lines changed

‎core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java

+5
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,11 @@ public Object getProperty(Map context, Object target, Object name) throws OgnlEx
109109
if (listSize <= index) {
110110
Object result;
111111

112+
if (index > autoGrowCollectionLimit) {
113+
throw new OgnlException("Error auto growing collection size to " + index + " which limited to "
114+
+ autoGrowCollectionLimit);
115+
}
116+
112117
for (int i = listSize; i < index; i++) {
113118
list.add(null);
114119
}

‎core/src/main/java/org/apache/struts2/StrutsConstants.java

+3
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,9 @@ public final class StrutsConstants {
145145
/** The maximized number of files allowed to upload */
146146
public static final String STRUTS_MULTIPART_MAXFILES = "struts.multipart.maxFiles";
147147

148+
/** The maximum length of a string parameter in a multipart request. */
149+
public static final String STRUTS_MULTIPART_MAX_STRING_LENGTH = "struts.multipart.maxStringLength";
150+
148151
/** The directory to use for storing uploaded files */
149152
public static final String STRUTS_MULTIPART_SAVEDIR = "struts.multipart.saveDir";
150153

‎core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java

+10
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ public class ConstantConfig {
6565
private String uiThemeExpansionToken;
6666
private Long multipartMaxSize;
6767
private Long multipartMaxFiles;
68+
private Long multipartMaxStringLength;
6869
private String multipartSaveDir;
6970
private Integer multipartBufferSize;
7071
private BeanConfig multipartParser;
@@ -197,6 +198,7 @@ public Map<String, String> getAllAsStringsMap() {
197198
map.put(StrutsConstants.STRUTS_UI_THEME_EXPANSION_TOKEN, uiThemeExpansionToken);
198199
map.put(StrutsConstants.STRUTS_MULTIPART_MAXSIZE, Objects.toString(multipartMaxSize, null));
199200
map.put(StrutsConstants.STRUTS_MULTIPART_MAXFILES, Objects.toString(multipartMaxFiles, null));
201+
map.put(StrutsConstants.STRUTS_MULTIPART_MAX_STRING_LENGTH, Objects.toString(multipartMaxStringLength, null));
200202
map.put(StrutsConstants.STRUTS_MULTIPART_SAVEDIR, multipartSaveDir);
201203
map.put(StrutsConstants.STRUTS_MULTIPART_BUFFERSIZE, Objects.toString(multipartBufferSize, null));
202204
map.put(StrutsConstants.STRUTS_MULTIPART_PARSER, beanConfToString(multipartParser));
@@ -590,6 +592,14 @@ public void setMultipartMaxFiles(Long multipartMaxFiles) {
590592
this.multipartMaxFiles = multipartMaxFiles;
591593
}
592594

595+
public Long getMultipartMaxStringLength() {
596+
return multipartMaxStringLength;
597+
}
598+
599+
public void setMultipartMaxStringLength(Long multipartMaxStringLength) {
600+
this.multipartMaxStringLength = multipartMaxStringLength;
601+
}
602+
593603
public String getMultipartSaveDir() {
594604
return multipartSaveDir;
595605
}

‎core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java

+10
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
5858
*/
5959
protected Long maxFiles;
6060

61+
/**
62+
* Specifies the maximum length of a string parameter in a multipart request.
63+
*/
64+
protected Long maxStringLength;
65+
6166
/**
6267
* Specifies the buffer size to use during streaming.
6368
*/
@@ -96,6 +101,11 @@ public void setMaxFiles(String maxFiles) {
96101
this.maxFiles = Long.parseLong(maxFiles);
97102
}
98103

104+
@Inject(StrutsConstants.STRUTS_MULTIPART_MAX_STRING_LENGTH)
105+
public void setMaxStringLength(String maxStringLength) {
106+
this.maxStringLength = Long.parseLong(maxStringLength);
107+
}
108+
99109
@Inject
100110
public void setLocaleProviderFactory(LocaleProviderFactory localeProviderFactory) {
101111
defaultLocale = localeProviderFactory.createLocaleProvider().getLocale();

‎core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java

+12-1
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,19 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
137137
values = new ArrayList<>();
138138
}
139139

140-
if (item.getSize() == 0) {
140+
long size = item.getSize();
141+
if (size == 0) {
141142
values.add(StringUtils.EMPTY);
143+
} else if (size > maxStringLength) {
144+
String errorKey = "struts.messages.upload.error.parameter.too.long";
145+
LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), errorKey, null,
146+
new Object[] { item.getFieldName(), maxStringLength, size });
147+
148+
if (!errors.contains(localizedMessage)) {
149+
errors.add(localizedMessage);
150+
}
151+
return;
152+
142153
} else if (charset != null) {
143154
values.add(item.getString(charset));
144155
} else {

‎core/src/main/resources/org/apache/struts2/default.properties

+1
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ struts.multipart.parser=jakarta
6969
struts.multipart.saveDir=
7070
struts.multipart.maxSize=2097152
7171
struts.multipart.maxFiles=256
72+
struts.multipart.maxStringLength=4096
7273

7374
### Load custom property files (does not override struts.properties!)
7475
# struts.custom.properties=application,org/apache/struts2/extension/custom

‎core/src/main/resources/org/apache/struts2/struts-messages.properties

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ struts.messages.invalid.content.type=Could not find a Content-Type for {0}. Veri
2626
struts.messages.removing.file=Removing file {0} {1}
2727
struts.messages.error.uploading=Error uploading: {0}
2828
struts.messages.error.file.too.large=File {0} is too large to be uploaded. Maximum allowed size is {4} bytes!
29+
struts.messages.upload.error.parameter.too.long=The request parameter "{0}" was too long. Max length allowed is {1}, but found {2}!
2930
struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0} "{1}" "{2}" {3}
3031
struts.messages.error.file.extension.not.allowed=File extension not allowed: {0} "{1}" "{2}" {3}
3132

‎core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java

+9-7
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
import com.opensymphony.xwork2.XWorkTestCase;
2323
import com.opensymphony.xwork2.util.ListHolder;
2424
import com.opensymphony.xwork2.util.ValueStack;
25-
import ognl.ListPropertyAccessor;
25+
import com.opensymphony.xwork2.util.reflection.ReflectionContextState;
2626
import ognl.PropertyAccessor;
2727

2828
import java.util.ArrayList;
@@ -42,11 +42,11 @@ public void testContains() {
4242

4343
assertNotNull(listHolder.getLongs());
4444
assertEquals(3, listHolder.getLongs().size());
45-
assertEquals(new Long(1), (Long) listHolder.getLongs().get(0));
46-
assertEquals(new Long(2), (Long) listHolder.getLongs().get(1));
47-
assertEquals(new Long(3), (Long) listHolder.getLongs().get(2));
45+
assertEquals(new Long(1), listHolder.getLongs().get(0));
46+
assertEquals(new Long(2), listHolder.getLongs().get(1));
47+
assertEquals(new Long(3), listHolder.getLongs().get(2));
4848

49-
assertTrue(((Boolean) vs.findValue("longs.contains(1)")).booleanValue());
49+
assertTrue((Boolean) vs.findValue("longs.contains(1)"));
5050
}
5151

5252
public void testCanAccessListSizeProperty() {
@@ -60,8 +60,8 @@ public void testCanAccessListSizeProperty() {
6060

6161
vs.push(listHolder);
6262

63-
assertEquals(new Integer(myList.size()), vs.findValue("strings.size()"));
64-
assertEquals(new Integer(myList.size()), vs.findValue("strings.size"));
63+
assertEquals(myList.size(), vs.findValue("strings.size()"));
64+
assertEquals(myList.size(), vs.findValue("strings.size"));
6565
}
6666

6767
public void testAutoGrowthCollectionLimit() {
@@ -73,12 +73,14 @@ public void testAutoGrowthCollectionLimit() {
7373
listHolder.setStrings(myList);
7474

7575
ValueStack vs = ActionContext.getContext().getValueStack();
76+
ReflectionContextState.setCreatingNullObjects(vs.getContext(), true);
7677
vs.push(listHolder);
7778

7879
vs.setValue("strings[0]", "a");
7980
vs.setValue("strings[1]", "b");
8081
vs.setValue("strings[2]", "c");
8182
vs.setValue("strings[3]", "d");
83+
vs.findValue("strings[3]");
8284

8385
assertEquals(3, vs.findValue("strings.size()"));
8486
}

‎core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java

+57-7
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ public void testInvalidContentTypeMultipartRequest() throws Exception {
235235
mai.setInvocationContext(ActionContext.getContext());
236236

237237
ActionContext.getContext().setParameters(HttpParameters.create().build());
238-
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
238+
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
239239

240240
interceptor.intercept(mai);
241241

@@ -257,7 +257,7 @@ public void testNoContentMultipartRequest() throws Exception {
257257
mai.setInvocationContext(ActionContext.getContext());
258258

259259
ActionContext.getContext().setParameters(HttpParameters.create().build());
260-
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
260+
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
261261

262262
interceptor.intercept(mai);
263263

@@ -288,7 +288,7 @@ public void testSuccessUploadOfATextFileMultipartRequest() throws Exception {
288288
mai.setInvocationContext(ActionContext.getContext());
289289
Map<String, Object> param = new HashMap<>();
290290
ActionContext.getContext().setParameters(HttpParameters.create(param).build());
291-
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
291+
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
292292

293293
interceptor.intercept(mai);
294294

@@ -349,7 +349,7 @@ public void testMultipleAccept() throws Exception {
349349
mai.setInvocationContext(ActionContext.getContext());
350350
Map<String, Object> param = new HashMap<String, Object>();
351351
ActionContext.getContext().setParameters(HttpParameters.create(param).build());
352-
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
352+
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
353353

354354
interceptor.setAllowedTypes("text/html");
355355
interceptor.intercept(mai);
@@ -402,7 +402,7 @@ public void testUnacceptedNumberOfFiles() throws Exception {
402402
mai.setInvocationContext(ActionContext.getContext());
403403
Map<String, Object> param = new HashMap<>();
404404
ActionContext.getContext().setParameters(HttpParameters.create(param).build());
405-
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
405+
ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
406406

407407
interceptor.setAllowedTypes("text/html");
408408
interceptor.intercept(mai);
@@ -413,6 +413,55 @@ public void testUnacceptedNumberOfFiles() throws Exception {
413413
assertEquals("Request exceeded allowed number of files! Max allowed files number is: 3!", action.getActionErrors().iterator().next());
414414
}
415415

416+
public void testMultipartRequestMaxStringLength() throws Exception {
417+
MockHttpServletRequest req = new MockHttpServletRequest();
418+
req.setCharacterEncoding(StandardCharsets.UTF_8.name());
419+
req.setMethod("post");
420+
req.addHeader("Content-type", "multipart/form-data; boundary=---1234");
421+
422+
// inspired by the unit tests for jakarta commons fileupload
423+
String content = ("-----1234\r\n" +
424+
"Content-Disposition: form-data; name=\"file\"; filename=\"deleteme.txt\"\r\n" +
425+
"Content-Type: text/html\r\n" +
426+
"\r\n" +
427+
"Unit test of FileUploadInterceptor" +
428+
"\r\n" +
429+
"-----1234\r\n" +
430+
"Content-Disposition: form-data; name=\"normalFormField1\"\r\n" +
431+
"\r\n" +
432+
"it works" +
433+
"\r\n" +
434+
"-----1234\r\n" +
435+
"Content-Disposition: form-data; name=\"normalFormField2\"\r\n" +
436+
"\r\n" +
437+
"long string should not work" +
438+
"\r\n" +
439+
"-----1234--\r\n");
440+
req.setContent(content.getBytes(StandardCharsets.US_ASCII));
441+
442+
MyFileupAction action = container.inject(MyFileupAction.class);
443+
444+
MockActionInvocation mai = new MockActionInvocation();
445+
mai.setAction(action);
446+
mai.setResultCode("success");
447+
mai.setInvocationContext(ActionContext.getContext());
448+
Map<String, Object> param = new HashMap<>();
449+
ActionContext.getContext()
450+
.withParameters(HttpParameters.create(param).build())
451+
.withServletRequest(createMultipartRequest(req, -1, 20));
452+
453+
interceptor.intercept(mai);
454+
455+
assertTrue(action.hasActionErrors());
456+
457+
Collection<String> errors = action.getActionErrors();
458+
assertEquals(1, errors.size());
459+
String msg = errors.iterator().next();
460+
assertEquals(
461+
"The request parameter \"normalFormField2\" was too long. Max length allowed is 20, but found 27!",
462+
msg);
463+
}
464+
416465
public void testMultipartRequestLocalizedError() throws Exception {
417466
MockHttpServletRequest req = new MockHttpServletRequest();
418467
req.setCharacterEncoding(StandardCharsets.UTF_8.name());
@@ -439,7 +488,7 @@ public void testMultipartRequestLocalizedError() throws Exception {
439488
ActionContext.getContext()
440489
.withParameters(HttpParameters.create(param).build())
441490
.withLocale(Locale.GERMAN)
442-
.withServletRequest(createMultipartRequest(req, 10));
491+
.withServletRequest(createMultipartRequest(req, 10, -1));
443492

444493
interceptor.intercept(mai);
445494

@@ -472,10 +521,11 @@ private String encodeTextFile(String bondary, String endline, String name, Strin
472521
return sb.toString();
473522
}
474523

475-
private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize) throws IOException {
524+
private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxStringLength) throws IOException {
476525
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
477526
jak.setMaxSize(String.valueOf(maxsize));
478527
jak.setMaxFiles("3");
528+
jak.setMaxStringLength(String.valueOf(maxStringLength));
479529
return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
480530
}
481531

0 commit comments

Comments
 (0)