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

[4.0] Remove com_csp and move the CSP configuration to the HttpHeaders plugin #33550

Merged
merged 11 commits into from
May 7, 2021

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented May 4, 2021

Summary of Changes

After discussions within Production it has been decided today (official note will be included in the upcomming meeting minutes) to drop com_csp from Joomla 4.0 and move the "manuall" CSP settings back to the Plugin. While it could be re-implemented in a future version.
This PR now does that by removing the backend and frontend code of com_csp as well as install and update sql logic stuff.

With that removal the collection and autogeneration of CSP rules will be gone from Joomla 4.0 but the Plugin will still allow to setup the CSP rules.

Testing Instructions

Test Case #1

Test Case #2

Actual result BEFORE applying this Pull Request

com_csp is there

Expected result AFTER applying this Pull Request

com_csp is gone

Documentation Changes Required

  • The docs pages for the component and the options page needs to be removed
  • The existing doc page needs to be adjusted and updated with the changes done here.

B/C implications

With the removal of com_csp the auto generated csp rules are gone and they will not be migrated, please extract them before you install the update that drops this feature and set it as Forced HTTP Header or via the new setting implmented here.

Personal note

I would like to thank all the people that helped to get to this feature to this place specificly @yvesh and @SniperSister who worked together with me to bring that idea to live. As well as all the other people who put work and effort into extending and improving the current implementation until now. I personally still think this is an important feature to the CMS but I will follow the decision taken by Production.

@zero-24 zero-24 added this to the Joomla 4.0 milestone May 4, 2021
@zero-24 zero-24 requested a review from richard67 May 4, 2021 19:36
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels May 4, 2021
@zero-24 zero-24 changed the title [4.0] Remove com_csp and move the CSP configuration to the httpHeaders plugin [4.0] Remove com_csp and move the CSP configuration to the HttpHeaders plugin May 4, 2021
This was referenced May 4, 2021
@brianteeman
Copy link
Contributor

When this is removed the helpTOC script will need to be rerun

@richard67
Copy link
Member

The removal of the files vis the script.php should be part of the build script and is not added here in for that reason.

If so, it has to be done also before the next J4 release (Beta or RC, whatever it will be ;-) ).

In order not to miss that I'd prefer it to be done with this PR, since the files and folder removal in script.php has just recently been updated after the upmerge so it is ready for release, except of changes due to this PR here. Or @wilsonge gives me enough time to do it after the merge of this PR and before the release, so I can do it using the script.

@brianteeman
Copy link
Contributor

100% agree with @richard67

@brianteeman
Copy link
Contributor

Please remove

MOD_MENU_MANAGE_CSP="Content Security Policy"

@sandewt
Copy link
Contributor

sandewt commented May 5, 2021

Install the build (https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/33550/downloads/43088/Joomla_4.0.0-beta8-dev+pr.33550-Development-Full_Package.zip)

Not Found
The requested URL was not found on this server.

@richard67
Copy link
Member

@sandewt The links might be outdated due to new commits to this PR. You can find the right links when going down to the bottom, using the "Show all checks" to expand the CI checks, then follow the "Details" link at the right side of the "Downloads" step. There you can find the link to the full installation package for the 1st test and the link to a custom update URL which you can use for the 2nd test.

@richard67
Copy link
Member

richard67 commented May 5, 2021

@sandewt Please wait a bit with testing, the PR will receive an update soon, and then new packages will be built again.

@sanderpotjer Sorry, ignore the notification, I mentioned you by accident.

@sandewt
Copy link
Contributor

sandewt commented May 5, 2021

@sandewt The links might be outdated due to new co...

Thanks @richard67, I found the package(s).

Following question, how to install 4.0.0beta7 ?

@richard67
Copy link
Member

@sandewt The PR meanwhile is ready again for testing. You have to get the new packages because there have been changes.

@sandewt
Copy link
Contributor

sandewt commented May 5, 2021

I have tested this item ✅ successfully on f5cf7fe

Test Case #1 is OK
Test Case #2 is OK

Didn't look / did nothing with the B/C implications !?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33550.

@ReLater
Copy link
Contributor

ReLater commented May 5, 2021

Bad news. Thank you for your efforts concerning com_csp @zero-24 et al.

@zero-24
Copy link
Contributor Author

zero-24 commented May 5, 2021

Please remove

MOD_MENU_MANAGE_CSP="Content Security Policy"

Sorry i have missed that comment yesterday. Its done now: ea24029

@richard67
Copy link
Member

I've restored @sandewt 's test result in the issue tracker so it's properly counted, since the commit which has invalidated the test result was just a removal of an unused language string.

@sandewt
Copy link
Contributor

sandewt commented May 6, 2021

Didn't look / did nothing with the B/C implications !?

@zero-24 Is this allowed for the test?

@zero-24
Copy link
Contributor Author

zero-24 commented May 6, 2021

Didn't look / did nothing with the B/C implications !?

@zero-24 Is this allowed for the test?

Yes it is just for the information. Given that there is no stable version of that feature shipped i dont see an issue here.

@Quy
Copy link
Contributor

Quy commented May 6, 2021

I have tested this item ✅ successfully on d4866ed


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33550.

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone May 6, 2021
@Quy
Copy link
Contributor

Quy commented May 6, 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33550.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 6, 2021
@richard67 richard67 added this to the Joomla 4.0 milestone May 6, 2021
@rdeutz rdeutz merged commit 328b7d3 into joomla:4.0-dev May 7, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 7, 2021
@zero-24 zero-24 deleted the remove_csp branch May 7, 2021 14:10
@zero-24
Copy link
Contributor Author

zero-24 commented May 7, 2021

Thanks for testing and merging here. 👍

@richard67
Copy link
Member

Silly me: When reviewing especially the SQL parts of this PR, I have not noticed that the "#__csp" table hasn't been removed from the "supports.sql" files so it is still created on new installations. Am just preparing the PR to fix that.

@richard67
Copy link
Member

See #33835 .

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 4, 2021

The http headers docs page has been updated and com_csp mention has been removed and the help pages have just been requested to be removed from the docs page too.

@c-schmitz
Copy link

Just out of interest and since the meeting minutes are not linked: What was the reason for the decision to remove the component?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants