Misc. feedback

[ThinkChaos]

Hi,

Thanks for developing this, it's already quite useful and has enable me to get something working quick.

I noted a couple things and wanted to give you feedback, I'm no expert, but hopefully this is constructive and useful :)

• [x] unwrap is used in a couple places outside of tests. Would be nice to return Result so apps can be sure there's no panics.
• [x] web_push::decrypt indexes encrypted_message without any length validation
• [x] using Error = Box<dyn std::error::Error> is not recommended in libraries. Made PR #3 to address this.
• [ ] Using "native" in the crate names is confusing since other Rust projects use it to mean the opposite ("platform native" aka openssl in most cases). See Tokio TLS.

[leotaku]

Thanks for the feedback, I'll try to respond as best as I can:

• I think all non-test unwraps are used in places where the error case will never occur. E.g. converting a slice of n bytes into an array of n bytes. I could be wrong though, so if you have found any unwrap that can actually panic, please tell me.
• Yeah, the decryption routine should really check for that. I'll work something out, probably after I've also improved the general error handling of the crate.
• That does make sense, but I'm not too keen on using thiserror or exposing quite the level of low-level errors as you have done in your PR. This is not at all your fault, but I think error handling could be improved quite a bit by making some internal functions that do not need to be able to fail actually infallible.
• Yeah, I noticed that too. Maybe web-push-pure/ece-pure would be a better name? I don't think it is too late to change the name, but I'm not sure it's worth it, as the one-line description describes that the implementation is pure Rust. Any other name ideas?

[imbolc]

When unwrapping, even if you've checked every place, further changes could potentially lead to a overlooked panic. I've reviewed the code and noticed that in most places where unwrap is used, the functions already return Result. Would you consider adding an Unreachable error variant maybe? This change could also save time for people assessing the crate, especially if you consider adding #![warn(clippy::unwrap_used)] at crates level.

[leotaku]

@imbolc I don't understand why removing the unwraps would be beneficial. As far as I am aware it is perfectly good practice to use unwrap for runtime invariants. All places I use unwrap in clearly cannot panic, no matter what an user of the public API does.

I also do not think this would save time for people who want to audit the crate. clippy::unwrap_used as an audit tool seems like am especially bad idea, as it might make people think that code will never panic, but expect, array indexing, etc. are still allowed.

[imbolc]

All places I use unwrap in clearly cannot panic, no matter what an user of the public API does

My reasoning here is similar to why we avoid using unsafe when possible. You should manually check for potential panics each time you change the code.

I also do not think this would save time for people who want to audit the crate.

Wouldn't every reviewer have to reevaluate the safety of each unwrap? I mean, at least two of them have already done this. ;)

clippy::unwrap_used as an audit tool seems like am especially bad idea, as it might make people think that code will never panic, but expect, array indexing, etc. are still allowed

I don't understand why someone would assume there are no panics when the lint clearly states "no unwraps". By the way, clippy::expect_used is also available.

[imbolc]

Also, when you examine the code, it's not immediately clear whether a specific unwrap is intentional or simply left forgotten after prototyping. Perhaps we could at least replace it with something like unwrap_or_else(|_| unreachable!()) to clarify its purpose if you're against using Error::Unreachable.

[leotaku]

@imbolc I see your point about it being potentially hard to determine if an unwrap is intentional or not. In my opinion, Error::Unreachable is not a good pattern, basically for the reasons outlined here. What do you think about replacing the unwraps with expects that specify the invariant that makes them safe? I.e. .expect("pad_size is always <16, which is guaranteed to fit into usize").

[imbolc]

Regarding the mentioned article, I'm not sure if its reasoning is applicable in this case

function from the previous section can be rewritten to return an error instead of panicking... And why return an error if the docs guarantee that an error will never be returned?

As I mentioned, your code is already mostly returning results. The only exception is impl From<VapidSignature> for http::HeaderValue.

There's even a related clippy lint panic_in_result_fn - checks for usage of panic! or assertions in a function of type result.

Notice how much more complicated this function got. And notice how ham-fisted the documentation is.

I don't see how .map_err(Error::Unreachable)? is more complicated than .unwrap(). I'd even argue it clarifies meaning of the code by explaining justification for the unwrap. Also I don't think it needs any extra documentation, the error variant seems self describing for me.

What do you think about replacing the unwraps with expects that specify the invariant that makes them safe?

I think it's better than bare unwraps :)

[leotaku]

@imbolc The change to remove all unwraps in library code has now been published in v0.2.1.

[ThinkChaos]

Glad to see expect instead of unwrap, I agree it helps readers and even users in case of mistake :)

I updated the original post with a checklist to make explicit what's been addressed already.

While doing that, I noticed the fix for " web_push::decrypt indexes encrypted_message without any length validation" doesn't validate idlen, so malformed data could still cause a panic!

The "native" naming thing is not that big a deal, and the README already makes it clear it means "rust-native", so if that's the only left point I'd be fine with closing this.

[ThinkChaos]

I'll go ahead and close this since #8 fixes the idlen issue mentioned above.

Thanks!