KEMBAR78
[WIP] New API Implementation by oreoshake · Pull Request #181 · github/secure_headers · GitHub
Skip to content

[WIP] New API Implementation #181

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

[WIP] New API Implementation #181

wants to merge 7 commits into from

Conversation

@oreoshake
Copy link
Contributor

This diff is unnecessarily large, probably better to just checkout the source and view it without the diff.

major changes

  • Per-request overrides are stored in request.env (is Thread.local better?), values are determined and set in rack middleware.
  • API available for overriding/managing CSP (append_content_security_policy_exception, override_content_security_policy_directives, content_security_policy_nonce), XFO (override_x_frame_options), and HPKP (override_hpkp) per request, unofficial API for overriding the rest. All are instance methods on a controller object and have SecureHeaders class methods too.
  • Opting out is available with SecureHeaders::opt_out_of(SecureHeaders::<HEADERNAME>::CONFIG_KEY)

Under the hood changes

  • Only CSP and HPKP are instantiated as objects, all other headers will be configured Strings or defaults. This should keep the number of created objects small, hopefully.
  • Configs are validated at boot time, or per request if RAILS_ENV is dev/test (controversial)

Config changes

  • Only CSP and HPKP support hash arguments. All other values must be simple strings
  • CSP directive values must be arrays of strings (not space-delimited values)
  • false does not mean opt-out. SecureHeaders::OPT_OUT does.
  • Old values such as self none inline eval will raise exceptions when validated. They are no longer valid and the standard values should be used 'none', 'self', 'unsafe-inline', 'unsafe-eval'

TODO

  • Add tests for the case when secureheaders is mixed in to a class (rather than statically calling it each time)
  • Handle nonces and precomputed headers
  • Ensure the README is still valid (it probably isn't)
  • Clean this shit up
  • Handle overriding of default CSP configs (i.e. CSP config isn't supplied but override/append is called)
  • Cache default headers
  • Cache CSP headers per browser (Firefox, Chrome/Opera, Safari, other)
  • Add "precomputing" of values concept per @mastahyeti
  • Re-add script hash support (ripped it out since this is a re-write and it was slowing me down)

/cc @mastahyeti @ptoomey3 @gregose

btoews
btoews reviewed Oct 16, 2015
View reviewed changes
lib/secure_headers/headers/public_key_pins.rb Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're using these in the request.env, it seems like they should be more unique to avoid conflicts. Maybe SECURE_HEADERS_HPKP, for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these CONFIG_KEYS will be scoped to https://github.com/twitter/secureheaders/pull/181/files#diff-e7fcd10663004caed933b95e0ff2e643R23 in https://github.com/twitter/secureheaders/pull/181/files#diff-e7fcd10663004caed933b95e0ff2e643R160. That will be "unsupported" when I remove the line from your comment about pulling the config from two different places in request.env.

Unfortunately that's a left over from when I thought passing a hash directly to header_hash_for would be a thing. 💩

@oreoshake
Copy link
Contributor Author

This PR is entirely too large. Having responded to @mastahyeti's comments I'd say it's pretty unusable. I'm going to create a 3.x branch, commit the mass deletion of the fixtures directory, and create a new PR based on the 3.x branch. It will somewhat shorten this (although I still think it's better to look at the code as new and ignore the diff in general)

@oreoshake
delete fixture apps
441b7a1
@oreoshake
Major rewrite:
32bb3f5 
Configure a global default and named overrides
Use helper methods to set/modify configurations at runtime
Set the headers in middleware based on the configuration saved to request.env

Configuration changes:
All headers require string values except for CSP and HPKP
CSP directives must be arrays of strings, no more support for space-delimited strings or procs
@oreoshake oreoshake mentioned this pull request Nov 5, 2015
Merged
@oreoshake
Copy link
Contributor Author

clsoing in favor of #191

@oreoshake oreoshake closed this Nov 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@oreoshake @reedloden @btoews