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] com_csp and http_headers plugin conflict with each other #25592

Closed
brianteeman opened this issue Jul 16, 2019 · 6 comments
Closed

[4.0] com_csp and http_headers plugin conflict with each other #25592

brianteeman opened this issue Jul 16, 2019 · 6 comments

Comments

@brianteeman
Copy link
Contributor

Steps to reproduce the issue

In com_csp go to the options and make the mode Detect
image

Check the response headers on the front end and you should see
image

Browse several pages on the front end and then go back to com_csp and refresh and you should see some entries
Publish those entries
image

Go back to the options and change the mode to Automatic
image

Now refresh the front end and check the response headers. You should see something like
image

Now go the http_headers plugin and create an additional csp header
image

Now refresh the front end and check the response headers. You should see something like
image

The expected behaviour was that the additional csp header added in the plugin would be added to the csp headers already set in com_csp

The actual behaviour is that the plugin overwrites the component settings

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

The expected behaviour was that the additional csp header added in the plugin would be added to the csp headers already set in com_csp

Where is that expected behavior based on? At least from me it is intended to be working exactly that way. That "additional" header is coming from the initial plugin and there was no com_csp support so that additional was referring to be additional to the suggested default headers presented in the plugin. Maybe it is not the correct word for that option anymore, maybe Other Http Headers would be better now?

In the early stages of com_csp I was thinking about an extra Extend option that would use the auto generated rules and merge them with static rules but this has been dropped because I have not found any use case where this would be necessary for that reason it is not implemented yet.

But that Extend option is now back on my list as it seams to be useful for some use cases.

The implementation will be reconsidered at the time I can do the next steps on the http headers.

Thanks.

@zero-24 zero-24 self-assigned this Jul 16, 2019
@brianteeman
Copy link
Contributor Author

Where is that expected behavior based on?

It is called "additional"
It is not called "replaced"

As com_csp wont work without the plugin being enabled I expect them to work together not to compete with each other

Maybe if you could point me to the documentation?

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

As com_csp wont work without the plugin being enabled I expect them to work together not to compete with each other

Technical they are only work together :D But I agree that mention option is conflicting as the plugin was in the initial version intended to work different.

Maybe if you could point me to the documentation?

I have started to document the http headers in the docs but not yet the details you mention here. The main reason is that there are still plans to rewrite bigger parts of the plugin code to work better with com_csp and the core.

It is called "additional"
it is not called "replaced"

Why it is called additional and why this might be misleading now is mention above already. :)

What is you opinion on the following proposals:

1. Proposal

The mention Extended mode that would combine the two fields into one header where possible.

2. Proposal

Remove the csp header form the additional headers section so it can only be set via com_csp and no overwrite is happening.

3. Proposal

Keep both options but make sure there are documentation that mention the intended consequences.

4. Proposal

Rename the option to Manual Http headers an clearly state in the description what is happening here.

What do you think about the proposals and do you have another suggestion to fix the issue?

@brianteeman
Copy link
Contributor Author

  1. Dont let you set CSP headers if you have used com_csp

  2. Change the wording to Replace

  3. Move all the plugin options to the component. Then you are only configuring headers in one place

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

Thanks for the suggestions.

@zero-24
Copy link
Contributor

zero-24 commented Jul 27, 2019

Please checkout: #25713

@zero-24 zero-24 closed this as completed Jul 27, 2019
@zero-24 zero-24 removed their assignment May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants