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

Fix vulnerability in jQuery used in ScalaDoc #8179

Merged
merged 8 commits into from Jul 11, 2019

Conversation

exoego
Copy link

@exoego exoego commented Jun 27, 2019

Closes scala/bug#11567

This PR updates jQuery bundled in scaladoc mainly to fix potential vulnerabilities of XSS.

Changes

  • Upgrades jQuery from 1.8.2 (released in 2012-9-20) to 3.4.1 (latest release as of 2019-6-27) using webjars, so that no need to include jQuery file in this repository.
  • SRI (Subresrouce Integrity) checks are performed before extracting files from webjars. SRI for each files can be found in CDN providers (see the following subsection)
  • Addresses all (as far as I investigated) jQuery migration warnings those produced by official jQuery migrate plugin.
  • Replaces the outdated, source-not-found tools.tooltip jQuery plugin with browser's native tooltip (title attribute). Because this plugin uses the deprecated-and-removed jQuery.browser.msie and there are no direct alternative for that. Native tooltip is fine I think.

Internet Explorer 8 and below will no longer be supported, since jQuery 3.x dropped them.
I think dropping IE8 is reasonable, since

  1. Microsoft had already terminated IE8 support
  2. The sum of IE6-9 global usage are negligible (less than 0.5%).
  3. jQuery 3 is supposed to be faster and more stable, due to its usage of modern web browser features.

Where to find SRI

https://www.jsdelivr.com/package/npm/jquery
image

http://code.jquery.com/
image

Note

I will open PRs of same fixes to 2.12.x branche, once this PR is merged.
Scala 2.11.x seems not actively mainated, so I guess that no fix on 2.11.x branche is fine.
But I will open a PR if it is desired.

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Jun 27, 2019
@exoego exoego changed the title Fix vulnerability in jQuery used in ScalaDoc [scaladoc] Fix vulnerability in jQuery used in ScalaDoc Jun 27, 2019
@xuwei-k xuwei-k added the tool:scaladoc Changes to ScalaDoc, the API documentation generator label Jun 27, 2019
@exoego exoego changed the title [scaladoc] Fix vulnerability in jQuery used in ScalaDoc Fix vulnerability in jQuery used in ScalaDoc Jun 27, 2019
@xuwei-k
Copy link
Contributor

xuwei-k commented Jun 27, 2019

[error] 1 of 16 test tasks failed:
[error] - doc
[error]   - reflect/compile:doc failed: java.lang.AssertionError: assertion failed: /META-INF/resources/webjars/jquery/3.4.1/jquery.min.js
[error]   - compiler/compile:doc failed: java.lang.AssertionError: assertion failed: /META-INF/resources/webjars/jquery/3.4.1/jquery.min.js
[error]   - scalap/compile:doc failed: java.lang.AssertionError: assertion failed: /META-INF/resources/webjars/jquery/3.4.1/jquery.min.js
[error] java.lang.RuntimeException
[error] 	at $bb2f36b2eaf2c61a8957$.$anonfun$root$26(build.sbt:957)
[error] 	at $bb2f36b2eaf2c61a8957$.$anonfun$root$26$adapted(build.sbt:896)
[error] 	at scala.Function1.$anonfun$compose$1(Function1.scala:44)
[error] 	at sbt.internal.util.$tilde$greater.$anonfun$$u2219$1(TypeFunctions.scala:40)
[error] 	at sbt.std.Transform$$anon$4.work(System.scala:67)
[error] 	at sbt.Execute.$anonfun$submit$2(Execute.scala:269)
[error] 	at sbt.internal.util.ErrorHandling$.wideConvert(ErrorHandling.scala:16)
[error] 	at sbt.Execute.work(Execute.scala:278)
[error] 	at sbt.Execute.$anonfun$submit$1(Execute.scala:269)
[error] 	at sbt.ConcurrentRestrictions$$anon$4.$anonfun$submitValid$1(ConcurrentRestrictions.scala:178)
[error] 	at sbt.CompletionService$$anon$2.call(CompletionService.scala:37)
[error] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[error] 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[error] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[error] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[error] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[error] 	at java.lang.Thread.run(Thread.java:748)
[error] (testAll) java.lang.RuntimeException

https://travis-ci.org/scala/scala/jobs/551256076 🤔

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

I'm not an expert, but this looks awesome!

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
@sjrd
Copy link
Member

sjrd commented Jul 1, 2019

TBH, I'd like to question the value of all that machinery to pull jQuery from a webjar, check its integrity, then insert it into our own jar, versus the old way of simply having a copy of the file in this repo. It seems to me that the old system was simpler. What is the real reason for this change? What value does it bring?

@plokhotnyuk
Copy link
Contributor

@sjrd I believe that the value is that in some not so far future @scala-steward will be able to create a PR with the "Update jQuery to x.x.x" comment.

@NthPortal
Copy link
Contributor

or perhaps as an alternative, next time we want to update jQuery the diff will only be a couple lines.

@sjrd
Copy link
Member

sjrd commented Jul 1, 2019

@plokhotnyuk In this specific case we wouldn't be able to trust a scala-steward PR, because we still need to check out the branch, run Scaladoc locally and then test the generated doc manually by navigating a bit through it. That's because we're talking about dynamically typed code, for which I'm pretty sure we don't have nearly enough automated tests.

@dwijnand
Copy link
Member

dwijnand commented Jul 1, 2019

True, the PR will need manual validation, but at least it'll be a 1-line PR (instead of the diff of jQuery code) and we won't be using a version of jQuery that's 7 years old.

@exoego
Copy link
Author

exoego commented Jul 1, 2019

There are 3 external JavaScripts for scaladoc -diagram that prevent offline view.

((if (!universe.settings.docDiagrams.value) Nil
else (List(
extScript("https://d3js.org/d3.v4.js"),
extScript("https://cdn.jsdelivr.net/npm/graphlib-dot@0.6.2/dist/graphlib-dot.min.js"),
extScript("https://cdnjs.cloudflare.com/ajax/libs/dagre-d3/0.6.1/dagre-d3.min.js")))) :+

I think machinary extraction from webjars can be applied to those 3, and achieve fully offline view, instead of bundling those 3 large files into repository.

Updating dependencies can be 1-line and be performed by bots (Scala Steward or something).
But it may require manual treatments since SRI hash in build.sbt needs updated too.

@NthPortal
Copy link
Contributor

I don't see why a bot couldn't get the SRI hash from your local JS CDN

@exoego
Copy link
Author

exoego commented Jul 1, 2019

It could be automated if bots support rewriting SRI hash

build.sbt Outdated Show resolved Hide resolved
@exoego exoego force-pushed the update-jquery branch 2 times, most recently from 6ac3ef7 to 7beb436 Compare July 10, 2019 00:20
@exoego
Copy link
Author

exoego commented Jul 11, 2019

Now CI is all green

@eed3si9n eed3si9n merged commit 80fd88d into scala:2.13.x Jul 11, 2019
@NthPortal
Copy link
Contributor

Thank you @exoego!!

@exoego exoego deleted the update-jquery branch July 11, 2019 20:15
eed3si9n pushed a commit to eed3si9n/scala that referenced this pull request Jul 26, 2019
This is a backport of scala#8179

- Use browser-native tooltip instead of outdated plugin.
- Replace input.attr("value") with input.val().
- Replace event handler shorthands with on/off.
- Do not use offset on invalid jQuery object.
  It will throw instead of {top:0,left:0} from jQuery 3.
- Add SRI check to verify the resource is correct
@szeiger szeiger added the release-notes worth highlighting in next release notes label Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes tool:scaladoc Changes to ScalaDoc, the API documentation generator
Projects
None yet
10 participants