From 11d3448e284789d56a5663bd980d7d5e36b0f8c1 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Tue, 16 Feb 2016 17:41:19 +0100 Subject: [PATCH] Issue #81 Exception not always thrown in Jetty to application when upload part is too big Issue #82 Request.getPart() that results in Exception still allows other parts to be fetched --- .../org/eclipse/jetty/server/RequestTest.java | 3 +- .../util/MultiPartInputStreamParser.java | 523 +++++++++--------- .../jetty/util/MultiPartInputStreamTest.java | 71 +++ 3 files changed, 347 insertions(+), 250 deletions(-) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 081f8eebb3f..169aa54763a 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -1489,14 +1489,13 @@ public class RequestTest request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce); //We should get an error when we getParams if there was a problem parsing the multipart - request.getParameter("xxx"); + request.getPart("xxx"); //A 200 response is actually wrong here } catch (RuntimeException e) { response.sendError(500); } - } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java index 7703835ce0d..23437475d88 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java @@ -60,6 +60,7 @@ public class MultiPartInputStreamParser protected MultipartConfigElement _config; protected String _contentType; protected MultiMap _parts; + protected Exception _err; protected File _tmpDir; protected File _contextTmpDir; protected boolean _deleteOnExit; @@ -414,6 +415,9 @@ public class MultiPartInputStreamParser throws IOException { parse(); + throwIfError(); + + Collection> values = _parts.values(); List parts = new ArrayList<>(); for (List o: values) @@ -436,20 +440,36 @@ public class MultiPartInputStreamParser throws IOException { parse(); + throwIfError(); return _parts.getValue(name, 0); } + /** + * Throws an exception if one has been latched. + * + * @throws IOException + */ + protected void throwIfError () + throws IOException + { + if (_err != null) + { + if (_err instanceof IOException) + throw (IOException)_err; + if (_err instanceof IllegalStateException) + throw (IllegalStateException)_err; + throw new IllegalStateException(_err); + } + } /** * Parse, if necessary, the multipart stream. * - * @throws IOException if unable to parse */ protected void parse () - throws IOException { //have we already parsed the input? - if (_parts != null) + if (_parts != null || _err != null) return; //initialize @@ -460,233 +480,253 @@ public class MultiPartInputStreamParser if (_contentType == null || !_contentType.startsWith("multipart/form-data")) return; - //sort out the location to which to write the files - - if (_config.getLocation() == null) - _tmpDir = _contextTmpDir; - else if ("".equals(_config.getLocation())) - _tmpDir = _contextTmpDir; - else - { - File f = new File (_config.getLocation()); - if (f.isAbsolute()) - _tmpDir = f; - else - _tmpDir = new File (_contextTmpDir, _config.getLocation()); - } - - if (!_tmpDir.exists()) - _tmpDir.mkdirs(); - - String contentTypeBoundary = ""; - int bstart = _contentType.indexOf("boundary="); - if (bstart >= 0) - { - int bend = _contentType.indexOf(";", bstart); - bend = (bend < 0? _contentType.length(): bend); - contentTypeBoundary = QuotedStringTokenizer.unquote(value(_contentType.substring(bstart,bend)).trim()); - } - - String boundary="--"+contentTypeBoundary; - String lastBoundary=boundary+"--"; - byte[] byteBoundary=lastBoundary.getBytes(StandardCharsets.ISO_8859_1); - - // Get first boundary - String line = null; try { - line=((ReadLineInputStream)_in).readLine(); - } - catch (IOException e) - { - LOG.warn("Badly formatted multipart request"); - throw e; - } + //sort out the location to which to write the files - if (line == null) - throw new IOException("Missing content for multipart request"); - - boolean badFormatLogged = false; - line=line.trim(); - while (line != null && !line.equals(boundary) && !line.equals(lastBoundary)) - { - if (!badFormatLogged) - { - LOG.warn("Badly formatted multipart request"); - badFormatLogged = true; - } - line=((ReadLineInputStream)_in).readLine(); - line=(line==null?line:line.trim()); - } - - if (line == null) - throw new IOException("Missing initial multi part boundary"); - - // Empty multipart. - if (line.equals(lastBoundary)) - return; - - // Read each part - boolean lastPart=false; - - outer:while(!lastPart) - { - String contentDisposition=null; - String contentType=null; - String contentTransferEncoding=null; - - MultiMap headers = new MultiMap<>(); - while(true) - { - line=((ReadLineInputStream)_in).readLine(); - - //No more input - if(line==null) - break outer; - - //end of headers: - if("".equals(line)) - break; - - total += line.length(); - if (_config.getMaxRequestSize() > 0 && total > _config.getMaxRequestSize()) - throw new IllegalStateException ("Request exceeds maxRequestSize ("+_config.getMaxRequestSize()+")"); - - //get content-disposition and content-type - int c=line.indexOf(':',0); - if(c>0) - { - String key=line.substring(0,c).trim().toLowerCase(Locale.ENGLISH); - String value=line.substring(c+1,line.length()).trim(); - headers.put(key, value); - if (key.equalsIgnoreCase("content-disposition")) - contentDisposition=value; - if (key.equalsIgnoreCase("content-type")) - contentType = value; - if(key.equals("content-transfer-encoding")) - contentTransferEncoding=value; - } - } - - // Extract content-disposition - boolean form_data=false; - if(contentDisposition==null) - { - throw new IOException("Missing content-disposition"); - } - - QuotedStringTokenizer tok=new QuotedStringTokenizer(contentDisposition,";", false, true); - String name=null; - String filename=null; - while(tok.hasMoreTokens()) - { - String t=tok.nextToken().trim(); - String tl=t.toLowerCase(Locale.ENGLISH); - if(t.startsWith("form-data")) - form_data=true; - else if(tl.startsWith("name=")) - name=value(t); - else if(tl.startsWith("filename=")) - filename=filenameValue(t); - } - - // Check disposition - if(!form_data) - { - continue; - } - //It is valid for reset and submit buttons to have an empty name. - //If no name is supplied, the browser skips sending the info for that field. - //However, if you supply the empty string as the name, the browser sends the - //field, with name as the empty string. So, only continue this loop if we - //have not yet seen a name field. - if(name==null) - { - continue; - } - - //Have a new Part - MultiPart part = new MultiPart(name, filename); - part.setHeaders(headers); - part.setContentType(contentType); - _parts.add(name, part); - part.open(); - - InputStream partInput = null; - if ("base64".equalsIgnoreCase(contentTransferEncoding)) - { - partInput = new Base64InputStream((ReadLineInputStream)_in); - } - else if ("quoted-printable".equalsIgnoreCase(contentTransferEncoding)) - { - partInput = new FilterInputStream(_in) - { - @Override - public int read() throws IOException - { - int c = in.read(); - if (c >= 0 && c == '=') - { - int hi = in.read(); - int lo = in.read(); - if (hi < 0 || lo < 0) - { - throw new IOException("Unexpected end to quoted-printable byte"); - } - char[] chars = new char[] { (char)hi, (char)lo }; - c = Integer.parseInt(new String(chars),16); - } - return c; - } - }; - } + if (_config.getLocation() == null) + _tmpDir = _contextTmpDir; + else if ("".equals(_config.getLocation())) + _tmpDir = _contextTmpDir; else - partInput = _in; + { + File f = new File (_config.getLocation()); + if (f.isAbsolute()) + _tmpDir = f; + else + _tmpDir = new File (_contextTmpDir, _config.getLocation()); + } + if (!_tmpDir.exists()) + _tmpDir.mkdirs(); + String contentTypeBoundary = ""; + int bstart = _contentType.indexOf("boundary="); + if (bstart >= 0) + { + int bend = _contentType.indexOf(";", bstart); + bend = (bend < 0? _contentType.length(): bend); + contentTypeBoundary = QuotedStringTokenizer.unquote(value(_contentType.substring(bstart,bend)).trim()); + } + + String boundary="--"+contentTypeBoundary; + String lastBoundary=boundary+"--"; + byte[] byteBoundary=lastBoundary.getBytes(StandardCharsets.ISO_8859_1); + + // Get first boundary + String line = null; try { - int state=-2; - int c; - boolean cr=false; - boolean lf=false; + line=((ReadLineInputStream)_in).readLine(); + } + catch (IOException e) + { + LOG.warn("Badly formatted multipart request"); + throw e; + } - // loop for all lines + if (line == null) + throw new IOException("Missing content for multipart request"); + + boolean badFormatLogged = false; + line=line.trim(); + while (line != null && !line.equals(boundary) && !line.equals(lastBoundary)) + { + if (!badFormatLogged) + { + LOG.warn("Badly formatted multipart request"); + badFormatLogged = true; + } + line=((ReadLineInputStream)_in).readLine(); + line=(line==null?line:line.trim()); + } + + if (line == null) + throw new IOException("Missing initial multi part boundary"); + + // Empty multipart. + if (line.equals(lastBoundary)) + return; + + // Read each part + boolean lastPart=false; + + outer:while(!lastPart) + { + String contentDisposition=null; + String contentType=null; + String contentTransferEncoding=null; + + MultiMap headers = new MultiMap<>(); while(true) { - int b=0; - while((c=(state!=-2)?state:partInput.read())!=-1) + line=((ReadLineInputStream)_in).readLine(); + + //No more input + if(line==null) + break outer; + + //end of headers: + if("".equals(line)) + break; + + total += line.length(); + if (_config.getMaxRequestSize() > 0 && total > _config.getMaxRequestSize()) + throw new IllegalStateException ("Request exceeds maxRequestSize ("+_config.getMaxRequestSize()+")"); + + //get content-disposition and content-type + int c=line.indexOf(':',0); + if(c>0) { - total ++; - if (_config.getMaxRequestSize() > 0 && total > _config.getMaxRequestSize()) - throw new IllegalStateException("Request exceeds maxRequestSize ("+_config.getMaxRequestSize()+")"); + String key=line.substring(0,c).trim().toLowerCase(Locale.ENGLISH); + String value=line.substring(c+1,line.length()).trim(); + headers.put(key, value); + if (key.equalsIgnoreCase("content-disposition")) + contentDisposition=value; + if (key.equalsIgnoreCase("content-type")) + contentType = value; + if(key.equals("content-transfer-encoding")) + contentTransferEncoding=value; + } + } - state=-2; + // Extract content-disposition + boolean form_data=false; + if(contentDisposition==null) + { + throw new IOException("Missing content-disposition"); + } - // look for CR and/or LF - if(c==13||c==10) + QuotedStringTokenizer tok=new QuotedStringTokenizer(contentDisposition,";", false, true); + String name=null; + String filename=null; + while(tok.hasMoreTokens()) + { + String t=tok.nextToken().trim(); + String tl=t.toLowerCase(Locale.ENGLISH); + if(t.startsWith("form-data")) + form_data=true; + else if(tl.startsWith("name=")) + name=value(t); + else if(tl.startsWith("filename=")) + filename=filenameValue(t); + } + + // Check disposition + if(!form_data) + { + continue; + } + //It is valid for reset and submit buttons to have an empty name. + //If no name is supplied, the browser skips sending the info for that field. + //However, if you supply the empty string as the name, the browser sends the + //field, with name as the empty string. So, only continue this loop if we + //have not yet seen a name field. + if(name==null) + { + continue; + } + + //Have a new Part + MultiPart part = new MultiPart(name, filename); + part.setHeaders(headers); + part.setContentType(contentType); + _parts.add(name, part); + part.open(); + + InputStream partInput = null; + if ("base64".equalsIgnoreCase(contentTransferEncoding)) + { + partInput = new Base64InputStream((ReadLineInputStream)_in); + } + else if ("quoted-printable".equalsIgnoreCase(contentTransferEncoding)) + { + partInput = new FilterInputStream(_in) + { + @Override + public int read() throws IOException { - if(c==13) + int c = in.read(); + if (c >= 0 && c == '=') { - partInput.mark(1); - int tmp=partInput.read(); - if (tmp!=10) - partInput.reset(); - else - state=tmp; + int hi = in.read(); + int lo = in.read(); + if (hi < 0 || lo < 0) + { + throw new IOException("Unexpected end to quoted-printable byte"); + } + char[] chars = new char[] { (char)hi, (char)lo }; + c = Integer.parseInt(new String(chars),16); + } + return c; + } + }; + } + else + partInput = _in; + + + try + { + int state=-2; + int c; + boolean cr=false; + boolean lf=false; + + // loop for all lines + while(true) + { + int b=0; + while((c=(state!=-2)?state:partInput.read())!=-1) + { + total ++; + if (_config.getMaxRequestSize() > 0 && total > _config.getMaxRequestSize()) + throw new IllegalStateException("Request exceeds maxRequestSize ("+_config.getMaxRequestSize()+")"); + + state=-2; + + // look for CR and/or LF + if(c==13||c==10) + { + if(c==13) + { + partInput.mark(1); + int tmp=partInput.read(); + if (tmp!=10) + partInput.reset(); + else + state=tmp; + } + break; + } + + // Look for boundary + if(b>=0&&b0) + part.write(byteBoundary,0,b); + + b=-1; + part.write(c); } - break; } - // Look for boundary - if(b>=0&&b0&&b0) - part.write(byteBoundary,0,b); - + part.write(byteBoundary,0,b); b=-1; - part.write(c); } - } - // Check for incomplete boundary match, writing out the chars we matched along the way - if((b>0&&b0||c==-1) + { + + if(b==byteBoundary.length) + lastPart=true; + if(state==10) + state=-2; + break; + } + + // handle CR LF if(cr) part.write(13); if(lf) part.write(10); - cr=lf=false; - part.write(byteBoundary,0,b); - b=-1; - } - - // Boundary match. If we've run out of input or we matched the entire final boundary marker, then this is the last part. - if(b>0||c==-1) - { - - if(b==byteBoundary.length) - lastPart=true; + cr=(c==13); + lf=(c==10||state==10); if(state==10) state=-2; - break; } + } + finally + { - // handle CR LF - if(cr) - part.write(13); - - if(lf) - part.write(10); - - cr=(c==13); - lf=(c==10||state==10); - if(state==10) - state=-2; + part.close(); } } - finally - { - - part.close(); - } + if (!lastPart) + throw new IOException("Incomplete parts"); + } + catch (Exception e) + { + _err = e; } - if (!lastPart) - throw new IOException("Incomplete parts"); } public void setDeleteOnExit(boolean deleteOnExit) diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java index 293fb3d6de2..4e87d66a432 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java @@ -354,6 +354,42 @@ public class MultiPartInputStreamTest assertTrue(e.getMessage().startsWith("Request exceeds maxRequestSize")); } } + + + @Test + public void testRequestTooBigThrowsErrorOnGetParts () + throws Exception + { + MultipartConfigElement config = new MultipartConfigElement(_dirname, 60, 100, 50); + MultiPartInputStreamParser mpis = new MultiPartInputStreamParser(new ByteArrayInputStream(_multi.getBytes()), + _contentType, + config, + _tmpDir); + mpis.setDeleteOnExit(true); + Collection parts = null; + + //cause parsing + try + { + parts = mpis.getParts(); + fail("Request should have exceeded maxRequestSize"); + } + catch (IllegalStateException e) + { + assertTrue(e.getMessage().startsWith("Request exceeds maxRequestSize")); + } + + //try again + try + { + parts = mpis.getParts(); + fail("Request should have exceeded maxRequestSize"); + } + catch (IllegalStateException e) + { + assertTrue(e.getMessage().startsWith("Request exceeds maxRequestSize")); + } + } @Test public void testFileTooBig() @@ -376,6 +412,41 @@ public class MultiPartInputStreamTest assertTrue(e.getMessage().startsWith("Multipart Mime part")); } } + + @Test + public void testFileTooBigThrowsErrorOnGetParts() + throws Exception + { + MultipartConfigElement config = new MultipartConfigElement(_dirname, 40, 1024, 30); + MultiPartInputStreamParser mpis = new MultiPartInputStreamParser(new ByteArrayInputStream(_multi.getBytes()), + _contentType, + config, + _tmpDir); + mpis.setDeleteOnExit(true); + Collection parts = null; + try + { + parts = mpis.getParts(); //caused parsing + fail("stuff.txt should have been larger than maxFileSize"); + } + catch (IllegalStateException e) + { + assertTrue(e.getMessage().startsWith("Multipart Mime part")); + } + + //test again after the parsing + try + { + parts = mpis.getParts(); //caused parsing + fail("stuff.txt should have been larger than maxFileSize"); + } + catch (IllegalStateException e) + { + assertTrue(e.getMessage().startsWith("Multipart Mime part")); + } + } + + @Test public void testPartFileNotDeleted () throws Exception