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

feat: add capability to handle http requests with repeated slashes #258

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add capability to handle http requests with repeated slashes #258

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfc092d
feat: parse path with multiple slashes
Jul 26, 2022
4fceb20
fix: test cases passing
Aug 11, 2022
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
134 changes: 104 additions & 30 deletions crates/server/src/handle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

use super::{repos, tags, trees, users};

use drawbridge_type::{RepositoryName, TagName, TreePath, UserName};
use drawbridge_type::{RepositoryName, TagName, TreeName, TreePath, UserName};

use std::str::FromStr;

use axum::body::Body;
use axum::handler::Handler;
Expand All @@ -28,7 +30,7 @@ pub async fn handle(mut req: Request<Body>) -> impl IntoResponse {
}

trace!(target: "app::handle", "begin HTTP request handling {:?}", req);
let path = req.uri().path().trim_start_matches('/');
let path = req.uri().path().trim_matches('/');
let (ver, path) = path
.strip_prefix("api")
.ok_or_else(|| not_found(path))?
Expand All @@ -52,23 +54,29 @@ pub async fn handle(mut req: Request<Body>) -> impl IntoResponse {
let (head, tail) = path
.trim_start_matches('/')
.split_once("/_")
.map(|(left, right)| (left.to_string(), format!("_{right}")))
.unwrap_or((path.to_string(), "".into()));
.map(|(left, right)| (left.trim_end_matches('/').to_string(), format!("_{right}")))
.unwrap_or((path.to_string(), "".to_string()));
if head.is_empty() {
return Err(not_found(path));
}

let extensions = req.extensions_mut();

let (user, head) = head.split_once('/').unwrap_or((&head, ""));
let user = user.parse::<UserName>().map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse user name: {e}"),
)
})?;
let (user, head) = head
.split_once('/')
.unwrap_or((head.trim_start_matches('/'), ""));
let user = user
.trim_end_matches('/')
.parse::<UserName>()
.map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse user name: {e}"),
)
})?;
trace!(target: "app::handle", "parsed user name: `{user}`");
assert_eq!(extensions.insert(user), None, "duplicate user name");
let head = head.trim_start_matches('/');
if head.is_empty() {
return match *req.method() {
Method::HEAD => Ok(users::head.into_service().call(req).await.into_response()),
Expand All @@ -81,16 +89,20 @@ pub async fn handle(mut req: Request<Body>) -> impl IntoResponse {
};
}

let repo = head.parse::<RepositoryName>().map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse repository name: {e}"),
)
})?;
let repo = head
.trim_end_matches('/')
.parse::<RepositoryName>()
.map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse repository name: {e}"),
)
})?;
trace!(target: "app::handle", "parsed repository name: `{repo}`");
assert_eq!(extensions.insert(repo), None, "duplicate repository name");

let mut tail = tail.splitn(4, '/');
let mut tail = tail.split('/').filter(|x| !x.is_empty());

match (tail.next(), tail.next(), tail.next()) {
(None | Some(""), None, None) => match *req.method() {
Method::HEAD => Ok(repos::head.into_service().call(req).await.into_response()),
Expand All @@ -109,12 +121,16 @@ pub async fn handle(mut req: Request<Body>) -> impl IntoResponse {
)),
},
(Some("_tag"), Some(tag), prop @ (None | Some("tree"))) => {
let tag = tag.parse::<TagName>().map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse tag name: {e}"),
)
})?;
let tag = tag
.trim_start_matches('/')
.trim_end_matches('/')
Comment on lines +125 to +126
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant due to the split and the filter. You'll never get slashes here.
BTW, just FYI, trim_matches trims from both sides

.parse::<TagName>()
.map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse tag name: {e}"),
)
})?;
trace!(target: "app::handle", "parsed tag name: `{tag}`");
assert_eq!(extensions.insert(tag), None, "duplicate tag name");

Expand All @@ -130,12 +146,15 @@ pub async fn handle(mut req: Request<Body>) -> impl IntoResponse {
};
}

let path = tail.next().unwrap_or("").parse::<TreePath>().map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse tree path: {e}"),
)
})?;
let path = tail
.map(TreeName::from_str)
.collect::<Result<TreePath, _>>()
.map_err(|e| {
(
StatusCode::BAD_REQUEST,
format!("Failed to parse tree path: {e}"),
)
})?;
trace!(target: "app::handle", "parsed tree path: `{path}`");
assert_eq!(extensions.insert(path), None, "duplicate tree path");
match *req.method() {
Expand All @@ -154,3 +173,58 @@ pub async fn handle(mut req: Request<Body>) -> impl IntoResponse {
)),
}
}

#[async_std::test]
async fn multiple_slashes_missing() {
let request = Request::builder()
.uri("https://www.rust-lang.org/")
.header("User-Agent", "my-awesome-agent/1.0")
.body(hyper::Body::empty());
println!("{:?}", request);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant print statement


let res = handle(request.unwrap()).await;

// Temporary print to ensure test is working as intended.
// println!("{}", res.into_response().status());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid commented-out code

assert_eq!(res.into_response().status(), 404);

let request = Request::builder()
.uri("https://www.rust-lang.org///")
.header("User-Agent", "my-awesome-agent///1.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary

.body(hyper::Body::empty());
println!("{:?}", request);

let res = handle(request.unwrap()).await;

// Temporary print to ensure test is working as intended.
// println!("{}", res.into_response().status());
assert_eq!(res.into_response().status(), 404);
return ();
}

/// Unit test to handle multiple slash path parsing
#[async_std::test]
async fn multiple_slashes_found() {
let request = Request::builder()
.uri("http://httpbin.org/ip")
.body(hyper::Body::empty());
println!("{:?}", request);

let res = handle(request.unwrap()).await;

// Temporary print to ensure test is working as intended.
// println!("{}", res.into_response().status());
assert_eq!(res.into_response().status(), 404);

let request = Request::builder()
.uri("http://httpbin.org///ip")
.body(hyper::Body::empty());
println!("{:?}", request);

let res = handle(request.unwrap()).await;

// Temporary print to ensure test is working as intended.
// println!("{}", res.into_response().status());
assert_eq!(res.into_response().status(), 404);
return ();
}
Comment on lines +176 to +230
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to have unit tests in this file, they have to reside in mod tests, see https://doc.rust-lang.org/book/ch11-01-writing-tests.html

It could also be a better idea to write a test for this in https://github.com/profianinc/drawbridge/blob/f03fb5815f28f5196ff4c93c4137791dc6aab67b/tests/mod.rs instead.
We could just add a few direct URL calls there, something like https://github.com/profianinc/drawbridge/blob/1871d2a7f2519c835818e33a9ca6074e67109006/crates/app/tests/helpers/tree.rs, except with additional slashes.

Basically we need to just test at least one URL, something like: https://store.profian.com//api////v0.2.0////examples//////fibonacci-rust////_tag///0.2.0//tree//PATH/////IN/TREE

if that works, we can assume that everything works.
This could be added to the end of the integration test, where the tree already exists.