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

[JENKINS-71726] [JENKINS-71727] remove inline javascript #8313

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Jul 30, 2023

See JENKINS-71727.
See JENKINS-71726.

Testing done

JENKINS-71727:

  • Do not log in
  • visit a page that is restricted
  • you get redirected to the login page
  • login
  • get redirected to the original page
  • run curl -u http://jenkins/job/test/ and checked output contains data-redirect-url to loginform and javascript src points to proper script url.
  • Message Authentication required is included in the output

JENKINS-71726:

  • Create a job without parameters
  • run it once
  • change job to require parameters
  • run job with parameters
  • visit job/2/parameters
  • click on Previous Build
  • you get redirected to job/1/
  • run curl -u user:token http://jenkins/job/test/1/parameters/ and checked output contains data-redirect-url and javascript src points to proper script url.
  • Proper message Not found is included in the output

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

N/A

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@mawinter69 mawinter69 marked this pull request as ready for review July 31, 2023 06:25
@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Jul 31, 2023
@NotMyFault NotMyFault requested review from a team July 31, 2023 08:23
@NotMyFault NotMyFault changed the title [JENKINS-71727] remove inline javascript [JENKINS-71726] [JENKINS-71727] remove inline javascript Jul 31, 2023
@Kevin-CB
Copy link
Contributor

Kevin-CB commented Aug 3, 2023

Thanks for this PR @mawinter69, I'll try to review it as soon as possible.

@Kevin-CB
Copy link
Contributor

Kevin-CB commented Aug 9, 2023

From a security perspective, this is fine. However, I've noticed some latency during each redirection, which, if I'm correct, wasn't present previously.

For example, when I try to access my job without being authenticated, it doesn't redirect me to the login form instantly but displays a blank page for longer than usual.

I think, the timing difference is related to the time it takes to load the redirect.js script and execute it.

Perhaps we could choose a different CSP approval approach, using nonce or hash.

@Kevin-CB Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Aug 9, 2023
@NotMyFault NotMyFault removed the request for review from a team August 13, 2023 09:49
Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

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

I investigated further as to why I was experiencing this delay, and found that it was simply because the script wasn't being executed. The delay I was encountering was due to the redirection initiated by the meta refresh tag, which has a delay set to 1 second (content='1).

Upon closer look on what happened, the rendered page was:

image

The script tag is malformed, taking the remainder of our request.
I'm not sure why, but the self-closing tag isn't functioning correctly. Closing it with a full tag resolves this issue.

Co-authored-by: Kevin Guerroudj <[email protected]>
Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

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

I retested it with your change, and it works fine.

@MarkEWaite MarkEWaite added the skip-changelog Should not be shown in the changelog label Nov 19, 2023
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

I've tested with this change applied to the master branch and it works well in my testing also.

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 19, 2023
@MarkEWaite MarkEWaite self-assigned this Nov 19, 2023
@MarkEWaite MarkEWaite merged commit d9cbaa0 into jenkinsci:master Nov 20, 2023
@mawinter69 mawinter69 deleted the JENKINS-71727 branch January 31, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants