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]Make sure we do not add one header twice but we support to set a different header per client. #31128

Merged
merged 2 commits into from
Nov 14, 2020

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Oct 17, 2020

Pull Request for Issue #30962 cc @PhilETaylor

Summary of Changes

Make sure we do not add one header twice but we support to set a different header per client.

Testing Instructions

Setup Plugins -> System - HTTP Headers as this screenshot:

Screenshot 2020-10-07 at 01 04 35

Actual result BEFORE applying this Pull Request

Content-Security-Policy: TWO

Expected result AFTER applying this Pull Request

Content-Security-Policy: ONE

Documentation Changes Required

None

@PhilETaylor I'm not sure whether we should block the save as it could be valid to configure two headers with the same value. e.g. one for the backend and one for the frontend. With this change here we make sure only the first configured per site (or for both) is choosen. I'm not sure how to cleanly validate the subform at that point other than hooking into the onExtensionBeforeSave Event but than this pugin would be triggered on all extension saves does not sound that optimal to me?

@adj9
Copy link

adj9 commented Oct 17, 2020

I have tested this item ✅ successfully on a004fa3

done :)


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

@Giuse69
Copy link
Contributor

Giuse69 commented Oct 17, 2020

hi, the PR works but why not having a javascript check to warn the user that the same header with the same client is already set when fill the second one? There might more even more effective solutions (like to disable options when already set) but probably not worth the effort.
I think it's useful to at least inform the user that what he has set (multiple values for same header/client) is not what he will get (correctly with this patch but we need to inform the user of the "mistake" if not prevent)

@zero-24
Copy link
Contributor Author

zero-24 commented Oct 17, 2020

the PR works but why not having a javascript check to warn the user that the same header with the same client is already set when fill the second one?

hmm I get the point but I'm no JS expert do you know how to implement such check in the subform? The other way around could be a validation rule I guess.

@ceford
Copy link
Contributor

ceford commented Oct 18, 2020

I have tested this item ✅ successfully on a004fa3

It is not so obvious that you look for the result in the headers sent. I was expecting the form to changes! Anyway, I used my browser HTTP Header Live tool and could see the the change from TWO to ONE as described.


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

@Giuse69
Copy link
Contributor

Giuse69 commented Oct 18, 2020

hmm I get the point but I'm no JS expert do you know how to implement such check in the subform? The other way around could be a validation rule I guess.

yes, maybe better validate server-side the configuration of the plugin when saving and not accepting the save operation when parameters, also displaying an error message

Co-authored-by: Quy <quy@fluxbb.org>
@vijaykhollam
Copy link
Contributor

I have tested this item ✅ successfully on 376a8cb


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

1 similar comment
@tushar33
Copy link

tushar33 commented Nov 7, 2020

I have tested this item ✅ successfully on 376a8cb


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 8, 2020
@richard67
Copy link
Member

There is a strange error shown in drone for javascript-cs, but that's not related to this PR.

@richard67 richard67 merged commit fbdb102 into joomla:4.0-dev Nov 14, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 14, 2020
@richard67
Copy link
Member

Thanks!

@richard67 richard67 added this to the Joomla 4.0 milestone Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants