Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6702

Update SimpleContentViewHandler to return mime type on file extension and use inline for content-disposition

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Implemented
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: content
    • Labels:
      None

      Description

      SimpleContentViewHandler will return mime type 'text/html' for all DataResource values without a specified mimeTypeId. Changing to DataResourceWorker.getMimeType will allow determining the mimeTypeId by file extension

      Fixing the mime type will allow the browsers to display content inline if UtilHttp is updated aswell. All unknown extensions will be set to octet-stream causing the browser to prompt for download

      1. ContentDisposition.patch
        5 kB
        Gareth Carter
      2. OFBIZ-6702.patch
        6 kB
        Jacques Le Roux
      3. OFBIZ-6702-2.patch
        6 kB
        Gareth Carter
      4. SimpleContentViewHandler.java.patch
        0.8 kB
        Gareth Carter

        Activity

        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        HI Gareth, I'm not sure replacing Content-Disposition/attachment by Content-Disposition/inline is a good idea for security reason.

        This is old but interesting http://forums.mozillazine.org/viewtopic.php?f=7&t=202891

        https://stackoverflow.com/questions/1012437/uses-of-content-disposition-in-an-http-response-header gives some information.

        Note that even attachment is not completly safe http://i8jesus.com/2009/07/26/content-disposition-is-not-a-security-mechanism/ though in recent browsers I believe it's better (if they follow RFC 6266)

        You can find more Googling for "Content-Disposition security"

        Show
        jacques.le.roux Jacques Le Roux added a comment - HI Gareth, I'm not sure replacing Content-Disposition/attachment by Content-Disposition/inline is a good idea for security reason. This is old but interesting http://forums.mozillazine.org/viewtopic.php?f=7&t=202891 https://stackoverflow.com/questions/1012437/uses-of-content-disposition-in-an-http-response-header gives some information. Note that even attachment is not completly safe http://i8jesus.com/2009/07/26/content-disposition-is-not-a-security-mechanism/ though in recent browsers I believe it's better (if they follow RFC 6266) You can find more Googling for "Content-Disposition security"
        Hide
        gareth.carter Gareth Carter added a comment -

        The inline change is not super important. I just felt it was better for our users to be able to view images and pdfs in the browser window (rather than force download) and thought it might be worth committing.

        Content-Disposition could be ignored by the browser (although I doubt latest mainstream browsers do not) or could be overridden by browser configuration, I know in firefox you can configure what to do based on mime type - https://support.mozilla.org/en-US/kb/applications-panel-set-how-firefox-handles-files. I do not know if inline/attachment is honoured if a specific action set

        Show
        gareth.carter Gareth Carter added a comment - The inline change is not super important. I just felt it was better for our users to be able to view images and pdfs in the browser window (rather than force download) and thought it might be worth committing. Content-Disposition could be ignored by the browser (although I doubt latest mainstream browsers do not) or could be overridden by browser configuration, I know in firefox you can configure what to do based on mime type - https://support.mozilla.org/en-US/kb/applications-panel-set-how-firefox-handles-files . I do not know if inline/attachment is honoured if a specific action set
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I noticed this Note there:

        Note: A particular configuration of the server hosting a PDF document could force the download of the file (disabling the preview).

        It seems this is related to a wrong mime type.

        I tried to understand if inline was less secure than attachment. It seems that the most important part are the filename and it's mime type, see http://blog.fox-it.com/2012/05/08/mime-sniffing-feature-or-vulnerability/ for instance to see how HTTPD is more secure than the Microsoft IIS web server. But I could not find a clear difference between the 2 options.

        I propose we still use attachment by default but provide a general property "content-disposition=attachment" which can be turned to "inline" when people want to. Something like

        #-- attachment might be replaced by inline if you prefer to offer this option to your users. 
        #   attachment is supposed to be more secure, but this is a bit unclear see OFBIZ-6702 for details 
        content-disposition=attachment
        

        Opinions?

        Show
        jacques.le.roux Jacques Le Roux added a comment - I noticed this Note there: Note: A particular configuration of the server hosting a PDF document could force the download of the file (disabling the preview). It seems this is related to a wrong mime type. I tried to understand if inline was less secure than attachment. It seems that the most important part are the filename and it's mime type, see http://blog.fox-it.com/2012/05/08/mime-sniffing-feature-or-vulnerability/ for instance to see how HTTPD is more secure than the Microsoft IIS web server. But I could not find a clear difference between the 2 options. I propose we still use attachment by default but provide a general property "content-disposition=attachment" which can be turned to "inline" when people want to. Something like #-- attachment might be replaced by inline if you prefer to offer this option to your users. # attachment is supposed to be more secure, but this is a bit unclear see OFBIZ-6702 for details content-disposition=attachment Opinions?
        Hide
        gareth.carter Gareth Carter added a comment -

        Seems like firefox will honour Content-Disposition: attachment, I am sure others will.

        If IE (or any other browser) does mime type sniffing then potentially setting Content-Disposition to inline may mean javascript could execute if the determined mime type is text/html. Ofbiz itself could sniff the mime type when files are uploaded. Even validated against the mime type determined by the file extension.

        The solution you propose is a good fit, it atleast keeps the same behaviour with the option to change. Might I suggest content.properties instead of general.properties

        Show
        gareth.carter Gareth Carter added a comment - Seems like firefox will honour Content-Disposition: attachment, I am sure others will. If IE (or any other browser) does mime type sniffing then potentially setting Content-Disposition to inline may mean javascript could execute if the determined mime type is text/html. Ofbiz itself could sniff the mime type when files are uploaded. Even validated against the mime type determined by the file extension. The solution you propose is a good fit, it atleast keeps the same behaviour with the option to change. Might I suggest content.properties instead of general.properties
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I thought about content.properties but there are other cases of Content-Disposition use in several components (9 at all), that's why I prefer to have it in general.properties.

        Show
        jacques.le.roux Jacques Le Roux added a comment - I thought about content.properties but there are other cases of Content-Disposition use in several components (9 at all), that's why I prefer to have it in general.properties.
        Hide
        gareth.carter Gareth Carter added a comment -

        general.properties it is then

        Show
        gareth.carter Gareth Carter added a comment - general.properties it is then
        Hide
        pfm.smits Pierre Smits added a comment -

        I don't think that is a good solution. The file general.properties is related to how OFBiz operates. It shouldn't be so that it contains settings regarding a specific component that is not part of the framework stack.

        Besides, what is wrong with pointing to settings in the content component?

        Show
        pfm.smits Pierre Smits added a comment - I don't think that is a good solution. The file general.properties is related to how OFBiz operates. It shouldn't be so that it contains settings regarding a specific component that is not part of the framework stack. Besides, what is wrong with pointing to settings in the content component?
        Hide
        gareth.carter Gareth Carter added a comment -

        I initially said content because of SimpleContentViewHandler but could webapp be an alternative? Since Content-Disposition only relates to http. I have no preference here

        I have found 7 references in code (that actually set it on the HttpServletReponse) for Content-Disposition. It might be worth adding a new method to UtilHttp so the filename can be formatted correctly (it's inconsistent)

        Show
        gareth.carter Gareth Carter added a comment - I initially said content because of SimpleContentViewHandler but could webapp be an alternative? Since Content-Disposition only relates to http. I have no preference here I have found 7 references in code (that actually set it on the HttpServletReponse) for Content-Disposition. It might be worth adding a new method to UtilHttp so the filename can be formatted correctly (it's inconsistent)
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        webapp makes sense, maybe in requestHandler.properties or a new file?

        Show
        jacques.le.roux Jacques Le Roux added a comment - webapp makes sense, maybe in requestHandler.properties or a new file?
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Gareth,

        Could you provide a patch for

        I have found 7 references in code (that actually set it on the HttpServletReponse) for Content-Disposition. It might be worth adding a new method to UtilHttp so the filename can be formatted correctly (it's inconsistent)

        ?

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Gareth, Could you provide a patch for I have found 7 references in code (that actually set it on the HttpServletReponse) for Content-Disposition. It might be worth adding a new method to UtilHttp so the filename can be formatted correctly (it's inconsistent) ?
        Hide
        gareth.carter Gareth Carter added a comment -

        I use the requestHandler properties file to store the default type. Not been tested though

        Show
        gareth.carter Gareth Carter added a comment - I use the requestHandler properties file to store the default type. Not been tested though
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        I tried but so far did not find a way to discern a sure difference between the two. As you kinda suggested before, it seems we are shooting a dead man. It more depends on the setting of your (modern) browser. All current major modern browsers handle that internally:

        I did not test on a smarphone, but I guess they are at least as modern as my PC

        This said I still believe it's a good idea because not everyone use a modern browser, and security is a must. So will commit soon the patch I will attach if nobody is against.

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited I tried but so far did not find a way to discern a sure difference between the two. As you kinda suggested before, it seems we are shooting a dead man. It more depends on the setting of your (modern) browser. All current major modern browsers handle that internally: FF: https://support.mozilla.org/en-US/kb/disable-built-pdf-viewer-and-use-another-viewer Chrome: http://www.cnet.com/how-to/how-to-disable-chromes-pdf-viewer/ IE: https://stackoverflow.com/questions/30070363/how-to-disable-the-built-in-pdf-viewer-in-opera Safari: http://osxmactips.blogspot.fr/2009/06/disable-safari-pdf-viewing.html Opera: https://stackoverflow.com/questions/30070363/how-to-disable-the-built-in-pdf-viewer-in-opera I did not test on a smarphone, but I guess they are at least as modern as my PC This said I still believe it's a good idea because not everyone use a modern browser, and security is a must. So will commit soon the patch I will attach if nobody is against.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        This patch (OFBIZ-6702.patch) is the right combination of Gareth's patches + the small comment I suggested for the content-disposition-type property

        Show
        jacques.le.roux Jacques Le Roux added a comment - This patch ( OFBIZ-6702 .patch) is the right combination of Gareth's patches + the small comment I suggested for the content-disposition-type property
        Hide
        gareth.carter Gareth Carter added a comment -

        Here is an interesting link about security https://code.google.com/p/browsersec/wiki/Part2

        https://tools.ietf.org/html/rfc6266

        On the other hand, if it matches "inline" (case-insensitively), this
        implies default processing. Therefore, the disposition type "inline"
        is only useful when it is augmented with additional parameters, such
        as the filename (see below).

        Seems like specifying inline is the equivalent of not adding Content-Disposition.

        Show
        gareth.carter Gareth Carter added a comment - Here is an interesting link about security https://code.google.com/p/browsersec/wiki/Part2 https://tools.ietf.org/html/rfc6266 On the other hand, if it matches "inline" (case-insensitively), this implies default processing. Therefore, the disposition type "inline" is only useful when it is augmented with additional parameters, such as the filename (see below). Seems like specifying inline is the equivalent of not adding Content-Disposition.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Yes it's a bit complex, so better to let the decision to users

        Show
        jacques.le.roux Jacques Le Roux added a comment - Yes it's a bit complex, so better to let the decision to users
        Hide
        gareth.carter Gareth Carter added a comment -

        Patch seems to work.

        May have found a slight security issue with this (could have been in the links in previous comments), if a html file is stored in DataResource, the the mime type would be text/html. This causes the html to be rendered as a web page rather than viewed as text in the browser window. My suggestion would be return text/plain or application/octet-stream for uploaded html files, this would cause either download or viewing the html contents if inline was set

        Show
        gareth.carter Gareth Carter added a comment - Patch seems to work. May have found a slight security issue with this (could have been in the links in previous comments), if a html file is stored in DataResource, the the mime type would be text/html. This causes the html to be rendered as a web page rather than viewed as text in the browser window. My suggestion would be return text/plain or application/octet-stream for uploaded html files, this would cause either download or viewing the html contents if inline was set
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Good point, again this should be a property, users should need to render HTML (safely internally) and not have to compile for that

        Show
        jacques.le.roux Jacques Le Roux added a comment - Good point, again this should be a property, users should need to render HTML (safely internally) and not have to compile for that
        Hide
        gareth.carter Gareth Carter added a comment -

        I should mention I mean this only from SimpleContentViewHandler for downloads, I don't think SimpleViewContentHandler is used anywhere to render html ?

        Like I said, we should force text/plain or application/octet-stream for text/html through SimpleContentViewHandler or even force disposition type attachment regardless of the property. This will ensure any Content/DataResource records that are html do not render as html web pages when accessed via SimpleContentViewHandler only

        Show
        gareth.carter Gareth Carter added a comment - I should mention I mean this only from SimpleContentViewHandler for downloads, I don't think SimpleViewContentHandler is used anywhere to render html ? Like I said, we should force text/plain or application/octet-stream for text/html through SimpleContentViewHandler or even force disposition type attachment regardless of the property. This will ensure any Content/DataResource records that are html do not render as html web pages when accessed via SimpleContentViewHandler only
        Hide
        gareth.carter Gareth Carter added a comment -

        See slight mod to SimpleContentViewHandler

        Show
        gareth.carter Gareth Carter added a comment - See slight mod to SimpleContentViewHandler
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Gareth,

        Your patch is commited in trunk at r1712835

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Gareth, Your patch is commited in trunk at r1712835

          People

          • Assignee:
            jacques.le.roux Jacques Le Roux
            Reporter:
            gareth.carter Gareth Carter
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development