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 (https://github.com/clap-rs/clap/pull/4249, https://github.com/rust-cli/env_logger/pull/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 (https://github.com/mackwic/colored/pull/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 https://github.com/daboross/fern/issues/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! :heart:

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 https://github.com/colored-rs/colored/pull/133#issuecomment-1616767490 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?

[hwittenborn]

Once we get that resolved a release should be good to come out real soon as well, I'm thinking of getting that done tomorrow. This vulnerability stuff is definitely something I want to get rid of ASAP, so as soon as it's good to it's definitely going to happen.

[kurtlawrence]

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.

Yep, that sounds good to me.

[hwittenborn]

Alright this should be good to go in the latest release of colored @daboross! Let me know if there's any issues you face :)

[hwittenborn]

I looked at is-terminal and it looks like their MSRV is 1.63 instead of 1.62 though, so that's what's going to be in place for v2 of colored. I'm assuming that's fine with you since you were going to be fine with 1.70 @daboross, but let me know if any changes need to be made.

[faern]

colored 2.0.2 swapping out atty for is-terminal is finally released! :partying_face: Now all fern has to do is to upgrade from colored 1 to colored 2

[faern]

Sadly this is a breaking change due to colored types being public in the API of fern (fern::colors::Color is a colored type). So this will need to be a bump to fern 0.7.0.

[daboross]

Yep - that'll be fine for me. I had forgotten colored had done a 2.0 release, but for a security fix like this I'll do a major release and up it.

If you feel like backporting to colored 1.0 and doing a 1.9.4 release from a branch (branched off of the 1.9.3 release), that could fix it for more people today, but I don't think it's that big of a deal either way. Most rust crates don't support old major versions with security fixes in any case.

[faern]

The important part IMO is to give people a working version of fern without atty as a transitive dependency. And I currently don't have the bandwidth to work on colored. So just getting fern 0.7 with colored ^2 out there is fine for me.

[faern]

I submitted a PR on getting this updated and fixed. #128

[faern]

Bump on this. There seems to be a pretty clear and easy way forward here. Anything stopping the PR from being reviewed/merged?

[hwittenborn]

Hey! I'm back, I've been a bit busy with some other projects I work on.

If you feel like backporting to colored 1.0 and doing a 1.9.4 release from a branch (branched off of the 1.9.3 release), that could fix it for more people today, but I don't think it's that big of a deal either way.

I went ahead and got that done, that'll probably be the last major update for v1 though. I'd definitely recommend upgrade to v2 if y'all are able to though - were there any major blockers stopping that from happening? (From what I saw the only major thing that created a version bump was the interface of colored::Color returning Cow<&str> over &str.)

v3 is definitely going to be more of a rewrite, so if you'd prefer to bump to any version and keep compatibility I'd make it v2.

[faern]

The signatures of the methods on colored::Color does not matter. If we change to a different version of colored that is not semver compatible (2 != 1) then it's going to be breaking even if 100% of the API surface were to be identical. Because colored_v1::Color and colored_v2::Color will be 100% different types according to the compiler.

If I in my own crate have these dependencies:

[dependencies]
colored = "1.0"
fern = "0.6"

And then I have the code:

let a_color: colored::Color = fern::colors::Color::Black;

The the compiler will error on these being different types if fern changes to colored 2.

But great that you have backported the atty-removal! :heart: I can look at making an as-small-as-possible bump here in fern and hope we can release a non-breaking 0.6 version of fern.

[faern]

I realize no change to fern is required. Your new release of colored is just compatible. I can just do cargo update -p colored and get rid of the last instance of atty in my dependency tree :partying_face: Thank you very much.

[hwittenborn]

Cool, I'm glad to hear @faern! I didn't realize bumping v1 would make it automatic like that either, I'm definitely glad that all got done.

Y'all can handle the major/minor version bump however y'all would like, it won't bother me one way or the other. It's good to see everything's pretty much in the clear now though :).

[faern]

Thank you again for the patch! Now fern does not need to rush any colored upgrade any longer