search

highlightjs / highlightjs-octave

A Highlight.js grammar for GNU Octave
BSD 3-Clause "New" or "Revised" License
1 stars 1 forks source link

Please resolve security alerts and bump Highlight.js dependency #1

Open joshgoebel opened 2 years ago

joshgoebel commented 2 years ago

@apjanke

If you no longer have time to maintain this we'll need mark it archived and I'll add a notice to the README that it's currently unmaintained and in need of a maintainer.

Please let me know.

Thanks,

apjanke commented 2 years ago

Hi!

Sorry, I must have missed a GitHub notification on this one. I'll resolve this over the weekend, and if I can't, let you know that it needs to be archived.

apjanke commented 2 years ago

Sorry if I'm being a pain here, but I'm kind of a newbie Javascript developer.

I re-read through the Highlightjs contributor doco at:

and still have a couple questions.

Which version of Highlight.js do you want me to bump to? Is this a request to upgrade to major version 11 (per https://highlightjs.readthedocs.io/en/latest/upgrade-11.html), or a newer minor version of the 10.x series?

And which security alerts are we talking about here? Is there an easy way for me to view a list of them? I don't see anything under the Security > Security Advisories tab in the GitHub page for this repo. Is this something that pops up during an installation process, or are you using a different auditing tool or something?

apjanke commented 2 years ago

Ah: I see in https://github.com/highlightjs/highlight.js/security/policy that Highlight.js 10.x is "no longer supported" now, and only 11.x is maintained. I will bump to 11.x.

image
joshgoebel commented 2 years ago

https://github.com/highlightjs/highlightjs-octave/security/dependabot

Is this something that pops up during an installation process, or are you using a different auditing tool or something?

But npm install probably yells pretty loudly also...npm audit is your friend.

apjanke commented 2 years ago

Cool. I've started using npm audit and I see the security warnings. Will clean those up.

Hmm. https://github.com/highlightjs/highlightjs-octave/security/dependabot is 404 for me. When I try to enable Dependabot on the highlightjs-octave repo under Insights > Dependency graph Dependabot, I see a "Dependabot is not enabled" message.

image

This page says I need to enable Dependabot for the repo in the Settings > Security sidebar, but when I go to Settings, there's no Security sidebar.

image

Maybe that's because I'm only a Contributor on this repo, since it's under the highlightjs GitHub Organization, and I need you or another highlightjs org admin to enable Dependabot here? Or maybe I'm misunderstanding the configuration instructions?

apjanke commented 2 years ago

I've bumped the Highlight.js dependendency to 11.4.0, and various other dependencies to their latest versions, which I believe resolves the security issues. (In https://github.com/highlightjs/highlightjs-octave/commit/8448bbcf697b41789c543902dd481fe243a00751 and https://github.com/highlightjs/highlightjs-octave/commit/e466a4b925185a3bc21400370be50c8ef30595f3)

npm audit comes back clean now.

[highlightjs-octave] $ npm audit
found 0 vulnerabilities
[highlightjs-octave] $

Look good to you?

I'm adding a couple other minor improvements to the doco and repo organization. Full work is on the highlightjs-11.x-and-security-fixes branch. When that's done, if it looks right to you, I'll make a new v0.2.0 release.

apjanke commented 2 years ago

I think I've got it ready for Highlight.js 11.x. But when I'm doing testing (by running npm test in my local highlight.js repo with extra/octave symlinked to my highlightjs-octave repo), I'm getting a test failure:

[highlight.js] $ npm test
[...]

  1718 passing (7s)
  3 pending
  1 failing

  1) hljs.highlightAuto()
       should be detected as matlab:

      AssertionError: default.txt should be detected as matlab, but was octave
      + expected - actual

      -octave
      +matlab

      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /Users/janke/local/repos/highlight.js/test/detect/index.js:28:33
      at async Promise.all (index 0)
      at async Context.<anonymous> (test/detect/index.js:21:5)

[highlight.js] $

I think this is because my octave language definition is causing Highlight.js's autodetection to mis-detect existing Matlab examples as Octave, maybe due to my test/detect/octave/default.txt language file. My default.txt has enough distinctive Octave-specific features that I think it might correctly cause actually-Octave code to be correctly detected as Octave instead of Matlab. But I'm guessing it's similar enough to regular Matlab syntax that it's causing a false-positive on the existing Matlab example code that's used for Matlab detection.

I tried to fix this by disabling autodetection for Octave files in https://github.com/highlightjs/highlightjs-octave/commit/35f7c5daf5fba43ecfdb869df26a373380aa0889, but that seemed to have no effect.

Do you know what I should do here?

joshgoebel commented 2 years ago

I tried to fix this by disabling autodetection for Octave files in https://github.com/highlightjs/highlightjs-octave/commit/35f7c5daf5fba43ecfdb869df26a373380aa0889, but that seemed to have no effect.

Did you rebuild first? That's exactly how you fix it. You can also just leave it on and ignore the error (yeah, it's annoying)... or delve into the weeds and learn how to fix it (but it's hard and a tight wire)... I've wondered in the past if we should just disable auto-detect on all 3rd party grammars by default and then force people to turn it back on.

joshgoebel commented 2 years ago

If you don't want to support auto-detect you should also remove the auto-detect test default file... it's possible that it's still keying off of that files existence... I'd also be open to a IGNORE_3RD_PARTY_AUTODETECT_ISSUES env option for tests if someone wanted to make a small PR for that... to allow 3rd party maintainers to say "I feel I'm close enough"... Auto-detect is very hard to balance against 200+ grammars

joshgoebel commented 2 years ago

Full work is on the highlightjs-11.x-and-security-fixes branch.

I'd be much easier to review if you opened a PR.

apjanke commented 2 years ago

Did you rebuild first? That's exactly how you fix it.

Aha! I had to rebuild Highlight.js itself, not just my highlightjs-octave package. That got the tests passing. I'll push that in and then open a PR to make it easier for you to review.

I think in this case, disabling auto-detection for octave is probably the right thing to do. It's too close to Matlab syntax; seems likely to just cause false-positives without some sort of finer-grained configuration or autodetection hints.

joshgoebel commented 2 years ago

without some sort of finer-grained configuration or autodetection hints.

That's your job, to accept or decline. ;-) But as I say all the time auto-detect is not magic (at all)... for best results people should not use it. If it wasn't one of our USPs (unique selling proposition) and part of what we are historically I'd probably consider ripping it out. It's often a lot of trouble.

Also worth noting: In many cases people don't use conflicting grammars side by side though... you have to remember matlab isn't included in our common bundle, so people have to "opt in" to it... so in very many cases you likely wouldn't be "competing" with it. But for when running the FULL test suites we expect 0 conflicts across ALL grammars - which is hard.

joshgoebel commented 2 years ago

Aha! I had to rebuild Highlight.js itself,

The build process builds a monolith npm build and then runs against that. So the tests don't "require" your library, it's compiled into Highlight.js at that point.

apjanke commented 2 years ago

That's your job, to accept or decline. ;-)

Gotcha. I probably need to reread some of the documentation here. Going to defer that until after fixing this stuff up, if I get to it at all; leaving autodetect disabled for now.

The build process builds a monolith npm build and then runs against that.

Oh! That explains it.

apjanke commented 2 years ago

Here's a Pull Request with these changes, to make it easier to review: https://github.com/highlightjs/highlightjs-octave/pull/3

It's currently a "Draft" PR because I have the package version as a "-snapshot" while still testing it using local NPM installs. Once it tests out okay, and you're happy with the state of things, I'll remove the "-snapshot" and make it a normal non-draft PR.

This PR excludes the changes I did for #2; I put those on a separate https://github.com/apjanke/highlightjs-octave/tree/fix-usage-instructions branch to make them easier to review separately, and because those changes aren't fully baked yet.