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.

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

feat: CSP enhancements #5516

Merged
merged 24 commits into from
Jan 12, 2022
Merged

feat: CSP enhancements #5516

merged 24 commits into from
Jan 12, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Dec 31, 2021 •
edited
Loading

Description

  • Auto CSP tag replacement
    • add CSP tag customization for security
    • add config for disabling auto CSP tag replacement
  • add csp_style_nonce() and csp_script_nonce() to get nonce attributes
  • add csp_style_nonce and csp_script_nonce plugin for View Parser
  • output one nonce for script and one for style tags
    • multiple nonces do not improve security
  • add Services::csp()

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Dec 31, 2021
@kenjis kenjis marked this pull request as draft December 31, 2021 07:34
@kenjis kenjis marked this pull request as ready for review December 31, 2021 07:57
MGatner
MGatner requested changes Dec 31, 2021
View reviewed changes
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

This looks like a great PR! Per our recent semver conversation: would this be best held for 4.2? I know the Kint changes were already merged...

I added a few questions. I'm abashedly not very up on CSP. I will read up on it a bit but would also appreciate if someone with better security mindset took a look. @lonnieezell maybe?

system/Common.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
system/Debug/Kint/RichRenderer.php Show resolved Hide resolved
tests/system/CommonFunctionsTest.php Outdated Show resolved Hide resolved
lonnieezell
lonnieezell reviewed Jan 1, 2022
View reviewed changes
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.

I like these changes a lot and don't see any security concerns with it.

@kenjis kenjis force-pushed the fix-csp branch 2 times, most recently from ae7d4a3 to d9c8608 Compare January 4, 2022 09:54
MGatner
MGatner approved these changes Jan 4, 2022
View reviewed changes
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Very impressive test-fu.

@kenjis
Copy link
Member Author

kenjis commented Jan 5, 2022

I will add the documentation.

@kenjis
Copy link
Member Author

kenjis commented Jan 5, 2022

Added the documentation. Review, please.

paulbalandan
paulbalandan reviewed Jan 5, 2022
View reviewed changes
tests/system/CommonFunctionsTest.php Outdated Show resolved Hide resolved
tests/system/CommonFunctionsTest.php Outdated Show resolved Hide resolved
MGatner
MGatner requested changes Jan 5, 2022
View reviewed changes
Copy link
Member

@MGatner MGatner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really good examples in the docs. A couple wording suggestions. Also, please mention this as a new feature in the changelog/upgrade guides so we don't need to go back and add it.

user_guide_src/source/general/common_functions.rst Outdated Show resolved Hide resolved
user_guide_src/source/general/common_functions.rst Outdated Show resolved Hide resolved
user_guide_src/source/outgoing/response.rst Outdated Show resolved Hide resolved
@kenjis kenjis added the 4.2 label Jan 5, 2022
@kenjis kenjis added this to the 4.2.0 milestone Jan 6, 2022
@kenjis kenjis removed the 4.2 label Jan 6, 2022
@kenjis
Copy link
Member Author

kenjis commented Jan 6, 2022

Also, please mention this as a new feature in the changelog/upgrade guides so we don't need to go back and add it.

Added changelogs/v4.2.0.

@kenjis kenjis requested review from MGatner and paulbalandan January 6, 2022 04:35
@MGatner
Copy link
Member

MGatner commented Jan 8, 2022

Let's hold this for 4.2

kenjis and others added 17 commits January 12, 2022 09:15
@kenjis
config: fix incorrect property name
e7b0727
@kenjis
feat: add config for disabling to replace nonce tag automatically
d548118
@kenjis
refactor: use csp_script_nonce() and csp_script_nonce() in Kint
bc52fad
@kenjis
feat: add Services::contentsecuritypolicy()
9b1f513
@kenjis
refactor: use Services::contentsecuritypolicy()
9ece590
@kenjis
fix: CSP nonce header is nottt sent when CSP tag replacement does not…
031e411 
… happen
@kenjis
test: make regex strict
571a34c
@kenjis
refactor: remove uneeded use statement
3fde024
@kenjis
refactor: rename from contentsecuritypolicy() to csp()
c4aaa4f
@kenjis
test: remove unneeded $this->resetFactories()
4b8b118 
It is called in CIUnitTestCase::setUp() via $setUpMethods.
@kenjis
feat: add csp_script_nonce and csp_style_nonce plugin for View Parser
b1e91dd
@kenjis
docs: update user guide
c472bb6
@kenjis @MGatner
docs: fix by proofreading
dee40c1 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis @MGatner
docs: fix by proofreading
ae90f8e 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis @MGatner
docs: fix by proofreading
161be10 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis
test: use more specific assert method
1184046
@kenjis
docs: add changelogs/v4.2.0
f686c25
@kenjis kenjis merged commit f354e3f into codeigniter4:develop Jan 12, 2022
@kenjis kenjis deleted the fix-csp branch January 12, 2022 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbalandan paulbalandan paulbalandan approved these changes

@MGatner MGatner MGatner approved these changes

@lonnieezell lonnieezell lonnieezell approved these changes

Assignees
No one assigned
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kenjis @MGatner @lonnieezell @paulbalandan