Skip to content

Forward fit router UI security fixes#25206

Merged
auden-woolfson merged 6 commits intoprestodb:masterfrom
auden-woolfson:router_whitesource_fix_oss
Jun 4, 2025
Merged

Forward fit router UI security fixes#25206
auden-woolfson merged 6 commits intoprestodb:masterfrom
auden-woolfson:router_whitesource_fix_oss

Conversation

@auden-woolfson
Copy link
Contributor

@auden-woolfson auden-woolfson commented May 27, 2025 •
edited
Loading

Description

Forward fit changes to the router ui from IBM's internal repo. These changes were introduced to patch security vulnerabilities

For folks with access to IBM's internal repo, here is the list of patches:
https://github.ibm.com/lakehouse/presto/security/dependabot?q=is%3Aclosed

Motivation and Context

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Update router UI to eliminate vulnerabilities

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label May 27, 2025
@auden-woolfson auden-woolfson force-pushed the router_whitesource_fix_oss branch from a0ecb46 to b9166ec Compare May 27, 2025 17:41
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 27, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@ZacBlanco
Copy link
Contributor

ZacBlanco commented May 27, 2025

IMO, we should instead try to refactor the router UI components into presto-ui since they share a lot of the base code, then maybe we can create a 2nd UI artifact during the presto-ui build process and use it when generating the presto-router artifact, similar to what we do with presto-main

@auden-woolfson
Copy link
Contributor Author

auden-woolfson commented May 27, 2025

IMO, we should instead try to refactor the router UI components into presto-ui since they share a lot of the base code, then maybe we can create a 2nd UI artifact during the presto-ui build process and use it when generating the presto-router artifact, similar to what we do with presto-main

This is a good idea. We will likely still need some router specific UI components though. @ShahimSharafudeen can you weigh in on this?

@ShahimSharafudeen
Copy link
Contributor

ShahimSharafudeen commented May 28, 2025

This is a good idea, especially from a security perspective. Through this refactoring, we can reduce security vulnerability issues in common dependencies by using the same base code in both UIs.

@auden-woolfson auden-woolfson changed the title Fix whitesource errors in router Forward fit router UI security fixes May 29, 2025
@auden-woolfson auden-woolfson force-pushed the router_whitesource_fix_oss branch from b9166ec to c99acd8 Compare May 29, 2025 17:41
@auden-woolfson
Cherry pick router whitesource fixes
c17cadb 
Co-authored-by: a-alosaimi <a.alosaimi@ibm.com>
@auden-woolfson auden-woolfson force-pushed the router_whitesource_fix_oss branch from c99acd8 to c17cadb Compare May 29, 2025 17:47
@auden-woolfson
Copy link
Contributor Author

auden-woolfson commented May 29, 2025

Going to move forward with this PR to quickly patch whitesource errors

@auden-woolfson auden-woolfson marked this pull request as ready for review May 29, 2025 17:48
@auden-woolfson auden-woolfson requested review from a team and yhwang as code owners May 29, 2025 17:48
@prestodb-ci prestodb-ci requested review from a team and Joe-Abraham and removed request for a team May 29, 2025 17:48
aaneja
aaneja reviewed May 29, 2025
View reviewed changes
presto-router/src/main/resources/router_ui/index.html
<link href="assets/presto.css" rel="stylesheet">

<!-- Bootstrap JS -->
<script src="vendor/bootstrap/js/bootstrap.bundle.min.js"></script>
Copy link
Contributor

@aaneja aaneja May 29, 2025

Choose a reason for hiding this comment

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

Its a little non-obvious that we went from bootstrap 3.3.5 to 5.3.3. Can we add the version number we use in the comment on L42 ?

aaneja
aaneja reviewed May 29, 2025
View reviewed changes
presto-ui/src/static/vendor/jquery/jquery-3.7.1.js Outdated
rhash = /#.*$/,
rantiCache = /([?&])_=[^&]*/,
rheaders = /^(.*?):[ \t]*([^\r\n]*)$/mg,
rheaders = /^([^\n\r:]*):[ \t]*([^\r\n]*)$/mg
Copy link
Contributor

@aaneja aaneja May 29, 2025

Choose a reason for hiding this comment

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

nit: this seem like a unrelated change ?

aaneja
aaneja reviewed May 29, 2025
View reviewed changes
presto-router/src/main/resources/router_ui/src/package.json Outdated
"@babel/preset-env": "^7.24.5",
"@babel/preset-flow": "^7.24.1",
"@babel/preset-react": "^7.24.1",
"babel-core": "^5.8.38",
Copy link
Contributor

@aaneja aaneja May 29, 2025

Choose a reason for hiding this comment

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

Does it matter that we moved to a lower level of babel-core ?

aaneja
aaneja previously approved these changes May 29, 2025
View reviewed changes
Copy link
Contributor

@aaneja aaneja left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Since we don't have UI based tests, it would be good to post a before and after screenshots/ gifs of basic functionality as a smoke test

auden-woolfson and others added 4 commits May 29, 2025 13:21
@auden-woolfson
Version change
71a1fb2
@auden-woolfson
Undo unrelated change
c878c02
@auden-woolfson
fix loading bug
b836228
@yhwang
update deps
e6021c7 
- update deps in the package.json
- add css files and font files
- update index.html to remove unnecessary code

Signed-off-by: Yihong Wang <yh.wang@ibm.com>
@auden-woolfson
Copy link
Contributor Author

auden-woolfson commented Jun 3, 2025

Before UI changes (master branch):

image

After

image

@auden-woolfson auden-woolfson requested a review from aaneja June 3, 2025 17:59
aaneja
aaneja approved these changes Jun 4, 2025
View reviewed changes
Copy link
Contributor

@aaneja aaneja left a comment

Choose a reason for hiding this comment

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

LGTM. Basing this on screenshots reviewed, and a basic look at the upgraded dep versions

@auden-woolfson auden-woolfson merged commit a471b47 into prestodb:master Jun 4, 2025
98 checks passed
@unidevel unidevel mentioned this pull request Jun 19, 2025
Closed
6 tasks
This was referenced Jul 4, 2025
Merged
Closed
Closed
This was referenced Jul 24, 2025
Merged
Merged
@prestodb-ci prestodb-ci mentioned this pull request Jul 28, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaneja aaneja aaneja approved these changes

@ZacBlanco ZacBlanco ZacBlanco approved these changes

@ShahimSharafudeen ShahimSharafudeen Awaiting requested review from ShahimSharafudeen

@yhwang yhwang Awaiting requested review from yhwang

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner automatically assigned from prestodb/committers

@Joe-Abraham Joe-Abraham Awaiting requested review from Joe-Abraham Joe-Abraham was automatically assigned from prestodb/ibm-reviewers

Assignees

No one assigned

Labels

from:IBM PR from IBM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@auden-woolfson @ZacBlanco @ShahimSharafudeen @aaneja @prestodb-ci @yhwang