Idea: big flashing warning on "dubious" whitespace

[toothbrush]

G'day! I'm opening this issue as a bit of a straw man discussion spot, because while i'm happy to provide a PR, i'd like to discuss the problem and some potential improvements with maintainers first.

The problem

Basically, colleagues often get bitten by the following (probably extremely well-known) issue:

$ echo "MYT0K3N" | shush encrypt alias/foo

Forgetting echo -n or shush encrypt --trim leaves one with an encrypted token that isn't accepted by a lot of programs, due to the trailing newline. The feedback cycle tends to be slow and annoying because it'll only get noticed when the app is deployed and tries to use the secret.

Some potential improvements

1. Warn clearly on stderr, when encrypting, if input != trim(input). This might be a spurious warning, but i can't think of many instances where you really need surrounding whitespace.
2. Be a bit smarter when decrypting. This one might be a bit more controversial because it's a behavioural change – but hear me out 😅. I believe it'd be reasonable to special-case a trailing \n when decrypting. I would distinguish two cases: a. The trailing character is \n, and the string contains no other whitespace characters. In this case, trim the \n. b. The trailing character is \n, but there are whitespace characters elsewhere in the string too. In this case, return the string verbatim.

Especially for 2 i could imagine it'd be worth making a clear note in the CHANGELOG and bumping at least the minor version, perhaps also adding a flag to keep old behaviour. My belief is that in 99.99% of cases approach 2 would be perfectly fine, but i'm keen to hear if the maintainers are more creative than i in coming up with edge-cases that make this undesirable.

Option 1 though i imagine could be implemented today with little concern, right?

[mdub]

Original author (but now merely an ex-maintainer) here.

I can see that trailing whitespace is a bit of a footgun.

I think it would be dangerous to alter the default behaviour, but adding warnings and/or options to alter behaviour seems pragmatic.

I like the idea of having encrypt issue a warning when it sees trailing whitespace, by default. We could add an option to suppress that warning:

• --on-trailing-whitespace warn is the default
• --on-trailing-whitespace error makes it barf
• --on-trailing-whitespace allow avoids the warning (i.e. current behaviour)

I'm less enthusiastic about making decrypt "smart", though we could add a warning (to stderr) there, too.

[toothbrush]

Thanks for the response @mdub! I agree about your suggestions for encrypt behaviour.

For decrypt, i also initially thought about a warning on decryption (if suspicious whitespace is found), but i had 2 issues with that:

• Typically it'll be hidden in a pod running somewhere that a user might not see – although if things fail & they investigate, the warning could be helpful.
• We use a lot of Make with secret := $(shell shush decrypt ...) and it ... mangles stderr/stdout... 😕 That's Make's fault for sure, but for my use case that's less nice.

That said, if all i get out of this is a warning on encrypt, i'm still happy, because arguably our systems work today and so we just care about the N+1st secret getting encrypted with correct whitespace.

Having thought out loud, i'm happy to start implementing the encrypt warning as you describe.

[toothbrush]

Say @mdub, not sure if you can help, but i haven't heard anything back from anyone at REA about #39. Before we make a fork (which has maintenance overhead of course), do you know if there's anyone we can poke one last time? Failing that, is there any chance i could become a maintainer to push out those updates? I understand that might not be desirable since i'm not an REA employee, but.. it's a small world :).