From d5d629b96f09f4255bf6671e6539663b47b3f22c Mon Sep 17 00:00:00 2001 From: Wim Looman Date: Wed, 31 Aug 2022 18:30:55 +0200 Subject: [PATCH] Assert that redirects go directly to their target location Previously the assertion allowed multiple redirect steps as long as it eventually got to the target. Now it's checked that the very first response directly returns a `Location` header to the final target. --- Cargo.lock | 12 ++++ Cargo.toml | 1 + src/test/mod.rs | 122 +++++++++++++++++++++++++-------------- src/web/crate_details.rs | 19 ++++-- src/web/metrics.rs | 6 ++ src/web/mod.rs | 6 +- src/web/releases.rs | 40 +++++-------- src/web/routes.rs | 16 +++++ src/web/rustdoc.rs | 30 +++++----- 9 files changed, 161 insertions(+), 91 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ba15b2994..3fe9dc280 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -999,6 +999,7 @@ dependencies = [ "dotenv", "env_logger", "failure", + "fn-error-context", "font-awesome-as-a-crate", "futures-util", "getrandom 0.2.7", @@ -1207,6 +1208,17 @@ dependencies = [ "miniz_oxide", ] +[[package]] +name = "fn-error-context" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "236b4e4ae2b8be5f7a5652f6108c4a0f2627c569db4e7923333d31c7dbfed0fb" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "fnv" version = "1.0.7" diff --git a/Cargo.toml b/Cargo.toml index 43071d5ef..c725b10ec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -113,6 +113,7 @@ kuchiki = "0.8" rand = "0.8" mockito = "0.31.0" test-case = "2.0.0" +fn-error-context = "0.2.0" [build-dependencies] time = "0.3" diff --git a/src/test/mod.rs b/src/test/mod.rs index 0b51f8787..1e67ce5de 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -7,16 +7,16 @@ use crate::repositories::RepositoryStatsUpdater; use crate::storage::{Storage, StorageKind}; use crate::web::Server; use crate::{BuildQueue, Config, Context, Index, Metrics}; +use anyhow::Context as _; +use fn_error_context::context; use log::error; use once_cell::unsync::OnceCell; use postgres::Client as Connection; use reqwest::{ - blocking::{Client, RequestBuilder}, + blocking::{Client, ClientBuilder, RequestBuilder}, Method, }; -use std::fs; -use std::net::SocketAddr; -use std::{panic, sync::Arc}; +use std::{fs, net::SocketAddr, panic, sync::Arc, time::Duration}; pub(crate) fn wrapper(f: impl FnOnce(&TestEnvironment) -> Result<()>) { let _ = dotenv::dotenv(); @@ -56,45 +56,57 @@ pub(crate) fn assert_not_found(path: &str, web: &TestFrontend) -> Result<()> { Ok(()) } -/// Make sure that a URL redirects to a specific page -pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> { - // Reqwest follows redirects automatically - let response = web.get(path).send()?; +fn assert_redirect_common(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> { + let response = web.get_no_redirect(path).send()?; let status = response.status(); + if !status.is_redirection() { + anyhow::bail!("non-redirect from GET {path}: {status}"); + } + + let mut redirect_target = response + .headers() + .get("Location") + .context("missing 'Location' header")? + .to_str() + .context("non-ASCII redirect")?; + + if !expected_target.starts_with("http") { + // TODO: Should be able to use Url::make_relative, + // but https://github.com/servo/rust-url/issues/766 + let base = format!("http://{}", web.server_addr()); + redirect_target = redirect_target + .strip_prefix(&base) + .unwrap_or(redirect_target); + } - let mut tmp; - let redirect_target = if expected_target.starts_with("https://") { - response.url().as_str() - } else { - tmp = String::from(response.url().path()); - if let Some(query) = response.url().query() { - tmp.push('?'); - tmp.push_str(query); - } - &tmp - }; - // Either we followed a redirect to the wrong place, or there was no redirect if redirect_target != expected_target { - // wrong place - if redirect_target != path { - panic!( - "{}: expected redirect to {}, got redirect to {}", - path, expected_target, redirect_target - ); - } else { - // no redirect - panic!( - "{}: expected redirect to {}, got {}", - path, expected_target, status - ); - } + anyhow::bail!("got redirect to {redirect_target}"); } - assert!( - status.is_success(), - "failed to GET {}: {}", - expected_target, - status - ); + + Ok(()) +} + +/// Makes sure that a URL redirects to a specific page, but doesn't check that the target exists +#[context("expected redirect from {path} to {expected_target}")] +pub(crate) fn assert_redirect_unchecked( + path: &str, + expected_target: &str, + web: &TestFrontend, +) -> Result<()> { + assert_redirect_common(path, expected_target, web) +} + +/// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect +#[context("expected redirect from {path} to {expected_target}")] +pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> { + assert_redirect_common(path, expected_target, web)?; + + let response = web.get_no_redirect(expected_target).send()?; + let status = response.status(); + if !status.is_success() { + anyhow::bail!("failed to GET {expected_target}: {status}"); + } + Ok(()) } @@ -113,6 +125,7 @@ pub(crate) fn init_logger() { // initializing rustwide logging also sets the global logger rustwide::logging::init_with( env_logger::Builder::from_env(env_logger::Env::default().filter("DOCSRS_LOG")) + .format_timestamp_millis() .is_test(true) .build(), ); @@ -372,22 +385,35 @@ impl Drop for TestDatabase { pub(crate) struct TestFrontend { server: Server, pub(crate) client: Client, + pub(crate) client_no_redirect: Client, } impl TestFrontend { fn new(context: &dyn Context) -> Self { + fn build(f: impl FnOnce(ClientBuilder) -> ClientBuilder) -> Client { + let base = Client::builder() + .connect_timeout(Duration::from_millis(2000)) + .timeout(Duration::from_millis(2000)) + // The test server only supports a single connection, so having two clients with + // idle connections deadlocks the tests + .pool_max_idle_per_host(0); + f(base).build().unwrap() + } + Self { server: Server::start(Some("127.0.0.1:0"), context) .expect("failed to start the web server"), - client: Client::new(), + client: build(|b| b), + client_no_redirect: build(|b| b.redirect(reqwest::redirect::Policy::none())), } } - fn build_request(&self, method: Method, mut url: String) -> RequestBuilder { + fn build_url(&self, url: &str) -> String { if url.is_empty() || url.starts_with('/') { - url = format!("http://{}{}", self.server.addr(), url); + format!("http://{}{}", self.server.addr(), url) + } else { + url.to_owned() } - self.client.request(method, url) } pub(crate) fn server_addr(&self) -> SocketAddr { @@ -395,6 +421,14 @@ impl TestFrontend { } pub(crate) fn get(&self, url: &str) -> RequestBuilder { - self.build_request(Method::GET, url.to_string()) + let url = self.build_url(url); + log::debug!("getting {url}"); + self.client.request(Method::GET, url) + } + + pub(crate) fn get_no_redirect(&self, url: &str) -> RequestBuilder { + let url = self.build_url(url); + log::debug!("getting {url} (no redirects)"); + self.client_no_redirect.request(Method::GET, url) } } diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 890b7d080..2afe1c468 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -76,6 +76,7 @@ pub struct Release { pub yanked: bool, pub is_library: bool, pub rustdoc_status: bool, + pub target_name: String, } impl CrateDetails { @@ -246,15 +247,16 @@ pub(crate) fn releases_for_crate( ) -> Result, anyhow::Error> { let mut releases: Vec = conn .query( - "SELECT - id, + "SELECT + id, version, build_status, yanked, is_library, - rustdoc_status + rustdoc_status, + target_name FROM releases - WHERE + WHERE releases.crate_id = $1", &[&crate_id], )? @@ -269,6 +271,7 @@ pub(crate) fn releases_for_crate( yanked: row.get("yanked"), is_library: row.get("is_library"), rustdoc_status: row.get("rustdoc_status"), + target_name: row.get("target_name"), }), Err(err) => { report_error(&anyhow!(err).context(format!( @@ -505,6 +508,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[0].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.12.0")?, @@ -513,6 +517,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[1].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.3.0")?, @@ -521,6 +526,7 @@ mod tests { is_library: true, rustdoc_status: false, id: details.releases[2].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.2.0")?, @@ -529,6 +535,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[3].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.2.0-alpha")?, @@ -537,6 +544,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[4].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.1.1")?, @@ -545,6 +553,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[5].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.1.0")?, @@ -553,6 +562,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[6].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.0.1")?, @@ -561,6 +571,7 @@ mod tests { is_library: false, rustdoc_status: false, id: details.releases[7].id, + target_name: "foo".to_owned(), }, ] ); diff --git a/src/web/metrics.rs b/src/web/metrics.rs index fc3d677da..7ba1794d6 100644 --- a/src/web/metrics.rs +++ b/src/web/metrics.rs @@ -92,6 +92,12 @@ impl<'a> RenderingTimesRecorder<'a> { fn record_current(&mut self) { if let Some(current) = self.current.take() { + #[cfg(test)] + log::debug!( + "rendering time - {}: {:?}", + current.step, + current.start.elapsed() + ); self.metric .with_label_values(&[current.step]) .observe(duration_to_seconds(current.start.elapsed())); diff --git a/src/web/mod.rs b/src/web/mod.rs index ff9cf0cd1..b9027f7de 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -223,6 +223,7 @@ struct MatchVersion { pub corrected_name: Option, pub version: MatchSemver, pub rustdoc_status: bool, + pub target_name: String, } impl MatchVersion { @@ -324,6 +325,7 @@ fn match_version( corrected_name, version: MatchSemver::Exact((release.version.to_string(), release.id)), rustdoc_status: release.rustdoc_status, + target_name: release.target_name.clone(), }); } } @@ -358,6 +360,7 @@ fn match_version( MatchSemver::Semver((release.version.to_string(), release.id)) }, rustdoc_status: release.rustdoc_status, + target_name: release.target_name.clone(), }); } @@ -371,6 +374,7 @@ fn match_version( corrected_name: corrected_name.clone(), version: MatchSemver::Semver((release.version.to_string(), release.id)), rustdoc_status: release.rustdoc_status, + target_name: release.target_name.clone(), }) .ok_or(Nope::VersionNotFound); } @@ -759,7 +763,7 @@ mod test { .create() .unwrap(); let web = env.frontend(); - assert_redirect("/bat//", "/bat/latest/bat/", web)?; + assert_redirect_unchecked("/bat//", "/bat/", web)?; Ok(()) }) } diff --git a/src/web/releases.rs b/src/web/releases.rs index a5a734df5..c74652169 100644 --- a/src/web/releases.rs +++ b/src/web/releases.rs @@ -79,7 +79,7 @@ pub(crate) fn get_releases(conn: &mut Client, page: i64, limit: i64, order: Orde INNER JOIN builds ON releases.id = builds.rid LEFT JOIN repositories ON releases.repository_id = repositories.id WHERE - ((NOT $3) OR (releases.build_status = FALSE AND releases.is_library = TRUE)) + ((NOT $3) OR (releases.build_status = FALSE AND releases.is_library = TRUE)) AND {0} IS NOT NULL ORDER BY {0} DESC @@ -246,7 +246,7 @@ fn get_search_results( releases.rustdoc_status, repositories.stars - FROM crates + FROM crates INNER JOIN releases ON crates.latest_version_id = releases.id INNER JOIN builds ON releases.id = builds.rid LEFT JOIN repositories ON releases.repository_id = repositories.id @@ -498,7 +498,7 @@ fn redirect_to_random_crate(req: &Request, conn: &mut PoolClient) -> IronResult< releases.version, releases.target_name FROM ( - -- generate random numbers in the ID-range. + -- generate random numbers in the ID-range. SELECT DISTINCT 1 + trunc(random() * params.max_id)::INTEGER AS id FROM params, generate_series(1, $1) ) AS r @@ -573,27 +573,15 @@ pub fn search_handler(req: &mut Request) -> IronResult { let (version, _) = matchver.version.into_parts(); let krate = matchver.corrected_name.unwrap_or(krate); + let base = redirect_base(req); let url = if matchver.rustdoc_status { + let target_name = matchver.target_name; ctry!( req, - Url::parse(&format!( - "{}/{}/{}/{}", - redirect_base(req), - krate, - version, - query - )), + Url::parse(&format!("{base}/{krate}/{version}/{target_name}/{query}")) ) } else { - ctry!( - req, - Url::parse(&format!( - "{}/crate/{}/{}", - redirect_base(req), - krate, - version, - )), - ) + ctry!(req, Url::parse(&format!("{base}/crate/{krate}/{version}"))) }; let mut resp = Response::with((status::Found, Redirect(url))); @@ -688,12 +676,12 @@ pub fn activity_handler(req: &mut Request) -> IronResult { " WITH dates AS ( -- we need this series so that days in the statistic that don't have any releases are included - SELECT generate_series( + SELECT generate_series( CURRENT_DATE - INTERVAL '30 days', CURRENT_DATE - INTERVAL '1 day', '1 day'::interval )::date AS date_ - ), + ), release_stats AS ( SELECT release_time::date AS date_, @@ -706,16 +694,16 @@ pub fn activity_handler(req: &mut Request) -> IronResult { release_time < CURRENT_DATE GROUP BY release_time::date - ) - SELECT + ) + SELECT dates.date_ AS date, COALESCE(rs.counts, 0) AS counts, - COALESCE(rs.failures, 0) AS failures + COALESCE(rs.failures, 0) AS failures FROM - dates + dates LEFT OUTER JOIN Release_stats AS rs ON dates.date_ = rs.date_ - ORDER BY + ORDER BY dates.date_ ", &[], diff --git a/src/web/routes.rs b/src/web/routes.rs index 5c666f8e8..5d1f74126 100644 --- a/src/web/routes.rs +++ b/src/web/routes.rs @@ -173,22 +173,38 @@ pub(super) fn build_routes() -> Routes { &format!("/{}", redirect), super::rustdoc::RustLangRedirector::new("stable", redirect), ); + routes.internal_page( + &format!("/{}/", redirect), + super::rustdoc::RustLangRedirector::new("stable", redirect), + ); } // redirect proc-macro to proc_macro routes.internal_page( "/proc-macro", super::rustdoc::RustLangRedirector::new("stable", "proc_macro"), ); + routes.internal_page( + "/proc-macro/", + super::rustdoc::RustLangRedirector::new("stable", "proc_macro"), + ); // redirect rustc to nightly rustc docs routes.internal_page( "/rustc", super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc"), ); + routes.internal_page( + "/rustc/", + super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc"), + ); // redirect rustdoc to nightly rustdoc docs routes.internal_page( "/rustdoc", super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc/rustdoc"), ); + routes.internal_page( + "/rustdoc/", + super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc/rustdoc"), + ); routes } diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index b9e8bd594..53b873393 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -20,7 +20,7 @@ use iron::{ use lol_html::errors::RewritingError; use router::Router; use serde::Serialize; -use std::path::Path; +use std::{fmt::Write, path::Path}; #[derive(Clone)] pub struct RustLangRedirector { @@ -29,9 +29,8 @@ pub struct RustLangRedirector { impl RustLangRedirector { pub fn new(version: &str, target: &str) -> Self { - let url = - iron::url::Url::parse(&format!("https://doc.rust-lang.org/{}/{}", version, target)) - .expect("failed to parse rust-lang.org doc URL"); + let url = iron::url::Url::parse(&format!("https://doc.rust-lang.org/{version}/{target}/")) + .expect("failed to parse rust-lang.org doc URL"); let url = Url::from_generic_url(url).expect("failed to convert url::Url to iron::Url"); Self { url } @@ -547,8 +546,8 @@ pub fn rustdoc_html_server_handler(req: &mut Request) -> IronResult { /// Note that path is overloaded in this context to mean both the path of a URL /// and the file path of a static file in the DB. /// -/// `req_path` is assumed to have the following format: -/// `rustdoc/crate/version[/platform]/module/[kind.name.html|index.html]` +/// `file_path` is assumed to have the following format: +/// `[/platform]/module/[kind.name.html|index.html]` /// /// Returns a path that can be appended to `/crate/version/` to create a complete URL. fn path_for_version(file_path: &[&str], crate_details: &CrateDetails) -> String { @@ -589,11 +588,16 @@ fn path_for_version(file_path: &[&str], crate_details: &CrateDetails) -> String // else, don't try searching at all, we don't know how to find it last_component.strip_suffix(".rs.html") }; - if let Some(search) = search_item { - format!("{}?search={}", platform, search) + let target_name = &crate_details.target_name; + let mut result = if platform.is_empty() { + format!("{target_name}/") } else { - platform.to_owned() + format!("{platform}/{target_name}/") + }; + if let Some(search) = search_item { + write!(result, "?search={search}").unwrap(); } + result } pub fn target_redirect_handler(req: &mut Request) -> IronResult { @@ -663,13 +667,7 @@ pub fn target_redirect_handler(req: &mut Request) -> IronResult { path_for_version(&file_path, &crate_details) }; - let url = format!( - "{base}/{name}/{version_or_latest}/{path}", - base = base, - name = name, - version_or_latest = version_or_latest, - path = path - ); + let url = format!("{base}/{name}/{version_or_latest}/{path}"); let url = ctry!(req, Url::parse(&url)); let mut resp = Response::with((status::Found, Redirect(url)));