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.

Sign up for GitHub

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

Jump to bottom

Don't normalize URIs with no valid path #2246

Merged
merged 2 commits into from
Jun 5, 2021
Merged

Don't normalize URIs with no valid path #2246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5506fd3
Don't normalize URIs with no valid path
thalesfragoso Jun 3, 2021
507911b
Small clean-up suggestion on normalize code
thalesfragoso Jun 4, 2021
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 CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@
* Update `language-tags` to `0.3`.
* `ServiceResponse::take_body`. [#2201]
* `ServiceResponse::map_body` closure receives and returns `B` instead of `ResponseBody<B>` types. [#2201]
* `middleware::normalize` now will not try to normalize URIs with no valid path [#2246]

### Removed
* `HttpResponse::take_body` and old `HttpResponse::into_body` method that casted body type. [#2201]

[#2200]: https://github.com/actix/actix-web/pull/2200
[#2201]: https://github.com/actix/actix-web/pull/2201
[#2246]: https://github.com/actix/actix-web/pull/2246


## 4.0.0-beta.6 - 2021-04-17
Expand Down
96 changes: 58 additions & 38 deletions src/middleware/normalize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,53 +137,57 @@ where

let original_path = head.uri.path();

// Either adds a string to the end (duplicates will be removed anyways) or trims all slashes from the end
let path = match self.trailing_slash_behavior {
TrailingSlash::Always => original_path.to_string() + "/",
TrailingSlash::MergeOnly => original_path.to_string(),
TrailingSlash::Trim => original_path.trim_end_matches('/').to_string(),
};

// normalize multiple /'s to one /
let path = self.merge_slash.replace_all(&path, "/");

// Ensure root paths are still resolvable. If resulting path is blank after previous step
// it means the path was one or more slashes. Reduce to single slash.
let path = if path.is_empty() { "/" } else { path.as_ref() };

// Check whether the path has been changed
//
// This check was previously implemented as string length comparison
//
// That approach fails when a trailing slash is added,
// and a duplicate slash is removed,
// since the length of the strings remains the same
//
// For example, the path "/v1//s" will be normalized to "/v1/s/"
// Both of the paths have the same length,
// so the change can not be deduced from the length comparison
if path != original_path {
let mut parts = head.uri.clone().into_parts();
let query = parts.path_and_query.as_ref().and_then(|pq| pq.query());

let path = if let Some(q) = query {
Bytes::from(format!("{}?{}", path, q))
} else {
Bytes::copy_from_slice(path.as_bytes())
// An empty path here means that the URI has no valid path. We skip normalization in this
// case, because adding a path can make the URI invalid
if !original_path.is_empty() {
// Either adds a string to the end (duplicates will be removed anyways) or trims all
// slashes from the end
let path = match self.trailing_slash_behavior {
TrailingSlash::Always => format!("{}/", original_path),
TrailingSlash::MergeOnly => original_path.to_string(),
TrailingSlash::Trim => original_path.trim_end_matches('/').to_string(),
};
parts.path_and_query = Some(PathAndQuery::from_maybe_shared(path).unwrap());

let uri = Uri::from_parts(parts).unwrap();
req.match_info_mut().get_mut().update(&uri);
req.head_mut().uri = uri;
// normalize multiple /'s to one /
let path = self.merge_slash.replace_all(&path, "/");

// Ensure root paths are still resolvable. If resulting path is blank after previous
// step it means the path was one or more slashes. Reduce to single slash.
let path = if path.is_empty() { "/" } else { path.as_ref() };

// Check whether the path has been changed
//
// This check was previously implemented as string length comparison
//
// That approach fails when a trailing slash is added,
// and a duplicate slash is removed,
// since the length of the strings remains the same
//
// For example, the path "/v1//s" will be normalized to "/v1/s/"
// Both of the paths have the same length,
// so the change can not be deduced from the length comparison
if path != original_path {
let mut parts = head.uri.clone().into_parts();
let query = parts.path_and_query.as_ref().and_then(|pq| pq.query());

let path = match query {
Some(q) => Bytes::from(format!("{}?{}", path, q)),
None => Bytes::copy_from_slice(path.as_bytes()),
};
parts.path_and_query = Some(PathAndQuery::from_maybe_shared(path).unwrap());

let uri = Uri::from_parts(parts).unwrap();
req.match_info_mut().get_mut().update(&uri);
req.head_mut().uri = uri;
}
}

self.service.call(req)
}
}

#[cfg(test)]
mod tests {
use actix_http::StatusCode;
use actix_service::IntoService;

use super::*;
Expand Down Expand Up @@ -387,6 +391,22 @@ mod tests {
}
}

#[actix_rt::test]
async fn no_path() {
let app = init_service(
App::new()
.wrap(NormalizePath::default())
.service(web::resource("/").to(HttpResponse::Ok)),
)
.await;

// This URI will be interpreted as an authority form, i.e. there is no path nor scheme
// (https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3)
let req = TestRequest::with_uri("eh").to_request();
let res = call_service(&app, req).await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
}

#[actix_rt::test]
async fn test_in_place_normalization() {
let srv = |req: ServiceRequest| {
Expand Down