Rocket: Failure to write response for request with a data file to route with a request guard that returns an Outcome::Failure

[geauxvirtual]

Rocket version 0.3.1 (also seen with Rocket 0.3.0).

Mac OS X 10.12.6

I have request guard in place on select routes for user authentication. I also have a route that has a data guard which accepts a data file from a multipart form input. If the request to this route does not include an authorization header along with the data file, I would expect the user to receive the Outcome::Failure defined in the request guard. Instead, what I'm seeing on the console is:

And the user sees:

I've written a small example that duplicates what I was seeing in my code, included below. Sending a POST request to this route with a file will result in the error seen above. I would say typical file sizes that my application could see would range from 2 to 200KBs.

Example curl request:
curl -X POST http://localhost:8000/ -F 'file=@somefile'

Example Rocket server with route

#![feature(plugin)]
#![plugin(rocket_codegen)]

extern crate rocket;

use rocket::{Request, Outcome};
use rocket::request::{self, FromRequest};
use rocket::http::Status;

pub struct Guard(pub String);

impl<'a, 'r> FromRequest<'a, 'r> for Guard {
    type Error = ();

    fn from_request(request: &'a Request<'r>) -> request::Outcome<Guard, ()> {
        Outcome::Failure((Status::Unauthorized, ()))
    }
}

#[post("/")]
fn index(guard: Guard) -> &'static str {
    "Hello World!"
}

fn main() {
    rocket::ignite()
        .mount("/", routes![index])
        .launch();
}

[joep27182]

I got the same problem on rocket 0.3.3 using Ubuntu 16.04.3. Wondering how to reject file uploads based on cookies / headers / whatever without running into "Data left unread. Force closing network stream." and the resulting broken pipe error. Appreciate any thoughts on this :)

[tcr-ableton]

Up. I have the exact same issue using rocket "0.3.6".

[SergioBenitez]

The error is spot-on here: you've left data unread in the DataStream in Data. Rocket cannot process the next HTTP message until either the entire data stream is read, even if discarded, or the connection is closed. The former is unsafe for Rocket to do automatically as it can trivially lead to denial of service attacks, so Rocket does the latter. (In reality, it discards data up to a 1KiB limit and only closes the connection if that doesn't suffice.)

If you're handling Data directly via a FromData guard, the onus is on you to handle the entire data stream. You can discard it, store it somewhere, or use it in some way, but you _must_ do something with it or Rocket will do the safe thing and close the connection.

[tcr-ableton]

I see! Thanks for your thorough input @SergioBenitez

In my case, I want to be able to send back a 409 Conflict when the data posted correspond to a resource that already exists. Therefore I'd like to avoid having to read the Data when this condition is met. You mention the ability to discard the data, I couldn't find the way to do that, could you give me pointers?

Cheers

[SergioBenitez]

There's no avoiding reading the data without closing the connection. It _must_ be read. The data is a stream; it is not buffered anywhere. To get to the next point in the stream, we _must_ read all prior points. Thus "discard" simply means to read the data but do nothing with it.

The only safe approach is to limit the amount of data you're willing to discard. Otherwise, it is a trivial attack vector. You can do this with something like:

use std::io;

io::copy(&mut data.open().take(LIMIT), &mut io::sink());

[Yarn]

@SergioBenitez
The issue here doesn't seem to have been addressed at all. In a request guard you can't access the request body. You must do something with the data but also can't do anything with the data.

The way it works currently it would seem that any guards that may be used with file uploads just can't return Outcome::Failure. This seems like a major ergonomics issue.

Even with access to the request body in many cases this behavior isn't really workable. Receiving a potentially multi-GB file upload before sending back an error is an issue as you've stated but the alternative of leaving the client to blindly guess about if the server/db is down, the file is too large, the file already exists, the client's credentials don't have the needed permissions (which could vary by folder or other things), some upload quota has been reached (and when to try again), the request was malformed (e.g. multipart data), or any number of other issues isn't really a solution.

You've said you can't avoid reading the data without closing the connection but the issue isn't that the connection is being closed; it's that it's being closed before sending the response.

This is the most relevant part of the spec i could find (which seems to allow this behavior).
See the last bullet point for origin servers. Note that while it says "SHOULD NOT" that is comparing it to reading the full body and returning a response, not closing the connection without sending a response. The cited concern that the client may not receive the response is obviously a larger concern when the response is not sent at all.
https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.3

https://github.com/SergioBenitez/Rocket/blob/v0.3/lib/src/data/data_stream.rs#L43
Changing Shutdown::Both to Shutdown::Read here seems to have the desired effect but could cause issues elsewhere in the code.
result of cargo test test result: ok. 358 passed; 0 failed; 18 ignored; 0 measured; 0 filtered out