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

Move all inline code to .js files #4401

Open
rhansen opened this issue Oct 7, 2020 · 7 comments
Open

Move all inline code to .js files #4401

rhansen opened this issue Oct 7, 2020 · 7 comments

Comments

@rhansen
Copy link
Member

rhansen commented Oct 7, 2020

I would like to move all of the inline code into .js files so that users can set a Content Security Policy (CSP) that prohibits inline code. Prohibiting inline code reduces the potential number of XSS vulnerabilities.

@rhansen
Copy link
Member Author

rhansen commented Oct 7, 2020

As a first step, I would like to delete the indexCustomInlineScripts hook that was added in PR #3996 for issue #3978.

cc @brunob

@JohnMcLear
Copy link
Member

Makes sense to me. I used ep_helmet to handle CSP by adding nonces btw but moving it out from being inline is way better. Peep ep_helmet first and the nonce open PR as reference points 👍

@brunob
Copy link
Contributor

brunob commented Oct 7, 2020

Why not, but what alternative would we have to modify the index inline script after that ?

@JohnMcLear
Copy link
Member

@brunob isn't there a hook to include JavaScript in the head?

@brunob
Copy link
Contributor

brunob commented Oct 7, 2020

@JohnMcLear i don't think so, here is what we do with that hook https://framagit.org/infini/ep_infini/-/blob/master/infini.js#L10

@JohnMcLear
Copy link
Member

JohnMcLear commented Oct 7, 2020

Okay so @rhansen why remove it? Just ensure the docs say to people to only reference scripts here IE how the template does it..

<script src="static/skins/<%=encodeURI(settings.skinName)%>/index.js?v=<%=settings.randomVersionString%>"></script>

An example of how not to do it could be

function(){alert("pow")};

An example of how to do it could be..

<script src="myawesomepluginscript.js"></script>

We could also just in the HTML put a comment saying not to include inline JS...

I'm lazy too which is why I prefer nonce approach over removing inline completely because this catches all potential plugin issues...

@rhansen
Copy link
Member Author

rhansen commented Oct 7, 2020

I moved the indexCustomInlineScripts-specific discussion to PR #4402.

I prefer nonce approach over removing inline completely because this catches all potential plugin issues...

Adding a nonce might be a good temporary fix, but I don't like it as a long-term fix because:

  • It is easy to do it incorrectly (insecurely).
  • It breaks caching. (By definition, a nonce value can only be used once.)
  • We would have to set the CSP header in Etherpad itself rather than let users set the CSP header in their reverse proxy. (Only Etherpad knows the nonce value.)
  • We should migrate away from inline JavaScript anyway because it's harder to maintain.

rhansen added a commit that referenced this issue Nov 21, 2021
This avoids the reliance on the global `chat` variable, and it is a
step toward supporting a strict Content Security Policy (#4401).
rhansen added a commit that referenced this issue Jan 21, 2022
This avoids the reliance on the global `chat` variable, and it is a
step toward supporting a strict Content Security Policy (#4401).
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