Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow compression for partial content #3429

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions actix-files/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Do not set content-encoding to identity for range requests

## 0.6.6

- Update `tokio-uring` dependency to `0.4`.
Expand Down
23 changes: 1 addition & 22 deletions actix-files/src/named.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use actix_web::{
http::{
header::{
self, Charset, ContentDisposition, ContentEncoding, DispositionParam, DispositionType,
ExtendedValue, HeaderValue,
ExtendedValue,
},
StatusCode,
},
Expand Down Expand Up @@ -534,27 +534,6 @@ impl NamedFile {
length = ranges[0].length;
offset = ranges[0].start;

// When a Content-Encoding header is present in a 206 partial content response
// for video content, it prevents browser video players from starting playback
// before loading the whole video and also prevents seeking.
//
// See: https://github.com/actix/actix-web/issues/2815
//
// The assumption of this fix is that the video player knows to not send an
// Accept-Encoding header for this request and that downstream middleware will
// not attempt compression for requests without it.
//
// TODO: Solve question around what to do if self.encoding is set and partial
// range is requested. Reject request? Ignoring self.encoding seems wrong, too.
// In practice, it should not come up.
if req.headers().contains_key(&header::ACCEPT_ENCODING) {
// don't allow compression middleware to modify partial content
res.insert_header((
header::CONTENT_ENCODING,
HeaderValue::from_static("identity"),
));
}

res.insert_header((
header::CONTENT_RANGE,
format!("bytes {}-{}/{}", offset, offset + length - 1, self.md.len()),
Expand Down
32 changes: 2 additions & 30 deletions actix-files/tests/encoding.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use actix_files::{Files, NamedFile};
use actix_files::Files;
use actix_web::{
http::{
header::{self, HeaderValue},
StatusCode,
},
test::{self, TestRequest},
web, App,
App,
};

#[actix_web::test]
Expand Down Expand Up @@ -35,31 +35,3 @@ async fn test_utf8_file_contents() {
Some(&HeaderValue::from_static("text/plain")),
);
}

#[actix_web::test]
async fn partial_range_response_encoding() {
let srv = test::init_service(App::new().default_service(web::to(|| async {
NamedFile::open_async("./tests/test.binary").await.unwrap()
})))
.await;

// range request without accept-encoding returns no content-encoding header
let req = TestRequest::with_uri("/")
.append_header((header::RANGE, "bytes=10-20"))
.to_request();
let res = test::call_service(&srv, req).await;
assert_eq!(res.status(), StatusCode::PARTIAL_CONTENT);
assert!(!res.headers().contains_key(header::CONTENT_ENCODING));

// range request with accept-encoding returns a content-encoding header
let req = TestRequest::with_uri("/")
.append_header((header::RANGE, "bytes=10-20"))
.append_header((header::ACCEPT_ENCODING, "identity"))
.to_request();
let res = test::call_service(&srv, req).await;
assert_eq!(res.status(), StatusCode::PARTIAL_CONTENT);
assert_eq!(
res.headers().get(header::CONTENT_ENCODING).unwrap(),
"identity"
);
}
4 changes: 4 additions & 0 deletions actix-http/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- Implement `FromIterator<(HeaderName, HeaderValue)>` for `HeaderMap`.

### Changed

- Prevent compression of Partial Content.

## 3.8.0

### Added
Expand Down
1 change: 1 addition & 0 deletions actix-http/src/encoding/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ impl<B: MessageBody> Encoder<B> {
let should_encode = !(head.headers().contains_key(&CONTENT_ENCODING)
|| head.status == StatusCode::SWITCHING_PROTOCOLS
|| head.status == StatusCode::NO_CONTENT
|| head.status == StatusCode::PARTIAL_CONTENT
|| encoding == ContentEncoding::Identity);

let body = match body.try_into_bytes() {
Expand Down
Loading