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

Content type is html instead of text/plain #2

Closed
Guuz opened this issue Nov 3, 2014 · 6 comments
Closed

Content type is html instead of text/plain #2

Guuz opened this issue Nov 3, 2014 · 6 comments

Comments

@Guuz
Copy link

Guuz commented Nov 3, 2014

Hello,

Took me a while to track this down from express to here ;-)
When my express app has an unhandled route or a server error this middleware will respond to the request with a 404 or 500 and a very simple message. (Cannot GET /foo)
However; the content type is explicitly set to text/html but the actual content is not HTML but plain text. (text/plain).

I did not want to submit a pull request right away but wanted to check first with you if there is a reason behind this. And in the case that this middelware will not change; is it possible to overrule this or should I define my own "catch all" route to do this?

Thanks! :)

@dougwilson
Copy link
Contributor

Hi! I cannot change it because of backward-compatibility (just think of all the people expecting the response of Cannot GET / from Express, using it with nagios rules, etc. It's already fixed to send actual HTML for the 1.0 release, though: aa080c2 which will be part of Express 5.0.

@dougwilson
Copy link
Contributor

P.S. the body Cannot GET / is actually 100% valid HTML5 (http://www.w3.org/TR/html5/syntax.html#parsing) :) though I don't like it anyway and it doesn't take advantage of things like the <title> tag.

@Guuz
Copy link
Author

Guuz commented Nov 4, 2014

Thanks for the quick reply!

I was not suggesting changing the content but changing the content-type from html to text.
I agree it could be valid HTML, but it makes more sense that it is plain text.
Not even sure it's technically valid: http://stackoverflow.com/questions/9797046/whats-a-valid-html5-document

So if we are not changing the content-type for the <1.0 release do you have a nice workaround or should i just catch the 404 and 500 in express and return my own error instead of using this middleware? I don't want to change anything but the content-type. So i'd rather not reïmplement this middleware ;-)

@dougwilson
Copy link
Contributor

Not even sure it's technically valid

I provided you the HTML5 parser spec to read, which I have many times :) It's definitely valid HTML.

So if we are not changing the content-type for the <1.0 release do you have a nice workaround or should i just catch the 404 and 500 in express and return my own error instead of using this middleware?

So it's correct that we won't be chaining it, only because we actually did change it a long time ago, but it then causes XSS in older IE browsers on your site! Please realize we are HTML-escaping the URLs that are printed, so if you request the URL /some<text>/ it's going to be in the response body as Cannot GET /some&lt;text&gt;/ so if you set it to text/plain, the browser is not going to un-escape that. This means that you can't exactly just alter the type. If you still want to, let me know and I can come up with a way to alter the type in your express 4 app.

@Guuz
Copy link
Author

Guuz commented Nov 5, 2014

That last parts convinced me ;-) The body can actually contain html entities. So you want text/html. Makes sense. Thanks! I'll now have enough arguments to convince my team that we should keep it as is and we will upgrade to Express 5.0 in the future.

@dougwilson
Copy link
Contributor

Cool, no problem :) Feel free to bring up any other concerns! I actually forgot myself that it had HTML entities in the response until I re-looked at the code, otherwise I would have noted that upfront :O But yea, Express 5.0 (using 1.0 of this module) will respond with a HTML document that contains a little more than the minimally-required HTML tags :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants