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

Inject 'strict-dynamic' into CSPRO #60

Merged
merged 4 commits into from Aug 27, 2017

Conversation

aidantwoods
Copy link
Owner

@aidantwoods aidantwoods commented Jul 30, 2017

WIP at present.

Min requirements before merge:

  • Config for whether we want to inject into CSP, CSPRO (, or both)
  • Document the new config
  • Test cases

Closes #56

@aidantwoods aidantwoods force-pushed the fix/inject-strict-dynamic-on-cspro branch from 7f4a09f to 866010d Compare July 30, 2017 22:50
"<b>Strict-Mode</b> is enabled, but
<b>'strict-dynamic'</b> could not be added to <b>"
. $Header->getFriendlyName()
. '</b> because no hash or nonce was used.',
E_USER_WARNING
Copy link

Choose a reason for hiding this comment

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

please throw an exception instead, it's ugly codestyle imo.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Might be something to think about for 3.0, would be a BC break to start doing it now though.

Things to think about with errors v.s. exceptions:

  • Exceptions will generally cause headers not to reach the adapter, and so won't be sent. i.e.
    • Is no CSP better than suboptimal CSP? (bearing in mind loss of reporting header which might be stricter too).
    • Is not sending a suboptimal CSP better than HSTS, Referrer-Policy, cookie upgrades, ...?
  • It we throw exceptions, they should be genuine problems. That means not throwing an exception for advice: we cannot use exceptions to tell the dev they they should use something (learning opportunity). Learning opportunities would have to be disposed of if we converted all errors as used currently to exceptions.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Feel free to open an issue, this would be a methodology change for the library so probably isn't worth debating for this one specific case 😉

I like exceptions, I just don't think they're appropriate for every case in which errors are used in the library currently (probably a separate issue is best to discuss the decoupling).

Copy link

Choose a reason for hiding this comment

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

done.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks :D

@aidantwoods
Copy link
Owner Author

I'm considering whether these newly added config options belong really in 2.1, so might split this PR into fix + tests, and another for the new option

@aidantwoods aidantwoods removed the WIP label Aug 27, 2017
@aidantwoods aidantwoods merged commit f33c996 into master Aug 27, 2017
@aidantwoods aidantwoods deleted the fix/inject-strict-dynamic-on-cspro branch August 27, 2017 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants