diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index c7116862a..6628a89cb 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -1,4 +1,3 @@ -use super::error::Nope; use super::{match_version, redirect_base, render_markdown, MatchSemver, MetaData}; use crate::{db::Pool, impl_webpage, web::page::WebPage}; use chrono::{DateTime, NaiveDateTime, Utc}; @@ -297,13 +296,13 @@ pub fn crate_details_handler(req: &mut Request) -> IronResult { let mut conn = extension!(req, Pool).get()?; match match_version(&mut conn, &name, req_version).and_then(|m| m.assume_exact()) { - Some(MatchSemver::Exact((version, _))) => { + Ok(MatchSemver::Exact((version, _))) => { let details = cexpect!(req, CrateDetails::new(&mut conn, &name, &version)); CrateDetailsPage { details }.into_response(req) } - Some(MatchSemver::Semver((version, _))) => { + Ok(MatchSemver::Semver((version, _))) => { let url = ctry!( req, Url::parse(&format!( @@ -317,7 +316,7 @@ pub fn crate_details_handler(req: &mut Request) -> IronResult { Ok(super::redirect(url)) } - None => Err(IronError::new(Nope::CrateNotFound, status::NotFound)), + Err(err) => Err(IronError::new(err, status::NotFound)), } } diff --git a/src/web/error.rs b/src/web/error.rs index b039d683a..2b1ed0681 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -10,6 +10,7 @@ use std::{error::Error, fmt}; pub enum Nope { ResourceNotFound, CrateNotFound, + VersionNotFound, NoResults, InternalServerError, } @@ -19,6 +20,7 @@ impl fmt::Display for Nope { f.write_str(match *self { Nope::ResourceNotFound => "Requested resource not found", Nope::CrateNotFound => "Requested crate not found", + Nope::VersionNotFound => "Requested crate does not have specified version", Nope::NoResults => "Search yielded no results", Nope::InternalServerError => "Internal server error", }) @@ -52,6 +54,17 @@ impl Handler for Nope { .into_response(req) } + Nope::VersionNotFound => { + // user tried to navigate to a crate with a version that does not exist + // TODO: Display the attempted crate and version + ErrorPage { + title: "The requested version does not exist", + message: Some("no such version for this crate".into()), + status: Status::NotFound, + } + .into_response(req) + } + Nope::NoResults => { let mut params = req.url.as_ref().query_pairs(); @@ -100,11 +113,37 @@ mod tests { use kuchiki::traits::TendrilSink; #[test] - fn check_404_page_content() { + fn check_404_page_content_crate() { + wrapper(|env| { + let page = kuchiki::parse_html().one( + env.frontend() + .get("/crate-which-doesnt-exist") + .send()? + .text()?, + ); + assert_eq!(page.select("#crate-title").unwrap().count(), 1); + assert_eq!( + page.select("#crate-title") + .unwrap() + .next() + .unwrap() + .text_contents(), + "The requested crate does not exist", + ); + + Ok(()) + }); + } + + #[test] + fn check_404_page_content_resource() { + // Resources with a `.js` and `.ico` extension are special cased in the + // routes_handler which is currently run last. This means that `get("resource.exe")` will + // fail with a `no so such crate` instead of 'no such resource' wrapper(|env| { let page = kuchiki::parse_html().one( env.frontend() - .get("/page-which-doesnt-exist") + .get("/resource-which-doesnt-exist.js") .send()? .text()?, ); @@ -121,4 +160,66 @@ mod tests { Ok(()) }); } + + #[test] + fn check_404_page_content_not_semver_version() { + wrapper(|env| { + env.fake_release().name("dummy").create()?; + let page = + kuchiki::parse_html().one(env.frontend().get("/dummy/not-semver").send()?.text()?); + assert_eq!(page.select("#crate-title").unwrap().count(), 1); + assert_eq!( + page.select("#crate-title") + .unwrap() + .next() + .unwrap() + .text_contents(), + "The requested version does not exist", + ); + + Ok(()) + }); + } + + #[test] + fn check_404_page_content_nonexistent_version() { + wrapper(|env| { + env.fake_release().name("dummy").version("1.0.0").create()?; + let page = kuchiki::parse_html().one(env.frontend().get("/dummy/2.0").send()?.text()?); + assert_eq!(page.select("#crate-title").unwrap().count(), 1); + assert_eq!( + page.select("#crate-title") + .unwrap() + .next() + .unwrap() + .text_contents(), + "The requested version does not exist", + ); + + Ok(()) + }); + } + + #[test] + fn check_404_page_content_any_version_all_yanked() { + wrapper(|env| { + env.fake_release() + .name("dummy") + .version("1.0.0") + .yanked(true) + .create()?; + let page = kuchiki::parse_html().one(env.frontend().get("/dummy/*").send()?.text()?); + assert_eq!(page.select("#crate-title").unwrap().count(), 1); + assert_eq!( + page.select("#crate-title") + .unwrap() + .next() + .unwrap() + .text_contents(), + "The requested version does not exist", + ); + + Ok(()) + }); + } } diff --git a/src/web/mod.rs b/src/web/mod.rs index 90dbb3205..2c68f1110 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -91,6 +91,7 @@ mod statics; use crate::{impl_webpage, Context}; use chrono::{DateTime, Utc}; +use error::Nope; use extensions::InjectExtensions; use failure::Error; use iron::{ @@ -177,13 +178,14 @@ impl Handler for CratesfyiHandler { } }; - // try serving shared rustdoc resources first, then router, then db/static file handler - // return 404 if none of them return Ok + // try serving shared rustdoc resources first, then db/static file handler and last router + // return 404 if none of them return Ok. It is important that the router comes last, + // because it gives the most specific errors, e.g. CrateNotFound or VersionNotFound self.shared_resource_handler .handle(req) - .or_else(|e| if_404(e, || self.router_handler.handle(req))) .or_else(|e| if_404(e, || self.database_file_handler.handle(req))) .or_else(|e| if_404(e, || self.static_handler.handle(req))) + .or_else(|e| if_404(e, || self.router_handler.handle(req))) .or_else(|e| { let err = if let Some(err) = e.error.downcast::() { *err @@ -245,12 +247,12 @@ struct MatchVersion { impl MatchVersion { /// If the matched version was an exact match to the requested crate name, returns the /// `MatchSemver` for the query. If the lookup required a dash/underscore conversion, returns - /// `None`. - fn assume_exact(self) -> Option { + /// `CrateNotFound`. + fn assume_exact(self) -> Result { if self.corrected_name.is_none() { - Some(self.version) + Ok(self.version) } else { - None + Err(Nope::CrateNotFound) } } } @@ -284,7 +286,11 @@ impl MatchSemver { /// This function will also check for crates where dashes in the name (`-`) have been replaced with /// underscores (`_`) and vice-versa. The return value will indicate whether the crate name has /// been matched exactly, or if there has been a "correction" in the name that matched instead. -fn match_version(conn: &mut Client, name: &str, version: Option<&str>) -> Option { +fn match_version( + conn: &mut Client, + name: &str, + version: Option<&str>, +) -> Result { // version is an Option<&str> from router::Router::get, need to decode first use iron::url::percent_encoding::percent_decode; @@ -320,24 +326,37 @@ fn match_version(conn: &mut Client, name: &str, version: Option<&str>) -> Option .collect() }; + if versions.is_empty() { + return Err(Nope::CrateNotFound); + } + // first check for exact match, we can't expect users to use semver in query if let Some((version, id, _)) = versions.iter().find(|(vers, _, _)| vers == &req_version) { - return Some(MatchVersion { + return Ok(MatchVersion { corrected_name, version: MatchSemver::Exact((version.to_owned(), *id)), }); } // Now try to match with semver - let req_sem_ver = VersionReq::parse(&req_version).ok()?; + let req_sem_ver = VersionReq::parse(&req_version).map_err(|_| Nope::VersionNotFound)?; // we need to sort versions first let versions_sem = { let mut versions_sem: Vec<(Version, i32)> = Vec::with_capacity(versions.len()); for version in versions.iter().filter(|(_, _, yanked)| !yanked) { - // in theory a crate must always have a semver compatible version, but check result just in case - versions_sem.push((Version::parse(&version.0).ok()?, version.1)); + // in theory a crate must always have a semver compatible version, + // but check result just in case + let version_sem = Version::parse(&version.0).map_err(|_| { + log::error!( + "invalid semver in database for crate {}: {}", + name, + version.0 + ); + Nope::InternalServerError + })?; + versions_sem.push((version_sem, version.1)); } versions_sem.sort(); @@ -349,22 +368,27 @@ fn match_version(conn: &mut Client, name: &str, version: Option<&str>) -> Option .iter() .find(|(vers, _)| req_sem_ver.matches(vers)) { - return Some(MatchVersion { + return Ok(MatchVersion { corrected_name, version: MatchSemver::Semver((version.to_string(), *id)), }); } - // semver is acting weird for '*' (any) range if a crate only have pre-release versions + // semver is acting weird for '*' (any) range if a crate only has pre-release versions // return first non-yanked version if requested version is '*' if req_version == "*" { - return versions_sem.first().map(|v| MatchVersion { - corrected_name, - version: MatchSemver::Semver((v.0.to_string(), v.1)), - }); + return versions_sem + .first() + .map(|v| MatchVersion { + corrected_name, + version: MatchSemver::Semver((v.0.to_string(), v.1)), + }) + .ok_or(Nope::VersionNotFound); } - None + // Since we return with a CrateNotFound earlier if the db reply is empty, + // we know that versions were returned but none satisfied the version requirement + Err(Nope::VersionNotFound) } /// Wrapper around the Markdown parser and renderer to render markdown @@ -580,8 +604,13 @@ mod test { } fn version(v: Option<&str>, db: &TestDatabase) -> Option { - match_version(&mut db.conn(), "foo", v) - .and_then(|version| version.assume_exact().map(|semver| semver.into_parts().0)) + let version = match_version(&mut db.conn(), "foo", v) + .ok()? + .assume_exact() + .ok()? + .into_parts() + .0; + Some(version) } fn semver(version: &'static str) -> Option { diff --git a/src/web/releases.rs b/src/web/releases.rs index 22498a8d7..cbf2803ab 100644 --- a/src/web/releases.rs +++ b/src/web/releases.rs @@ -557,7 +557,7 @@ pub fn search_handler(req: &mut Request) -> IronResult { // since we never pass a version into `match_version` here, we'll never get // `MatchVersion::Exact`, so the distinction between `Exact` and `Semver` doesn't // matter - if let Some(matchver) = match_version(&mut conn, &query, None) { + if let Ok(matchver) = match_version(&mut conn, &query, None) { let (version, id) = matchver.version.into_parts(); let query = matchver.corrected_name.unwrap_or_else(|| query.to_string()); diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index 3b205eef5..6564c0b4e 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -143,7 +143,7 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult { // it doesn't matter if the version that was given was exact or not, since we're redirecting // anyway let (version, id) = match match_version(&mut conn, &crate_name, req_version) { - Some(v) => { + Ok(v) => { if let Some(new_name) = v.corrected_name { // `match_version` checked against -/_ typos, so if we have a name here we should // use that instead @@ -151,8 +151,8 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult { } v.version.into_parts() } - None => { - return Err(IronError::new(Nope::CrateNotFound, status::NotFound)); + Err(err) => { + return Err(IronError::new(err, status::NotFound)); } }; @@ -282,32 +282,31 @@ pub fn rustdoc_html_server_handler(req: &mut Request) -> IronResult { rendering_time.step("match version"); // Check the database for releases with the requested version while doing the following: + // * If no matching releases are found, return a 404 with the underlying error + // Then: // * If both the name and the version are an exact match, return the version of the crate. // * If there is an exact match, but the requested crate name was corrected (dashes vs. underscores), redirect to the corrected name. // * If there is a semver (but not exact) match, redirect to the exact version. - // * Otherwise, return a 404. - let version = if let Some(match_vers) = match_version(&mut conn, &name, url_version) { - match match_vers.version { - MatchSemver::Exact((version, _)) => { - // Redirect when the requested crate name isn't correct - if let Some(name) = match_vers.corrected_name { - return redirect(&name, &version, &req_path); - } - - version + let release_found = match_version(&mut conn, &name, url_version) + .map_err(|err| IronError::new(err, status::NotFound))?; + + let version = match release_found.version { + MatchSemver::Exact((version, _)) => { + // Redirect when the requested crate name isn't correct + if let Some(name) = release_found.corrected_name { + return redirect(&name, &version, &req_path); } - // Redirect when the requested version isn't correct - MatchSemver::Semver((v, _)) => { - // to prevent cloudfront caching the wrong artifacts on URLs with loose semver - // versions, redirect the browser to the returned version instead of loading it - // immediately - return redirect(&name, &v, &req_path); - } + version + } + + // Redirect when the requested version isn't correct + MatchSemver::Semver((v, _)) => { + // to prevent cloudfront caching the wrong artifacts on URLs with loose semver + // versions, redirect the browser to the returned version instead of loading it + // immediately + return redirect(&name, &v, &req_path); } - } else { - // Return a 404, as a crate by that name and version doesn't exist - return Err(IronError::new(Nope::ResourceNotFound, status::NotFound)); }; rendering_time.step("crate details"); @@ -527,7 +526,7 @@ pub fn badge_handler(req: &mut Request) -> IronResult { let options = match match_version(&mut conn, &name, Some(&version)).and_then(|m| m.assume_exact()) { - Some(MatchSemver::Exact((version, id))) => { + Ok(MatchSemver::Exact((version, id))) => { let rows = ctry!( req, conn.query( @@ -552,7 +551,7 @@ pub fn badge_handler(req: &mut Request) -> IronResult { } } - Some(MatchSemver::Semver((version, _))) => { + Ok(MatchSemver::Semver((version, _))) => { let base_url = format!("{}/{}/badge.svg", redirect_base(req), name); let url = ctry!( req, @@ -562,7 +561,13 @@ pub fn badge_handler(req: &mut Request) -> IronResult { return Ok(super::redirect(iron_url)); } - None => BadgeOptions { + Err(Nope::VersionNotFound) => BadgeOptions { + subject: "docs".to_owned(), + status: "version not found".to_owned(), + color: "#e05d44".to_owned(), + }, + + Err(_) => BadgeOptions { subject: "docs".to_owned(), status: "no builds".to_owned(), color: "#e05d44".to_owned(),