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

Upgrade to jQuery 1.12.4, addressing security warning from GitHub #1358

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thomthom
Copy link
Contributor

@thomthom thomthom commented Oct 21, 2020

Description

We host our generated docs on GitHub via GitHub pages. Via GitHub's security warnings we started seeing this recent:

image

We can patch this on our end, but we figured it was better to try to patch upstream.

However, jQuery 1.9 removed a number of functions which caused rendering issues:

image

This PR upgrades jQuery to the latest 1.x version; 1.12.4 along with the jQuery Migration plugin. The migration plugin ensures that YARD renders correctly again.

This does add another HTTP request to be made. If that's a concern we could merge jQuery and the Migration plugin into a single file.

Related discussion:

#1298 (comment)

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@thomthom
Copy link
Contributor Author

Ah, I checked the issues list first, but I didn't check the open PRs. I see now that there is #1351 that also addresses this.

@thomthom thomthom closed this Oct 21, 2020
@coveralls
Copy link

coveralls commented Oct 21, 2020

Coverage Status

Coverage decreased (-0.02%) to 93.416% when pulling 2e093bf on thomthom:dev/jquery-upgrade into 2b16190 on lsegal:main.

@lsegal
Copy link
Owner

lsegal commented Oct 25, 2020

@thomthom I actually like this as a solution that uses jquery-migrate to handle the deprecated APIs. Did you verify that all pages load correctly with this version?

@thomthom
Copy link
Contributor Author

I didn't check the guide template. That is a template I haven't noticed before.

But for the default template it seemed to work well. I tested the front page (README) along with class/module pages, additional pages, search.

I've put the fix into our custom YARD template (https://github.com/SketchUp/sketchup-yard-template) and we'll soon be publishing with these fixes applied locally to our doc builds.

If you want, I can zip up a copy of the YARD doc output from that branch if you (or anyone else) wish to have a look for review.

Btw, does YARD itself use the guide template? Or any project you are aware of that uses it that can be easily tested?

@thomthom
Copy link
Contributor Author

thomthom commented Nov 2, 2020

Here are builds of the YARD docs and the YARD guide using a build from this PR branch.

@thomthom thomthom reopened this Nov 2, 2020
@thomthom
Copy link
Contributor Author

thomthom commented Nov 2, 2020

Btw, I saw warning from YARD when building the guide, not sure if that's related to how in invoked the build?

C:\Users\Thomas\SourceTree\yard>ruby bin\yard --yardopts .yardopts_guide 
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.tag
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.directive
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.directive
[warn]: Unknown tag @yard.directive
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.directive
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.directive
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.directive
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.directive
[warn]: Unknown tag @yard.signature
[warn]: Unknown tag @yard.directive
Files:         184
Modules:        49 (    7 undocumented)
Classes:       218 (   49 undocumented)
Constants:     101 (   41 undocumented)
Attributes:    189 (    0 undocumented)
Methods:       971 (  165 undocumented)
 82.85% documented

@noraj
Copy link

noraj commented Jul 23, 2021

Updating to 1.12.4 is nearly useless since 4 vulnerabilities will remain out of the 6 vulnerabilities present in 1.7.1.

I know it as already be downgraded (3.4.1 back to 1.7.1) because breaking changes were not taken into account https://github.com/lsegal/yard/pull/1298/files.

Dependabot security alerts are sent to many project using yard:

image

The only versions with zero vulnerabilities now are 3.5.x and 3.6.x.

@trivikr
Copy link

trivikr commented Aug 31, 2021

Thank you @thomthom for posting this PR.
I used it as a reference as posted new PR for bumping jQuery to latest v3.6.0 without having to add jquery-migrate in #1397

Can you provide your feedback there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants