diff --git a/src/config/server.rs b/src/config/server.rs index cbd785040c..84e61b64bf 100644 --- a/src/config/server.rs +++ b/src/config/server.rs @@ -8,6 +8,7 @@ use crate::Env; use super::base::Base; use super::database_pools::DatabasePools; use crate::config::balance_capacity::BalanceCapacityConfig; +use crate::middleware::cargo_compat::StatusCodeConfig; use crate::storage::StorageConfig; use crates_io_env_vars::{required_var, var, var_parsed}; use http::HeaderValue; @@ -56,6 +57,10 @@ pub struct Server { pub cdn_user_agent: String, pub balance_capacity: BalanceCapacityConfig, + /// Instructs the `cargo_compat` middleware whether to adjust response + /// status codes to `200 OK` for all endpoints that are relevant for cargo. + pub cargo_compat_status_code_config: StatusCodeConfig, + /// Should the server serve the frontend assets in the `dist` directory? pub serve_dist: bool, @@ -221,6 +226,8 @@ impl Server { cdn_user_agent: var("WEB_CDN_USER_AGENT")? .unwrap_or_else(|| "Amazon CloudFront".into()), balance_capacity: BalanceCapacityConfig::from_environment()?, + cargo_compat_status_code_config: var_parsed("CARGO_COMPAT_STATUS_CODES")? + .unwrap_or(StatusCodeConfig::AdjustAll), serve_dist: true, serve_html: true, content_security_policy: Some(content_security_policy.parse()?), diff --git a/src/middleware.rs b/src/middleware.rs index 22a787b1a4..803c7f8368 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -1,7 +1,7 @@ pub mod app; mod balance_capacity; mod block_traffic; -mod cargo_compat; +pub mod cargo_compat; mod common_headers; mod debug; mod ember_html; @@ -63,7 +63,10 @@ pub fn apply_axum_middleware(state: AppState, router: Router<()>) -> Router { })); let middlewares_2 = tower::ServiceBuilder::new() - .layer(from_fn(cargo_compat::middleware)) + .layer(from_fn_with_state( + state.config.cargo_compat_status_code_config, + cargo_compat::middleware, + )) .layer(from_fn_with_state(state.clone(), session::attach_session)) .layer(from_fn_with_state( state.clone(), diff --git a/src/middleware/cargo_compat.rs b/src/middleware/cargo_compat.rs index a97991a322..0a3a6bc440 100644 --- a/src/middleware/cargo_compat.rs +++ b/src/middleware/cargo_compat.rs @@ -1,11 +1,41 @@ -use axum::extract::{MatchedPath, Request}; +use axum::extract::{MatchedPath, Request, State}; use axum::middleware::Next; use axum::response::{IntoResponse, Response}; use axum::{Extension, Json}; use http::{header, Method, StatusCode}; +use std::str::FromStr; + +#[derive(Clone, Copy, Debug)] +pub enum StatusCodeConfig { + /// Use the original response status code that the backend returned. + Disabled, + /// Use `200 OK` for all responses to cargo-relevant endpoints. + AdjustAll, + /// Use `200 OK` for all `2xx` responses to cargo-relevant endpoints, and + /// the original status code for all other responses. + AdjustSuccess, +} + +#[derive(Debug, thiserror::Error)] +#[error("Failed to parse StatusCodeConfig")] +pub struct StatusCodeConfigError; + +impl FromStr for StatusCodeConfig { + type Err = StatusCodeConfigError; + + fn from_str(s: &str) -> Result { + match s { + "disabled" => Ok(Self::Disabled), + "adjust-all" => Ok(Self::AdjustAll), + "adjust-success" => Ok(Self::AdjustSuccess), + _ => Err(StatusCodeConfigError), + } + } +} /// Convert plain text errors into JSON errors and adjust status codes. pub async fn middleware( + State(config): State, matched_path: Option>, req: Request, next: Next, @@ -30,7 +60,13 @@ pub async fn middleware( // 2xx status code. This will change with cargo 1.76.0 (see https://github.com/rust-lang/cargo/pull/13158), // but for backwards compatibility we still return "200 OK" for now for // all endpoints that are relevant for cargo. - *res.status_mut() = StatusCode::OK; + let adjust_status_code = matches!( + (config, res.status().is_success()), + (StatusCodeConfig::AdjustAll, _) | (StatusCodeConfig::AdjustSuccess, true) + ); + if adjust_status_code { + *res.status_mut() = StatusCode::OK; + } } res @@ -95,7 +131,7 @@ async fn convert_to_json_response(res: Response) -> anyhow::Result { mod tests { use super::*; use axum::body::Body; - use axum::middleware::from_fn; + use axum::middleware::from_fn_with_state; use axum::routing::{get, put}; use axum::Router; use bytes::Bytes; @@ -121,7 +157,7 @@ mod tests { "/api/v1/crates/:crate_id/owners", get(|| async { StatusCode::INTERNAL_SERVER_ERROR }), ) - .layer(from_fn(middleware)) + .layer(from_fn_with_state(StatusCodeConfig::AdjustAll, middleware)) } async fn request(path: &str) -> anyhow::Result<(Parts, Bytes)> { diff --git a/src/tests/krate/publish/auth.rs b/src/tests/krate/publish/auth.rs index 52f3beb18f..4fb2894a79 100644 --- a/src/tests/krate/publish/auth.rs +++ b/src/tests/krate/publish/auth.rs @@ -13,7 +13,7 @@ fn new_wrong_token() { // Try to publish without a token let crate_to_publish = PublishBuilder::new("foo", "1.0.0"); let response = anon.publish_crate(crate_to_publish); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) @@ -29,7 +29,7 @@ fn new_wrong_token() { let crate_to_publish = PublishBuilder::new("foo", "1.0.0"); let response = token.publish_crate(crate_to_publish); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) diff --git a/src/tests/owners.rs b/src/tests/owners.rs index fcfa17c965..f9f8ceab3d 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -366,7 +366,7 @@ fn owner_change_via_change_owner_token_with_wrong_crate_scope() { let body = json!({ "owners": [user2.gh_login] }); let body = serde_json::to_vec(&body).unwrap(); let response = token.put::<()>(&url, body); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) @@ -388,7 +388,7 @@ fn owner_change_via_publish_token() { let body = json!({ "owners": [user2.gh_login] }); let body = serde_json::to_vec(&body).unwrap(); let response = token.put::<()>(&url, body); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) @@ -409,7 +409,7 @@ fn owner_change_without_auth() { let body = json!({ "owners": [user2.gh_login] }); let body = serde_json::to_vec(&body).unwrap(); let response = anon.put::<()>(&url, body); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) diff --git a/src/tests/pagination.rs b/src/tests/pagination.rs index a16ad07ce6..6a6228a130 100644 --- a/src/tests/pagination.rs +++ b/src/tests/pagination.rs @@ -21,7 +21,7 @@ fn pagination_blocks_ip_from_cidr_block_list() { }); let response = anon.get_with_query::<()>("/api/v1/crates", "page=2&per_page=1"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "requested page offset is too large" }] }) diff --git a/src/tests/read_only_mode.rs b/src/tests/read_only_mode.rs index 153033aefa..057b8b728d 100644 --- a/src/tests/read_only_mode.rs +++ b/src/tests/read_only_mode.rs @@ -32,7 +32,7 @@ fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() { }); let response = token.delete::<()>("/api/v1/crates/foo_yank_read_only/1.0.0/yank"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::SERVICE_UNAVAILABLE); assert_json_snapshot!(response.into_json()); } diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index d5ac3fb24e..c3d392e7d5 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -799,7 +799,7 @@ fn invalid_seek_parameter() { let (_app, anon, _cookie) = TestApp::init().with_user(); let response = anon.get::<()>("/api/v1/crates?seek=broken"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_json_snapshot!(response.into_json()); } @@ -816,7 +816,7 @@ fn pagination_parameters_only_accept_integers() { let response = anon.get_with_query::<()>("/api/v1/crates", "page=1&per_page=100%22%EF%BC%8Cexception"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "invalid digit found in string" }] }) @@ -824,7 +824,7 @@ fn pagination_parameters_only_accept_integers() { let response = anon.get_with_query::<()>("/api/v1/crates", "page=100%22%EF%BC%8Cexception&per_page=1"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "invalid digit found in string" }] }) diff --git a/src/tests/routes/crates/versions/yank_unyank.rs b/src/tests/routes/crates/versions/yank_unyank.rs index cbf113e1fd..892e64a26e 100644 --- a/src/tests/routes/crates/versions/yank_unyank.rs +++ b/src/tests/routes/crates/versions/yank_unyank.rs @@ -115,14 +115,14 @@ mod auth { let (_, client, _) = prepare(); let response = client.yank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) ); let response = client.unyank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) @@ -182,14 +182,14 @@ mod auth { client.db_new_scoped_token("test-token", None, None, Some(expired_at.naive_utc())); let response = client.yank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) ); let response = client.unyank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) @@ -222,14 +222,14 @@ mod auth { ); let response = client.yank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) ); let response = client.unyank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) @@ -286,14 +286,14 @@ mod auth { ); let response = client.yank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) ); let response = client.unyank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) @@ -311,14 +311,14 @@ mod auth { ); let response = client.yank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) ); let response = client.unyank(CRATE_NAME, CRATE_VERSION); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.into_json(), json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) diff --git a/src/tests/server.rs b/src/tests/server.rs index cea0dd9352..2786f145da 100644 --- a/src/tests/server.rs +++ b/src/tests/server.rs @@ -11,7 +11,7 @@ fn user_agent_is_required() { let req = Request::get("/api/v1/crates").body("").unwrap(); let resp = anon.run::<()>(req); - assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); assert_json_snapshot!(resp.into_json()); let req = Request::get("/api/v1/crates") @@ -19,7 +19,7 @@ fn user_agent_is_required() { .body("") .unwrap(); let resp = anon.run::<()>(req); - assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); assert_json_snapshot!(resp.into_json()); } @@ -101,6 +101,6 @@ fn block_traffic_via_ip() { .empty(); let resp = anon.get::<()>("/api/v1/crates"); - assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); assert_json_snapshot!(resp.into_json()); } diff --git a/src/tests/util/response.rs b/src/tests/util/response.rs index 03ae0d5351..8ccbd7199a 100644 --- a/src/tests/util/response.rs +++ b/src/tests/util/response.rs @@ -75,7 +75,7 @@ impl Response { detail: String, } - assert_eq!(self.status(), StatusCode::OK); + assert_eq!(self.status(), StatusCode::TOO_MANY_REQUESTS); let expected_message_start = format!("{}. Please try again after ", action.error_message()); let error: ErrorResponse = json(self.response); diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index 114205d764..daed04e0fc 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -3,6 +3,7 @@ use crate::util::chaosproxy::ChaosProxy; use crate::util::github::{MockGitHubClient, MOCK_GITHUB_DATA}; use anyhow::Context; use crates_io::config::{self, BalanceCapacityConfig, Base, DatabasePools, DbPoolConfig}; +use crates_io::middleware::cargo_compat::StatusCodeConfig; use crates_io::models::token::{CrateScope, EndpointScope}; use crates_io::rate_limiter::{LimitedAction, RateLimiterConfig}; use crates_io::storage::StorageConfig; @@ -428,6 +429,11 @@ fn simple_config() -> config::Server { cdn_user_agent: "Amazon CloudFront".to_string(), balance_capacity, + // The middleware has its own unit tests to verify its functionality. + // Here, we can test what would happen if we toggled the status code + // enforcement off eventually. + cargo_compat_status_code_config: StatusCodeConfig::Disabled, + // The frontend code is not needed for the backend tests. serve_dist: false, serve_html: false,