Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Start splitting up ContentSecurityPolicy. WIP #10

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

bemurphy commented Jan 31, 2013

This is total WIP on gh-6, just submitting PR to get feedback if on the right
track or not....

Still not done but split out a bunch so far. Factorized with
.build instead of calling .new to get the right class.

I think the factory could further split the FF into 2 classes
for standard and non-standard, to further kill conditional checks
in the class. Perhaps it should call a class method like .buildable?
on classes so it can roll through and pick the first that responds true.

bemurphy added some commits Jan 31, 2013

@bemurphy bemurphy Start splitting up ContentSecurityPolicy. WIP
Still not done but split out a bunch so far.  Factorized with
.build instead of calling .new to get the right class.  Still
struggling with the browser ivar.

I think the factory could further split the FF into 2 classes
for standard and non-standard, to further kill conditional checks
in the class.  Perhaps it should call a class method like .buildable?
on classes so it can roll through and pick the first that responds true.
3d8f9cb
@bemurphy bemurphy Don't unnecessarily filter directives fa063ae
@bemurphy bemurphy Remove #supports_standard? outside of Firefox 7a6cbd5
@bemurphy bemurphy Move FF related constants to appropriate class
As a side note, I'm smelling the Constant modules a little, we can
probably clean this up later.  It would probably be better to have
constants available in the modules that aren't prefixed with a vendor
name, because it will cut out subclass methods that are just one
liners going for the constant.
92a38b0
@bemurphy bemurphy Pull out empty template methods only needed for FF
Use an #after_configure method to handle the last bits of config
work
ac180ac
@bemurphy bemurphy Join strings rather than building using += 97733fd
@bemurphy bemurphy Get rid of tempvar in #generic_directives 80eccc3
Contributor

bemurphy commented Jan 31, 2013

I think there's some chrome specific stuff (like the extensions) that could be split out to webkit as well. Maybe have a general #browser_extensions method or something that can handle that kind of thing?

Contributor

bemurphy commented Jan 31, 2013

BTW, here's my feature branch flogged on the file currently:

180.2: flog total
7.5: flog/method average

Here's master:
250.1: flog total
8.9: flog/method average

Collaborator

oreoshake commented Jan 31, 2013

👍

Definitely what I wanted to do.

RE: browser_extensions, there's some debate as to whether this will be part of the spec (i.e. don't report on extension activities at all). It might be premature abstraction, which I'm not completely against. Likely the method to manipulate extensions across browsers will not be standardized very soon which justifies this entirely.

Contributor

bemurphy commented Jan 31, 2013

@oreoshake ok I'll keep trucking with it tonight then. I think I can get it far enough it's worthy of master (it's pretty close already) and then we can keep pulling in features for little extractions here and there.

I think the tests were pretty helpful btw (I hope!) because I've relied on them entirely for the refactoring so far. I do need to do another sweep on them though to make sure nothing is breaking and stubs are lying to me or something...

Collaborator

oreoshake commented Jan 31, 2013

While you're at it, let me know if you figure out why #3 wasn't caught in tests? I thought I had tests for exactly that. Either they are invalid or missing.

@oreoshake oreoshake commented on the diff Jan 31, 2013

...ecure_headers/headers/content_security_policy_spec.rb
@@ -119,22 +119,16 @@ def request_for user_agent, request_uri = nil
end
describe "#supports_standard?" do
- ['IE', 'Safari', 'Chrome'].each do |browser_name|
@oreoshake

oreoshake Jan 31, 2013

Collaborator

Got beef with this test?

@bemurphy

bemurphy Jan 31, 2013

Contributor

I ripped supports_standard? from the other classes, it's only a private for FF now because that was the only place using it. Does this make sense?

It was in commit 7a6cbd5

@oreoshake

oreoshake Jan 31, 2013

Collaborator

I like.

Contributor

bemurphy commented Jan 31, 2013

@oreoshake hah, yeah that issue just caught my eye, too. I'll check it out.

Collaborator

oreoshake commented Jan 31, 2013

fwiw that code is some jank. It's used in different contexts and for different purposes IIRC

Collaborator

oreoshake commented Jan 31, 2013

Not to put any pressure on you... but I was about to integrate the code to apply two headers (on that enforces, and an "experimental" one that only generates reports).

We will have a mega conflict, but I can easily resolve my conflicts with this change so I'll hold off until then.

@oreoshake oreoshake commented on the diff Feb 1, 2013

...re_headers/headers/firefox_content_security_policy.rb
+ private
+
+ def supports_standard?
+ browser.version.to_i >= 18
+ end
+
+ def build_impl_specific_directives
+ header_value = ""
+ default = expect_directive_value(:default_src)
+ header_value += build_preamble(default) || ''
+ header_value
+ end
+
+ def build_preamble(default_src_value)
+ header_value = ''
+ if supports_standard?
@oreoshake

oreoshake Feb 1, 2013

Collaborator

honestly this whole condition isn't needed, which reduces the need for the browser ivar. I doubt firefox will drop support for "allow" anytime soon. @imelven?

@imelven

imelven Feb 4, 2013

Contributor

The tentative plan is to deprecate the old header (including the old syntax e.g 'allow') somewhere around Firefox 24 which is an ESR, but there's a lot of dependencies that need to be addressed first.

Collaborator

oreoshake commented Feb 13, 2013

Just rebased, conflicts were not trivial to resolve 😿, as in I gave up quickly. I'll try to take a stab at this soon

Collaborator

oreoshake commented Feb 18, 2013

Yo, just merge in the smelly_csp branch and this should be good :) just merged in real_tests and everything but travis is working! (I think i know what is causing travis fail)

Splitting up the tests into separate classes would be awesome but I think the genericizing (aka coupling) of the classes could be justifiable for now 🌳

Contributor

bemurphy commented Feb 18, 2013

@oreoshake sounds good. A couple questions/points first:

  • I split into a different implementation Thursday night at the hackfest. It's in my branch at: https://github.com/bemurphy/secureheaders/tree/content_security_policy_refactor
  • See if you have an implementation preference. I will say in the new branch, I was having trouble pushing #normalize_reporting_endpoint down into a strategy so that says something.
  • I agree that ultimately the tests should be split out but that it's a win for now.
  • Lastly, the one thing that led me to the different attempt last thursday was that the ContentSecurityPolicy. initialize went to 3 optional arguments, which broke the notions of a factory from the browser in the build class method. Maybe that request really ought to never be nil, but also, that method signature is a bit messy feeling to me now. The messiness bit is something that can be addressed later.

p.s. that's right...there was hacking at the hackfest!

Contributor

bemurphy commented Feb 18, 2013

thought about this some more, fwiw it's a good idea to look at the new implementation but if the old branch will work I think it'll be less headache, I'll tell you more later.

Collaborator

oreoshake commented Feb 20, 2013

I like the new impl more. The normalize_reporting_endpoint is a little janky, but it's definitely an improvement.

See if you have an implementation preference. I will say in the new branch, I was having trouble pushing #normalize_reporting_endpoint down into a strategy so that says something.

Meh no biggie, I don't think there's anything horribly wrong w/ that. I like the strategy pattern. The tests pass. I'm going to do some manual testing but it looks like it's ready to merge if you wanna open a PR

Collaborator

oreoshake commented Feb 20, 2013

merged your other branch in 29cba15

@oreoshake oreoshake closed this Feb 20, 2013

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