2.10.0 — diff review from 2.9.6 only (older version)
From bytecodealliance/wasmtime. By Andrew Brown.
These reviews are from cargo-vet. To add your review, set up cargo-vet
and submit your URL to its registry.
The current version of Ureq is 3.0.0-rc2.
2.10.0 — diff review from 2.9.6 only (older version)
From bytecodealliance/wasmtime. By Andrew Brown.
2.9.1 (older version)
From kornelski/crev-proofs copy of git.savannah.gnu.org.
Packaged for Guix (crates-io)
cargo-vet does not verify reviewers' identity. You have to fully trust the source the audits are from.
This crate will not introduce a serious security vulnerability to production software exposed to untrusted input. More…
This crate can be compiled, run, and tested on a local workstation or in controlled automation without surprising consequences. More…
May have been packaged automatically without a review
This review is from Crev, a distributed system for code reviews. To add your review, set up cargo-crev
.
The current version of Ureq is 3.0.0-rc2.
0.11.1 (older version) Thoroughness: Low Understanding: Medium
by dpc on 2019-10-16
Header::new
is unsound? This unsafe
there seem unneccessary in the first place. There's not much performance to gain here.
Header: value
- there can be more spaces preceeding the value. Header::from_str
could take a HeaderName: Value
"The field value MAY be preceded by any amount of LWS, though a single SP is preferred. " (https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2)
pub fn add_header(headers: &mut Vec<Header>, header: Header) {
if !header.name().to_lowercase().starts_with("x-") {
Probably can be done faster by just comparing slice with eq_ignore_ascii_case
, instead of allocating a lowercase copy.
src/lib.rs
: Tests doing http calls to external network can fail on offline machines, are a potential privacy problem etc.
PoolKey::new
- failing to get a port should probably be an error, since that means te scheme was neither http nor https, so why are we even handling it?
Unit
indeed is a so-so name. If the comment is "unit of work" then it probably should be UnitOfWork
.
// pointer to underlying stream
stream: *mut Stream,
Ouch. When I see mutable raw pointers, I already know that I will not be using this code as is. :D . From what I can see later, it seems this pointer is used just for the drop implementation? In that case, just use Option<Stream>
or Option<Box<Stream>>
. Option<Box<T>>
even compiles down to the same data/code as nullable pointer.
I fail to see the point of Request::build
...
Request::query
and query_str
seems silent about the matter of escaping, and I wonder if it will work correctly at all.
Request::timeout
... Deadlines are better than timeouts, and are not harder to implement.
index: (usize, usize), // index into status_line where we split: HTTP/1.1 200 OK
I see no reason, why these two would be touple, instead of being separate and named appropriatly.
Response::new
works by ... parsing? I don't know how I feel about that. Seems wasteful.
/// Rather than exposing a custom error type through results, this library has opted
/// for representing potential connection/TLS/etc errors as HTTP response codes.
/// These invented codes are called "synthetic".
I don't know how I feel about this. Seems like a bad idea. :D . It will lead to confusion during debugging eg. by people who don't know about this "feature" (eg. DevOps that will be reading logs of software that is using this library). They will see "error: 535", and wonder how the hell this code happened.
let mut yolo = YoloRead {
does not build confidence. :D
pub(crate) struct YoloRead {
stream: *mut Stream,
dealloc: bool, // whether we are to dealloc stream on drop
}
Oh. Here is another *mut Stream
. I don't really get why it is neccessary.
fn from_str(s: &str) -> Result<Self, Self::Err> {
let bytes = s.as_bytes().to_owned();
I don't think this clone is neccessary.
Generally - negative review, since there's some unsafe
code that I don't think is neccessary, and I have
no confidence that it is actually correct (quite the opposite... I suspect some stuff is wrong with it).
There were some other minor problems, potentially bugs, a lot of casual needless cloning and stuff that looks like plain inefficiencies, and generally this crate at its current state does not look like something I'd recommend for any serious production use. The goal seems good but it seem not there yet. I think crates like this need either a lot of usage and pair of eyes and developers to iron out all the details, or some extensive test suite.
Lib.rs has been able to verify that all files in the crate's tarball are in the crate's repository with a git tag matching the version. Please note that this check is still in beta, and absence of this confirmation does not mean that the files don't match.
Crates in the crates.io registry are tarball snapshots uploaded by crates' publishers. The registry is not using crates' git repositories, so there is a possibility that published crates have a misleading repository URL, or contain different code from the code in the repository.
To review the actual code of the crate, it's best to use cargo crev open ureq
. Alternatively, you can download the tarball of ureq v3.0.0-rc2 or view the source online.
No
unsafe
changes; this audit observed mainly license and documentation changes.