dxbjavid commented on code in PR #3572:
URL: https://github.com/apache/cxf/pull/3572#discussion_r4280185385
##########
core/src/main/java/org/apache/cxf/attachment/ContentDisposition.java:
##########
@@ -127,6 +127,13 @@ public String getType() {
return type;
}
Review Comment:
I went with the second option, since changing what getFilename() returns is
hard to judge for existing callers. getFilename() is unchanged and still
returns the raw value, and there's a new getFilenameSanitized() that returns
FileUtils.safeFileName(getFilename()). The EntityPartProvider and
EntityPartUtils call sites now use it for the name, and its javadoc says it can
return null. getFilename()'s javadoc points at it too. I also added a small
test in ContentDispositionTest covering both accessors.
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/EntityPartProvider.java:
##########
@@ -165,13 +166,13 @@ private <T> EntityPart fromAttachment(Attachment
attachment, Class<T> c, final T
final InputStream is =
attachment.getDataHandler().getInputStream();
final ContentDisposition cd = attachment.getContentDisposition();
- final String fileName = (cd != null) ? cd.getFilename() : null;
+ final String rawFileName = (cd != null) ? cd.getFilename() : null;
final String cdName = cd == null ? null : cd.getParameter("name");
final String contentId = attachment.getContentId();
- if (!StringUtils.isEmpty(fileName)) {
- return new EntityPartImpl(mc.getProviders(), contentId,
fileName, is, c, genericType,
- attachment.getHeaders(), attachment.getContentType());
+ if (!StringUtils.isEmpty(rawFileName)) {
Review Comment:
That's deliberate, following the earlier review. Whether a part counts as a
file part depends on whether the header had a filename at all, so filename=".."
or filename="reports/" doesn't quietly turn into a plain form field. The
sanitised value is only used as the name. safeFileName can return null in those
cases (it never returns an empty string, since an empty result is mapped to
null), so the part stays a file part and getFileName() gives Optional.empty().
testTrailingSeparatorStaysAFilePart covers that. I agree it wasn't obvious from
the code, so there's now a short comment at that check in EntityPartProvider,
and the call goes through cd.getFilenameSanitized().
--
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]
