? ? ? ? Pending

User tests: Successful: Unsuccessful:

avatar bembelimen
bembelimen
28 Mar 2021

Pull Request for Issue #31709 .

Summary of Changes

Rebuild of the com_csp extension.

Instead of generic configuration one can apply specific entries.

Changes:

  • Detect mode separated from reporting
  • Custom reporting URL
  • Full URL + information saving
  • Individual entries
  • Minor improvements

Testing Instructions

Probably needs some fine tuning here and there. Feedback is really appreciated.

Known issues

  • base-uri
  • plugin-types
  • form-action
  • frame-ancestors
  • navigate-to

not fully supported. Not sure if they're needed in the first version, could be added later.

@zero-24

avatar bembelimen bembelimen - open - 28 Mar 2021
avatar bembelimen bembelimen - change - 28 Mar 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Mar 2021
Category Administration com_csp Language & Strings Front End SQL Installation Postgresql Plugins
avatar bembelimen bembelimen - change - 28 Mar 2021
The description was changed
avatar bembelimen bembelimen - edited - 28 Mar 2021
avatar bembelimen bembelimen - change - 28 Mar 2021
Labels Added: ? ?
avatar bembelimen bembelimen - change - 28 Mar 2021
Title
Improve CSP
[4.0] Improve CSP
avatar bembelimen bembelimen - edited - 28 Mar 2021
avatar bembelimen bembelimen - change - 28 Mar 2021
The description was changed
avatar bembelimen bembelimen - edited - 28 Mar 2021
avatar brianteeman
brianteeman - comment - 28 Mar 2021

As soon as the component is enabled in either enforce or report mode and site or admin then every page says an error has occurred

image

avatar bembelimen
bembelimen - comment - 28 Mar 2021

Hello @brianteeman ,
thank you for your review. I reinstalled everything and tried the different options, but couldn't trigger the error.

Do you have an idea?

avatar brianteeman
brianteeman - comment - 28 Mar 2021

Do you have an idea?

Sorry no idea. I can't see anything in logs etc.

avatar brianteeman
brianteeman - comment - 28 Mar 2021

I've done a complete fresh install and git reset etc and that "error" has gone - weird

I realise now that I was supposed to do a reinstall of joomla after applying the pr

avatar brianteeman
brianteeman - comment - 28 Mar 2021

So now I have it working I am unsure on how to test.

With the previous component I could just enable detect mode in either site or admin and would almost immediately get entries

avatar bembelimen
bembelimen - comment - 28 Mar 2021

So now I have it working I am unsure on how to test.

With the previous component I could just enable detect mode in either site or admin and would almost immediately get entries

It was too late yesterday, I missed some changed in the reporting controller. Now it should report again, if you are in "report" mode or if you have enabled reporting manually.

avatar brianteeman
brianteeman - comment - 28 Mar 2021

are you sure? still not seeing it

avatar brianteeman
brianteeman - comment - 28 Mar 2021

Those last commits did the trick and I can now see reports

image

avatar bembelimen
bembelimen - comment - 28 Mar 2021

Yes, the order was not correct. Thanks for testing.

avatar brianteeman
brianteeman - comment - 28 Mar 2021

Its a bit late for me to continue testing tonight. Unlike you I need my sleep ;)

One observation regarding the ui/ux

The clickable link is the url. This is confusing especially as there are links that appear to be to the page you are on.

I'm not sure what the text would be but I would make the url non-clickable and add something else as the clickable link - perhaps something like "view report" - although it would need to be unique for accessibility

Actually the current links arent accessible anyway as you have multiple identically named links

avatar brianteeman
brianteeman - comment - 28 Mar 2021

The other thing I would consider is to have a message at the top of the screen when you are running in "report" mode. Otherwise if i understand correctly publishing a report would have no impact. Perhaps something similar to com_redirects

image

avatar bembelimen
bembelimen - comment - 29 Mar 2021

The whole URL (document_uri) has only one purpose: tell the user where he/she/it can find the violation. But for CSP itself it's absolut unnecessary. So it could be "random" (as a violation appears in most cases on different pages on a website). So if you have a good idea, I'm happy to remove the link there and put something else in (and have the URL e.g. as small information below).

I like the idea of the message. Will try to do some cleanups in the next days (like DB table migration) and then I will add the message.

avatar brianteeman
brianteeman - comment - 29 Mar 2021

Another thing that came to mind is that there should not be a trash instead they should be deleted. The reason is that if you trash an item then the violation will never be reported again (just like an unpublished report) except this time you never know about it.

The other thing is that from the UI there is no way to tell what the meaning of publishing a report is. I don't know myself.

avatar bembelimen bembelimen - change - 29 Mar 2021
Labels Added: ?
avatar bembelimen bembelimen - change - 31 Mar 2021
Labels Added: ?
Removed: ?
avatar brianteeman
brianteeman - comment - 31 Mar 2021

Thanks for the updates. Will try to find some time to test tomorrow.

Can you also please change all references in the language file from CSP to Content Security Policy

avatar bembelimen
bembelimen - comment - 31 Mar 2021

Thanks for the feedback, added the missing validation. Yep, will start renaming it tomorrow.

avatar brianteeman
brianteeman - comment - 31 Mar 2021

I am still missing

The other thing is that from the UI there is no way to tell what the meaning of publishing a report is. I don't know myself.

avatar bembelimen
bembelimen - comment - 1 Apr 2021

When you publish a report (which is now renamed to enabled/disabled) the set value is used in the header of the CSP header.

avatar brianteeman
brianteeman - comment - 1 Apr 2021

Click on the status icon
Expected behavior: The item status is toggled
Actual behavior: The item is selected only

This only happens in enforce mode and means that you cannot disable a line that is breaking your site without disabling com_csp first

avatar brianteeman
brianteeman - comment - 1 Apr 2021

Run CSP in detect mote and gather some reports
Disable CSP in the options
image

The list view is now completely broken
image

avatar brianteeman
brianteeman - comment - 1 Apr 2021

So for me I still think that even with all of the changes in this PR this component is not usable by the average user and should not be included in the core of Joomla. The UI is far too confusing and its far too easy to completely break your site. I can foresee that the default response to any bug report in the future will be "did you enable com_csp? please disable it and try again"

The problems are from my perspective

  1. An assumption that csp is there to protect my site from "unwelcome" insecure content on my site. I would not expect that enabling one of these rules would have any impact on a clean install of Joomla. Try it and you will see your admin break.
  2. I have worked out that the url and information columns are for reference only. The columns that count are blocked element and directive. This is the cause of the main problem with the component. In a regular joomla component eg com_content the clickable item in the list is the unique identifier. So the expected behaviour when you enable that item is that it applies to the url. But what it actually refers to is the Blocked Element and Directive
  3. Which leads directly to the third problem. You only need to enable one line that has a blocked element of "unsafe-inline" and a directive of "script-src-attr" for this to be applied to the entire site. However there is no change in the UI for the other lines so a user would not expect that line to be enabled.

Looking at the screenshot below a typical Joomla user eg like me who is still learning about content-security-policy would assume that only the enabled rows are "active" and that the items in the red box are all disabled. After all that is what the status says.
image

However its not true. enabling one line will make any other line with the same blocked element and directive also enabled but the UI will never show that

As a result I would expect that a user would (just like I did) enable mutliple rows with the same blocked element and directive. Then on finding that their site is broken they go to the com_csp (that would be a guess from the user based on it perhaps being the last they thing did - otherwise they waste hours tracking down the bug) and look for a line that is enabled for the URL they have observed the problem on. Seeing that there is an enabled rule they make the educated guess that the rule is what is breaking their site and they disable the line. But guess what their site is still broken. Because there is another line for a completely different component which is unrelated to the bug you are investigating but happens by coincidence have the same blocked element and directive and that is enabled. So disabling the first line had no impact as the second line (which might be on page 99) also needs to be disabled.

Sorry but I can't see a way around these issues. Its not suitable for the core and should be an extension. I have to be honest and say that even though I now understand the different csp directives etc I wouldn't install it myself.

avatar brianteeman
brianteeman - comment - 1 Apr 2021

Final comment.
On a completely clean install of Joomla (with this PR of course) go to com_csp and enable it and set it to enforce and admin. Do not change anything else
image

As I have not enabled anything in the options OR enabled anything that has been reported I would expect that Joomla will continue to work as before.

However that is not the case. The skipto plugin is broken, the ability to select an item in a list is broken, tinymce is broken and I suspect if I continued there would be even more things broken.

This is because by default without doing anything more than enabling the component the following policy is enabled
content-security-policy: default-src 'self' ; frame-ancestors 'self'; upgrade-insecure-requests; block-all-mixed-content

Even if you go into the component options again and disable the only two options that were enabled by default you still have a broken site as you still have a csp that breaks the core

content-security-policy: default-src 'self' ; block-all-mixed-content

avatar brianteeman
brianteeman - comment - 1 Apr 2021

@rdeutz please re-open my pr to remove com_csp

avatar zero-24
zero-24 - comment - 1 Apr 2021

Yes but thats not an issue of com_csp but that the core still uses inline scripts. The intention for 4.x is the focus on the frontend, for that reason its also the default option. ;-)

And when the complete process is followed meaning: report only, detect mode, review the reports, enforce mode. Than also the backend works.

With the extensions done here even that can be avoided when the hashes have been added to the reports.

CSP isnt as easy as just enabeling two options but with the tooling here it is much simpler than running all of that manually.

avatar brianteeman
brianteeman - comment - 1 Apr 2021

Yes but thats not an issue of com_csp but that the core still uses inline scripts.

You cant separate the two

And when the complete process is followed meaning: report only, detect mode, review the reports, enforce mode. Than also the backend works.

That would be the undocumented process that I'be been requesting documentation for

avatar bembelimen
bembelimen - comment - 1 Apr 2021

I agree with you, that CSP is not that easy and there are still some big gaps in the component, but I'm more in favour of disabling minor stuff we don't need and try to improve the rest. (Now as the component is tested the first time in a "real" environment)

So for starter (I hope you're willing to continue giving feedback) I would suggest:

  • remove the admin client to prevent breaking in the backend
  • find a good solution for the list link. THB I have no good idea yet, the link is only important to see, where the violation occoured. So it can be made small.
  • Add some more hints/warnings when enabling e.g. the enforce mode (a text separator with showon?)
  • Get some documentation written for the help page (maybe @zero-24 can help here?)
  • Improve the warning text recognizion when unsafe-inline is activated somewhere (there is still a check, but I'm not sure if it's still correct).
avatar brianteeman
brianteeman - comment - 1 Apr 2021

The intention for 4.x is the focus on the frontend, for that reason its also the default option. ;-)

Just by enabling com_csp for the site (instead of the admin as I did before) you block 5 assets including the main banner image

Now as the component is tested the first time in a "real" environment

I tested this before and made many of these comments before. However almost no one else has tested it because they simply didnt understand what it was or how to use it. Thats still the case and I am still the only person testing. I am aware that both the bug squad and cms release team have said that they are working on testing the release blockers so I can only assume that they are also not testing this because theydont understand it.

Do you see a pattern here. Software merged before it was ready so it could be "fixed later" only for the originaly committer to abdon it for others to try and fix. All the while adding further delay to the release of J4.

@bembelimen you should know this better than most as 15 months ago there was a debate about pulling workflows or rewriting as it was seen as THE release blocker for J4. Some of us pointed out that there were many release blockers and here we are 15 moths later still with the same release blockers.

find a good solution for the list link. THB I have no good idea yet, the link is only important to see, where the violation occoured. So it can be made small.

I would change it so that the first column is the blocked element / directive. and then the individual reports eg url and sample are grouped as sub-record of that master row

Get some documentation written for the help page (maybe @zero-24 can help here?)

I've been asking for this since the very first day as @zero-24 knows and still no signs of it.

avatar bembelimen
bembelimen - comment - 1 Apr 2021

@bembelimen you should know this better than most as 15 months ago there was a debate about pulling workflows or rewriting as it was seen as THE release blocker for J4. Some of us pointed out that there were many release blockers and here we are 15 moths later still with the same release blockers.

Yep, I know that the workflow was also on the brick, but I also hope, that you saw, that I try to hold my promises by fixing it. (And I'm aware, that there are still two RB concerning the workflow).

I also see, that you spend a lot of time into the CSP, that's why I'm grateful, that you give feedback here. I'm willing to spend time to get this component release ready (probably by cutting different features out). But it will not work without you helping, so I would give you the choice: if you see that we get this component working, I'm willing to spend the time to finish this PR. If you say: no way, then we can stop and throw it away.
I'm very unemotional here, both ways are good for me, I just wanted to try the positive way first.

avatar brianteeman
brianteeman - comment - 1 Apr 2021

I also hope, that you saw, that I try to hold my promises by fixing it.

Exactly which is why when you asked for two weeks I sat back and waited patiently

I also see, that you spend a lot of time into the CSP, that's why I'm grateful, that you give feedback here.

Just as I spent a lot of time testing workflows, atum and cassiopeia

But it will not work without you helping,

What about the rest of the contributors. There are 46 voting members of the production team. Are you really saying that if I (not a member of any team) dont spend my time testing this then there is no one else?

If you say: no way, then we can stop and throw it away.

Now who is being emotional ;) I can guarantee that if I say no way then the code wont be removed.

If the release lead doesn't have the time to make decisions then hopefully the department lead will make those decisions.

It shouldnt all be left to one person writing code and one person testing it

avatar zero-24
zero-24 - comment - 1 Apr 2021

I've been asking for this since the very first day as @zero-24 knows and still no signs of it.

Initial docs and help pages have been written long before a few other joomla 4 features have even been added to the docs ans how com_csp works haven been even recored on video and reported on in the magazine ;-)

But as mentiond many times i'm happy to extend that docs with whatever is usefull. Given its changing here a bit again i will add the docs required label here and review the docs again once thats merged here.

It shouldnt all be left to one person writing code and one person testing it

Its not only you and Benjamin i'm also following here but have not found the time yet to test this PR but its still on my radar.

Just by enabling com_csp for the site (instead of the admin as I did before) you block 5 assets including the main banner image

What browser? When the image directive is not set it should fallback to the default-src and therby allow all images loaded from the local site. When there are issues with that i would like to take a look into that browser.

And what other settings do you have enabled?

Without any rules set (no detect have been run) it should block google fonts but thats expected as it has not been allowed yet.

avatar brianteeman
brianteeman - comment - 1 Apr 2021

Clean install with blog sample data
Enable csp and set to enforce. Leave all other settings at default

Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-DAE1fuxB77vgFrnzh/MDpvXhwa2NBTvJI03Bi5bjlao='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:569 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:574 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:579 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:584 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

The first of those blocks the hero image
<div class="mod-custom custom banner-overlay" style="background-image: url(/joomla-cms/images/banners/banner.jpg)" >

avatar zero-24
zero-24 - comment - 1 Apr 2021

Thats are all inline style issues and are blocked because inline styles are not allowed by your CSP. Using the report stuff done here you can whitelist them by using the hash and dont relay on unsafe-inline.

avatar brianteeman
brianteeman - comment - 1 Apr 2021

You are completely missing the point.

Simply enabling com_csp on a default joomla install should not break it.

avatar alikon
alikon - comment - 1 Apr 2021

it's already disabled by default
so use it if you know what your are doing could help ?
no release blocker imho

avatar zero-24
zero-24 - comment - 1 Apr 2021

You are completely missing the point.

Simply enabling com_csp on a default joomla install should not break it.

Agree but thats not an issue within the CSP Component but that some core stuff still uses inline styles or inline scripts while it should not.

So the sample data has to be patched in this case. ;-)

avatar zero-24
zero-24 - comment - 1 Apr 2021

The component only Highlights that issue now, so please dont shout the mesanger ;-)

avatar brianteeman
brianteeman - comment - 1 Apr 2021

Whichever way you look at it and pass the blame. Enabling the component breaks both the site and the admin. They both work perfectly without it. Even if you know what csp is you would not expect it to break a default installation just by turning it on. The fact that you dont see it as a release blocker or worthy of your time to test it is because like far too many members of the production department you never use Joomla in the real world.

@bembelimen now I see why people don't test this. they are told that they dont know what they are doing or that the error is elsewhere.

To answer your previous question if I was still willing to test your changes - the answer is therefore no. I would rather clean the fluff from my navel, it would be more productive.

avatar alikon
alikon - comment - 1 Apr 2021

They both work perfectly without it.

that's my point
?

avatar bembelimen
bembelimen - comment - 1 Apr 2021

To answer your previous question if I was still willing to test your changes - the answer is therefore no. I would rather clean the fluff from my navel, it would be more productive.

Thanks for your past tests.

avatar bembelimen bembelimen - change - 1 Apr 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-04-01 19:41:44
Closed_By bembelimen
Labels Added: ? ?
Removed: ?
avatar bembelimen bembelimen - close - 1 Apr 2021
avatar brianteeman
brianteeman - comment - 1 Apr 2021

@bembelimen thanks for trying

avatar brianteeman
brianteeman - comment - 2 Apr 2021

So the sample data has to be patched in this case. ;-)

Just for future reference it is NOT the sample data that has the background style - it is a core functionality of the custom module

avatar zero-24
zero-24 - comment - 2 Apr 2021

I'm sad that this PR got closed :( I would still be happy to work and improve the CSP stuff.

Enabling the component breaks both the site and the admin.
They both work perfectly without it.

Thats true for most security systems right? When you install a lock on your door you cant got into it without giving the key to everyone who still should use it (setting up an allowlist) but it works too without it but than everyone can come into that door. And without that seccond step the door is locked for everyone else but yourself.

Even if you know what csp is you would not expect it to break a default installation just by turning it on.

Agree but the people that know CSP also know that when i still use inline styles or scripts on my site there is no magic button i can hit and expect everything to work too as CSP is usually very site specific and intended to block inline stuff by default.

The fact that you dont see it as a release blocker

That has not been said here right? The label is also here right?

or worthy of your time to test it is

Its 5 days since this PR was opend, I'm sorry that i dont test all PRs i'm looking into insted after they are opend here? I only said i have not found yet the time to test it.

Just for future reference it is NOT the sample data that has the background style - it is a core functionality of the custom module

Point taken while the sample data also has a few places where it still uses inline styles too. I thourgh this would be one of them.

avatar brianteeman
brianteeman - comment - 2 Apr 2021

The fact that you dont see it as a release blocker

That has not been said here right? The label is also here right?

Not every comment is addressed at Tobias. Yes it has been said here.

Its 5 days since this PR was opend,

And how many months since people started reporting that it was not usable?

Your analogy about the door is a good one. When you fit a door with a new lock you can still open the door with the key. You dont have to find a keymaker who is able to make a new key for you. Even if you did have to find a keymaker then the door would be closed for everyone and not left hanging on its hinges only partially ,opening and closing.

The simple fact is that as I have said since day one this component is not suitable for inclusion in the core of joomla as it is beyond the skillset of the average user to configure. If it didn't break their site if they enabled it to see what it did it wouldn't be so bad but as it does then its really bad experience.

As for the documentation do you really think that this is a correct description and enough information https://help.joomla.org/proxy?keyref=Help40:Content_Security_Policy_Reports&lang=en

image

Add a Comment

Login with GitHub to post a comment