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

Version 9 EOL & End of Support (even for security issues) #2877

Closed
6 tasks done
joshgoebel opened this issue Nov 18, 2020 · 66 comments
Closed
6 tasks done

Version 9 EOL & End of Support (even for security issues) #2877

joshgoebel opened this issue Nov 18, 2020 · 66 comments
Assignees
Labels
big picture Policy or high level discussion parser security Security related issues

Comments

@joshgoebel
Copy link
Member

joshgoebel commented Nov 18, 2020

If you're trying to upgrade your (or someone else's dependencies) you may want to see A note about upgrading dependencies from version 9 version 10 big picture


A recent vulnerability has brought the topic of version 9 support back into the forefront. The core team doesn't really have the time or focus to maintain two very different forks of the project. Especially since the version 9 only truly exits to service a tiny audience (IE11, <1% browser share by caniuse.com). We were kind of OK letting it live a bit longer as long as it didn't require any time/effort from us, but that's no longer proving to be the case.

  • Many, many issues and bugs have been fixed since the v10 release.
  • Many, many new features have been added.
  • Many grammars highlight WAY better than they did previously.
  • The v10 parsing engine is so much more flexible and reliable.
  • There are quite likely numerous vulnerabilities we've fixed along the way without even realizing it.

It's so obvious that v10 and modern browsers (released in the last ~4-5 years) is where we need to be focusing our time, not supporting very old browsers. So if you're still using version 9:

If you aren't supporting IE11 users (or others with very obscure browsers)

You need to upgrade to v10. No question. For 90% of simple cases it's a trivial upgrade. For complex integrations it may be a bit more effort (like most things). https://github.com/highlightjs/highlight.js/blob/master/VERSION_10_UPGRADE.md

If you're supporting IE11 users (or others with very obscure browsers)

Someone truly requiring IE11 support for "enterprisey" projects should perhaps look at Prism.js, which is a great project that still supports IE11. Or perhaps consider maintaining their own private fork of version 10 that supports IE11. This should still technically be possible now (AFAIK) but may prove more difficult as time goes on.

So here is the current plan:

  • One last v9 release (security fix).
  • That release's release notes will mention the EOL and link to this issue. (and perhaps spew some other output, need to look at what is possible with npm).
  • That release itself (and the current v9 release) will both be marked deprecated with a message to upgrade to v10.
  • We may mark other prior releases deprecated as well. I'm really curious why more npm projects don't do this. Does anyone know?
  • SECURITY.md will be updated accordingly.
  • v9 support will then be official EOLed.

So v9 users would want to upgrade to the latest v9 release and then either start working on upgrading to v10 or come up with other plans.

@joshgoebel joshgoebel added big picture Policy or high level discussion parser security Security related issues labels Nov 18, 2020
@joshgoebel joshgoebel self-assigned this Nov 18, 2020
@joshgoebel joshgoebel pinned this issue Nov 18, 2020
@TimotheDavid
Copy link

TimotheDavid commented Nov 19, 2020

hello I try to upgrade highlight with npm in node and typeorm but I don't find the folder ... and you don't explain ho to do .... could you explain ?

thanks !

@TimotheDavid
Copy link

TimotheDavid commented Nov 19, 2020

could you remove the warning when ou install highlight 10.4 : I have already the warning for 9 version .... thanks a lot !

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

There is no warning on the new version.

@Romakita
Copy link

Romakita commented Nov 19, 2020

Hello @joshgoebel
Which new version? Because the warning still occur on 9.18.4 and it's a bit aggressive. Especially if the warning comes from a transitive dependency. Can you remove it or change the warning by a simple (and small) depreciation warning please.

Capture d’écran 2020-11-19 à 12 47 29

Capture d’écran 2020-11-19 à 12 46 57

Deprecation must be instrumented with npm deprecate: https://docs.npmjs.com/cli/v6/commands/npm-deprecate

Thanks :)

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

The warning should only be on post-installation.

Because the warning still occur on 9.18.4

Yes, the entire 9 series is no longer supported. We pushed one last update to 9 just so anyone still using 9 would have a window to work on upgrading to 10. It's not intended that anyone use 9.18.14 long-term. The intention is that everyone upgrades to v10.

Deprecation must be instrumented with npm deprecate

Yes, we're deprecating them also. I'm not sure anyone pays attention to those though. I certainly haven't in the past.

@Romakita
Copy link

Romakita commented Nov 19, 2020

This is exactly the problem. My users install the cli and have an unexpected warning in full page, for a dependency that does not concern them (nor me for that matter). I can fix this problem because the helper-markdown maintainer is responsible to update his code.

I think the right solution is to use npm deprecate instead of a postinstall script!! (it's really a bad practice). By using npm deprecate, you inform npm and many other tools like snyk about the depreciation. Your postinstall script has no benefit for the final user ^^.

See you
Romain

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

First time doing something like this on NPM. :-) Yes, I understand the issue you're raising now. We need to think about what message we intend to send to users where we are not a direct dependency. Which we didn't take into account (though it seems obvious in hindsight). Are there specific guidelines published somewhere that specifically say "don't do this" with post-install?

It's definitely annoying that the "end user" here doesn't have a direct course of action, but I assume they would bug you, then you would bug upstream, etc... The intended message was supposed to be more of "you REALLY need to upgrade" vs "hey this is old now". I feel like maybe there is no great choice here (or I just don't know best practices). I feel deprecation alone is too quiet and it's possible post-install is too noisy...

I'll talk it over with the team. I assume at this point we'd have to push out yet another release just to remove the warning, right? :-|

@Romakita
Copy link

Romakita commented Nov 19, 2020

Are there specific guidelines published somewhere that specifically say "don't do this" with post-install?
Not especially, it's true. It's clearly my point of view, and we can agree on the fact that the big warning in post install is a bad use of postinstall ^^

I'll talk it over with the team. I assume at this point we'd have to push out yet another release just to remove the warning, right? :-|
I'm you can take a time to find a proper way to instrument that or just change the message so that it's less intrusive ;)
I understand your point of view though
Thanks

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

I'm you can take a time to find a proper way to instrument that

Did you have a suggestion other than deprecate?

or just change the message so that it's less intrusive ;)

What level of intrusion would you find reasonable? (just curious) :-)

@ZQun
Copy link

ZQun commented Nov 19, 2020

Hello, the warning message when prompted for this version upgrade has some impact on the visual effect. It will prompt the version upgrade every time you save the file. Because we rely on other libraries, we don’t have the permission to update, but the upgrade prompt message will be updated every time during our development process. It will prompt you every time you save the file, which is very unfriendly!

@ZQun
Copy link

ZQun commented Nov 19, 2020

@joshgoebel
Hello,
The code here will cause us to prompt update information every time we save a file during program development, bad experience!

warn("Version 9 of Highlight.js has reached EOL and is no longer supported. Please upgrade to version 10.");

@Romakita
Copy link

Romakita commented Nov 19, 2020

What level of intrusion would you find reasonable? (just curious) :-)

Only one or two lines with the link is enough, not a big banner. This is the responsibility of each maintainer to upgrade his dependencies. Your point is just to said to theses maintainers to upgrade his dependencies. If the maintainer doesn't have a time or doesn't want upgrade it, isn't not your responsibility ;). Your work is accomplished as long as you have indicated that version 9 is deprecated and that you will no longer support it for many very justified reasons (and posted on your github project).

My point is, because you've added this banner, it has a direct impact on my CLI installation (and user experience), when any developer install/use it on his system. It's not something that I wanted and I don't want to have feedback on a subject that is not my scope either ;)

Did you have a suggestion other than deprecate?

npm deprecated is the standard for your problem (or I forgot something). So I haven't other solution.

@ManuGM91
Copy link

ManuGM91 commented Nov 19, 2020

I would also like to echo what @Romakita has said. I think your job would be done, when you put out a warning saying this isn't supported any more and you have to go for new version. It is completely up to the maintainers to upgrade or not, their dependencies according to their requirements. Any issues they would face in future due to not upgrading to the latest version should be completely owned by them.

If you see the two bugs(one raised by me) where this issue has been mentioned, I believe both of them have no direct connection with what is currently there, but the impact is a bit big.

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

@ZQun Probably helpful to discuss the warn separately from the post-install notices.

It will prompt the version upgrade every time you save the file.

That's a bit unusual... why are you running npm install every time a file is saved?

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

I think your job would be done, when you put out a warning saying this isn't supported any more and you have to go for new version.

That's what I thought I was doing. I don't consider deprecate a "warning". The warning is visual (for sure) but if it actually BREAKS someones build I don't understand why... taht's what I'd be very curious to understand before we decide what to do here.

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

@MGM19 I saw your single issue, is there two?

@ZQun
Copy link

ZQun commented Nov 19, 2020

@ZQun Probably helpful to discuss the warn separately from the post-install notices.

It will prompt the version upgrade every time you save the file.

That's a bit unusual... why are you running npm install every time a file is saved?

@joshgoebel
Hello,
You see, after I run the program in npm run dev, I will prompt every time I save a file

image

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

Ah, I understand what you mean now. Yeah, it is a bit more annoying if you're restarting your whole application every time a single file is saved.

@ZQun
Copy link

ZQun commented Nov 19, 2020

Ah, I understand what you mean now. Yeah, it is a bit more annoying if you're restarting your whole application every time a single file is saved.

@joshgoebel
Hope you can handle it, thank you!

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

@ZQun Well, it's not causing any harm other than being a little noisy... in development you could simply comment out the warning line in highlight.js and then it would shut up...

I'm still trying to figure out how we want to approach this big-picture.

@ManuGM91
Copy link

ManuGM91 commented Nov 19, 2020

@joshgoebel
Sorry, if I put that out wrong, but I was meaning that total of two bugs - one from me and the other from @ZQun .

For me it's not the visual impact to be honest. I am using https://github.com/DannyDainton/newman-reporter-htmlextra to generate html reports for our API testing using Postman and Newman. The reporter have a dependency with highlight. So now I have a step in my Pipeline in which I use powershell script to run my collection and generate a report using the reporter. The step is now failing with an ##[error] badge. Since this step decides whether my pipeline should pass or fail, I can't suppress this error.
image

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 19, 2020

@MGM19 Yes, I see your issue now. Just to be clear though we are NOT raising (or logging) any kind of error, only a warning. Your build system is choosing to interpret the warning as an error.

@ManuGM91
Copy link

ManuGM91 commented Nov 19, 2020

in development you could simply comment out the warning line in highlight.js and then it would shut up...
My situation is that I can't even take this approach. All our pipelines are failing due to this and I am unable to explain a valid reason why it's happening to many, since highlight doesn't have any direct dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big picture Policy or high level discussion parser security Security related issues
Projects
None yet
Development

No branches or pull requests

6 participants