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.

Sign up for GitHub

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

Jump to bottom

upgrade jquery version #214

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

upgrade jquery version #214

wants to merge 4 commits into from

Conversation

loulou2u
Copy link
Collaborator

replace older version of jquery to patch xss vulnerability

@loulou2u loulou2u requested review from bjagg and mgillian June 21, 2023 18:02
@loulou2u loulou2u self-assigned this Jun 21, 2023
@ChristianMurphy
Copy link
Member

A couple questions:

  1. should the be pulled from the resource server, instead of being included directly here?
    It looks like there is at least one spot in the code where it does already, maybe that should also be applied else where?

    WebproxyPortlet/src/main/webapp/WEB-INF/jsp/gateway.jsp

    Line 25 in 2caa339

    <script src="<rs:resourceURL value='/rs/jquery/1.11.0/jquery-1.11.0.min.js'/>" type="text/javascript"></script>
  2. If we do keep it here, will we need to update the URLs like:

    WebproxyPortlet/src/main/webapp/WEB-INF/jsp/gatewayNewPage.jsp

    Line 31 in 2caa339

    <script type="text/javascript" src="${pageContext.request.contextPath}/scripts/jquery-1.11.0.min.js" ></script>
    to use the new script?

@loulou2u
Copy link
Collaborator Author

Yikes, yeah I'm not a fan of the hardcoding. I'll make some adjustments.. Thanks!

@loulou2u
Copy link
Collaborator Author

I changed to the latest resource-server version that uses jquery 1.12.4, and updated the references in the jsp files to use the resource path.

ChristianMurphy
ChristianMurphy approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristianMurphy ChristianMurphy ChristianMurphy approved these changes

@bjagg bjagg Awaiting requested review from bjagg

@mgillian mgillian Awaiting requested review from mgillian

Assignees

@loulou2u loulou2u

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@loulou2u @ChristianMurphy