StandardMultipartFile.transferTo should fall back to manual copy if Part.write doesn't support absolute locations (e.g. on Jetty) [SPR-15257]

[spring-projects-issues]

Aleš Laňar opened SPR-15257 and commented

I found problem with StandardMultipartHttpServletRequest.transferTo(File dest).
This method calls write method with destination path of given file:

@Override
		public void transferTo(File dest) throws IOException, IllegalStateException {
			this.part.write(dest.getPath());
		}

But if I'll check write method of jetty-util, it wants only String fileName and problem is in this part:

//part data is only in the ByteArrayOutputStream and never been written to disk
                _file = new File (_tmpDir, fileName);

Result is that there is _tmpDir (path of file) concated with filename where is actually complete path, so path is duplicated.

---

Affects: 4.2.6

Issue Links:

• Document how MultiPartFile.transferTo works with Servlet 3 [SPR-12650] #17251 Document how MultiPartFile.transferTo works with Servlet 3
• CommonsMultipartFile.getOriginalFilename() should be able to preserve header-specified filename as-is [SPR-14613] #19180 CommonsMultipartFile.getOriginalFilename() should be able to preserve header-specified filename as-is
• Revise FileSystemResource / FileSystemUtils / FileCopyUtils towards NIO.2 [SPR-15748] #20304 Revise FileSystemResource / FileSystemUtils / FileCopyUtils towards NIO.2

Referenced from: commits 2233ec0, b73153c

[spring-projects-issues]

Juergen Hoeller commented

This is arguably a bug in Jetty: Both Tomcat and Undertow check whether the given file name indicates an absolute location and only prepend a directory if it is relative, so they properly support our existing arrangement. I suppose Jetty is always assuming a relative file name, not doing an isAbsolute check against its resolved File/Path first?

In any case, I see nothing that we can do on our end since we definitely want absolute file locations to keep working with Tomcat and Undertow.

[spring-projects-issues]

Aleš Laňar commented

Their method doesn't checking it. From their JavaDoc:

*@param fileName the name of the file to which the stream will be
* written. The file is created relative to the location as
* specified in the MultipartConfig

Location is from _tmpDir, so I think that Spring should follow it. Are you sure that this is correctly call from Spring?

[spring-projects-issues]

Aleš Laňar commented

Ok, I found a problem that you was right. So there is need to set MultiPartConfig.location for Jetty.

[spring-projects-issues]

Juergen Hoeller commented

The standard javax.servlet.http.Part#write javadoc says:
param fileName: The location into which the uploaded part should be stored. Relative locations are relative to javax.servlet.MultipartConfigElement#getLocation().

It doesn't explicitly talk about absolute locations, but the second sentence is worded such that relative locations get special treatment... implying that absolute locations are supported as well.

Our MultipartFile#transferTo semantically stores the content into an absolute location. So if a particular Servlet container only supports relative paths in its temp directory, we'd have to store it there first and then manually move it to the absolute location that we've been given. We don't need to do that on Tomcat and Undertow, so we would have to specifically detect Jetty and adapt accordingly.

It still seems appropriate to report this to the Jetty project. Let them know that Tomcat and Undertow detect absolute paths there, and that Spring's MultipartFile abstraction generally expects that to work.

[spring-projects-issues]

Joakim Erdfelt commented

You can't use the Tomcat specific Servlet API javadoc as official / canon.

This is non-standard -> https://tomcat.apache.org/tomcat-7.0-doc/servletapi/javax/servlet/http/Part.html#write(java.lang.String)

That text you quoted only shows up in the forked Servlet Spec that Tomcat has.

https://github.com/apache/tomcat85/blob/trunk/java/javax/servlet/http/Part.java#L88-L90

The official spec on Part.write(String) has always been relative to MutliPartConfig.location, and MultiPartConfig.location always has a value / is specified.

Check the the official javadoc for Servlet Spec 3.1 / JEE 7 at ...

http://docs.oracle.com/javaee/7/api/javax/servlet/http/Part.html#write-java.lang.String-

.. it says ...

fileName - the name of the file to which the stream will be written. The file is created relative to the location as specified in the MultipartConfig

Download the official javadoc jar at ...
http://download.oracle.com/otndocs/jcp/servlet-3_1-fr-eval-spec/index.html

Or the official source jar at ...
http://central.maven.org/maven2/javax/servlet/javax.servlet-api/3.1.0/javax.servlet-api-3.1.0-sources.jar

You can verify the official source jar on central.maven.org if you don't trust it ...
http://central.maven.org/maven2/javax/servlet/javax.servlet-api/3.1.0/javax.servlet-api-3.1.0-sources.jar.asc

$ gpg --verify javax.servlet-api-3.1.0-sources.jar.asc 
gpg: assuming signed data in `javax.servlet-api-3.1.0-sources.jar'
gpg: Signature made Thu 25 Apr 2013 04:52:34 PM MST using RSA key ID 47CC79C4
gpg: Good signature from "java_re <GF_RELEASE_WW@oracle.com>"
Primary key fingerprint: 4F7E 32D4 40EF 90A8 3011  A8FC 6425 559C 47CC 79C4

The distinction between relative and absolute paths in regards to the official spec only apply to the declaration of the MultiPartConfig.location value.

See Section 8.1.5 @MultiPartConfig

https://java.net/downloads/servlet-spec/Final/servlet-3_1-final.pdf

This annotation, when specified on a Servlet, indicates that the request it expects
is of type mime/multipart. The HttpServletRequest object of the
corresponding servlet MUST make available the mime attachments via the
getParts and getPart methods to iterate over the various mime attachments. The
location attribute of the javax.servlet.annotation.MultipartConfig and
the <location> element of the <multipart-config> is interpreted as an absolute
path and defaults to the value of the javax.servlet.context.tempdir. If a
relative path is specified, it will be relative to the tempdir location. The test for
absolute path vs relative path MUST be done via java.io.File.isAbsolute.

[spring-projects-issues]

Juergen Hoeller commented

Fair enough. However, the semantics of our MultipartFile method are the way they are, and we are able to translate them properly on various Servlet containers even through the Servlet API, and on all container through CommonsMultipartResolver. I guess we could be smarter about detecting whether it's not working as intended, throwing an exception in that case.

An argument could be made that we should deprecate that existing method and provide a writeTo(String filename) variant instead. However, that'd be a different method with different semantics... Or we could try to implement transferTo(File) in StandardMultipartFile with storage in the temp dir first, manually moving it to the specified target location afterwards.

[spring-projects-issues]

Joakim Erdfelt commented

This change isn't Jetty specific btw.

Caucho Resin follows the Servlet 3.1 spec the same way with regards to Part.write(String)

[spring-projects-issues]

Joakim Erdfelt commented

Just checked Glassfish, it also interprets the behavior as always being relative to MutliPartConfig.location. (See PartItem.java in web-core)

Seeing as Glassfish is the reference implementation, and definitely has had the Servlet TCK run against it, I would say the interpretation of behavior is now tipped on the official javadoc side.

[spring-projects-issues]

Joakim Erdfelt commented

According to an old Tomcat Javadoc bug about Path.write(String) ...

https://bz.apache.org/bugzilla/show_bug.cgi?id=54971#c3

The support they have for absolute paths is "a container specific extension".

At this point, the "StandardMultipartHttpServletRequest.transferTo" is only following the tomcat extension behavior, not the spec.

[spring-projects-issues]

Juergen Hoeller commented

I see that Greg raised this with the Servlet EG: https://java.net/projects/servlet-spec/lists/jsr369-experts/archive/2017-03/message/15

From the Spring perspective, we have established semantics in MultipartFile.transferTo that we have been able to translate to Commons FileUpload for many years (ever since Spring Framework 1.0 in 2003). Note that MultipartFile.transferTo is not a method enforced by Spring anywhere: It is a method offered by our multipart API which users choose to call for their purposes, namely when wanting to efficiently transfer an uploaded file to a location of their choice.

So this isn't about "following" the Servlet spec, it's about translating existing Spring API semantics to the Servlet API. Our transferTo method is designed for use with absolute file locations, so we either need that to work with Part.write... or we need to mimic it by writing to the temporary directory first and then manually moving the outcome to the actual location specified by the user, or by reading the input stream and manually writing it to the specified file location.

It seems a shame to take the current capability away from Tomcat and Undertow users. At the very least, we need to document the potential limitations here, and be smarter in detecting an invalid outcome on other containers. In any case, I'd definitely prefer this to be clarified in the Servlet EG. Otherwise we are forced to recommend CommonsMultipartResolver with Commons FileUpload over standard Servlet multipart handling for portable MultipartFile semantics.

[spring-projects-issues]

Juergen Hoeller commented

Instead of simply documenting this as a non-portable method, there is a way out: In case of an absolute file destination, we can easily detect that Part.write hasn't interpreted the given path as absolute and just offloaded its memory storage to a temp dir location with the given local name. Since users of our MultipartFile.transferTo API expect storage in the exact given location in case of an absolute destination, we simply manually copy the part's content into the given destination in that case. This provides a reliable outcome for Spring's variant of that API, potentially not as efficient as technically possible but at least with portable semantics now.

I've rolled this into 5.0 RC1 and will also backport it to 4.3.8.