scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-07 03:00:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

refactor(server): drop TOCTOU branch_exists pre-check on PR create

Browse source 
POST /runs/{id}/pull_request was calling fabro_github::branch_exists to
distinguish a missing head ref before invoking maybe_open_pull_request.
GitHub's POST /pulls already returns 422 for an unknown ref, so the
pre-check was an extra round-trip on the happy path (and would race a
concurrent branch deletion anyway).

Drop the check plus the now-unused missing_remote_branch_error helper;
let GitHub's validation error bubble up as BAD_GATEWAY. Replace the
(owner, repo) binding with an if-let Err on parse_github_owner_repo_from_url
since we only needed it for the unsupported_host validation side-effect.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-23 22:53:31 -04:00
parent 233d47391d
commit 45d0cc4415
No known key found for this signature in database
1 changed files with 3 additions and 40 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

43
lib/crates/fabro-server/src/server.rs
View file

@ -5125,16 +5125,6 @@ fn run_not_successful_error(status: &fabro_types::StageStatus) -> ApiError {
)
}
fn missing_remote_branch_error(run_branch: &str) -> ApiError {
ApiError::with_code(
StatusCode::BAD_REQUEST,
format!(
"Branch '{run_branch}' not found on GitHub. Was it pushed? Try: git push origin {run_branch}"
),
"missing_remote_branch",
)
}
struct PullRequestGithubContext {
record: PullRequestRecord,
owner: String,
@ -5231,32 +5221,14 @@ async fn create_run_pull_request(
}
let normalized_origin = fabro_github::ssh_url_to_https(origin_url);
let (owner, repo) =
match parse_github_owner_repo_from_url(&normalized_origin, "repo origin URL") {
Ok(owner_repo) => owner_repo,
Err(err) => return err.into_response(),
};
if let Err(err) = parse_github_owner_repo_from_url(&normalized_origin, "repo origin URL") {
return err.into_response();
}
let creds = match load_server_github_credentials(state.as_ref()) {
Ok(creds) => creds,
Err(err) => return err.into_response(),
};
let branch_exists = match fabro_github::branch_exists(
&creds,
&owner,
&repo,
run_branch,
state.github_api_base_url.as_str(),
)
.await
{
Ok(branch_exists) => branch_exists,
Err(err) => return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(),
};
if !branch_exists {
return missing_remote_branch_error(run_branch).into_response();
}
let model = if let Some(model) = body.model {
model
} else {
@ -9541,14 +9513,6 @@ slug = "fabro"
install_mock_llm();
let github = MockServer::start();
let branch_mock = github.mock(|when, then| {
when.method("GET")
.path("/repos/acme/widgets/branches/fabro/run/42")
.header("authorization", "Bearer ghu_test");
then.status(200)
.header("content-type", "application/json")
.body(json!({ "name": "fabro/run/42" }).to_string());
});
let create_mock = github.mock(|when, then| {
when.method("POST")
.path("/repos/acme/widgets/pulls")
@ -9617,7 +9581,6 @@ slug = "fabro"
assert_eq!(state_body["pull_request"]["number"], 42);
assert!(state_body["pull_request"]["title"].as_str().is_some());
branch_mock.assert();
create_mock.assert();
}