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 security policy #160

Open
wants to merge 239 commits into
base: main
Choose a base branch
from
Open

Content security policy #160

wants to merge 239 commits into from

Conversation

snuggs
Copy link
Member

@snuggs snuggs commented Jan 18, 2018

Content Security Policy (MDN)

Middleware used for CSP (Content Security Policy).

Proposals

  • Default policy settings to None by default (white label opt in as per @tmornini 's suggestions)
  • Does each Resource have it's own Policy as well?

Specificaions

Services

Links

brandondees
brandondees previously approved these changes Jan 19, 2018
@snuggs
Copy link
Member Author

snuggs commented Jan 19, 2018

@brandondees what did i clobber? didn't force push anything. But can't see what you wrote.

capture d ecran 2018-01-19 a 10 34 26

@brandondees
Copy link
Collaborator

it was just an approval. when the code changes, the review becomes obsolete

// Is this a security breach?
// Will someone be able to disable CSP with this?
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy-Report-Only
+ ( 'report' in context.request.query ? '-Report-Only' : '' )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandondees trying to figure out a way to trigger reporting. I feel having a querystring parameter ?report
Should only work in development but that's a side effect that now has to be tracked. Apparently can do BOTH
Content-Security-Policy and Content-Security-Policy-Report-Only and they are complimentary and inconsequential. Perhaps that's our strategy. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy-Report-Only

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow am i behind the times on this. the report only feature looks like something everybody should enable all the time as a general best practice, but there are always exceptions to the rule for this kind of thing. where's this code going to have an effect? just on snuggsi.es? or for all users of the shim? or just on the docs website? i'm trying to gauge how much we're thinking about "show off and prescribe a best practice we figured out" vs "we need this to work right" vs "just exploring what's possible"

both csp and the reporting are things that there might be valid reasons to not use, or to configure differently, naturally. but for typical web developers building apps, we should be able to come up with some sort of sensible default starting point that errs towards more secure rather than less. whitelist by default, blacklist instead only if it fits your model better, disable altogether if you really know what you're doing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandondees great question. Basically for now eating own dogfood. (And great fallback reference of "we did this before here's where it's at since it's relevant moving forward". Great statements and thoughts. I'm having the same.

I found this as well. Perhaps ?report just enables reporting. Should be inconsequential of the explicit Content-Security-Policy. And would only report on what is in Content-Security-Policy-Report-Only. I feel making them mirror images of each other ALWAYS would at least provide more Occam's razor and less Murphy's Law.

Thoughts...

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy#Multiple_content_security_policies

Copy link
Member Author

@snuggs snuggs Jan 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. @brandondees better get up on it. YUGE part of Rails 5.2. And was a MAJOR update to modern HTTP header conventions and is 100% opt-in server side. (Even client with <meta> if that's your thing.) The description of this PR is pretty much the 20% you would recover from Google mining that would get you 80% of the way with best practices.

We also have them documented in the middleware/README.md from this PR as well.

/cc @tmornini

@snuggs snuggs force-pushed the content-security-policy branch 3 times, most recently from 1f8b4fa to 9e2d4cd Compare January 21, 2018 08:47
@snuggs
Copy link
Member Author

snuggs commented Jan 21, 2018

capture d ecran 2018-01-21 a 04 22 43

@tmornini @aspleenic perfect example of a conditional expression vs a conditional statement (if).

TL;DR Statements & Expressions both need a predicate. But only one is used (primarily) to return something. And the other is used (primarily) for side effects.

Statements don't return values. Big epiphany for me recently was if statements aren't
(or don't have to be) used for control flow at all. Merely used for side effects.
(Unless you're programming in Ruby. But even Matz says that's a leaky abstraction).
The "control flow" is a bi-product of an if statement. A side effect of THE side effect if you will.

"Hello World" if true # Leaky abstraction. Works in Ruby. Not natural to majority of other languages.

One thing that stands out is the previous statement cannot be composed. Expressions are composable
(as in this example within our content negotiation middleware.) I learned this in scheme/LISP programming where I had an "A-ha" moment where even the "if statements" were merely functions themselves. Yes they controlled flow. And yes they (not often) performed side effects (anti-pattern). But were "expressions" because they must return something or purity goes out the window re: flow in functional programming.

#F Sharp / Haskell sheds more clarity and has great examples of the difference
and why imperative if statements and loops should be avoided from a functional perspective. However a rubyist would consider everything above asinine because of that one glorious/dreadful bug/feature of an if statement returning a value. Ruby has lots of these bugs/features that we love to hate to love.

To be clear expressions and statements both control flow at the end of the day. I think we all agree on this. All the other stuff is what's debatable. ;-). I can say this though...IF statements are a cesspool for bugs. Another thing I think we all can agree on. ...And the debate goes on (great song by the way)

Feel free to chime in. Shared experience is always enlightening since control-flow effects every programmer ever. Cheers!

/cc @brandondees @btakita @kurtcagle @janz93 @mrbernnz @halorium @johnrlive

image

@snuggs
Copy link
Member Author

snuggs commented Jan 21, 2018

@brandondees P.S. WIP is off. As soon as we figure out what to do with ?report we can merge. We can also do the reporting in a later PR. As that may need some insight on how/where the report should go. These are auto-magically POSTed from web browser to endpoint. How that endpoint logs I feel should be left open with a sane default. Currently we are just logging to https://report-uri.com.

Full test coverage ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅

We still have work to do after this PR. By default we set everything to 'none'. Perhaps self is a better baseline. However, I prefer everything off in regards to security by default.

See the following images of with report and without report respectively. Notice the without disables EVERYTHING! (Excellent...I think...).

Actually amazing when you have a test suite. Truth be told this was the very first time I opened the browser since starting working on the feature a few days ago. Exactly what we would expect to happen. Tests don't lie unless you tell them to!

capture d ecran 2018-01-21 a 05 02 50
capture d ecran 2018-01-21 a 05 03 04

@tmornini
Copy link
Collaborator

tmornini commented Jun 15, 2018

Perhaps self is a better baseline. However, I prefer everything off in regards to security by default.

While I generally agree with your statement, I don’t believe it makes any sense to default to off if that means an entity is not allowed to access itself — which I believe is what that means.

@snuggs
Copy link
Member Author

snuggs commented Jun 16, 2018

@brandondees @tmornini hmmmm in retrospect you bring up great points. Given some time to rethink what do you feel is the immediate feature to test. We have the (passing) tests that are spec approved. But TOO restrictive. Can't even get the server to show anything locally as is locked down like a fort. Looks like "ON" is/was working lol. At first thought some config file for "settings" but assumed we are smarter than that. @brandondees Define "Batteries included?" for CSP (to the best of your recolleciton). What do we NOT want to think about?

TL;DR; https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP

Also the de-facto is helmet. However it feels more like the devise of CSP. I'd rather take the meat and throw away the bones. Can always grow into it if we need that much dependency. https://github.com/helmetjs/helmet

As @brandondees would say "Those 'None's ain't gonna un-none themselves" lol.

The patterns i'm picking up on can be solved with a hash map in less than a handful of LOC if we #CATAT (Call A Thing A Thing) However, that's merely an implementation detail. WHAT are the defaults and WHERE do they "go" (pun intended @tmornini)

capture d ecran 2018-06-15 a 23 06 11

capture d ecran 2018-06-15 a 21 09 35

@snuggs
Copy link
Member Author

snuggs commented Jun 16, 2018

@brandondees @tmornini policy.json ? Would live quite nicely next to policy.test and policy.es

For a ruby port that would be policy.yml and policy.rb. Funny enough all those files can live harmoniously juxtaposed to one another although different implementations. #ThatsWhatFriendsAndFileNamesAreFor #CATAT

Lastly @brandondees you have that fetish for security....this is your year! Merry Christmas 🎅 Should be ready by then lol. https://github.com/w3c/webappsec#web-application-security-working-group

@@ -8,20 +8,17 @@

Copy link
Member Author

@snuggs snuggs Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandondees @tmornini should this be called policy or security. The latter seems too vague in retrospect based off #160 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security doesn't imply a specific meaning. neither does policy out of context, but if the context for it is obvious, then maybe that works. when in doubt i qualify my terms, e.g. content_security_policy which is I think the actual name of what we're discussing here now, right?

@brandondees
Copy link
Collaborator

as for what's "batteries included" for csp, I am not expert enough to really have a definitive answer to that. rails 5.2 added default CSP headers which might be a good place to look for some "sensible defaults" cues to follow.

As a security wonk, I'd like to say lock it all down by default and require intentional whitelisting, but reality is that may be raising the friction level too high and would lead to folks disabling it altogether instead of making thoughtful and appropriate choices. It should probably let folks do the most basic low-risk things they expect out of the box (careful what you assume is low-risk on the web though), but provide some safety railing to keep folks out of the canyon.

snuggs added 29 commits July 2, 2018 11:29
@snuggs
Copy link
Member Author

snuggs commented Nov 28, 2018

@brandondees @tmornini yup gonna blow the dust off this one and merge this in ASAP!

SUICIDE not having CSP in 2019.

https://medium.com/intrinsic/compromised-npm-package-event-stream-d47d08605502

dominictarr/event-stream#116 (comment)

capture d ecran 2018-11-28 a 11 21 30

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

Successfully merging this pull request may close these issues.

3 participants