HIVE-28496: Address CVE-2020-28487 due to 4.20.0 version of vis.js #5430

KiranVelumuri · 2024-09-03T12:37:09Z

What changes were proposed in this pull request?

https://issues.apache.org/jira/browse/HIVE-28496
Upgrade vis.js from 4.20.0 to address CVE-2020-28487

Why are the changes needed?

To address CVE-2020-28487

Does this PR introduce any user-facing change?

No. Please see the next comment for screenshots.

Is the change a dependency upgrade?

Yes. Not attaching the dependency tree output since this does not show up

How was this patch tested?

Using the HTML generated from the template file QueryProfileTmpl.jamon in the checkWebuiShowGraph test from TestQueryDisplay.java

KiranVelumuri · 2024-09-03T12:40:18Z

Link to resource vis-network.min.js from vis.js library:
vis-network.min.js

Before (with vis.min.js):

After (with vis-network.min.js):

KiranVelumuri · 2024-09-05T10:49:28Z

@okumin @zhangbutao
Could you please review this?

zhangbutao · 2024-09-05T14:28:15Z

Does this PR introduce any user-facing change?

Yes. Please see the next comment for screenshots.

You said this pr introduced user-facing change, but i don't see any difference from the screenshots you provided.

KiranVelumuri · 2024-09-05T14:40:26Z

You said this pr introduced user-facing change, but i don't see any difference from the screenshots you provided.

Yes, they are identical even after the library upgrade. I meant in the sense that this change is at the user-facing end. Updated the comment now.

zhangbutao

+1 LGTM
Thanks for the fix!

KiranVelumuri · 2024-09-06T15:56:06Z

Thank you @zhangbutao for the review!

okumin · 2024-09-08T15:08:22Z

Thanks! The motivation looks good to me.

I am checking the content on CDN and that on this PR.

CDN

% curl -o unpkg.js https://unpkg.com/[email protected]/standalone/umd/vis-network.min.js
% shasum unpkg.js
021306dceabc3358057f91b84c7dbbe6f48ac12e  unpkg.js

This PR

The checksum didn't match.

% shasum service/src/resources/hive-webapps/static/js/vis-network.min.js
173617cf2974080c9857f949ddeb424b50410c87  service/src/resources/hive-webapps/static/js/vis-network.min.js

The diff is too big to put here.

% diff service/src/resources/hive-webapps/static/js/vis-network.min.js /tmp/vis/unpkg.js

I have some questions.

Is it legal to add the Apache 2.0 header on our own? Looks like, the file includes a part with dual licenses of Apache 2.0 and MIT, and a part with MIT. I am not confident that our manner is acceptable
Could you please create vis-network.min.js so that it is exactly(or + Apache 2.0 header) the same as the file on the CDN? We have to ensure the new file is definitely safe, but it is hard to review a minified JS codes

KiranVelumuri · 2024-09-09T08:57:21Z

Thanks! The motivation looks good to me.

I am checking the content on CDN and that on this PR.

CDN
% curl -o unpkg.js https://unpkg.com/[email protected]/standalone/umd/vis-network.min.js
% shasum unpkg.js
021306dceabc3358057f91b84c7dbbe6f48ac12e  unpkg.js
This PR

The checksum didn't match.
% shasum service/src/resources/hive-webapps/static/js/vis-network.min.js
173617cf2974080c9857f949ddeb424b50410c87  service/src/resources/hive-webapps/static/js/vis-network.min.js
The diff is too big to put here.
% diff service/src/resources/hive-webapps/static/js/vis-network.min.js /tmp/vis/unpkg.js
I have some questions.

Is it legal to add the Apache 2.0 header on our own? Looks like, the file includes a part with dual licenses of Apache 2.0 and MIT, and a part with MIT. I am not confident that our manner is acceptable

Could you please create vis-network.min.js so that it is exactly(or + Apache 2.0 header) the same as the file on the CDN? We have to ensure the new file is definitely safe, but it is hard to review a minified JS codes

Thank you for the review.

I have updated the file vis-network.min.js to be the same as from the CDN.
shasum - 021306dceabc3358057f91b84c7dbbe6f48ac12e
About the licensing part, I have added the license following from earlier file vis.min.js, and yes I think it's better to keep the license and the file as is from the CDN. Also, not having the license header should be okay as it is mentioned vis.js may be distributed under either license.

okumin · 2024-09-09T14:55:52Z

Thanks. I verified the content of 55ca3d8 is precisely the same as that in the internet.

$ shasum service/src/resources/hive-webapps/static/js/vis-network.min.js
021306dceabc3358057f91b84c7dbbe6f48ac12e  service/src/resources/hive-webapps/static/js/vis-network.min.js
$ shasum /tmp/vis/unpkg.js                                              
021306dceabc3358057f91b84c7dbbe6f48ac12e  /tmp/vis/unpkg.js
$ diff service/src/resources/hive-webapps/static/js/vis-network.min.js /tmp/vis/unpkg.js
$

I also confirmed the revision is likely to work in the same way as 4.0.0.

Configuration I used.

  <property>
    <name>hive.server2.webui.explain.output</name>
    <value>true</value>
  </property>
  <property>
    <name>hive.server2.webui.show.graph</name>
    <value>true</value>
  </property>

CI failed but I believe it is unrelated.

https://ci.hive.apache.org/blue/rest/organizations/jenkins/pipelines/hive-precommit/branches/PR-5430/runs/3/nodes/455/log/?start=0

As for the license, the MIT part about Hammer.JS shall be mentioned here? To be honest, I am not confident with the rule.

https://github.com/apache/hive/blob/master/LICENSE

okumin

I approved. I would like someone to check the license point as well.

KiranVelumuri · 2024-09-10T13:50:09Z

force pushed now to retrigger Jenkins, no changes.

KiranVelumuri · 2024-09-13T07:11:22Z

I approved. I would like someone to check the license point as well.

@ayushtkn Could you please comment on the license point? Thank you.

KiranVelumuri · 2024-09-17T06:42:54Z

Both Apache Hive and vis.js are licensed under Apache License 2.0. In Applying the Apache license, version 2.0, it is mentioned that each source file must include a header.

@okumin @zhangbutao
So, we should add the Apache LICENSE 2.0 header at the top of vis-network.min.js to comply with the standards. Please reply/react so that I will apply the header to the current change and update the commit.

okumin · 2024-09-17T13:55:27Z

I assume you are not an author of vis.js, and the rule is unlikely applied if I undeistand correctly.
https://www.apache.org/legal/src-headers.html#3party

P.S. The new file includes a part of MIT license, and that's why I thought adding a header could be confusing. I wonder if this fact should be mentioned explicitly in LICENSE as we do so for such as jQuery. I'm not a professional of legal problems

KiranVelumuri · 2024-09-23T09:38:50Z

I approved. I would like someone to check the license point as well.

@zabetak Could you please comment on the license point? Thank you.

zabetak · 2024-09-23T10:12:22Z

When we include code not owned by the contributor it is best to leave the file as it is (no modifications).
Since the file is not developed by us it may be nice to add a mention in LICENSE although not strictly necessary since it is licensed under AL2.

More general, it is best to avoid minified javascript files in Apache source releases but this is a bit of a grey area so we can continue as is for the moment.

KiranVelumuri · 2024-09-23T10:33:42Z

@zabetak Thank you, we will proceed as is.
@okumin @zhangbutao I think no further changes are needed, please help in merging. Thank you.

okumin · 2024-09-24T11:47:32Z

@zabetak Just a question from my curiosity. L44- of the file is not licensed as AL2, as it is MIT. Don't we need special care in this case? Additionally, if it was GPL or something, would we have to take care of it?

I think Butao can merge this PR as a committer.

zabetak · 2024-09-24T13:47:43Z

@okumin Good catch! I haven't noticed that portions of the file (L44-49) are licensed differently. Yes, we need to handle such cases carefully especially when we are dealing with licenses that are not in category-a. Since it seems that portion of the file is only under the MIT license then we should include both a copy of the license in our source code distribution and an appropriate entry in our main LICENSE file.

In general, whenever there is a doubt about how to handle a licensing case, the correct reaction is to contact the Apache legal team (e.g., by opening a LEGAL jira ticket) and request guidance.

service/src/jamon/org/apache/hive/tmpl/QueryProfileTmpl.jamon

sonarcloud · 2024-09-26T17:41:35Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

okumin · 2024-09-27T11:31:40Z

I confirmed it would also work on my machine

asf-ci-hive added the tests pending label Sep 3, 2024

asf-ci-hive added tests unstable and removed tests pending labels Sep 3, 2024

KiranVelumuri force-pushed the HIVE-28496 branch from 1a8c9bd to 49f66c2 Compare September 4, 2024 16:54

asf-ci-hive added tests pending tests passed and removed tests unstable tests pending labels Sep 4, 2024

zhangbutao approved these changes Sep 6, 2024

View reviewed changes

KiranVelumuri force-pushed the HIVE-28496 branch from 49f66c2 to 55ca3d8 Compare September 9, 2024 08:20

asf-ci-hive added tests pending and removed tests passed labels Sep 9, 2024

asf-ci-hive added tests unstable and removed tests pending labels Sep 9, 2024

okumin approved these changes Sep 9, 2024

View reviewed changes

KiranVelumuri force-pushed the HIVE-28496 branch from 55ca3d8 to f129be6 Compare September 10, 2024 13:49

asf-ci-hive added tests pending and removed tests unstable labels Sep 10, 2024

asf-ci-hive added tests unstable and removed tests pending labels Sep 10, 2024

KiranVelumuri force-pushed the HIVE-28496 branch from f129be6 to c24e71c Compare September 11, 2024 04:04

asf-ci-hive added tests passed and removed tests pending labels Sep 11, 2024

zabetak requested changes Sep 24, 2024

View reviewed changes

service/src/jamon/org/apache/hive/tmpl/QueryProfileTmpl.jamon Outdated Show resolved Hide resolved

KiranVelumuri force-pushed the HIVE-28496 branch from c24e71c to e43ed3a Compare September 26, 2024 10:56

asf-ci-hive added tests pending tests unstable and removed tests passed tests pending labels Sep 26, 2024

HIVE-28496: Address CVE-2020-28487 due to 4.20.0 version of vis.js

a5a57a7

KiranVelumuri force-pushed the HIVE-28496 branch from e43ed3a to a5a57a7 Compare September 26, 2024 16:24

asf-ci-hive added tests pending and removed tests unstable labels Sep 26, 2024

asf-ci-hive added tests passed and removed tests pending labels Sep 26, 2024

KiranVelumuri requested review from zhangbutao and okumin September 27, 2024 06:07

zhangbutao approved these changes Sep 27, 2024

View reviewed changes

okumin approved these changes Sep 27, 2024

View reviewed changes

zabetak approved these changes Sep 27, 2024

View reviewed changes

zhangbutao merged commit d85b87c into apache:master Sep 28, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-28496: Address CVE-2020-28487 due to 4.20.0 version of vis.js #5430

HIVE-28496: Address CVE-2020-28487 due to 4.20.0 version of vis.js #5430

KiranVelumuri commented Sep 3, 2024 •

edited

Loading

KiranVelumuri commented Sep 3, 2024

KiranVelumuri commented Sep 5, 2024

zhangbutao commented Sep 5, 2024 •

edited

Loading

KiranVelumuri commented Sep 5, 2024 •

edited

Loading

zhangbutao left a comment

KiranVelumuri commented Sep 6, 2024

okumin commented Sep 8, 2024 •

edited

Loading

KiranVelumuri commented Sep 9, 2024

CDN

This PR

okumin commented Sep 9, 2024

okumin left a comment

KiranVelumuri commented Sep 10, 2024

KiranVelumuri commented Sep 13, 2024

KiranVelumuri commented Sep 17, 2024

okumin commented Sep 17, 2024 •

edited

Loading

KiranVelumuri commented Sep 23, 2024

zabetak commented Sep 23, 2024

KiranVelumuri commented Sep 23, 2024

okumin commented Sep 24, 2024

zabetak commented Sep 24, 2024

sonarcloud bot commented Sep 26, 2024

okumin commented Sep 27, 2024

HIVE-28496: Address CVE-2020-28487 due to 4.20.0 version of vis.js #5430

HIVE-28496: Address CVE-2020-28487 due to 4.20.0 version of vis.js #5430

Conversation

KiranVelumuri commented Sep 3, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

KiranVelumuri commented Sep 3, 2024

Before (with vis.min.js):

After (with vis-network.min.js):

KiranVelumuri commented Sep 5, 2024

zhangbutao commented Sep 5, 2024 • edited Loading

KiranVelumuri commented Sep 5, 2024 • edited Loading

zhangbutao left a comment

Choose a reason for hiding this comment

KiranVelumuri commented Sep 6, 2024

okumin commented Sep 8, 2024 • edited Loading

CDN

This PR

KiranVelumuri commented Sep 9, 2024

CDN

This PR

okumin commented Sep 9, 2024

okumin left a comment

Choose a reason for hiding this comment

KiranVelumuri commented Sep 10, 2024

KiranVelumuri commented Sep 13, 2024

KiranVelumuri commented Sep 17, 2024

okumin commented Sep 17, 2024 • edited Loading

KiranVelumuri commented Sep 23, 2024

zabetak commented Sep 23, 2024

KiranVelumuri commented Sep 23, 2024

okumin commented Sep 24, 2024

zabetak commented Sep 24, 2024

sonarcloud bot commented Sep 26, 2024

Quality Gate passed

okumin commented Sep 27, 2024

KiranVelumuri commented Sep 3, 2024 •

edited

Loading

zhangbutao commented Sep 5, 2024 •

edited

Loading

KiranVelumuri commented Sep 5, 2024 •

edited

Loading

okumin commented Sep 8, 2024 •

edited

Loading

okumin commented Sep 17, 2024 •

edited

Loading