Broken when content securiy policy headers are applied

[jantimon]

For security reasons we are disallowing inline styles and scripts. http://www.html5rocks.com/en/tutorials/security/content-security-policy/

As we are using the same security settings in our development environments we are running into issues when using serve-index as it relies heavily on inline styles, scripts.

Any ideas how we could handle this?

[dougwilson]

This module does all inline by default, otherwise it would have to reserve URLs in every directory that you couldn't use. This would be awful. If you don't want inline styles, you just need to override the HTML page generation:

var serveIndex = require('serveIndex')
serveIndex.html = function(req, res, files, next, dir){
  // generate your index page here that
  // references your own external stylesheets
  // and the like
}

[jantimon]

Okay I already expected that security is not an issue for a module which exposes the file structure. However I don't understand why this would reserve URLs.

A possible solution would be to split the request into three requests:

just/a/example/path/ -> responds with directory.html
just/a/example/path/?styles -> responds with the styles
just/a/example/path/?scripts -> responds with the scripts

[dougwilson]

A possible solution would be to split the request into three requests

OK. Can you send a PR that does this, please?

[dougwilson]

In the meantime I'll keep this issue open.

[jantimon]

Okay will look into it

[jantimon]

There you go.

As you can see I didn't change the exports.html function so it won't break existing customized directory.html files in foreign projects. Instead I added a exports.css and a exports.javascript function and the according mediaTypes and edited the directory.html file.

[dougwilson]

Thanks, I commented on your changes.

As you can see I didn't change the exports.html function so it won't break existing customized directory.html files in foreign projects.

The main thing is that the {style} token cannot be removed, because people who use their own exports.html will be using that to inject their styles.

[jantimon]

Thanks for your feedback. As mentioned before the style token is still working as the html function was not touched.

What is wrong with the usage of specifying the requested mime type in the url? Where would you prefer to place the url.req logic?

[dougwilson]

As mentioned before the style token is still working as the html function was not touched.

It is not working. You can see the test for the {style} token is failing...

Whats wrong with the usage of specifying the requested mime type in the url?

That just seems weird. Also, you really need to encode the / character, so you should at least make it ?text%2Fcss

[jantimon]

Hey - thanks for mentioning the test.

The problem with csp is that inline styles are not allowed. So those styles were moved to the dynamic "?text/css" request.

The issue with the test was that, the tested template did not contain the {style} token anymore. I have modified the old test and added a new test to show that custom styles will remain part of the page when using external styles instead of inline styles.

[dougwilson]

The issue with the test was that, the tested template did not contain the {style} token anymore.

That's OK, but people need to be able to use a custom stylesheet with the default template and the default template MUST provide a {style} token as documented in the readme, otherwise this is not backwards-compatible and cannot be merged until sometime in the future when we release a new major version.

You cannot alter any existing tests, otherwise your change is not backwards-compatible for sure.

[jantimon]

I guess I am not explaining it well enough - custom style sheets still work but they are not inline anymore. Therefore testing if they are inline will fail. However they are part of the new "?css/text" request.

[dougwilson]

Therefore testing if they are inline will fail. However they are part of the new "?css/text" request.

Can you make the test check the correct place instead, then? That test must use the default template, not the template from the shared directory.

[jantimon]

Okay travis is testing it - please let me know if the test meets your expectations.

[dougwilson]

please let me know if the test meets your expectations

Yes, they looks fine now.

My only other main problem is it still isn't backwards-compatible :( because exports.html does not have the documented {style} token in the template string when the default template is used. This means all the people currently doing a .replace('{style}', mystyles) in their override function will no longer have any styles.

This is not terrible, but it is a breaking change and would have to hold this until a 2.0 release until this can be remedied. If you can make it so that it still works so backwards-compatibility is kept and have the external stylesheets work, that would be great and would let me put it in a 1.0 release.

Another pattern that would need to work to not hold this until 2.0 is people who do not do .replace('{style}', mystyles) in their custom exports.html should not get the default styles (i.e. they should get no styles).

[dougwilson]

You don't have to change any of those things if you don't want, but I just cannot merge it right away without the backward-compatibility kept.

[dougwilson]

Another issue is that the documentation says that if you provide your custom template, you place a {style} that will get replaced with the default styles, but the changes also don't do that, so people who only provide a new template and put the {style} token in their template won't have any styles applied any longer.

[dougwilson]

If you like, I can add tests for all these features to the test suite (no one says our test suite isn't lacking) and you can rebase your changes on top of the new tests so you can see what stuff you are still missing support for.

[jantimon]

Okay cool lets do that

[dougwilson]

Okay cool lets do that

OK :) I'll let you know when I added them based tests I would like to see continue to work :) Should be sometime later today.

[dougwilson]

@jantimon sorry for the delay; I found a couple bugs while adding tests, etc. So right now there should be most tests to help you with backwards-compatibility if you are interested. I believe I can integrate your changes in a backwards-compatible manner at this stage, though, if you are not interested in making additional changes :)

[jantimon]

Wow nice that sounds awesome. I can't think of any further changes right now. Do you need any help integrating the code?

[dougwilson]

Do you need any help integrating the code?

I think we are at a good point right now :) The only thing more I can think of is if you were to rebase your commits on top of the current master, but it's probably unnecessary :) I hope to this this out very soon.