Skip to content

Commit 6bb4988

Browse files
labkey-nickalabkey-nicka
committed
Issue 54218: Escape special characters in form field names
1 parent d6e74d6 commit 6bb4988

3 files changed

Lines changed: 122 additions & 101 deletions

File tree

‎api/src/org/labkey/api/assay/AssayFileWriter.java‎

Lines changed: 71 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.labkey.api.audit.provider.FileSystemAuditProvider;
2525
import org.labkey.api.collections.CollectionUtils;
2626
import org.labkey.api.data.Container;
27+
import org.labkey.api.data.TableViewForm;
2728
import org.labkey.api.exp.ExperimentException;
2829
import org.labkey.api.exp.api.ExpProtocol;
2930
import org.labkey.api.pipeline.PipeRoot;
@@ -40,9 +41,11 @@
4041
import java.io.IOException;
4142
import java.io.InputStream;
4243
import java.util.ArrayDeque;
44+
import java.util.ArrayList;
45+
import java.util.Collections;
4346
import java.util.Deque;
47+
import java.util.HashMap;
4448
import java.util.HashSet;
45-
import java.util.Iterator;
4649
import java.util.List;
4750
import java.util.Map;
4851
import java.util.Set;
@@ -229,72 +232,84 @@ public String getFileName(MultipartFile file)
229232

230233
public Map<String, FileLike> savePostedFiles(ContextType context, @NotNull Set<String> parameterNames, boolean allowMultiple, boolean ensureExpData) throws ExperimentException, IOException
231234
{
235+
if (!(context.getRequest() instanceof MultipartHttpServletRequest multipartRequest))
236+
return Collections.emptyMap();
237+
232238
Map<String, FileLike> files = CollectionUtils.enforceValueClass(new TreeMap<>(), FileLike.class);
233239
Set<String> originalFileNames = new HashSet<>();
234-
if (context.getRequest() instanceof MultipartHttpServletRequest multipartRequest)
240+
Map<String, String> encodedParameterNames = new HashMap<>();
241+
for (String parameterName : parameterNames)
242+
encodedParameterNames.put(TableViewForm.getMultiPartFormFieldNameForColumn(parameterName), parameterName);
243+
244+
Deque<FileLike> overflowFiles = new ArrayDeque<>(); // using a deque for easy removal of single elements
245+
Set<String> unusedParameterNames = new HashSet<>(parameterNames);
246+
List<FileSystemAuditProvider.FileSystemAuditEvent> auditEvents = new ArrayList<>();
247+
248+
for (Map.Entry<String, List<MultipartFile>> entry : multipartRequest.getMultiFileMap().entrySet())
235249
{
236-
Iterator<Map.Entry<String, List<MultipartFile>>> iter = multipartRequest.getMultiFileMap().entrySet().iterator();
237-
Deque<FileLike> overflowFiles = new ArrayDeque<>(); // using a deque for easy removal of single elements
238-
Set<String> unusedParameterNames = new HashSet<>(parameterNames);
239-
while (iter.hasNext())
250+
if (!(encodedParameterNames.containsKey(entry.getKey()) || parameterNames.contains(entry.getKey())))
251+
continue;
252+
253+
boolean isAfterFirstFile = false;
254+
for (MultipartFile multipartFile : entry.getValue())
240255
{
241-
Map.Entry<String, List<MultipartFile>> entry = iter.next();
242-
if (parameterNames.contains(entry.getKey()))
256+
String fileName = getFileName(multipartFile);
257+
if (!fileName.isEmpty() && !originalFileNames.add(fileName))
258+
throw new ExperimentException("The file '" + fileName + " ' was uploaded twice - all files must be unique");
259+
260+
if (multipartFile.isEmpty())
261+
continue;
262+
263+
FileLike dir = getFileTargetDir(context);
264+
FileLike file = FileUtil.findUniqueFileName(fileName, dir);
265+
multipartFile.transferTo(toFileForWrite(file));
266+
if (!dir.toURI().getPath().contains(TEMP_DIR_NAME))
243267
{
244-
List<MultipartFile> multipartFiles = entry.getValue();
245-
boolean isAfterFirstFile = false;
246-
for (MultipartFile multipartFile : multipartFiles)
247-
{
248-
String fileName = getFileName(multipartFile);
249-
if (!fileName.isEmpty() && !originalFileNames.add(fileName))
250-
{
251-
throw new ExperimentException("The file '" + fileName + " ' was uploaded twice - all files must be unique");
252-
}
253-
if (!multipartFile.isEmpty())
254-
{
255-
FileLike dir = getFileTargetDir(context);
256-
FileLike file = FileUtil.findUniqueFileName(fileName, dir);
257-
multipartFile.transferTo(toFileForWrite(file));
258-
if (!dir.toURI().getPath().contains(TEMP_DIR_NAME))
259-
{
260-
FileSystemAuditProvider.FileSystemAuditEvent event = new FileSystemAuditProvider.FileSystemAuditEvent(context.getContainer(), allowMultiple ? "File field provided for assay import" : "Primary file provided for assay import");
261-
event.setProvidedFileName(fileName);
262-
event.setFile(file.getName());
263-
event.setDirectory(dir.toURI().getPath());
264-
AuditLogService.get().addEvent(context.getUser(), event);
265-
}
266-
if (!isAfterFirstFile) // first file gets stored with multipartFile's name
267-
{
268-
files.put(multipartFile.getName(), file);
269-
isAfterFirstFile = true;
270-
unusedParameterNames.remove(multipartFile.getName());
271-
}
272-
else // other files get stored in leftover keys later to store only one file per key (bit of a hack)
273-
{
274-
overflowFiles.add(file);
275-
}
276-
277-
if (ensureExpData)
278-
AbstractQueryUpdateService.ensureExpData(context.getUser(), context.getContainer(), toFileForWrite(file));
279-
}
280-
}
268+
FileSystemAuditProvider.FileSystemAuditEvent event = new FileSystemAuditProvider.FileSystemAuditEvent(context.getContainer(), allowMultiple ? "File field provided for assay import" : "Primary file provided for assay import");
269+
event.setProvidedFileName(fileName);
270+
event.setFile(file.getName());
271+
event.setDirectory(dir.toURI().getPath());
272+
auditEvents.add(event);
281273
}
282-
}
283-
// now process overflow files, if any
284-
for (String unusedParameterName : unusedParameterNames)
285-
{
286-
if (overflowFiles.isEmpty())
287-
break; // we're done
288-
else
274+
if (!isAfterFirstFile) // first file gets stored with multipartFile's name
289275
{
290-
files.put(unusedParameterName, overflowFiles.remove());
276+
String name = multipartFile.getName();
277+
String param = encodedParameterNames.get(name);
278+
if (param == null && parameterNames.contains(name))
279+
param = name;
280+
if (param == null)
281+
throw new ExperimentException("No parameter name found for multipart file '" + name + "'");
282+
283+
files.put(param, file);
284+
isAfterFirstFile = true;
285+
unusedParameterNames.remove(param);
291286
}
287+
else // other files get stored in leftover keys later to store only one file per key (bit of a hack)
288+
{
289+
overflowFiles.add(file);
290+
}
291+
292+
if (ensureExpData)
293+
AbstractQueryUpdateService.ensureExpData(context.getUser(), context.getContainer(), toFileForWrite(file));
292294
}
295+
}
296+
297+
if (!auditEvents.isEmpty())
298+
AuditLogService.get().addEvents(context.getUser(), auditEvents);
293299

294-
if (!overflowFiles.isEmpty() && !allowMultiple) // too many files; shouldn't happen, but if it does, throw an error
295-
throw new ExperimentException("Tried to save too many files: number of keys is " + parameterNames.size() +
296-
", but " + overflowFiles.size() + " extra file(s) were found.");
300+
// now process overflow files, if any
301+
for (String unusedParameterName : unusedParameterNames)
302+
{
303+
if (overflowFiles.isEmpty())
304+
break; // we're done
305+
306+
files.put(unusedParameterName, overflowFiles.remove());
297307
}
308+
309+
if (!overflowFiles.isEmpty() && !allowMultiple) // too many files; shouldn't happen, but if it does, throw an error
310+
throw new ExperimentException("Tried to save too many files: number of keys is " + parameterNames.size() +
311+
", but " + overflowFiles.size() + " extra file(s) were found.");
312+
298313
return files;
299314
}
300315

‎api/src/org/labkey/api/data/TableViewForm.java‎

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.labkey.api.action.NullSafeBindException;
3232
import org.labkey.api.action.SpringActionController;
3333
import org.labkey.api.collections.CaseInsensitiveHashMap;
34+
import org.labkey.api.dataiterator.DataIteratorUtil;
3435
import org.labkey.api.ontology.Quantity;
3536
import org.labkey.api.security.permissions.DeletePermission;
3637
import org.labkey.api.security.permissions.InsertPermission;
@@ -759,20 +760,64 @@ private String getPropertyCaption(String propName)
759760
return null==column ? propName : column.getLabel();
760761
}
761762

763+
// RFC 2616 / RFC 7230: Escape characters inside the field name
764+
private static final char BACKSLASH = '\\';
765+
private static final String SPECIAL_CHARS = BACKSLASH + "\";=,";
766+
767+
public static String getFormFieldNameForColumn(@NotNull String columnName)
768+
{
769+
StringBuilder sb = new StringBuilder();
770+
for (char c : columnName.toCharArray())
771+
{
772+
if (SPECIAL_CHARS.indexOf(c) >= 0)
773+
sb.append(BACKSLASH);
774+
sb.append(c);
775+
}
776+
777+
return sb.toString();
778+
}
779+
762780
public String getFormFieldName(@NotNull ColumnInfo column)
763781
{
764-
return column.getName();
782+
return getFormFieldNameForColumn(column.getName());
783+
}
784+
785+
public static String getMultiPartFormFieldNameForColumn(@NotNull String columnName)
786+
{
787+
return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldNameForColumn(columnName));
765788
}
766789

767790
public String getMultiPartFormFieldName(@NotNull ColumnInfo column)
768791
{
769-
return getFormFieldName(column);
792+
return getMultiPartFormFieldNameForColumn(column.getName());
770793
}
771794

772795
@Nullable
773-
public ColumnInfo getColumnByFormFieldName(@NotNull String name)
796+
public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName)
774797
{
775-
return null == getTable() ? null : getTable().getColumn(name);
798+
if (null == getTable())
799+
return null;
800+
801+
StringBuilder sb = new StringBuilder(fieldName.length());
802+
boolean escaping = false;
803+
for (char c : fieldName.toCharArray())
804+
{
805+
if (escaping)
806+
{
807+
sb.append(c);
808+
escaping = false;
809+
}
810+
else if (c == BACKSLASH)
811+
escaping = true;
812+
else
813+
sb.append(c);
814+
}
815+
816+
// Issue 54094: Ensure it works when backslash is the last character
817+
if (escaping)
818+
sb.append(BACKSLASH);
819+
820+
return getTable().getColumn(sb.toString());
776821
}
777822

778823
@Override

‎api/src/org/labkey/api/query/QueryUpdateForm.java‎

Lines changed: 2 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import org.labkey.api.data.ColumnInfo;
2222
import org.labkey.api.data.TableInfo;
2323
import org.labkey.api.data.TableViewForm;
24-
import org.labkey.api.dataiterator.DataIteratorUtil;
2524
import org.labkey.api.view.ViewContext;
2625
import org.springframework.validation.BindException;
2726

@@ -68,10 +67,6 @@ public QueryUpdateForm(@NotNull TableInfo table, @NotNull ViewContext ctx, @Null
6867
}
6968
}
7069

71-
// RFC 2616 / RFC 7230: Escape characters inside the field name
72-
private static final char BACKSLASH = '\\';
73-
private static final String SPECIAL_CHARS = BACKSLASH + "\";=,";
74-
7570
@Override
7671
@Nullable
7772
public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName)
@@ -81,47 +76,13 @@ public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName)
8176

8277
var columnName = _ignorePrefix ? fieldName : fieldName.substring(PREFIX.length());
8378

84-
StringBuilder sb = new StringBuilder(columnName.length());
85-
boolean escaping = false;
86-
for (char c : columnName.toCharArray())
87-
{
88-
if (escaping)
89-
{
90-
sb.append(c);
91-
escaping = false;
92-
}
93-
else if (c == BACKSLASH)
94-
escaping = true;
95-
else
96-
sb.append(c);
97-
}
98-
99-
// Issue 54094: Ensure it works when backslash is the last character
100-
if (escaping)
101-
sb.append(BACKSLASH);
102-
103-
return getTable().getColumn(sb.toString());
79+
return super.getColumnByFormFieldName(columnName);
10480
}
10581

10682
@Override
10783
public String getFormFieldName(@NotNull ColumnInfo column)
10884
{
109-
String columnName = column.getName();
110-
StringBuilder sb = new StringBuilder();
111-
for (char c : columnName.toCharArray())
112-
{
113-
if (SPECIAL_CHARS.indexOf(c) >= 0)
114-
sb.append(BACKSLASH);
115-
sb.append(c);
116-
}
117-
118-
String fieldName = sb.toString();
85+
String fieldName = super.getFormFieldName(column);
11986
return _ignorePrefix ? fieldName : PREFIX + fieldName;
12087
}
121-
122-
@Override
123-
public String getMultiPartFormFieldName(@NotNull ColumnInfo column)
124-
{
125-
return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldName(column));
126-
}
12788
}

0 commit comments

Comments
 (0)