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

ContentSecurityPolicy testing & enhancement #1581

Merged
merged 7 commits into from
Dec 7, 2018

Conversation

jim-parry
Copy link
Contributor

  • unit testing substantially complete, pending resolution of a few issues here
  • reportOnly method & property were pointless - never referenced. I enhanced these to use the reportOnly property as a default until over-ridden, and adjusted the mutator methds accordingly
  • upgradeInsecureRequests was not handled properly. I added a snippet inside buildHeaders() to generate the CSP tag

@lonnieezell Why is the sandbox property treated different from the rest (setX vs addX; boolean first parameter ignored unless empty property and second arg; if the boolean is used, you end with true or false instead of an array of sandbox policy names to enforce, and the test gets an error).

@lonnieezell
Copy link
Member

It's been a while :) But the addX methods were because they could add multiple settings, where as setX methods typically overwrites the previous values.

Looking back at the sandbox directive and reading over specs - I don't know why I did that, honestly. Can't see a reason for it now. Feel free to alter. :)

@jim-parry jim-parry changed the title WIP ContentSecurityPolicy testing & enhancement ContentSecurityPolicy testing & enhancement Dec 6, 2018
@jim-parry jim-parry requested a review from lonnieezell December 6, 2018 07:30
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

In general, I like what you're doing, but did have a couple of comments, one about naming and one that I think may be causing an issue that's not being caught?

@jim-parry
Copy link
Contributor Author

Oops - I have some cross-pollination happening, and some broken tests. Fixing!

@jim-parry jim-parry merged commit 4027f38 into codeigniter4:develop Dec 7, 2018
@jim-parry jim-parry deleted the testing14/http branch December 7, 2018 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants