mraible commented on code in PR #174: URL: https://github.com/apache/roller/pull/174#discussion_r3891453061
########## 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/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/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/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]
