On 15/03/17 22:07, Greg Wilkins wrote:
>
> Mark,
>
> I guess that is true, although I suspect there are many applications
> with a short and non sanitary path from user data to the argument of write.
>
> So just to really clarify, you are treating the arg as a filename on the
> local filesystem, so an application that calls
>
>
> part.write("mysubdir/filename.txt");
>
> will not be portable, as that is assuming unix directory naming? A
> portable app is going to have to write:
>
>
> part.write(Paths.get("mysubdir","filename.txt").toString());
>
> and I bet most do not do that. The javadoc should really indicate this.
>
> What exactly is the mechanism you use to convert the passed arg to an
> absolute file? Is it
>
> pathOfConfigDir.resolve(Paths.get(filenameArg));
>
>
> ?
public void write(String fileName) throws IOException {
File file = new File(fileName);
if (!file.isAbsolute()) {
file = new File(location, fileName);
}
try {
fileItem.write(file);
} catch (Exception e) {
throw new IOException(e);
}
}
To date, I don't recall any issues being reported by users.
To your point regarding portability, the app is normally providing some
environment specific path with is picked / derived from environment
specific config which in turn would normally take account of the
appropriate file separator.
> I definitely think the javadoc needs to be clarified.
+1
> If we had our time over, the String argument should have been a URI not
> a filename and we could have had a variant of the method that took a File.
There is the option of deprecating the current method and adding a new
one. I'd prefer to clarify the existing method if we can, but if that
gets too messy a new method might be the answer.
Kind regards,
Mark
> On 15 March 2017 at 23:48, Mark Thomas <markt_at_apache.org
> <mailto:markt_at_apache.org>> wrote:
>
> On 15/03/17 04:24, Greg Wilkins wrote:
> >
> > Stuart,
> >
> > So do you accept an absolute path that is any where in the file system,
> > or does it have to be within the tree specified by the multipart config?
> >
> > For example, if the configuration has the location as C:\\foo\bar ,
> > then do you accept a call to write with a value of something like
> > C:\\somewhere\else ? or only something like C:\\foo\bar\more\specific.txt ?
> >
> > IMHO If absolute filenames can write anywhere on the file system, then
> > that's a bit of a security problem and it makes it a little pointless to
> > check relative paths for .. etc.
>
> If this was a user provided path I'd agree. But it isn't. The
> application is free to write to any part of the file system directly via
> the Java API so why not here?
>
> If the path does originate with the user then it is an application
> responsibility to sanitise it, not a container responsibility.
>
> If the application is not trusted to write to anywhere in the file
> system, that is what a security manager and/or OS file permissions
> are for.
>
> Finally, it think it is worth repeating that the primary reason Tomcat
> implemented it this way was for ease of migration from Commons
> FileUpload to the new Servlet API. The equivalent method in FileUpload
> allowed absolute paths so Tomcat did to.
>
> Mark
>
> > On 9 March 2017 at 07:20, Stuart Douglas <sdouglas_at_redhat.com <mailto:sdouglas_at_redhat.com>
> > <mailto:sdouglas_at_redhat.com <mailto:sdouglas_at_redhat.com>>> wrote:
> >
> > For what it is worth we also implement the Tomcat behavior.
> >
> > If we do make this behavior official we should probably put a warning
> > on the javadoc about it being up to the application to make sure the
> > path that is passed in is safe.
> >
> > For relative paths I think we should probably throw an exception if
> > the resulting path is outside the base path (e.g. files like
> > ../../../somefile.txt).
> >
> > Stuart
> >
> > On Wed, Mar 8, 2017 at 6:09 PM, Greg Wilkins <gregw_at_webtide.com <mailto:gregw_at_webtide.com>
> > <mailto:gregw_at_webtide.com <mailto:gregw_at_webtide.com>>> wrote:
> > >
> > > All,
> > >
> > > Janb has asked me to seek clarification on the
> Part.write(String)
> > method.
> > >
> > > The spec for @MultipartConfiguration.location says:
> > >
> > >> 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.
> > >
> > >
> > > The javadoc for Part.write(String fileName) 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
> > >
> > >
> > > It is not clear whether fileName should be interpreted as a Path
> > or not, nor
> > > what should be the result if it specifies an absolute
> location, or
> > contains
> > > special path elements such as "." or "..".
> > >
> > > Should an absolute filename throw an IAE or should it just be
> > interpreted
> > > relative to the config location? What if the filename is
> > something like
> > > "C:\\foo\bar"?
> > >
> > >
> > > Tomcat have changed the javadoc on their version of the API
> to say:
> > >
> > >> @param fileName The location into which the uploaded part
> should be
> > >> stored. Relative locations are relative to {_at_link
> > >> javax.servlet.MultipartConfigElement#getLocation()}
> > >
> > >
> > > For which we have raised an issue:
> > > https://bz.apache.org/bugzilla/show_bug.cgi?id=60802
> <https://bz.apache.org/bugzilla/show_bug.cgi?id=60802>
> > <https://bz.apache.org/bugzilla/show_bug.cgi?id=60802
> <https://bz.apache.org/bugzilla/show_bug.cgi?id=60802>>
> > > and Spring Framework at least is expecting the tomcat
> behaviour as
> > reported
> > > in this Jetty bug:
> > https://github.com/eclipse/jetty.project/issues/1337
> <https://github.com/eclipse/jetty.project/issues/1337>
> > <https://github.com/eclipse/jetty.project/issues/1337
> <https://github.com/eclipse/jetty.project/issues/1337>>
> > >
> > >
> > > I don't mind the Tomcat version (modulo some security
> concerns about
> > > allowing the container to try to write to any old path), but
> would
> > prefer if
> > > the official version could clarify how absolute paths should be
> > handled.
> > >
> > > cheers
> > >
> > >
> > >
> > >
> > > --
> > > Greg Wilkins <gregw_at_webtide.com <mailto:gregw_at_webtide.com>
> <mailto:gregw_at_webtide.com <mailto:gregw_at_webtide.com>>> CTO
> > http://webtide.com
> >
> >
> >
> >
> > --
> > Greg Wilkins <gregw_at_webtide.com <mailto:gregw_at_webtide.com>
> <mailto:gregw_at_webtide.com <mailto:gregw_at_webtide.com>>> CTO
> > http://webtide.com
>
>
>
>
> --
> Greg Wilkins <gregw_at_webtide.com <mailto:gregw_at_webtide.com>> CTO
> http://webtide.com