You're right, thanks for the catch. That's a fairly unappealing oversight. In some respects I prefer to set Content-Disposition in both ProxyHandler and MakeRequestHandler, but on consideration I think it's better the logic be restored to ProxyBase.
Fix: http://codereview.appspot.com/100050 On Tue, Aug 4, 2009 at 4:39 PM, Adam Winer <[email protected]> wrote: > Moving the content-disposition setting from ProxyBase to ProxyHandler > effectively makes MakeRequestHandler an open proxy. Sooo.... some > other fix is needed. > > On Tue, Aug 4, 2009 at 3:48 PM, <[email protected]> wrote: > > Author: johnh > > Date: Tue Aug 4 22:48:08 2009 > > New Revision: 801008 > > > > URL: http://svn.apache.org/viewvc?rev=801008&view=rev > > Log: > > * Adds support for gadgets.io.getCachedUrl(...) options to include { > > rewriteMime: "mimeType" }. > > > > * Uses this support in gadgets.flash.embedCachedFlash(...) to default > mimeType > > to application/x-shockwave-flash, in order to force this mime-type in > serving > > SWFs, even from servers that don't emit the proper headers for them > (which > > breaks these on most all browsers) > > > > * Moves Content-Disposition removal logic for > application/x-shockwave-flash > > content to ProxyHandler, since that's where HttpRequest, which has > rewriteMime > > access, is read and processed. > > > > * Updates tests accordingly. > > > > > > Modified: > > incubator/shindig/trunk/config/container.js > > incubator/shindig/trunk/features/src/main/javascript/features/ > core.io/io.js > > > incubator/shindig/trunk/features/src/main/javascript/features/core/legacy.js > > > incubator/shindig/trunk/features/src/main/javascript/features/flash/flash.js > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java > > > > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyBaseTest.java > > > > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java > > > > Modified: incubator/shindig/trunk/config/container.js > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/config/container.js?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- incubator/shindig/trunk/config/container.js (original) > > +++ incubator/shindig/trunk/config/container.js Tue Aug 4 22:48:08 2009 > > @@ -96,7 +96,7 @@ > > "gadgets.features" : { > > "core.io" : { > > // Note: /proxy is an open proxy. Be careful how you expose this! > > - "proxyUrl" : "http:// > %host%/gadgets/proxy?refresh=%refresh%&url=%url%", > > + "proxyUrl" : "http:// > %host%/gadgets/proxy?refresh=%refresh%&url=%url%&%rewriteMime%", > > "jsonProxyUrl" : "http://%host%/gadgets/makeRequest"; > > }, > > "views" : { > > > > Modified: incubator/shindig/trunk/features/src/main/javascript/features/ > core.io/io.js > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/features/src/main/javascript/features/core.io/io.js?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- incubator/shindig/trunk/features/src/main/javascript/features/ > core.io/io.js (original) > > +++ incubator/shindig/trunk/features/src/main/javascript/features/ > core.io/io.js Tue Aug 4 22:48:08 2009 > > @@ -470,12 +470,15 @@ > > > > var urlParams = gadgets.util.getUrlParameters(); > > > > + var rewriteMimeParam = > > + params.rewriteMime ? "rewriteMime=" + > encodeURIComponent(params.rewriteMime) : ""; > > return config.proxyUrl.replace("%url%", encodeURIComponent(url)). > > replace("%host%", document.location.host). > > replace("%rawurl%", url). > > replace("%refresh%", encodeURIComponent(refresh)). > > replace("%gadget%", encodeURIComponent(urlParams.url)). > > - replace("%container%", encodeURIComponent(urlParams.container > || urlParams.synd)); > > + replace("%container%", encodeURIComponent(urlParams.container > || urlParams.synd)). > > + replace("%rewriteMime%", rewriteMimeParam); > > } > > }; > > }(); > > > > Modified: > incubator/shindig/trunk/features/src/main/javascript/features/core/legacy.js > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/features/src/main/javascript/features/core/legacy.js?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- > incubator/shindig/trunk/features/src/main/javascript/features/core/legacy.js > (original) > > +++ > incubator/shindig/trunk/features/src/main/javascript/features/core/legacy.js > Tue Aug 4 22:48:08 2009 > > @@ -124,9 +124,10 @@ > > } > > > > function _IG_GetCachedUrl(url, opt_params) { > > - var params = { 'REFRESH_INTERVAL': 3600 }; > > - if (opt_params && opt_params.refreshInterval) { > > - params['REFRESH_INTERVAL'] = opt_params.refreshInterval; > > + var params = opt_params || {}; > > + params['REFRESH_INTERVAL'] = 3600; > > + if (params.refreshInterval) { > > + params['REFRESH_INTERVAL'] = params.refreshInterval; > > } > > return gadgets.io.getProxyUrl(url, params); > > } > > > > Modified: > incubator/shindig/trunk/features/src/main/javascript/features/flash/flash.js > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/features/src/main/javascript/features/flash/flash.js?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- > incubator/shindig/trunk/features/src/main/javascript/features/flash/flash.js > (original) > > +++ > incubator/shindig/trunk/features/src/main/javascript/features/flash/flash.js > Tue Aug 4 22:48:08 2009 > > @@ -190,7 +190,7 @@ > > */ > > gadgets.flash.embedCachedFlash = function() { > > var args = Array.prototype.slice.call(arguments); > > - args[0] = gadgets.io.getProxyUrl(args[0]); > > + args[0] = gadgets.io.getProxyUrl(args[0], { rewriteMime: > "application/x-shockwave-flash" }); > > return gadgets.flash.embedFlash.apply(this, args); > > }; > > > > > > Modified: > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java > (original) > > +++ > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java > Tue Aug 4 22:48:08 2009 > > @@ -131,12 +131,6 @@ > > refreshInterval = Math.max(60 * 60, (int)(results.getCacheTtl() / > 1000L)); > > } > > HttpUtil.setCachingHeaders(response, refreshInterval); > > - // We're skipping the content disposition header for flash due to an > issue with Flash player 10 > > - // This does make some sites a higher value phishing target, but > this can be mitigated by > > - // additional referer checks. > > - if > (!"application/x-shockwave-flash".equalsIgnoreCase(results.getHeader("Content-Type"))) > { > > - response.setHeader("Content-Disposition", > "attachment;filename=p.txt"); > > - } > > } > > > > /** > > > > Modified: > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java > (original) > > +++ > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java > Tue Aug 4 22:48:08 2009 > > @@ -153,21 +153,30 @@ > > } > > } > > > > + String responseType = results.getHeader("Content-Type"); > > if (!StringUtils.isEmpty(rcr.getRewriteMimeType())) { > > String requiredType = rcr.getRewriteMimeType(); > > - String responseType = results.getHeader("Content-Type"); > > // Use a 'Vary' style check on the response > > if (requiredType.endsWith("/*") && > > !StringUtils.isEmpty(responseType)) { > > requiredType = requiredType.substring(0, requiredType.length() - > 2); > > if > (!responseType.toLowerCase().startsWith(requiredType.toLowerCase())) { > > response.setContentType(requiredType); > > + responseType = requiredType; > > } > > } else { > > response.setContentType(requiredType); > > + responseType = requiredType; > > } > > } > > > > + // We're skipping the content disposition header for flash due to an > issue with Flash player 10 > > + // This does make some sites a higher value phishing target, but > this can be mitigated by > > + // additional referer checks. > > + if (!"application/x-shockwave-flash".equalsIgnoreCase(responseType)) > { > > + response.setHeader("Content-Disposition", > "attachment;filename=p.txt"); > > + } > > + > > if (results.getHttpStatusCode() != HttpResponse.SC_OK) { > > response.sendError(results.getHttpStatusCode()); > > } > > > > Modified: > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyBaseTest.java > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyBaseTest.java?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyBaseTest.java > (original) > > +++ > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyBaseTest.java > Tue Aug 4 22:48:08 2009 > > @@ -146,7 +146,6 @@ > > // Just verify that they were set. Specific values are configurable. > > assertNotNull("Expires header not set", > recorder.getHeader("Expires")); > > assertNotNull("Cache-Control header not set", > recorder.getHeader("Cache-Control")); > > - assertEquals("attachment;filename=p.txt", > recorder.getHeader("Content-Disposition")); > > } > > > > public void testSetResponseHeadersForFlash() throws Exception { > > @@ -161,8 +160,6 @@ > > // Just verify that they were set. Specific values are configurable. > > assertNotNull("Expires header not set", > recorder.getHeader("Expires")); > > assertNotNull("Cache-Control header not set", > recorder.getHeader("Cache-Control")); > > - assertNull("Content-Disposition header set for flash", > > - recorder.getHeader("Content-Disposition")); > > } > > > > public void testSetResponseHeadersNoCache() throws Exception { > > @@ -179,7 +176,6 @@ > > assertNotNull("Expires header not set", > recorder.getHeader("Expires")); > > assertEquals("no-cache", recorder.getHeader("Pragma")); > > assertEquals("no-cache", recorder.getHeader("Cache-Control")); > > - assertEquals("attachment;filename=p.txt", > recorder.getHeader("Content-Disposition")); > > } > > > > public void testSetResponseHeadersForceParam() throws Exception { > > @@ -190,7 +186,6 @@ > > proxy.setResponseHeaders(request, recorder, results); > > > > HttpUtilTest.checkCacheControlHeaders(HttpUtilTest.testStartTime, > recorder, 30, false); > > - assertEquals("attachment;filename=p.txt", > recorder.getHeader("Content-Disposition")); > > } > > > > public void testSetResponseHeadersForceParamInvalid() throws Exception > { > > > > Modified: > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java > > URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java?rev=801008&r1=801007&r2=801008&view=diff > > > ============================================================================== > > --- > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java > (original) > > +++ > incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java > Tue Aug 4 22:48:08 2009 > > @@ -87,6 +87,7 @@ > > verify(); > > > > assertEquals(DATA_ONE, recorder.getResponseAsString()); > > + assertEquals("attachment;filename=p.txt", > recorder.getHeader("Content-Disposition")); > > assertTrue(rewriter.responseWasRewritten()); > > } > > > > @@ -108,6 +109,7 @@ > > assertEquals(Uri.parse(URL_ONE), httpRequest.getValue().getUri()); > > > > assertEquals(DATA_ONE, recorder.getResponseAsString()); > > + assertEquals("attachment;filename=p.txt", > recorder.getHeader("Content-Disposition")); > > assertTrue(rewriter.responseWasRewritten()); > > } > > > > @@ -143,6 +145,26 @@ > > > > assertEquals(contentType, recorder.getHeader("Content-Type")); > > assertEquals(magicGarbage, recorder.getHeader("X-Magic-Garbage")); > > + assertEquals("attachment;filename=p.txt", > recorder.getHeader("Content-Disposition")); > > + assertTrue(rewriter.responseWasRewritten()); > > + } > > + > > + public void testFlashGetsNoContentDisposition() throws Exception { > > + String url = "http://example.org/swiff.swf";; > > + String domain = "example.org"; > > + String contentType = "application/x-shockwave-flash"; > > + Map<String, List<String>> headers = Maps.newHashMap(); > > + headers.put("Content-Type", Arrays.asList(contentType)); > > + > > + > > expect(lockedDomainService.isSafeForOpenProxy(domain)).andReturn(true).atLeastOnce(); > > + setupProxyRequestMock(domain, url); > > + expectGetAndReturnHeaders(url, headers); > > + > > + replay(); > > + > > + proxyHandler.fetch(request, recorder); > > + assertEquals(contentType, recorder.getHeader("Content-Type")); > > + assertNull("Content-disposition set for flash", > recorder.getHeader("Content-Disposition")); > > assertTrue(rewriter.responseWasRewritten()); > > } > > > > > > > > >
