Getting rid of `atty` as a transitive dependency (via `colored`)

[faern]

atty has a soundness issue (RUSTSEC-2021-0145), seems to be 100% unmaintained and people are moving away from it in general (clap-rs/clap#4249, rust-cli/env_logger#248, ...). atty is part of the dependency tree for fern via its direct dependency on colored. However, colored also does not seem to be too quick in wanting to fix the atty issues (colored-rs/colored#122).

So I'm posting this issue here also, in order to track the possibility of depending on fern without pulling in a soundness issue. For one it can act as some pressure on colored to finally merge and release that fix. Or fern can consider changing library for colors.

[daboross]

Thanks for noting this!

I think my preferred way forward here would be to add an alternative color library under a different feature flag, and then drop colored in the next major revision. I believe there's been some discussion of other color libraries in fern's other issues, but I'll have to dig it up.

Given the security issue, maybe it'd be best to release a major revision for this specifically... In any case, I'd still prefer to have a minor version upgrade path.

[faern]

Thanks for the quick reply and interest in solving this!

What's the rationale behind fixing soundness issue = major release? That if anything just makes it harder for people to upgrade. Better if it's a patch release so as many as possible get it without updating their Cargo.toml files? If it can be done without an API/behavior change of course. If the API changes it's by definition a breaking change.

[daboross]

What's the rationale behind fixing soundness issue = major release? That if anything just makes it harder for people to upgrade.

fern re-exports types directly from colored when the colored feature flag is enabled, so removing it as a dependency is a breaking change.

We can do a minor release adding an alternative, but since we currently directly re-export types from colored, switching away will necessarily require downstream users to change their Cargo.toml and color-using code.

[daboross]

Looks like owo-colors (mentioned in #67) is probably still the best choice here - only question I have is how well it works on Windows, which can be tested.

[daboross]

I think owo-colors will work, as long as we can combine it with enable-ansi-support for colors on Windows 10+ terminals, and is-terminal or similar to allow toggling colors.

It'll require a redesign of fern's color module, though, so I've not yet written it. Some of what we do now will become unnecessary, and other parts just won't work well with owo-colors color types. We could just drop the module entirely and let users use owo-colors themselves, but it would be nice to provide a one-stop solution for log coloring. Or, at the very least, a good example of how to do the "right thing".

In the meantime, I've released an update of fern with a warning for this security issue in the README.md and lib.rs documentation, and a temporary workaround. It should show up in https://crates.io/crates/fern and https://docs.rs/fern/

[mackwic]

Hello everyone, colored will release a new version soon.
We are investigating how to remove the dependency to atty and its eventual impact downstream. I believe fixing that will help you, right?

Fern has a MSRV of rust 1.35, which makes you our dependant with the largest rust compatibility so I think you should a say in this.

We think we have 3 options:

1. Drop atty, use std::io::isTerminal, and bump the MSRV to rust 1.70 (June 2023)
2. Drop atty and provide a dumb default with rustversion, keeping the MSRV around rust 1.31 (December 2018)
3. Drop atty and use is-terminal which MSRV is rust 1.62 (August 2022)

What would you prefer ?

Side-question: would you think that we should have a major release to reflect this change ? It's not clear to me if this should warrant a major bump or not.

[faern]

So happy to hear from a colored developer and that the project is not dead! ❤️

Will the public API of colored change? If not, it's not a breaking change. It's been debated to no end whether or not MSRV changes are breaking or not, but my observation is that the community moves towards treating it as not being breaking. Not because it's perfect, but because treating it as breaking is way worse.

I am not a fern developer. I'm just a happy library consumer who wants to get rid of atty. IMHO option 1 is the nicest. It's finally in stable, fewer dependencies etc.

It does not look like fern is strict about keeping its MSRV at all:

Fern isn't making a policy to support any one MSRV. However, it's useful to only break it intentionally.

[hwittenborn]

Hey, new contributor of colored here! I've been hopping on to help maintain some stuff, and I've been in charge of some things like new releases and getting this MSRV stuff figured out.

Will the public API of colored change?

It won't, this will just affect internals of how colored detect TTY usage.

It's been debated to no end whether or not MSRV changes are breaking or not, but my observation is that the community moves towards treating it as not being breaking. Not because it's perfect, but because treating it as breaking is way worse.

That's pretty much been my views on everything too. You never know when a dependency increases their MSRV and how that might affect how your own crate changes their's, so it's been my opinion to just not make it breaking.

IMHO option 1 is the nicest.

I agree that option 1 is the best too, and if combined with colored-rs/colored#133 (comment) that'd make every dependency (except one when targeting Windows) disappear. Just because 1.70 is so recent though I was thinking it should probably hold off until 1.70 gets a bit older. The main option I was looking at then was number 3, as then terminal detection can still be done without any problem.

It does not look like fern is strict about keeping its MSRV at all

It definitely looks like @daboross isn't wanting to be too strict, but I'll probably hold off until he can get a response in here. It doesn't look like his GitHub is too active though, so I'll probably wait a week or so and then get a decision going if he hasn't responded.

[daboross]

@mackwic, @hwittenborn Thanks for letting me chime in here!

@faern is correct - I'm mostly concerned with unnecessary version bumps in fern's internals. I'd definitely qualify this as necessary, or at least highly useful, and I wouldn't want to constrain dependencies of this library in any case, just the code itself. Maybe I can add some documentation in the CI file to specify that explicitly.

In any case, all three approaches sound reasonable to me! I'd leave it up to you which is best.

If you go for just upping the MSRV of colored to 1.62 or 1.70, I'd agree with still just doing a minor release. That may break builds, but that's why Cargo.lock exists, and it'll always be a direct compile-time failure of colored itself, not silently changing behavior nor breaking any downstream usage of the library. Also probably worth noting if you set package.rust-version, you even get nice error messages for this.

[hwittenborn]

That sounds great @daboross! There's plans for a v3 of colored in the near future, so what'll probably happen is is-terminal will be used on v2 so the MSRV isn't super new, and then std::io::IsTerminal will be used on v3, and by the time a release is ready for v3 the MSRV should hopefully have a few versions in front of it.

I'll let @kurtlawrence chime in too because he's helping maintain stuff too - do you think that'd a good approach, or would you prefer something else?