jsr369-experts@servlet-spec.java.net

[jsr369-experts] Re: [SERVLET_SPEC-172-Part.write] DISCUSSION

From: Mark Thomas <markt_at_apache.org>
Date: Fri, 24 Mar 2017 10:13:34 +0000

On 23/03/17 22:01, Edward Burns wrote:
> Executive Summary:
>
> Ed suggests we rewite the javadoc to explicitly allow or disallow the
> corner cases mentioned in the thread.
>
> Details:
>
>>>>>> On Wed, 22 Mar 2017 09:25:52 -0700, Edward Burns <edward.burns_at_oracle.com> said:
>
> EB> I had the pleasure of discussing this in person with Simone Bordet at
> EB> Devoxx US yesterday. Unfortunately, we didn't come to any conclusion
> EB> about the best way forward. Shing-wai and I are still discussing it.
>
>>>>>> On Wed, 8 Mar 2017 16:09:16 +0900, Greg Wilkins <gregw_at_webtide.com> said:
>
> GW> Janb has asked me to seek clarification on the Part.write(String) method.
>
> GW> The spec for @MultipartConfiguration.location says:
>
> Spec8.1.5> The location attribute of the
> Spec8.1.5> javax.servlet.annotation.MultipartConfig and the <location>
> Spec8.1.5> element of the <multipart-config> is interpreted as an
> Spec8.1.5> absolute path and defaults to the value of the
> Spec8.1.5> javax.servlet.context.tempdir. If a relative path is
> Spec8.1.5> specified, it will be relative to the tempdir location. The
> Spec8.1.5> test for absolute path vs relative path MUST be done via
> Spec8.1.5> java.io.File.isAbsolute.
>
> GW> The javadoc for Part.write(String fileName) says:
>
> Part.write> fileName - the name of the file to which the stream will be
> Part.write> written. The file is created relative to the location as
> Part.write> specified in the MultipartConfig
>
> Greg then shares Tomcat's approach:
>
> GW> Tomcat have changed the javadoc on their version of the API to say:
>
> Tomcat> @param fileName The location into which the uploaded part should
> Tomcat> be stored. Relative locations are relative to {_at_link
> Tomcat> javax.servlet.MultipartConfigElement#getLocation()}
>
> GW> I don't mind the Tomcat version
>
>>>>>> On Thu, 9 Mar 2017 07:20:53 +1100, Stuart Douglas <sdouglas_at_redhat.com> said:
>
> SD> For what it is worth we also implement the Tomcat behavior.
>
> This is an improvement on the version in the Servlet spec, but it still
> leaves some things unspecified:
>
> GW> It is not clear whether fileName should be interpreted as a Path or
> GW> not
>
> GW> what should be the result if it specifies an absolute location
>
> GW> what should be the result if it contains special path elements such
> GW> as "." or "..".
>
> GW> security concerns about allowing the container to try to write to
> GW> any old path)
>
> GW> Should an absolute filename throw an IAE or should it just be interpreted
> GW> relative to the config location? What if the filename is something like
> GW> "C:\\foo\bar"?
>
> I am hesitant to tighten the set of allowable values to Part.write()
> such that things that used to work cease working, with the exception of
> using "../" to escape one web app and write into the space of another.

That assumes that the location in the MultipartConfig is inside the web
application. That is rarely the case.

I don't see any need for this API to limit where a web application can
write. Web apps can access any part of the file system via the standard
Java API so I see no point in limiting it here.

The SecurityManager is the correct way to limit where web applications
can write.

> In this case, I think a better approach is to rewite the javadoc to
> explicitly allow or disallow the corner cases mentioned above.
>
> PROPOSAL: Modify the spec for Part.write() to be:
>
> Add this text in the main javadoc for the method.
>
> -----
>
> The following constraints apply to the fileName argument:
>
> * For all values of the fileName argument, the resultant location on the
> server's filesystem is relative to the location as specified in the
> MultipartConfig.
>
> * If fileName is an absolute location, it still is relative to the
> location, in a system-dependent manner, as specified in the
> MultiPartConfig.

How can an absolute location be relative? This makes no sense.

>
> * If fileName contains system-dependent relative path segments, such as
> "../", if the resultant path is "higher" than the location, as
> specified in the MultiPartConfig, throw an IOException.

I don't think the above text is an improvement. Neither do I understand
the rationale for trying to restrict writes to locations beneath the
location from MultiPartConfig.

My suggestion, starting from Tomcat's current Javadoc and taking account
Greg's feedback is:

@param fileName The location into which the uploaded part should
                be stored. file names and relative paths are
                relative to {_at_link
                javax.servlet.MultipartConfigElement#getLocation()}.
                Absolute paths are used as provided.
                Note: that this is a system dependent string and URI
                notation may not be acceptable on all systems. For
                portability, this string should be generated with the
                File or Path APIs.

Mark