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

joshgoebel commented 3 years ago

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:

[x] One last v9 release (security fix).
[x] 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).
[x] That release itself (and the current v9 release) will both be marked deprecated with a message to upgrade to v10.
[x] We may mark other prior releases deprecated as well. I'm really curious why more npm projects don't do this. Does anyone know?
[x] SECURITY.md will be updated accordingly.
[x] 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.

TimotheDavid commented 3 years ago

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 commented 3 years ago

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

joshgoebel commented 3 years ago

There is no warning on the new version.

Romakita commented 3 years ago

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.

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

Thanks :)

joshgoebel commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@joshgoebel Hello, The code here will cause us to prompt update information every time we save a file during program development, bad experience! https://github.com/highlightjs/highlight.js/blob/c34318b6a720a0852d27cd13dc55ca896e1292ec/src/highlight.js#L33

Romakita commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

ZQun commented 3 years ago

@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

joshgoebel commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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.

joshgoebel commented 3 years ago

@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 commented 3 years ago

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.

joshgoebel commented 3 years ago

@MGM19 You can always re-pin 9.18.3 until this is resolved to your satisfaction. The library (both v9 and v10) logs warnings for other reasons though as well, so I'd consider looking into why your build system is treating a warning as a hard error.

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.

What criteria is Powershell using to decide an "error" has occured? Just any output at all? I confirmed locally that using console.warn does not alter the error status returned when executing node.

ManuGM91 commented 3 years ago

To be Honest, I am not sure if I can answer the criteria question. I haven't specified any criteria. The Pipeline is being configure using YAML and the specific step looks something like this.

pwsh: |
newman run "${{ parameters.collectionName }}" -e "${{ parameters.environmentFileName }}" --reporters "cli,htmlextra" --reporter-htmlextra-template ${{ parameters.htmlextraVersionFileName }} displayName: 'run newman tests' enabled: true condition: succeeded() failOnStderr: true workingDirectory: '${{ parameters.workingDirectory }}'

ManuGM91 commented 3 years ago

I got your point that, your's is more than a warning rather than an error. But, I am not sure why powershell is considering it as a error.

joshgoebel commented 3 years ago

https://nodejs.org/api/console.html#console_console_warn_data_args

The console.warn() function is an alias for console.error().

Gah, cause Node is so dumb. :-) Even another very smart associate I asked just now got this wrong also. Warnings are not errors... but if you ask Node evidently they are (and they log to stderr). So we'll probably have to do something about it now I think (and probably change our library in other places also long-term [at least when running in Node]).

But if you're broke, broke atm, please revert to 9.18.3 until we get this sorted.

Romakita commented 3 years ago

@joshgoebel I given a demo today with the CLI. You can see, the log appear each time:

Is clearly not normal ^^.

ManuGM91 commented 3 years ago

The problem @joshgoebel currently is that, I believe I have very little/no control over upgrading/reverting highlight.js and it's more with the maintainers of the reporter. I may be wrong, but as I am not an expert with node or npm, I am not sure how to head forward? Any help would be really appreciated. With my limited knowledge, I believe I have to wait until this is resolved.

ManuGM91 commented 3 years ago

By the way, Thank you and happy that you found the root cause of why this happening. Really helpful is that bit of info.

joshgoebel commented 3 years ago

@MGM19 In your own project can you not pin the version yourself (making it a direct dependency just to pin it):

npm install highlight.js@9.18.3

Really helpful is that bit of info.

Would have been really helpful yesterday. :)

Romakita commented 3 years ago

@joshgoebel This is a small fix, but suddenly, we find ourselves adding a dependency that is not ours directly :)

joshgoebel commented 3 years ago

@Romakita I didn't say it was a permanent solution, but MGM19 was asking how he could take some control.

Romakita commented 3 years ago

yes :) no worries.

ManuGM91 commented 3 years ago

I have tried that and not sure why, that's not solving the issue. Still I can see the error. When I checked package.json inside the highlights.js folder inside the reporter's node_modules folder I am seeing this.

Romakita commented 3 years ago

have you tried npm dedup after using the npm install ?

joshgoebel commented 3 years ago

You can't change the reporters package version, but I thought if you installed it yourself that the reporter might use that version when invoked from your package - I'm not sure how such things work with NPM honestly.

ManuGM91 commented 3 years ago

have you tried npm dedup after using the npm install ?

Tried this now, but didn't help unfortunately.

ManuGM91 commented 3 years ago

You can't change the reporters package version, but I thought if you installed it yourself that the reporter might use that version when invoked from your package - I'm not sure how such things work with NPM honestly.

I too had a similar thought, but it didn't seems to be helping much. TBH, I am also not so aware how the upgardes work within NPM.

joshgoebel commented 3 years ago

I plan to push a 9.18.5 release in a bit to resolve the warn vs log issue (and give people a way to turn off the warning). Of course if your dependency has upgraded to 9.18.4 then they would also need them to upgrade to 9.18.5 to resolve your issue with warn logging to STDERR.

Romakita commented 3 years ago

I plan to push a 9.18.5 release in a bit to resolve the warn vs log issue (and give people a way to turn off the warning). Of course if your dependency has upgraded to 9.18.4 then they would also need them to upgrade to 9.18.5 to resolve your issue with warn logging to STDERR.

Not necessarily. Example here: https://github.com/helpers/helper-markdown/blob/master/package.json#L29 The maintainers allow all versions after "^9.12.0", it doesn't need to update the dependency, but it explain why your banner have a side effect on any projects which use hightlight.js today.

ManuGM91 commented 3 years ago

Apologies @Romakita if this is a stupid question. So you meant to say when @joshgoebel pushes 9.18.5 it should, by default fix the issue and there is no upgrade we have to do even if it's one of our dependency?

Romakita commented 3 years ago

@MGM19 It should but regarding the PR, It won't solve really the problem. @joshgoebel Has added options and env variable to shutdown the warning (at runtime) by the code. It's a good intention, but if in our case, the code is probably executed by a transitive dependencies, which it doesn't really know the existence of this issue/options.

And the questions as maintainers: why I should add an option or an environment variable in my code for a log problem which would come from a transitive dependency. If we allow that then, anyone can add an environment variable to disable unwanted logs.

Romakita commented 3 years ago

After explanation, the 9.18.5 should fix the problem if the transitive dependency don't call the method which contain the log. (but isn't sure).

@joshgoebel I'll give you feed when your release will be available. Can you consider, using this highlight method in Node.js context, the security issue is valid?

joshgoebel commented 3 years ago

The maintainers allow all versions after "^9.12.0",

@MGM19 If this is the case then you should indeed be able to fix it yourself now that 9.18.5 is released. The warn has been removed so your build system shouldn't see any error anymore. You still may need npm install highlight.js or similar to bump the dependency.

Can you consider, using this highlight method in Node.js context, the security issue is valid?

If Highlight.js is being used to highlight user provided content there are potential issues (whether in the browser or on the server). And again these are only the known unknowns. :-) Version 9 is really VERY VERY old an out-of-date.

from a transitive dependency.

It doesn't matter how deep a dependency it is, if you're using it for it's said purpose (even indirectly) then you should be aware of the deprecation and the security concerns of not upgrading. And if you aren't truly using it, then you shouldn't see the warning. We'll see how 9.18.5 goes. :)

If we allow that then, anyone can add an environment variable to disable unwanted logs.

I don't know what you mean here.

joshgoebel commented 3 years ago

@MGM19 Sorry about breaking your build. :) Lesson learned about warn. And I've created an issue to track it in version 10 and see what changes may need to be made:

https://github.com/highlightjs/highlight.js/issues/2880

Romakita commented 3 years ago

To be clearer if each npm module introduces an environment variable to disable logs that I did not have when developing my code, I will end up with a number of unwanted warnings and independent of my wish. I would therefore be forced to integrate these variables to fix the problem while everyone does the migration work. which may take time.

I don't want to deal with this complixity at the level of my code. Because it’ll be unmaintainable :)

we agree the best solution is to update but must it be possible.

joshgoebel commented 3 years ago

Again just to be clear: It's not just some random log output, it's a SERIOUS warning about version 9 deprecation and potential security concerns that people using the library (directly or indirectly) should know about. It now only shows up when the library is actively used. We provide 3 different paths to disable the warning. When it comes time to deprecate v10 we'll keep this bigger picture in mind and see if we can come up with something better.

We could have done better with 9.18.4 (hindsight is 20-20), but I'm pretty happy with 9.18.5. We'll see. :-)

Romakita commented 3 years ago

Yes sure ;) thanks again

joshgoebel commented 3 years ago

https://github.com/highlightjs/highlight.js/issues/2882

highlightjs / highlight.js

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