-
Notifications
You must be signed in to change notification settings - Fork 14
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
Lock down CSP in “.html” files to prove we can. #289
Conversation
We want integrators to be able to enforce the strictest content security policies possible — this change set places strict CSPs in our own documents to keep us honest.
@@ -16,12 +27,6 @@ export default class DemoLitHtml extends BaseElement { | |||
static template(html, { ifDefined }) { | |||
return ({ emoji, message }) => { | |||
return html` | |||
<style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply changing this inline <style>
tag usage so we can have a stricter content security policy on this particular demo. To be clear, this is not production code.
@@ -16,12 +27,6 @@ export default class DemoUhtml extends BaseElement { | |||
static template(html) { | |||
return ({ emoji, message }) => { | |||
return html` | |||
<style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply changing this inline <style>
tag usage so we can have a stricter content security policy on this particular demo. To be clear, this is not production code.
@@ -1,7 +1,8 @@ | |||
<!doctype html> | |||
<html> | |||
<head> | |||
<meta charset="UTF-8"> | |||
<meta charset="utf-8"> | |||
<meta http-equiv="content-security-policy" content="default-src 'self'; script-src 'self' 'sha256-vpd4e6Q2EYlXOAcBOCPYNRqOSK1caV1iPamby7fBE30=' https://esm.sh/[email protected]/;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because an import map is necessarily an inlined script (at least for now), we need to explicitly allow it. The browser actually makes this super easy since the CSP error will log out the hash in the console and you can just copy-paste it here. I was worried that this might be a maintenance burden, but it’s actually not bad at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's actually horribleawesome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha. I agree. I had the same emotional journey.
@@ -1,7 +1,10 @@ | |||
<!doctype html> | |||
<html> | |||
<head> | |||
<meta charset="UTF-8"> | |||
<meta charset="utf-8"> | |||
<!-- We use “unsafe-eval” below because we need to trick the browser into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where we loosen our policy — I added explanatory commentary.
@@ -1,7 +1,12 @@ | |||
<!doctype html> | |||
<html> | |||
<head> | |||
<meta charset="UTF-8"> | |||
<meta charset="utf-8"> | |||
<!-- TODO: The react library has a bunch of dependencies which makes the CSP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where we were forces to loosen our policy more than we’d like… I added a TODO
comment. Here, for example, it’s brutal to try and enumerate all the individual esm.sh
urls for transitive dependencies of React. So, I just allow any package there via https://esm.sh/
. This is just a demo, so I’m not terribly worried about it, but I think it’s good to call out when integration is cumbersome.
By contrast, the lit-html
policy was actually a breeze as it doesn’t have dependencies (at least not for our usage in the demo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working as intended!
The most important lines in this change set are the CSP |
FYI @klebba — Happily (and perhaps unsurprisingly), I was able to lock down policies in our documents without having to make any changes to the the production code. |
We want integrators to be able to enforce the strictest content security policies possible — this change set places strict CSPs in our own documents to keep us honest.