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

Fixes #24062: Implementing CSP headers without duplicating Lift scripts #5342

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Jan 23, 2024

https://issues.rudder.io/issues/24062

One of a few solutions to remove the scripts appended to the HTML page by Lift : use the response byte array. The only straightforward way is to make use of mutability (array copying here) because Lift only provides us the final response (which is a case class).

Lift has a mutable partial function convertResponse which we can re-assign to intercept the script if it contains duplicates, then remove the duplicates.

There is also some fix in the 2nd commit for the healthcheck page, which cannot have 'onclick' event handlers as from CSP restrictions...

Lift has a boolean attribute extractInlineJavaScript that, when the value is true, allows to include all inline Javascript into the lift page script (which already have a nonce). So we can actually leave onclick handlers now and Lift will do the rewrite !

@clarktsiory clarktsiory requested a review from fanf January 23, 2024 17:23
…t scripts

Fixes #24062: Implementing CSP headers without duplicating Lift scripts
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

…ing Lift scripts

Fixes #24062: Implementing CSP headers without duplicating Lift scripts
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

…uplicating Lift scripts

Fixes #24062: Implementing CSP headers without duplicating Lift scripts
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

…thout duplicating Lift scripts

Fixes #24062: Implementing CSP headers without duplicating Lift scripts
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

…ders without duplicating Lift scripts

Fixes #24062: Implementing CSP headers without duplicating Lift scripts
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

…CSP headers without duplicating Lift scripts

Fixes #24062: Implementing CSP headers without duplicating Lift scripts
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

No real idea about performances, but it looks like the Correct Way to do it in lift.
Headers seems OK with no js error and no duplicate.
LGTM 👍

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5342
-- Your faithful QA
Kant merge: "Thoughts without content are empty, intuitions without concepts are blind."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/79221/console)

@amousset amousset merged commit ee0dbc1 into Normation:branches/rudder/8.1 Jan 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants