ardaku / whoami

Rust crate to get the current user and environment.
Apache License 2.0
195 stars 31 forks source link

No direct migration path for whoami::hostname #117

Closed jsgf closed 2 days ago

jsgf commented 1 month ago

Is your feature request related to a problem? Please describe. whoami::hostname is deprecated. The suggested replacement is whoami::fallible::hostname, but this is not functionally identical. In principle the case of hostname shouldn't matter but in practice it means that we can't just directly replace calls to whoami::hostname() to whoami::fallible::hostname().unwrap_or_else(|| "localhost".to_string()) (or just expect/unwrap) without worrying about introducing a regression.

Describe the solution you'd like Maybe whoami::fallible::hostname_lower() which is functionally identical to the infallible version (aside from returning a Result).

Describe alternatives you've considered Open-coding a replacement in a local library, which is awkward. Disable warnings/errors for deprecated functions.

Additional context I'm trying to migrate a large amount of code from 1.4 to 1.5 to work around the security issue.

AldaronLau commented 1 month ago

Thanks for the issue, and PR!

Yeah, that function isn't functionally identical (on purpose).

I'm not sure what the purpose of adding that function to whoami would be. Converting to lowercase should probably only be done for certain display purposes (and even there I'm not sure), and hostname casing should otherwise be kept.

If possible, can you tell me what you're trying to do with a lowercase-normalized hostname, and why the casing shouldn't be preserved? Is a bug caused by not making it lowercase?

The reasoning for this change is discussed here: https://github.com/ardaku/whoami/issues/82

jsgf commented 1 month ago

I'm doing a mass change of other people's code, I'm not exactly sure what the precise requirements are in each case. It's quite possible that it makes no difference at all, but I'm not in a position to be able to evaluate every callsite. We have had problems in the past from strange failures relating the case-mismatches in hostnames (eg case-sensitive string cmp & mismatch in case between hostname() and a hostname in a config file), so I'd be hesitant about just assuming the case issue can be ignored.

BTW This is to resolve the possible security issue arising from #91. So I really want a very direct migration path without having to worry about any semantic differences.

AldaronLau commented 1 month ago

I'm doing a mass change of other people's code, I'm not exactly sure what the precise requirements are in each case.

Without a requirement, I'm hesitant to add this as a new function to whoami. It seems like an okay function for consumers of the library to have to write, if they really need it.

We have had problems in the past from strange failures relating the case-mismatches in hostnames (eg case-sensitive string cmp & mismatch in case between hostname() and a hostname in a config file), so I'd be hesitant about just assuming the case issue can be ignored.

To me, this sounds like switching to the new API would have fixed some of those old issues, so I'm questioning the value of the additional API in whoami besides as an intermediate step in addressing tech-debt.

BTW This is to resolve the possible security issue arising from #91. So I really want a very direct migration path without having to worry about any semantic differences.

While that function is deprecated, it's not going away any time soon (whoami 2.0 will likely be released in mid-2025, and 1.0 will still be maintained). I think it would be okay to #[allow(deprecated)] until that upgrade if you want the quickest migration path (unless you want to avoid the #[allow] and handle the Result case).

jsgf commented 1 month ago

I've fallen back to scattering a pile of #[allow(deprecated)] around for now.

AldaronLau commented 2 days ago

Closing as not planned