Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: unknown mime type sets Content-Type as 'null; charset=UTF-8' #354

Closed
1 of 2 tasks
clshortfuse opened this issue Jan 1, 2019 · 9 comments · Fixed by #355 or #809
Closed
1 of 2 tasks

bug: unknown mime type sets Content-Type as 'null; charset=UTF-8' #354

clshortfuse opened this issue Jan 1, 2019 · 9 comments · Fixed by #355 or #809

Comments

@clshortfuse
Copy link
Contributor

  • Operating System: Mac OS X
  • Node Version: v8.11.3
  • NPM Version: 6.5.0
  • webpack version: 4.18.0
  • webpack-dev-middleware Version: 3.4.0
  • This is a feature request
  • This is a bug

Code

return {
    entry: entries,
    devtool: isProduction ? 'source-map' : 'nosources-source-map',
    mode: process.env.NODE_ENV || 'development',
    devServer: {
      host: '0.0.0.0',
      hot: false,
      inline: false,
    },
    output: {
      filename: '[name].min.js',
      path: path.resolve(__dirname, DEST, folder),
    },
    module: {
      rules: [{
        test: /\.js$/,
        include: [
          path.resolve(__dirname, 's3', folder),
        ],
        use: {
          loader: 'babel-loader',
          options: {
            presets: [
              ['@babel/preset-env'],
            ],
          },
        },
      }, {
        test: /\.scss$/,
        use: [
          'file-loader?name=[name].min.css',
          'extract-loader',
          'css-loader?import=false',
          {
            loader: 'postcss-loader',
            options: {
              plugins: () => [
                cssnano(),
              ],
            },
          },
          'sass-loader',
        ],
      }, {
        test: /_.+\.pug$/,
        use: [
          'html-loader',
          'pug-html-loader',
        ],
      }, {
        test: /index\.pug$/,
        use: [
          'file-loader?name=index.html',
          'extract-loader',
          'html-loader',
          'pug-html-loader',
        ],
      },
      {
        test: /\.pug$/,
        exclude: /(_.+)|(index)\.pug$/,
        use: [
          'file-loader?name=[name]',
          'extract-loader',
          'html-loader',
          'pug-html-loader',
        ],
      }],
    },

Expected Behavior

Unknown mime-types should return a blank media-type in Content-Type

Actual Behavior

media-type is being listed as null

For Bugs; How can we reproduce the behavior?

Create a webpack package that outputs any file without an extension.


The issue is in middleware.js

let contentType = mime.getType(filename);

mime.getType(filename); will return null if it cannot lookup the media-type.

https://github.com/broofa/node-mime/blob/a1d884c1672f34031af15718b25b93dee00dc3ad/Mime.js#L82

When '; charset=UTF-8' is added to null, the result is Content-Type: null; charset=UTF-8.

contentType += '; charset=UTF-8';

Different browsers treat null differently. Chrome, Safari, and Firefox will try to interpret the file (which in my use case is text/html). IE and Edge will not and treat it as application/octet-stream which will cause a download prompt to appear. Using */* will produce the same result.

If we don't use 'null', and instead, leave the media-type blank, all the above browsers will try to interpret the file when Content-Type: ; charset=UTF-8 is used. Since I'm using it for HTML file, it gets interpreted correct by IE and Edge. That's the expected behavior according to RFC 7231:

If a Content-Type header field is not present, the recipient MAY either assume a media type of "application/octet-stream" or examine the data to determine its type.

https://tools.ietf.org/html/rfc7231#section-3.1.1.5

clshortfuse added a commit to clshortfuse/webpack-dev-middleware that referenced this issue Jan 1, 2019
clshortfuse added a commit to clshortfuse/webpack-dev-middleware that referenced this issue Jan 1, 2019
@clshortfuse
Copy link
Contributor Author

@alexander-akait This bug is back, but slightly different. Unknown mime types are now forced to application/octet-stream which means HTML pages (document) with no extension no longer load.

Now Chrome states: Resource interpreted as Document but transferred with MIME type application/octet-stream: "http://localhost:3000/login".

I'm testing with the 4.0.0-beta of webpack-dev-server to get it working with Webpack v5. This would be a regression. I'm not where exactly this starting showing issue, but I'd assume it happened with the move to mime-types.

@clshortfuse
Copy link
Contributor Author

Seems like this new behavior started with b83a1db#diff-9a117d52a8a2cb3e5f498575bdfdea4dc32418180c09029454fbaf7f95ebe5a1R76

I'm not seeing any reason specifically why this new behavior was introduced though, or a way to work around it.

@alexander-akait
Copy link
Member

Because Node.js do the same?

@alexander-akait
Copy link
Member

What is the problem, just use html extension or provide mimeTypes

@clshortfuse
Copy link
Contributor Author

Because Node.js do the same?

I don't know what you mean. NodeJS doesn't set Content-Type. It sets Transfer-Encoding, Connection, and Keep-Alive:

https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js

As explained in the issue from Jan 2019, an explicit Content-Type header isn't required.

What is the problem, just use html extension or provide mimeTypes

Having to rewrite all applications to new URLs simply for webpack-dev-middleware seems pretty invasive. Also, you can't provide the mime type because mime-types doesn't let you set the mimeType for blank ('') extensions. This doesn't work:

dev: {
        mimeTypes: { '': ['text/html'] },
      },

Again, I don't understand the reasoning behind incorporating a breaking change to forcing all unknown mime types to application/octet-stream. If a content-type is unknown it should be blank (or not sent at all), not forced to binary data. See RFC 7231:

A sender that generates a message containing a payload body SHOULD generate a Content-Type header field in that message unless the intended media type of the enclosed representation is unknown to the sender.

https://tools.ietf.org/html/rfc7231#section-3.1.1.5

@alexander-akait
Copy link
Member

PR welcome

@alexander-akait
Copy link
Member

Should be easy to fix

@clshortfuse
Copy link
Contributor Author

clshortfuse commented Dec 31, 2020

So, I wrote the code fix and I'll send in the PR tomorrow. But there's an issue with how to test it...

Background:

With, Express if you don't set a Content-Type, it will violate spec and send application/octet-stream on unknown mime types. With Connect, it doesn't send a Content-Type unless you specifically add one. The way the code is written is, basically, "Let's make Connect do what Express does and test that against that"

Express has a pretty long dependency tree, so trying to test what happens after the current middleware executes is a rather difficult task. In this case, it's express => send => mime. The problem is, Express relies on send@0.17.1, which in turn depends on mime@1.6.0.

mime@2+ has already fixed this: broofa/mime#139 (referencing the same RFC URL) but I honestly doubt it'll be fixed anytime soon on express (if ever) because of their third-party dependencies.

Observations:

There's really a more complex question with how exactly this library wants to support other web frameworks. Each has their own quirks and ways of doing things:

  • connect (node.js): won't touch Content-Type at all.

  • express will always set application/octet-stream if Content-Type isn't set.

  • fastify looks at the content. Any Buffer passed will automatically get application/octet-stream but a string will get text/plain.

  • kao does what fastify does, but it will test the string and if the first non-whitespace character is <, it sets the type to text/html.

Solution:

Honestly, the best solution I can come up with is just acknowledge this Content-Type issue in testing. Since we can't reconcile all those different ways frameworks manage Content-Type, we just don't test against it.

Recommendation:

In the future, there should probably be an abstract class that middleware.js uses to communicate with the other frameworks. The way the tests are built, it's only testing after all steps are completed. With res.send(), it's Express that sets the Content-Type, not this middleware. That means it'll be fine at the middleware step, but Express will break it. With an abstraction, you can create a mock one and test Content-Type against that.

@alexander-akait
Copy link
Member

Yes, I noticed that different frameworks is doing about different things, I think in this case, we just need to leave the commentary in the code and in the documentation

clshortfuse added a commit to clshortfuse/webpack-dev-middleware that referenced this issue Jan 1, 2021
Fixes webpack#354

* Only set 'Content-Type' header if mime type is known.
* Remove testing for 'Content-Type' of unknown types.
* Do not test against `application/octet-stream'.

Unknown media types should have content-type blank. Because
frameworks handle Content-Types differently, it is not possible to
standardize testing at the moment. Tests against
`application/octet-stream` can create false positives with Express
because of this.

https://tools.ietf.org/html/rfc7231#section-3.1.1.5
broofa/mime#139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants