Newbie Code Review Request 🙏

Slight odd post, let me know if this isn't appropriate.
I wrote a small rust web project as a replacement of imgix at my work (context: imgix is not appropriate for our use case and costs us too much)

I am trying to introduce rust at my work but i have never written any rust. I sat down and took 4 hours to write this project without ever using rust before or having any prior knowledge of rust.
So i am not sure if this is idiomatic rust. I come from a background of OCaml, Haskell, Scala, Clojure and JS.

here is the project
GitHub - shrynx/vulpix: An image processing service.
It's overall less than 200 lines (including imports and white space)
Not looking for a thorough review but just a quick glance if it makes sense.

PS: Honestly using rust for the first time, even without knowing anything was such a breathe of fresh air.
Very well designed language, amazing tooling, ide integration just works, cargo is dead simple build tool and the super friendly compiler error messages. i feel i am already turning into a fanboy :heart:

sorry should be good now. i accidentally had a aws bucket name

You might be interested in GraphicsMagick, which in my experience has been much faster than ImageMagick.

1 Like

The main file is pretty clean; the only thing I don't understand is the Once/call_once mechanism. You don't reference that value anywhere else, so you could literally just call magick_wand_genesis() and not use Once at all.

In, there are a couple of red flags:

  • &String parameters. Passing owned containers that have a borrowed counterpart (e.g. String or Vec) by reference is harmful. A &String isn't any more useful than a &str (since you can't mutate it!), and a &String can always be converted to a &str but the converse is not true. Hence, accepting a &String forces the caller to allocate a new String if they don't already have one, even if they had a type that was convertible to a &str. (This useless allocation is exactly what you are forced to do in test_validate_signature().)
  • This:
    format!("image/{}", format.to_string())
    should just be
    format!("image/{}", format)
    It's unnecessary to allocate a string just to splice it into a format string; chances are if something is ToString, it's already Display so you can just format it directly.
  • "".to_string() is more simply String::new().
  • You shouldn't use raw string replacement for URL manipulation (specifically, I mean the code in validate_signature()). Use a proper URL parser/formatter library.
  • If there is no secret key and no expiration date, then validation succeeds. That's probably a huge security hole and not what was intended.
  • There are a couple really weird pieces of code, e.g. if &format!("{:x}", md5_digest) == encrypted_key – you could just compare the formatted String directly with the key.


  • the get_error_message() function is doubly non-idiomatic. First, in Rust, one doesn't write "getters". get_xyz() shouldn't be used as a method name unless you have a very good reason. Second, this method should just be the ToString::to_string() impl, which is transitively implemented if your type is Display, which it should be. Use the thiserror library to remove From and Display/Error boilerplate from your error types.

In the enum variants should have CamelCase names, not ALLCAPS. It should be Png and Jpeg, not PNG and JPEG.


There's a function called validate_signature that takes an encryption_key (instead of a verifying_key) and uses md5...? Maybe you meant to use a Message Authentication Code (MAC) like HMAC-SHA256? Cryptographically, this looks at the very least extremely jumbled up, and in the worst case horribly broken. I think what you call:

  • secret_salt is the mac_key
  • encrypted_key is the received_mac
  • md5_digest should be a recomputed_mac

Also you're using == to compare the received MAC with the re-computed one, yielding a timing side-channel, which is like the 101 of CTF challenges.
Maybe you should pay a cryptographer to review this custom authentication scheme or use something well established instead.

Also it seems like it's parsing parameters from the query string (in validate_signature), when axum could be doing that for you, no?

1 Like

Thank you for such a comprehensive review, much appreciated.

I don't understand is the Once /call_once mechanism

yeah i don't either, directly taken from magick-rust but yeah i feel that can be removed as the example wraps it in a function.

You shouldn't use raw string replacement for URL manipulation.

i use extracted query params for rest of stuff but need the query string to create md5 hash (with the salt). That's how imgix works, and i want to keep it compatible so at work i can use it as a drop in replacement (at least initially).
Their api needs query param s to be md5 of secret_salt/image_path?foo=1&bar=2
It's pretty shit and not that secure honestly. after first compatible version i am going to change it.
This is their doc Securing Assets | imgix Documentation if your curious

if &format!("{:x}", md5_digest) == encrypted_key – you could just compare the formatted String directly with the key.

The md5 returns a Digest type, which i couldn't compare to String, i'll give another look.

  • If there is no secret key and no expiration date, then validation succeeds. That's probably a huge security hole and not what was intended.

yeah those will be non optional params when productionised. i just made them optional so i could test other features without calculation hash every time i change width or something

Use the thiserror library

Ahh nice, i'll give the library a spin.

Rest of the suggestions totally make sense. I'll improve the code. Thanks a ton again.

I had a quick look. seems imagemagick bindings are more maintained. but i'll try if i get speed ups. thanks !

That's a good suggestion. i mentioned in previous comment that the current version is to maintain compatibility with imgix which is sadly not that secure.
The next version i could definitely use this suggestion. thanks !

That's not what I meant. I wrote "compare the formatted String directly" – that sounds pretty clear to me, remove the leading &, there's no need to take a reference.

That's fine, just don't do it using .replace(format!("?q="), ""). That's going to break as soon as the externally-supplied parameter itself contains ?q= or something equally maliciously-crafted.

ahh gotcha. yeah i feel i should start reading the rust book and build knowledge on & String, &str and all. thanks for clarifying

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.