diff --git a/api/src/client.rs b/api/src/client.rs index 226d07cb..33548f29 100644 --- a/api/src/client.rs +++ b/api/src/client.rs @@ -68,10 +68,12 @@ impl ApiClient { const TRUNK_TELEMETRY_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(1); const TRUNK_API_TOKEN_HEADER: &'static str = "x-api-token"; const TRUNK_PUBLIC_REPO_ID_HEADER: &'static str = "x-trunk-public-repo-id"; + const TRUNK_ALLOW_FORKED_PR_UPLOADS_HEADER: &'static str = "x-trunk-allow-forked-pr-uploads"; pub fn new( api_token: Option, public_repo_id: Option, + allow_forked_pr_uploads: bool, org_url_slug: impl AsRef, render_sender: Option>, ) -> anyhow::Result { @@ -81,9 +83,11 @@ impl ApiClient { // request. Fork PRs can't read repo secrets, so they rely on the public repo id. let api_token = api_token.filter(|token| !token.trim().is_empty()); let public_repo_id = public_repo_id.filter(|id| !id.trim().is_empty()); - if api_token.is_none() && public_repo_id.is_none() { + // The forked-PR lane presents no credential at all: the collection id already in the + // request authorizes it, gated on that collection's opt-in server-side. + if api_token.is_none() && public_repo_id.is_none() && !allow_forked_pr_uploads { return Err(anyhow::anyhow!( - "Either a Trunk API token or a public repo id is required." + "Either a Trunk API token, a public repo id, or --allow-forked-pr-uploads is required." )); } @@ -132,6 +136,12 @@ impl ApiClient { public_repo_id_header_value, ); } + if allow_forked_pr_uploads { + trunk_api_client_default_headers.append( + Self::TRUNK_ALLOW_FORKED_PR_UPLOADS_HEADER, + HeaderValue::from_static("true"), + ); + } let trunk_api_client = Client::builder() .timeout(Self::TRUNK_API_TIMEOUT) @@ -520,8 +530,14 @@ mod tests { let state = mock_server_builder.spawn_mock_server().await; - let mut api_client = - ApiClient::new(Some(String::from("mock-token")), None, "mock-org", None).unwrap(); + let mut api_client = ApiClient::new( + Some(String::from("mock-token")), + None, + false, + "mock-org", + None, + ) + .unwrap(); api_client.api_host.clone_from(&state.host); assert!( @@ -569,8 +585,14 @@ mod tests { let state = mock_server_builder.spawn_mock_server().await; - let mut api_client = - ApiClient::new(Some(String::from("mock-token")), None, "mock-org", None).unwrap(); + let mut api_client = ApiClient::new( + Some(String::from("mock-token")), + None, + false, + "mock-org", + None, + ) + .unwrap(); api_client.api_host.clone_from(&state.host); assert!( @@ -613,8 +635,14 @@ mod tests { let state = mock_server_builder.spawn_mock_server().await; - let mut api_client = - ApiClient::new(Some(String::from("mock-token")), None, "mock-org", None).unwrap(); + let mut api_client = ApiClient::new( + Some(String::from("mock-token")), + None, + false, + "mock-org", + None, + ) + .unwrap(); api_client.api_host.clone_from(&state.host); assert!( @@ -642,19 +670,19 @@ mod tests { #[test] fn requires_a_token_or_public_repo_id() { // Neither credential is an error. - let err = ApiClient::new(None, None, "mock-org", None) + let err = ApiClient::new(None, None, false, "mock-org", None) .err() .expect("expected an error when no credentials are provided"); - assert!( - err.to_string() - .contains("Either a Trunk API token or a public repo id is required") - ); + assert!(err.to_string().contains( + "Either a Trunk API token, a public repo id, or --allow-forked-pr-uploads is required" + )); // Blank values are treated as absent. assert!( ApiClient::new( Some(String::from(" ")), Some(String::from(" ")), + false, "mock-org", None ) @@ -662,11 +690,18 @@ mod tests { ); // A token alone is sufficient - no public repo id required. - assert!(ApiClient::new(Some(String::from("token")), None, "mock-org", None).is_ok()); + assert!(ApiClient::new(Some(String::from("token")), None, false, "mock-org", None).is_ok()); // A public repo id alone is sufficient - no token required. assert!( - ApiClient::new(None, Some(String::from("public-repo-id")), "mock-org", None).is_ok() + ApiClient::new( + None, + Some(String::from("public-repo-id")), + false, + "mock-org", + None + ) + .is_ok() ); } @@ -695,6 +730,7 @@ mod tests { let mut api_client = ApiClient::new( None, Some(String::from("public-repo-id-123")), + false, "mock-org", None, ) @@ -730,4 +766,68 @@ mod tests { "token header should be absent when only a public repo id is provided" ); } + + #[tokio::test] + async fn sends_allow_forked_pr_uploads_header_with_no_credential() { + let mut mock_server_builder = MockServerBuilder::new(); + + lazy_static! { + static ref CAPTURED_HEADERS: Arc> = + Arc::new(Mutex::new(HeaderMap::new())); + } + + let quarantining_config_handler = move |headers: HeaderMap| async move { + *CAPTURED_HEADERS.lock().unwrap() = headers; + Response::builder() + .status(StatusCode::NOT_FOUND) + .body(String::from( + r#"{ "status_code": 404, "error": "not found" }"#, + )) + .unwrap() + }; + mock_server_builder.set_get_quarantining_config_handler(quarantining_config_handler); + + let state = mock_server_builder.spawn_mock_server().await; + + let mut api_client = ApiClient::new(None, None, true, "mock-org", None).unwrap(); + api_client.api_host.clone_from(&state.host); + + let _ = api_client + .get_quarantining_config(&message::GetQuarantineConfigRequest { + repo: context::repo::RepoUrlParts { + host: String::from("host"), + owner: String::from("owner"), + name: String::from("name"), + }, + org_url_slug: String::from("org_url_slug"), + test_identifiers: vec![], + remote_urls: vec![], + test_collection_short_id: Some(String::from("abc12345")), + }) + .await; + + let headers = CAPTURED_HEADERS.lock().unwrap(); + assert_eq!( + headers + .get(super::ApiClient::TRUNK_ALLOW_FORKED_PR_UPLOADS_HEADER) + .and_then(|value| value.to_str().ok()), + Some("true") + ); + // The lane presents no credential at all — the collection id in the body is what the + // server authorizes, against that collection's own opt-in. + assert!( + headers + .get(super::ApiClient::TRUNK_API_TOKEN_HEADER) + .is_none() + && headers + .get(super::ApiClient::TRUNK_PUBLIC_REPO_ID_HEADER) + .is_none(), + "no credential header should be sent on the forked-PR lane" + ); + } + + #[test] + fn allow_forked_pr_uploads_satisfies_the_credential_requirement() { + assert!(ApiClient::new(None, None, true, "mock-org", None).is_ok()); + } } diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index 3ac7873a..c976f85e 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -100,18 +100,27 @@ pub struct UploadArgs { pub test_collection_short_id: Option, #[arg( long, - required_unless_present = "public_repo_id", + required_unless_present_any = ["public_repo_id", "allow_forked_pr_uploads"], env = constants::TRUNK_API_TOKEN_ENV, help = "Organization token. Defaults to TRUNK_API_TOKEN env var." )] pub token: Option, #[arg( long, - required_unless_present = "token", + required_unless_present_any = ["token", "allow_forked_pr_uploads"], env = constants::TRUNK_PUBLIC_REPO_ID_ENV, help = "Non-secret per-repo identifier, usable instead of --token on fork PRs where repo secrets are unavailable." )] pub public_repo_id: Option, + /// Opting in explicitly is what keeps a job whose token secret failed to interpolate failing + /// loudly: without this, "no credential" would silently become an anonymous upload, and the + /// fail-open behaviour on auth errors would hide it behind a green CI step. + #[arg( + long, + env = constants::TRUNK_ALLOW_FORKED_PR_UPLOADS_ENV, + help = "Allow uploading from a forked pull request, which cannot read repository secrets. Requires --test-collection-id; the collection must have forked-PR uploads enabled in Trunk." + )] + pub allow_forked_pr_uploads: bool, #[arg( long, env = constants::TRUNK_REPO_ROOT_ENV, @@ -539,9 +548,24 @@ pub async fn run_upload( ); } + // Caught here rather than server-side: without a collection there is nothing for the forked + // lane to authorize against, and a 401 from a fork run is fail-open — a warning and a green + // step, which is exactly the silent misconfiguration this flag exists to prevent. + if upload_args.allow_forked_pr_uploads + && upload_args + .test_collection_short_id + .as_ref() + .is_none_or(|id| id.trim().is_empty()) + { + return Err(anyhow::anyhow!( + "--allow-forked-pr-uploads requires --test-collection-id: forked pull request uploads are authorized by the collection's own opt-in." + )); + } + let api_client = ApiClient::new( upload_args.token.clone(), upload_args.public_repo_id.clone(), + upload_args.allow_forked_pr_uploads, &upload_args.org_url_slug, render_sender, )?; diff --git a/constants/src/lib.rs b/constants/src/lib.rs index b442946e..02b0f8f1 100644 --- a/constants/src/lib.rs +++ b/constants/src/lib.rs @@ -33,6 +33,7 @@ pub const TRUNK_API_CLIENT_RETRY_DEADLINE_SECS_ENV: &str = "TRUNK_API_CLIENT_RET // Trunk CLI environment variable names for configuration overrides pub const TRUNK_API_TOKEN_ENV: &str = "TRUNK_API_TOKEN"; pub const TRUNK_PUBLIC_REPO_ID_ENV: &str = "TRUNK_PUBLIC_REPO_ID"; +pub const TRUNK_ALLOW_FORKED_PR_UPLOADS_ENV: &str = "TRUNK_ALLOW_FORKED_PR_UPLOADS"; pub const TRUNK_ORG_URL_SLUG_ENV: &str = "TRUNK_ORG_URL_SLUG"; pub const TRUNK_TEST_COLLECTION_ID_ENV: &str = "TRUNK_TEST_COLLECTION_ID"; pub const TRUNK_REPO_ROOT_ENV: &str = "TRUNK_REPO_ROOT"; @@ -85,6 +86,7 @@ pub const TRUNK_ENVS_TO_CAPTURE: &[&str] = &[ TRUNK_API_CLIENT_RETRY_COUNT_ENV, TRUNK_API_CLIENT_RETRY_DEADLINE_SECS_ENV, TRUNK_PUBLIC_REPO_ID_ENV, + TRUNK_ALLOW_FORKED_PR_UPLOADS_ENV, TRUNK_ORG_URL_SLUG_ENV, TRUNK_TEST_COLLECTION_ID_ENV, TRUNK_REPO_ROOT_ENV, diff --git a/test_report/src/report.rs b/test_report/src/report.rs index bea22ce8..f3b3e9fc 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -375,7 +375,7 @@ impl MutTestReport { tracing::warn!("Not checking quarantine status because TRUNK_ORG_URL_SLUG is empty"); return IsQuarantinedResult::default(); } - let api_client = ApiClient::new(Some(token), None, org_url_slug.clone(), None); + let api_client = ApiClient::new(Some(token), None, false, org_url_slug.clone(), None); let use_uncloned_repo = env::var(constants::TRUNK_USE_UNCLONED_REPO_ENV) .ok() .and_then(|v| v.parse().ok())