tower-rs / tower-http

HTTP specific Tower utilities.
682 stars 159 forks source link

CompressionLayer: `accept-encoding` header parsed incorrectly #215

Open mdickopp opened 2 years ago

mdickopp commented 2 years ago

Bug Report

Version

tower-http v0.2.1

Platform

Debian Linux 12 (“bookworm”) Linux feynman 5.15.0-3-amd64 #1 SMP Debian 5.15.15-1 (2022-01-18) x86_64 GNU/Linux rustc 1.58.1 (db9d1b20b 2022-01-20)

Description

When using CompressionLayer, the accept-encoding header sent by the client is not parsed correctly (i.e., according to RFC 7231, sections 5.3.1 and 5.3.4). The following program demonstrates the issues (using axum v0.4.5):

use axum::{routing::get, Router};
use tower_http::compression::{predicate::SizeAbove, CompressionLayer};

#[tokio::main]
async fn main() {
    let app = Router::new()
        .route("/", get(|| async { "" }))
        .layer(CompressionLayer::new().compress_when(SizeAbove::new(0)));
    axum::Server::bind(&"0.0.0.0:3000".parse().unwrap())
        .serve(app.into_make_service())
        .await
        .unwrap();
}

Case 1: Uppercase encodings and qvalues are not parsed (fixed by #220)

Encodings and qvalues are case-insensitive, i.e. the server should understand them whether they are lowercase, uppercase, or a mixture of both.

Command Expected Encoding Observed Encoding
curl -I -H 'accept-encoding: GZIP' http://127.0.0.1:3000/ gzip (none)
curl -I -H 'accept-encoding: gZiP' http://127.0.0.1:3000/ gzip (none)
curl -I -H 'accept-encoding: gzip;q=0.5, br;Q=0.8' http://127.0.0.1:3000/ br gzip

Case 2: Spaces before and after semicolon are not parsed (fixed by #220)

Space and horizontal tab characters are allowed before and after the semicolon separating the encoding from the qvalue.

Command Expected Encoding Observed Encoding
curl -I -H 'accept-encoding: gzip;q=0.5, br; q=0.8' http://127.0.0.1:3000/ br gzip
curl -I -H 'accept-encoding: gzip;q=0.5, br ;q=0.8' http://127.0.0.1:3000/ br gzip
curl -I -H 'accept-encoding: gzip;q=0.5, br ; q=0.8' http://127.0.0.1:3000/ br gzip

Case 3: Invalid qvalues are accepted (fixed by #220)

Qvalues are expected to have exactly 1 digit before and not more than 3 digits after the decimal point.

Command Expected Encoding Observed Encoding Explanation
curl -I -H 'accept-encoding: gzip;q=00.5' http://127.0.0.1:3000/ gzip (none) More than 1 digit before decimal point
curl -I -H 'accept-encoding: gzip;q=0.5000' http://127.0.0.1:3000/ gzip (none) More than 3 digits after decimal point
curl -I -H 'accept-encoding: gzip;q=.5' http://127.0.0.1:3000/ gzip (none) Missing digit before decimal point

Case 4: Request not rejected if client rejects identity encoding

If the client explicitly rejects the identity encoding or the wildcard encoding *, and accepts no encodings supported by the server, the request should be rejected.

Command Expected HTTP Status Code Observed HTTP Status Code
curl -I -H 'accept-encoding: identity;q=0' http://127.0.0.1:3000/ 406 Not Acceptable 200 OK
curl -I -H 'accept-encoding: *;q=0' http://127.0.0.1:3000/ 406 Not Acceptable 200 OK
davidpdrsn commented 2 years ago

Thanks for the detailed breakdown!

mdickopp commented 2 years ago

When I looked at the source code (to see if I can implement a fix), I found another case that is incorrect:

Case 5 (fixed by #220)

Command Expected Encoding Observed Encoding
curl -I -H 'accept-encoding: gzip;q=0.995, br;q=0.999' http://127.0.0.1:3000/ br gzip
mdickopp commented 2 years ago

There's another case:

Case 6

Command Expected Encoding Observed Encoding
curl -I -H 'accept-encoding: *;q=0.8, gzip;q=0.5' http://127.0.0.1:3000/ no encoding (identity) or any encoding supported by the server other than gzip gzip
mdickopp commented 2 years ago

I will work on a fix for this issue.

davidpdrsn commented 2 years ago

@mdickopp Thanks for your work on https://github.com/tower-rs/tower-http/pull/220! Would you mind editing your comments here with the cases that aren't handled yet? Should make it easier for people to pick up and help.

mdickopp commented 2 years ago

Sure. Cases 4 and 6 are currently unresolved.

bouk commented 1 year ago

Another related issue:

grpc-web relies on the normal Content-Encoding headers for compression. There's a default predicate in CompressionLayer to avoid compressing application/grpc requests which unfortunately also filters out application/grpc-web requests because the predicate just checks if there's a prefix match:

https://github.com/tower-rs/tower-http/blob/e8eb54966604ea7fa574a2a25e55232f5cfe675b/tower-http/src/compression/predicate.rs#L220-L233

This means I need to either remove this predicate or not have compression for grpc-web requests