mraible commented on code in PR #174:
URL: https://github.com/apache/roller/pull/174#discussion_r3891453059
##########
app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/ResourceServlet.java:
##########
@@ -159,8 +162,19 @@ public void doGet(HttpServletRequest request,
HttpServletResponse response)
}
// set the content type based on whatever is in our web.xml mime defs
- response.setContentType(this.context.getMimeType(resourceRequest
- .getResourcePath()));
+ String resourceType = this.context.getMimeType(
+ resourceRequest.getResourcePath());
+ if (fromUploadedMedia) {
+ // Uploaded through the media library, so it is governed by the
Review Comment:
This branch is also where customized-theme resources land:
`WeblogCustomTheme.getResource()` looks up the media file but never assigns it
to `resource`, so it always returns null, and `importTheme` stores theme CSS/JS
as media files with their original path. `resourceType` for `css/bootstrap.css`
is `text/css`, which isn't inline-safe, so the stylesheet goes out as an
octet-stream attachment with `nosniff` and the browser drops it. Fixing
`WeblogCustomTheme.getResource` to return the media file would route these
through the theme branch above; alternatively, treat `text/css` /
`text/javascript` from the servlet-context mime table as theme-authored here as
the description already promises.
##########
app/src/main/java/org/apache/roller/weblogger/util/MediaTypePolicy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License. For additional
+ * information regarding copyright in this work, please see the NOTICE
+ * file in the top level directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.util;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import javax.servlet.http.HttpServletResponse;
+
+/**
+ * Decides what type an uploaded file is stored as, and how it is served back.
+ *
+ * <p>A client uploading a file states a type, but the stored type is derived
+ * from the file name. The declared value is a hint only, consulted where the
+ * name yields nothing, and it cannot introduce a type the browser would
+ * execute.
+ *
+ * <p>Serving applies the second half. Only a short list of formats that
+ * browsers render passively are sent inline; everything else is sent as an
+ * attachment, and {@code nosniff} accompanies every response so browsers do
+ * not substitute their own type guess.
+ */
+public final class MediaTypePolicy {
+
+ private MediaTypePolicy() {
+ }
+
+ public static final String DEFAULT_TYPE = "application/octet-stream";
+
+ /**
+ * Formats browsers render without executing anything the file carries.
+ * SVG is deliberately absent: it is an XML document that can carry script.
+ */
+ private static final Set<String> INLINE_TYPES =
Collections.unmodifiableSet(
Review Comment:
Worth listing what this drops compared to
`response.setContentType(mediaFile.getContentType())` on master. Existing
`image/svg+xml` media (already stored, already embedded in entries with
`<img>`) now downloads instead of rendering, since browsers never sniff SVG;
and `image/x-png` (legacy IE uploads), `image/jpg` (some XML-RPC clients),
`image/avif` and `image/apng` are all passive but absent here. If SVG stays out
(defensible, it can carry script), the release note should say so explicitly,
and I'd add the other image subtypes.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileEdit.java:
##########
@@ -124,7 +125,10 @@ public String save() {
if (uploadedFile != null) {
mediaFile.setLength(this.uploadedFile.length());
- mediaFile.setContentType(this.uploadedFileContentType);
+ // Replacing the body re-decides the type, on the same
+ // terms as the original upload.
+ mediaFile.setContentType(MediaTypePolicy.storedTypeFor(
+ mediaFile.getName(),
this.uploadedFileContentType));
Review Comment:
On replace this derives the type from the record's (old) name rather than
the uploaded replacement's name (`this.uploadedFileName` is right there).
Replacing `photo.jpg` with `photo.png` without renaming stores `image/jpeg` for
PNG bytes and serves them with `nosniff`; replacing `clip.txt` with a video
keeps `text/plain` and forces a download. `putMedia` in `MediaCollection` has
the same mismatch with `mf.getName()`.
##########
app/src/main/java/org/apache/roller/weblogger/util/MediaTypePolicy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License. For additional
+ * information regarding copyright in this work, please see the NOTICE
+ * file in the top level directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.util;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import javax.servlet.http.HttpServletResponse;
+
+/**
+ * Decides what type an uploaded file is stored as, and how it is served back.
+ *
+ * <p>A client uploading a file states a type, but the stored type is derived
+ * from the file name. The declared value is a hint only, consulted where the
+ * name yields nothing, and it cannot introduce a type the browser would
+ * execute.
+ *
+ * <p>Serving applies the second half. Only a short list of formats that
+ * browsers render passively are sent inline; everything else is sent as an
+ * attachment, and {@code nosniff} accompanies every response so browsers do
+ * not substitute their own type guess.
+ */
+public final class MediaTypePolicy {
+
+ private MediaTypePolicy() {
+ }
+
+ public static final String DEFAULT_TYPE = "application/octet-stream";
+
+ /**
+ * Formats browsers render without executing anything the file carries.
+ * SVG is deliberately absent: it is an XML document that can carry script.
+ */
+ private static final Set<String> INLINE_TYPES =
Collections.unmodifiableSet(
+ new HashSet<>(Arrays.asList(
+ "image/jpeg", "image/pjpeg", "image/png", "image/gif",
+ "image/bmp", "image/x-ms-bmp", "image/webp", "image/tiff",
+ "image/x-icon", "image/vnd.microsoft.icon",
+ "application/pdf")));
+
+ /** Families served inline whatever the subtype. */
+ private static final String[] INLINE_PREFIXES = {"audio/", "video/"};
+
+ /**
+ * Types a browser may execute, or that can carry something it will. These
+ * are never adopted from a client's declaration.
+ */
+ private static final Set<String> ACTIVE_TYPES =
Collections.unmodifiableSet(
+ new HashSet<>(Arrays.asList(
+ "text/html", "application/xhtml+xml", "application/xhtml",
+ "image/svg+xml", "text/xml", "application/xml",
+ "text/javascript", "application/javascript",
+ "application/ecmascript", "text/ecmascript",
+ "text/vbscript", "application/x-shockwave-flash",
+ "text/xsl", "application/xslt+xml")));
+
+ /**
+ * @param fileName the uploaded file's name
+ * @param declaredType the type the client said it was, may be null
+ * @return the type to store: derived from the name where that is
+ * conclusive, otherwise the declared type if it is not one a
+ * browser would act on, otherwise the generic binary type
+ */
+ public static String storedTypeFor(String fileName, String declaredType) {
Review Comment:
Because active declared types collapse to `application/octet-stream`
*before* `canSave` / `checkFileType` run, an admin's content-type rules in
`uploads.types.forbid` (e.g. `image/svg+xml,application/xhtml+xml`) no longer
match those uploads; the file is stored under the generic type instead of being
refused. Run the forbid check against the declared type too, or document that
forbid rules are now extension-based.
##########
app/src/main/java/org/apache/roller/weblogger/util/MediaTypePolicy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License. For additional
+ * information regarding copyright in this work, please see the NOTICE
+ * file in the top level directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.util;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import javax.servlet.http.HttpServletResponse;
+
+/**
+ * Decides what type an uploaded file is stored as, and how it is served back.
+ *
+ * <p>A client uploading a file states a type, but the stored type is derived
+ * from the file name. The declared value is a hint only, consulted where the
+ * name yields nothing, and it cannot introduce a type the browser would
+ * execute.
+ *
+ * <p>Serving applies the second half. Only a short list of formats that
+ * browsers render passively are sent inline; everything else is sent as an
+ * attachment, and {@code nosniff} accompanies every response so browsers do
+ * not substitute their own type guess.
+ */
+public final class MediaTypePolicy {
+
+ private MediaTypePolicy() {
+ }
+
+ public static final String DEFAULT_TYPE = "application/octet-stream";
+
+ /**
+ * Formats browsers render without executing anything the file carries.
+ * SVG is deliberately absent: it is an XML document that can carry script.
+ */
+ private static final Set<String> INLINE_TYPES =
Collections.unmodifiableSet(
+ new HashSet<>(Arrays.asList(
+ "image/jpeg", "image/pjpeg", "image/png", "image/gif",
+ "image/bmp", "image/x-ms-bmp", "image/webp", "image/tiff",
+ "image/x-icon", "image/vnd.microsoft.icon",
+ "application/pdf")));
+
+ /** Families served inline whatever the subtype. */
+ private static final String[] INLINE_PREFIXES = {"audio/", "video/"};
+
+ /**
+ * Types a browser may execute, or that can carry something it will. These
+ * are never adopted from a client's declaration.
+ */
+ private static final Set<String> ACTIVE_TYPES =
Collections.unmodifiableSet(
+ new HashSet<>(Arrays.asList(
+ "text/html", "application/xhtml+xml", "application/xhtml",
+ "image/svg+xml", "text/xml", "application/xml",
+ "text/javascript", "application/javascript",
+ "application/ecmascript", "text/ecmascript",
+ "text/vbscript", "application/x-shockwave-flash",
+ "text/xsl", "application/xslt+xml")));
+
+ /**
+ * @param fileName the uploaded file's name
+ * @param declaredType the type the client said it was, may be null
+ * @return the type to store: derived from the name where that is
+ * conclusive, otherwise the declared type if it is not one a
+ * browser would act on, otherwise the generic binary type
+ */
+ public static String storedTypeFor(String fileName, String declaredType) {
+ String derived = normalize(deriveFromName(fileName));
+ if (isConclusive(derived)) {
+ return derived;
+ }
+
+ String declared = normalize(declaredType);
+ if (isConclusive(declared) && !isActive(declared)) {
+ return declared;
+ }
+
+ return DEFAULT_TYPE;
+ }
+
+ /** @return true when browsers render this type without executing it */
+ public static boolean isInlineSafe(String contentType) {
Review Comment:
`text/plain` with `nosniff` can't execute anything, so uploaded `.txt` files
(very common as attachments) could stay inline rather than becoming a save-as
dialog for an `application/octet-stream` `notes.txt`.
##########
app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/PreviewResourceServlet.java:
##########
@@ -160,8 +163,19 @@ public void doGet(HttpServletRequest request,
HttpServletResponse response)
}
// set the content type based on whatever is in our web.xml mime defs
- response.setContentType(this.context.getMimeType(resourceRequest
- .getResourcePath()));
+ String resourceType = this.context.getMimeType(
+ resourceRequest.getResourcePath());
+ if (fromUploadedMedia) {
+ // Uploaded through the media library, so it is governed by the
+ // same policy as any other media response.
+ MediaTypePolicy.applyResponseHeaders(response, resourceType,
Review Comment:
`context.getMimeType()` returns null for extensions not mapped in
`web.xml`/the container, and `applyResponseHeaders` turns null into an
octet-stream attachment. On master `setContentType(null)` left the type unset
and the browser could still display the file; now any unmapped (or uppercase,
on a case-sensitive container) extension downloads in the theme preview.
##########
app/src/main/java/org/apache/roller/weblogger/util/MediaTypePolicy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License. For additional
+ * information regarding copyright in this work, please see the NOTICE
+ * file in the top level directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.util;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import javax.servlet.http.HttpServletResponse;
+
+/**
+ * Decides what type an uploaded file is stored as, and how it is served back.
+ *
+ * <p>A client uploading a file states a type, but the stored type is derived
+ * from the file name. The declared value is a hint only, consulted where the
+ * name yields nothing, and it cannot introduce a type the browser would
+ * execute.
+ *
+ * <p>Serving applies the second half. Only a short list of formats that
+ * browsers render passively are sent inline; everything else is sent as an
+ * attachment, and {@code nosniff} accompanies every response so browsers do
+ * not substitute their own type guess.
+ */
+public final class MediaTypePolicy {
+
+ private MediaTypePolicy() {
+ }
+
+ public static final String DEFAULT_TYPE = "application/octet-stream";
+
+ /**
+ * Formats browsers render without executing anything the file carries.
+ * SVG is deliberately absent: it is an XML document that can carry script.
+ */
+ private static final Set<String> INLINE_TYPES =
Collections.unmodifiableSet(
+ new HashSet<>(Arrays.asList(
+ "image/jpeg", "image/pjpeg", "image/png", "image/gif",
+ "image/bmp", "image/x-ms-bmp", "image/webp", "image/tiff",
+ "image/x-icon", "image/vnd.microsoft.icon",
+ "application/pdf")));
+
+ /** Families served inline whatever the subtype. */
+ private static final String[] INLINE_PREFIXES = {"audio/", "video/"};
+
+ /**
+ * Types a browser may execute, or that can carry something it will. These
+ * are never adopted from a client's declaration.
+ */
+ private static final Set<String> ACTIVE_TYPES =
Collections.unmodifiableSet(
+ new HashSet<>(Arrays.asList(
+ "text/html", "application/xhtml+xml", "application/xhtml",
+ "image/svg+xml", "text/xml", "application/xml",
+ "text/javascript", "application/javascript",
+ "application/ecmascript", "text/ecmascript",
+ "text/vbscript", "application/x-shockwave-flash",
+ "text/xsl", "application/xslt+xml")));
+
+ /**
+ * @param fileName the uploaded file's name
+ * @param declaredType the type the client said it was, may be null
+ * @return the type to store: derived from the name where that is
+ * conclusive, otherwise the declared type if it is not one a
+ * browser would act on, otherwise the generic binary type
+ */
+ public static String storedTypeFor(String fileName, String declaredType) {
+ String derived = normalize(deriveFromName(fileName));
+ if (isConclusive(derived)) {
+ return derived;
+ }
+
+ String declared = normalize(declaredType);
+ if (isConclusive(declared) && !isActive(declared)) {
+ return declared;
+ }
+
+ return DEFAULT_TYPE;
+ }
+
+ /** @return true when browsers render this type without executing it */
+ public static boolean isInlineSafe(String contentType) {
+ String type = normalize(contentType);
+ if (type == null) {
+ return false;
+ }
+ if (INLINE_TYPES.contains(type)) {
+ return true;
+ }
+ for (String prefix : INLINE_PREFIXES) {
+ if (type.startsWith(prefix)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /** @return true when a browser may execute this type, or script inside it
*/
+ public static boolean isActive(String contentType) {
+ String type = normalize(contentType);
+ if (type == null) {
+ return false;
+ }
+ return ACTIVE_TYPES.contains(type) || type.endsWith("+xml");
+ }
+
+ /**
+ * Sets the type and the headers that govern how the response is treated.
+ * Anything outside the inline list is marked as an attachment.
+ */
+ public static void applyResponseHeaders(HttpServletResponse response,
+ String contentType, String
fileName) {
+ response.setHeader("X-Content-Type-Options", "nosniff");
+
+ String type = normalize(contentType);
+ if (type == null) {
+ type = DEFAULT_TYPE;
+ }
+
+ if (isInlineSafe(type)) {
+ response.setContentType(type);
+ return;
+ }
+
+ // Served as bytes to be saved rather than a document to be rendered.
+ response.setContentType(DEFAULT_TYPE);
+ response.setHeader("Content-Disposition",
+ "attachment; filename=\"" + headerSafe(fileName) + "\"");
+ }
+
+ private static String deriveFromName(String fileName) {
+ if (fileName == null || fileName.trim().isEmpty()) {
+ return null;
+ }
+ try {
+ return Utilities.getContentTypeFromFileName(fileName);
+ } catch (Exception undetermined) {
Review Comment:
`Utilities.getContentTypeFromFileName` is backed by `javax.activation`'s
default map, which knows about 22 extensions (no `pdf`, `svg`, `webp`, `mp4`,
`mp3`, `zip`, `css`, `js`). So "the name decides" only holds for those; for
everything else the declared type is stored after all (a `report.pdf` declared
`application/zip` is stored as zip and downloads). The servlets already use the
servlet context's mime table (`web.xml` has a full one); the policy should
consult the same table.
##########
app/src/main/java/org/apache/roller/weblogger/webservices/xmlrpc/MetaWeblogAPIHandler.java:
##########
@@ -381,7 +382,7 @@ public Object newMediaObject(String blogid, String userid,
String password,
mf.setDirectory(root);
mf.setWeblog(website);
mf.setName(name);
- mf.setContentType(type);
+ mf.setContentType(MediaTypePolicy.storedTypeFor(name, type));
Review Comment:
Previously `mf.setContentType(null)` made `checkFileType` reject an upload
with no `type` member; `storedTypeFor(name, null)` now always returns a type,
so the upload is accepted as `application/octet-stream` when the allow list is
empty (the default). Probably fine, but it's a loosening the description should
mention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
