-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
[JENKINS-66729] Un-inlining UsageStatistics/footer.jelly (CSP issue) #Hacktoberfest #5787
Conversation
@@ -0,0 +1,3 @@ | |||
Behaviour.addLoadEvent(function() { | |||
loadScript("https://usage.jenkins.io/usage-stats.js?${statData}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. Have you tested it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sir I used #5786 as reference for this issue.
I ran the yarn tests
as it was written but is showed 2 test case passed.
Can you help me out. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be tested interactively with a web browser in addition to the test automation. Human evaluation of presentation of a web page sees things that the automated test assertions may not be checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statData
is a java variable that is injected in the Jelly. If you are moving to a separate file, this value will be not provided to the JS file.
You can find more information in https://www.jenkins.io/doc/developer/security/xss-prevention/#passing-values-to-javascript.
But yeah, please test it manually ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Wadeck @MarkEWaite can you check now?
@MarkEWaite @daniel-beck @Wadeck can you review the PR now ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not manually tested but this version should work a lot better :)
Have you tested it?
core/src/main/resources/hudson/model/UsageStatistics/footer-resources.js
Outdated
Show resolved
Hide resolved
core/src/main/resources/hudson/model/UsageStatistics/footer-resources.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the new version should work.
We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process
Thanks a lot for your contribution @abhijeet007rocks8 ! |
See JENKINS-66729.
Script Tag for
UsageStatistics/footer.jelly
moved to separate file for CSP reasons.Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeDesired reviewers
@daniel-beck @Wadeck
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).