tower-http icon indicating copy to clipboard operation
tower-http copied to clipboard

CompressionLayer: `accept-encoding` header parsed incorrectly

Open mdickopp opened this issue 3 years ago • 7 comments

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

mdickopp avatar Feb 06 '22 15:02 mdickopp

Thanks for the detailed breakdown!

davidpdrsn avatar Feb 06 '22 15:02 davidpdrsn

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 avatar Feb 07 '22 13:02 mdickopp

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 avatar Feb 08 '22 09:02 mdickopp

I will work on a fix for this issue.

mdickopp avatar Feb 09 '22 20:02 mdickopp

@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.

davidpdrsn avatar Mar 05 '22 10:03 davidpdrsn

Sure. Cases 4 and 6 are currently unresolved.

mdickopp avatar Mar 05 '22 19:03 mdickopp