This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13316 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-security.git
commit 4bc3c342428b5200ef50aa4c512b45b81b119632 Author: Joerg Hoh <[email protected]> AuthorDate: Wed Aug 19 17:11:46 2026 +0200 SLING-13316 Improvements to resource adaption logic --- .../security/impl/ContentDispositionFilter.java | 166 ++++++++-- .../impl/ContentDispositionFilterTest.java | 346 +++++++++++++++++++++ 2 files changed, 483 insertions(+), 29 deletions(-) diff --git a/src/main/java/org/apache/sling/security/impl/ContentDispositionFilter.java b/src/main/java/org/apache/sling/security/impl/ContentDispositionFilter.java index 039635d..f9f3005 100644 --- a/src/main/java/org/apache/sling/security/impl/ContentDispositionFilter.java +++ b/src/main/java/org/apache/sling/security/impl/ContentDispositionFilter.java @@ -22,10 +22,13 @@ import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; import javax.servlet.ServletException; +import javax.servlet.ServletOutputStream; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import java.io.IOException; +import java.io.InputStream; +import java.io.PrintWriter; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -183,6 +186,8 @@ public class ContentDispositionFilter implements Filter { private static final String CONTENT_DISPOSTION_ATTACHMENT = "attachment"; + private static final String CONTENT_TYPE = "Content-Type"; + private static final String PROP_JCR_DATA = "jcr:data"; private static final String JCR_CONTENT_LEAF = "jcr:content"; @@ -197,6 +202,16 @@ public class ContentDispositionFilter implements Filter { private final Resource resource; + /** + * The content type this wrapper has already evaluated the + * Content-Disposition header for. The {@link #ATTRIBUTE_NAME} request + * attribute alone is not sufficient: the attribute is shared with the + * wrappers created for nested (forward) dispatches, and a value cached + * by an outer dispatch for a different resource must not suppress the + * header evaluation for this wrapper's own resource. + */ + private String evaluatedContentType; + public RewriterResponse(SlingHttpServletRequest request, SlingHttpServletResponse wrappedResponse) { super(wrappedResponse); this.request = request; @@ -206,6 +221,7 @@ public class ContentDispositionFilter implements Filter { @Override public void reset() { request.removeAttribute(ATTRIBUTE_NAME); + this.evaluatedContentType = null; super.reset(); } @@ -217,57 +233,136 @@ public class ContentDispositionFilter implements Filter { if (supportedMethods.contains(request.getMethod())) { String previousContentType = (String) request.getAttribute(ATTRIBUTE_NAME); - if (previousContentType != null && previousContentType.equals(type)) { + // only skip the evaluation if THIS wrapper has already + // evaluated the header decision for this content type: the + // request attribute may have been populated by the wrapper of + // another (outer) dispatch for a different resource + if (previousContentType != null + && previousContentType.equals(type) + && previousContentType.equals(this.evaluatedContentType)) { super.setContentType(type); return; } + this.evaluatedContentType = type; request.setAttribute(ATTRIBUTE_NAME, type); - String resourcePath = resource.getPath(); + applyContentDisposition(type); + } + super.setContentType(type); + } - if (!contentDispositionExcludedPaths.contains(resourcePath)) { + /** + * Setting the "Content-Type" header is equivalent to calling + * {@link #setContentType(String)} and must be mediated the same way. + * + * @see javax.servlet.http.HttpServletResponseWrapper#setHeader(java.lang.String, java.lang.String) + */ + @Override + public void setHeader(String name, String value) { + if (CONTENT_TYPE.equalsIgnoreCase(name)) { + this.setContentType(value); + return; + } + super.setHeader(name, value); + } - if (enableContentDispositionAllPaths) { - setContentDisposition(resource); - } else { + /** + * @see javax.servlet.http.HttpServletResponseWrapper#addHeader(java.lang.String, java.lang.String) + */ + @Override + public void addHeader(String name, String value) { + if (CONTENT_TYPE.equalsIgnoreCase(name)) { + this.setContentType(value); + return; + } + super.addHeader(name, value); + } - boolean contentDispositionAdded = false; - if (contentDispositionPaths.contains(resourcePath)) { + /** + * the content disposition decision is evaluated before the body can be written. + * + * @see javax.servlet.ServletResponseWrapper#getOutputStream() + */ + @Override + public ServletOutputStream getOutputStream() throws IOException { + this.ensureContentDispositionApplied(); + return super.getOutputStream(); + } - if (contentTypesMapping.containsKey(resourcePath)) { - Set<String> exceptions = contentTypesMapping.get(resourcePath); - if (!exceptions.contains(type)) { - contentDispositionAdded = setContentDisposition(resource); - } - } else { + /** + * @see javax.servlet.ServletResponseWrapper#getWriter() + */ + @Override + public PrintWriter getWriter() throws IOException { + this.ensureContentDispositionApplied(); + return super.getWriter(); + } + + // ---------- PRIVATE METHODS --------- + + private void ensureContentDispositionApplied() { + if (supportedMethods.contains(request.getMethod()) && request.getAttribute(ATTRIBUTE_NAME) == null) { + applyContentDisposition(this.getContentType()); + } + } + + private void applyContentDisposition(final String type) { + String resourcePath = resource.getPath(); + + // A file's jcr:content child node carries the file's binary + // (jcr:data) directly and thus serves the very same bytes + // under a second address. Match such a resource against the + // configured exact path of the file itself as well, so that + // /path/file.ext/jcr:content cannot bypass an exact entry + // protecting /path/file.ext. + String configMatchPath = resourcePath; + if (configMatchPath.endsWith("/" + JCR_CONTENT_LEAF)) { + configMatchPath = + configMatchPath.substring(0, configMatchPath.length() - JCR_CONTENT_LEAF.length() - 1); + } + + if (!contentDispositionExcludedPaths.contains(resourcePath)) { + + if (enableContentDispositionAllPaths) { + setContentDisposition(resource); + } else { + + boolean contentDispositionAdded = false; + if (contentDispositionPaths.contains(resourcePath) + || contentDispositionPaths.contains(configMatchPath)) { + + String mappingKey = + contentTypesMapping.containsKey(resourcePath) ? resourcePath : configMatchPath; + if (contentTypesMapping.containsKey(mappingKey)) { + Set<String> exceptions = contentTypesMapping.get(mappingKey); + if (!exceptions.contains(type)) { contentDispositionAdded = setContentDisposition(resource); } + } else { + contentDispositionAdded = setContentDisposition(resource); } - if (!contentDispositionAdded) { - for (String path : contentDispositionPathsPfx) { - if (resourcePath.startsWith(path)) { - if (contentTypesMapping.containsKey(path)) { - Set<String> exceptions = contentTypesMapping.get(path); - if (!exceptions.contains(type)) { - setContentDisposition(resource); - break; - } - } else { + } + if (!contentDispositionAdded) { + for (String path : contentDispositionPathsPfx) { + if (resourcePath.startsWith(path)) { + if (contentTypesMapping.containsKey(path)) { + Set<String> exceptions = contentTypesMapping.get(path); + if (!exceptions.contains(type)) { setContentDisposition(resource); break; } + } else { + setContentDisposition(resource); + break; } } } } } } - super.setContentType(type); } - // ---------- PRIVATE METHODS --------- - private boolean setContentDisposition(Resource resource) { boolean contentDispositionAdded = false; if (!this.containsHeader(CONTENT_DISPOSTION) && this.isJcrData(resource)) { @@ -286,11 +381,24 @@ public class ContentDispositionFilter implements Filter { } else { Resource jcrContent = resource.getChild(JCR_CONTENT_LEAF); if (jcrContent != null) { - props = jcrContent.adaptTo(ValueMap.class); - if (props != null && props.containsKey(PROP_JCR_DATA)) { + ValueMap contentProps = jcrContent.adaptTo(ValueMap.class); + if (contentProps != null && contentProps.containsKey(PROP_JCR_DATA)) { jcrData = true; } } + if (!jcrData && props == null) { + // A resource that does not adapt to a ValueMap is not + // a node but the shape of a property resource (e.g. + // .../file/jcr:content/jcr:data), whose response body + // is the property value itself. If it streams data, + // treat it as jcr:data so the header decision fails + // closed instead of silently skipping the header. + try (InputStream is = resource.adaptTo(InputStream.class)) { + jcrData = is != null; + } catch (IOException e) { + logger.debug("Failed to close InputStream adapted from resource {}", resource, e); + } + } } } return jcrData; diff --git a/src/test/java/org/apache/sling/security/impl/ContentDispositionFilterTest.java b/src/test/java/org/apache/sling/security/impl/ContentDispositionFilterTest.java index 1493a57..b85f309 100644 --- a/src/test/java/org/apache/sling/security/impl/ContentDispositionFilterTest.java +++ b/src/test/java/org/apache/sling/security/impl/ContentDispositionFilterTest.java @@ -18,6 +18,8 @@ */ package org.apache.sling.security.impl; +import java.io.ByteArrayInputStream; +import java.io.InputStream; import java.lang.annotation.Annotation; import java.util.Map; import java.util.Set; @@ -722,6 +724,142 @@ public class ContentDispositionFilterTest { Assert.assertEquals(1, counter.intValue()); } + /** + * A forward dispatch creates a new RewriterResponse for the forwarded-to + * resource. A content type cached in the request attribute by the wrapper + * of the outer dispatch (for a different, unprotected resource) must not + * suppress the Content-Disposition evaluation for the protected resource + * of this wrapper. + * @throws Throwable + */ + @Test + public void test_doFilterForwardReevaluatesForNewResource() throws Throwable { + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + final Resource resource = context.mock(Resource.class, "resource"); + final ValueMap properties = context.mock(ValueMap.class); + callActivateWithConfiguration(new String[] {"/content/usergenerated"}, new String[] {""}); + + final AtomicInteger counter = new AtomicInteger(); + + context.checking(new Expectations() { + { + allowing(request).getMethod(); + will(returnValue("GET")); + allowing(response).containsHeader("Content-Disposition"); + will(returnValue(false)); + // the outer dispatch already cached the same content type for + // a different (unprotected) resource + allowing(request).getAttribute(RewriterResponse.ATTRIBUTE_NAME); + will(returnValue("text/html")); + allowing(request).setAttribute(RewriterResponse.ATTRIBUTE_NAME, "text/html"); + allowing(request).getResource(); + will(returnValue(resource)); + allowing(resource).getPath(); + will(returnValue("/content/usergenerated")); + allowing(resource).adaptTo(ValueMap.class); + will(returnValue(properties)); + allowing(properties).containsKey(PROP_JCR_DATA); + will(returnValue(true)); + allowing(response).setContentType("text/html"); + // CONTENT DISPOSITION IS SET despite the cached content type + exactly(1).of(response).addHeader("Content-Disposition", "attachment"); + } + }); + final ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response) { + @Override + public void addHeader(String name, String value) { + counter.incrementAndGet(); + } + }; + rewriterResponse.setContentType("text/html"); + Assert.assertEquals(1, counter.intValue()); + } + + /** + * An exact (non-wildcard) protected path must also cover the file's + * jcr:content child node, which serves the same binary (jcr:data) under a + * second address. + * @throws Throwable + */ + @Test + public void test_doFilterExactPathCoversJcrContentChild() throws Throwable { + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + final Resource resource = context.mock(Resource.class, "resource"); + final ValueMap properties = context.mock(ValueMap.class); + callActivateWithConfiguration(new String[] {"/content/usergenerated/file.svg"}, new String[] {""}); + + final AtomicInteger counter = new AtomicInteger(); + + context.checking(new Expectations() { + { + allowing(request).getMethod(); + will(returnValue("GET")); + allowing(response).containsHeader("Content-Disposition"); + will(returnValue(false)); + allowing(request).getAttribute(RewriterResponse.ATTRIBUTE_NAME); + will(returnValue(null)); + allowing(request).setAttribute(RewriterResponse.ATTRIBUTE_NAME, "image/svg+xml"); + allowing(request).getResource(); + will(returnValue(resource)); + // the jcr:content child of the protected file serves the same bytes + allowing(resource).getPath(); + will(returnValue("/content/usergenerated/file.svg/jcr:content")); + allowing(resource).adaptTo(ValueMap.class); + will(returnValue(properties)); + allowing(properties).containsKey(PROP_JCR_DATA); + will(returnValue(true)); + allowing(response).setContentType("image/svg+xml"); + // CONTENT DISPOSITION IS SET + exactly(1).of(response).addHeader("Content-Disposition", "attachment"); + } + }); + final ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response) { + @Override + public void addHeader(String name, String value) { + counter.incrementAndGet(); + } + }; + rewriterResponse.setContentType("image/svg+xml"); + Assert.assertEquals(1, counter.intValue()); + } + + /** + * The content-type exceptions configured for an exact protected path apply + * to the file's jcr:content child address as well. + * @throws Throwable + */ + @Test + public void test_doFilterExactPathContentTypeMappingCoversJcrContentChild() throws Throwable { + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + final Resource resource = context.mock(Resource.class, "resource"); + callActivateWithConfiguration(new String[] {"/content/usergenerated/file.svg:image/jpeg"}, new String[] {""}); + + context.checking(new Expectations() { + { + allowing(request).getMethod(); + will(returnValue("GET")); + allowing(request).getAttribute(RewriterResponse.ATTRIBUTE_NAME); + will(returnValue(null)); + allowing(request).setAttribute(RewriterResponse.ATTRIBUTE_NAME, "image/jpeg"); + allowing(request).getResource(); + will(returnValue(resource)); + allowing(resource).getPath(); + will(returnValue("/content/usergenerated/file.svg/jcr:content")); + allowing(response).setContentType("image/jpeg"); + // CONTENT DISPOSITION MUST NOT SET (excepted content type) + never(response).addHeader("Content-Disposition", "attachment"); + } + }); + ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response); + rewriterResponse.setContentType("image/jpeg"); + } + /** * Test repeated setContentType calls don't add multiple headers, case 1 resetting the same mimetype * @throws Throwable @@ -1395,6 +1533,8 @@ public class ContentDispositionFilterTest { will(returnValue(null)); allowing(resource).getChild(JCR_CONTENT_LEAF); will(returnValue(null)); + allowing(resource).adaptTo(InputStream.class); + will(returnValue(null)); } }); final ContentDispositionFilter.RewriterResponse rewriterResponse = @@ -1406,6 +1546,41 @@ public class ContentDispositionFilterTest { Assert.assertFalse(result); } + /** + * A property resource (e.g. .../file/jcr:content/jcr:data) does not adapt + * to a ValueMap and has no jcr:content child, but its response body is the + * repository binary itself - it must be treated as jcr:data. + */ + @Test + public void test_isJcrData8() throws Throwable { + callActivateWithConfiguration(new String[] {"/content/usergenerated"}, new String[] {"/content/usergenerated"}); + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + + final Resource resource = context.mock(Resource.class); + final InputStream stream = new ByteArrayInputStream(new byte[0]); + + context.checking(new Expectations() { + { + allowing(request).getResource(); + will(returnValue(resource)); + allowing(resource).adaptTo(ValueMap.class); + will(returnValue(null)); + allowing(resource).getChild(JCR_CONTENT_LEAF); + will(returnValue(null)); + allowing(resource).adaptTo(InputStream.class); + will(returnValue(stream)); + } + }); + final ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response); + + Boolean result = (Boolean) PrivateAccessor.invoke( + rewriterResponse, "isJcrData", new Class[] {Resource.class}, new Object[] {resource}); + + Assert.assertTrue(result); + } + @Test public void test_isJcrData7() throws Throwable { callActivateWithConfiguration(new String[] {"/content/usergenerated"}, new String[] {"/content/usergenerated"}); @@ -1438,4 +1613,175 @@ public class ContentDispositionFilterTest { Assert.assertFalse(result); } + + /** + * Regression: setting the media type via setHeader("Content-Type", ...) is equivalent + * to setContentType(...) and must be mediated the same way (it used to bypass the filter). + */ + @Test + public void test_setHeaderContentTypeIsMediated() throws Throwable { + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + final Resource resource = context.mock(Resource.class, "resource"); + final ValueMap properties = context.mock(ValueMap.class); + callActivateWithConfiguration(new String[] {"/content/usergenerated"}, new String[] {""}); + + final AtomicInteger counter = new AtomicInteger(); + + context.checking(new Expectations() { + { + allowing(request).getMethod(); + will(returnValue("GET")); + allowing(response).containsHeader("Content-Disposition"); + will(returnValue(false)); + allowing(request).getAttribute(RewriterResponse.ATTRIBUTE_NAME); + will(returnValue(null)); + allowing(request).setAttribute(RewriterResponse.ATTRIBUTE_NAME, "image/svg+xml"); + allowing(request).getResource(); + will(returnValue(resource)); + allowing(resource).getPath(); + will(returnValue("/content/usergenerated")); + allowing(resource).adaptTo(ValueMap.class); + will(returnValue(properties)); + allowing(properties).containsKey(PROP_JCR_DATA); + will(returnValue(true)); + allowing(response).setContentType("image/svg+xml"); + } + }); + final ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response) { + @Override + public void addHeader(String name, String value) { + counter.incrementAndGet(); + } + }; + rewriterResponse.setHeader("Content-Type", "image/svg+xml"); + Assert.assertEquals(1, counter.intValue()); + } + + /** + * Regression: the header name comparison must be case-insensitive. + */ + @Test + public void test_setHeaderContentTypeIsMediatedCaseInsensitive() throws Throwable { + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + final Resource resource = context.mock(Resource.class, "resource"); + final ValueMap properties = context.mock(ValueMap.class); + callActivateWithConfiguration(new String[] {"/content/usergenerated"}, new String[] {""}); + + final AtomicInteger counter = new AtomicInteger(); + + context.checking(new Expectations() { + { + allowing(request).getMethod(); + will(returnValue("GET")); + allowing(response).containsHeader("Content-Disposition"); + will(returnValue(false)); + allowing(request).getAttribute(RewriterResponse.ATTRIBUTE_NAME); + will(returnValue(null)); + allowing(request).setAttribute(RewriterResponse.ATTRIBUTE_NAME, "text/html"); + allowing(request).getResource(); + will(returnValue(resource)); + allowing(resource).getPath(); + will(returnValue("/content/usergenerated")); + allowing(resource).adaptTo(ValueMap.class); + will(returnValue(properties)); + allowing(properties).containsKey(PROP_JCR_DATA); + will(returnValue(true)); + allowing(response).setContentType("text/html"); + } + }); + final ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response) { + @Override + public void addHeader(String name, String value) { + counter.incrementAndGet(); + } + }; + rewriterResponse.setHeader("content-type", "text/html"); + Assert.assertEquals(1, counter.intValue()); + } + + /** + * Regression: addHeader("Content-Type", ...) must be mediated as well. + */ + @Test + public void test_addHeaderContentTypeIsMediated() throws Throwable { + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + final Resource resource = context.mock(Resource.class, "resource"); + final ValueMap properties = context.mock(ValueMap.class); + callActivateWithConfiguration(new String[] {"/content/usergenerated"}, new String[] {""}); + + context.checking(new Expectations() { + { + allowing(request).getMethod(); + will(returnValue("GET")); + allowing(response).containsHeader("Content-Disposition"); + will(returnValue(false)); + allowing(request).getAttribute(RewriterResponse.ATTRIBUTE_NAME); + will(returnValue(null)); + allowing(request).setAttribute(RewriterResponse.ATTRIBUTE_NAME, "text/html"); + allowing(request).getResource(); + will(returnValue(resource)); + allowing(resource).getPath(); + will(returnValue("/content/usergenerated")); + allowing(resource).adaptTo(ValueMap.class); + will(returnValue(properties)); + allowing(properties).containsKey(PROP_JCR_DATA); + will(returnValue(true)); + allowing(response).setContentType("text/html"); + // CONTENT DISPOSITION IS SET + exactly(1).of(response).addHeader("Content-Disposition", "attachment"); + } + }); + final ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response); + rewriterResponse.addHeader("Content-Type", "text/html"); + context.assertIsSatisfied(); + } + + /** + * Regression: a response which never declares a content type (browsers would + * content-sniff it) must still get the Content-Disposition header before the + * body can be written. + */ + @Test + public void test_typelessResponseIsMediatedOnOutput() throws Throwable { + final SlingHttpServletRequest request = context.mock(SlingHttpServletRequest.class); + final SlingHttpServletResponse response = context.mock(SlingHttpServletResponse.class); + final Resource resource = context.mock(Resource.class, "resource"); + final ValueMap properties = context.mock(ValueMap.class); + callActivateWithConfiguration(new String[] {"/content/usergenerated"}, new String[] {""}); + + context.checking(new Expectations() { + { + allowing(request).getMethod(); + will(returnValue("GET")); + allowing(request).getAttribute(RewriterResponse.ATTRIBUTE_NAME); + will(returnValue(null)); + allowing(request).getResource(); + will(returnValue(resource)); + allowing(resource).getPath(); + will(returnValue("/content/usergenerated")); + allowing(resource).adaptTo(ValueMap.class); + will(returnValue(properties)); + allowing(properties).containsKey(PROP_JCR_DATA); + will(returnValue(true)); + allowing(response).getContentType(); + will(returnValue(null)); + allowing(response).containsHeader("Content-Disposition"); + will(returnValue(false)); + allowing(response).getOutputStream(); + will(returnValue(null)); + // CONTENT DISPOSITION IS SET + exactly(1).of(response).addHeader("Content-Disposition", "attachment"); + } + }); + final ContentDispositionFilter.RewriterResponse rewriterResponse = + contentDispositionFilter.new RewriterResponse(request, response); + rewriterResponse.getOutputStream(); + context.assertIsSatisfied(); + } }
