Skip to content

fix(cli): resolve list/wait projects by repo URL, not directory name (COR-1577) - #137

Open
juangaitanv wants to merge 1 commit into
mainfrom
juan/cor-1577-resolve-by-repo
Open

fix(cli): resolve list/wait projects by repo URL, not directory name (COR-1577)#137
juangaitanv wants to merge 1 commit into
mainfrom
juan/cor-1577-resolve-by-repo

Conversation

@juangaitanv

Copy link
Copy Markdown
Contributor

Fixes COR-1577. Replaces #122, which grew well past the ticket; this is the fix on its own, and the hardening follows in a stacked PR.

The bug

corgea list and standalone corgea wait derived the project from the working-directory basename and sent it as an exact ?project= match. Where the stored name differs — the Bank of Hope case, directory dotnet-azure-web-tsb vs project bohappdev/dotnet-azure-web-tsblist --issues and wait exited 1, and plain list silently printed an empty table.

The fix

Resolve the canonical project from the git remote — GET /api/v1/projects?repo_url=<org/repo>, discovered upward from the CWD so a subdirectory works — then query the listing endpoints by the name the backend actually stores.

Three things make it safe on older and partially-onboarded backends:

  • The path re-check. The backend filters repo_url__icontains, so a query for acme/api also returns acme/api-v2 and the nested mirrors/acme/api. Every candidate is re-checked against the whole post-host path. The same check guards a pre-COR-1426 backend, which ignores the unknown param and returns all projects: none match, so we fall back instead of listing a stranger's scans.
  • The fallback is unchanged behavior. Unconfirmed, the query stays exactly what the pre-COR-1577 CLI sent (determine_project_name), so an old or not-yet-onboarded backend keeps working.
  • Empty is no longer silently fine. An unresolved project with no scans errors and names what was tried; a confirmed project with no scans still exits 0, so CI polling is unaffected.

Escape hatches: --project-name queries an exact name and skips resolution entirely; --repo resolves from a given slug or URL instead of the remote. Both are on list and wait.

Also removes the client-side scan.project == cwd_basename filter — the server already filtered, and that pass discarded every resolved scan.

Deliberately not here

Kept out to hold this to the ticket; all of it lands in the stacked follow-up:

  • pagination of /projects (page 1 only today — see COR-1729, which would let the CLI drop both the walk and the re-check)
  • host disambiguation for the same org/repo on two forges
  • hard-vs-soft failure for malformed /projects envelopes
  • percent-encoding the project name into /issues (pre-existing)
  • --project-id, SCA selector scoping, argument structs

Verification

cargo build, cargo clippy --all-targets (clean), cargo fmt --check, and the full suite under CI=1 GITHUB_ACTIONS=true — 537 tests.

16 new e2e tests cover: the canonical name driving /scans and /issues (asserted on the request the CLI sent, from a checkout named build-123, so a fallback cannot pass); resolution from a subdirectory; --repo; the miss naming the repo with no table; confirmed-empty exiting 0; the legacy fallback name; --project-name skipping resolution; the JSON envelope preceding the miss; --scan-id skipping resolution; and the post-scan corgea scan path resolving nothing.

Size

429 lines of production code (~340 excluding comments and blank lines), 825 of tests.

COR-1577. `corgea list` and standalone `corgea wait` derived the project
from the working-directory basename and sent it as an exact `?project=`
match. Where the stored name differs — the Bank of Hope case, dir
`dotnet-azure-web-tsb` vs project `bohappdev/dotnet-azure-web-tsb` —
`list --issues` and `wait` exited 1 and plain `list` silently printed an
empty table.

Both now resolve the canonical project from the git remote:
`GET /api/v1/projects?repo_url=<org/repo>`, discovered upward from the CWD
so a subdirectory works too, then query the listing endpoints by the name
the backend actually stores.

- The backend filters `repo_url__icontains`, so every candidate is
  re-checked against the whole post-host path. That also guards a
  pre-COR-1426 backend, which ignores the unknown param and returns ALL
  projects: none match, and we fall back rather than list a stranger's
  scans.
- Unconfirmed, the query stays exactly what the pre-COR-1577 CLI sent, so
  an old or not-yet-onboarded backend keeps working.
- `--project-name` queries an exact name and skips resolution entirely;
  `--repo` resolves from a given slug or URL instead of the remote.
- An unresolved project with no scans is now an error naming what was
  tried, instead of an empty table; a confirmed project with no scans
  still exits 0 so CI polling is unaffected.
- The client-side `scan.project == cwd_basename` filter is gone — the
  server already filtered, and that pass discarded every resolved scan.
Comment thread src/utils/api.rs
));
let response = client
.get(&request_url)
.query(&[("repo_url", repo_path)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET /projects is paginated (default page_size=20, max 50), and repo_url is only a case-insensitive partial filter. This request sends neither page nor page_size, then searches only the first response page. With 20+ matching siblings such as acme/api-*, the exact acme/api can be on page 2; this returns None and the caller falls back to the legacy name, recreating the miss COR-1577 is meant to fix (and potentially selecting a different same-basename project). Deserialize total_pages, request page_size=50, and walk pages until the exact match or the last page, with a bounded cap that returns an error rather than a clean miss if the walk is truncated. Add a test where the exact match is only on page 2.

Comment thread src/utils/api.rs
Comment on lines +795 to +805
let text = response.text()?;
let parsed: ProjectsResponse = match serde_json::from_str(&text) {
Ok(parsed) => parsed,
Err(e) => {
debug(&format!(
"Failed to parse /projects response: {} | body: {}",
e, text
));
return Ok(None);
}
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A successful /projects response that cannot be decoded is treated as a clean “no match.” Together with #[serde(default)] on ProjectsResponse.projects, even a 200 error envelope such as {"status":"error"} silently takes the legacy-name fallback. The documented 200 schema always requires projects; proxy HTML or an incompatible/error payload is a hard resolver failure, otherwise list/wait can query a different legacy-named project and exit successfully. Remove the default from projects and propagate deserialization failures. For the parser half:

Suggested change
let text = response.text()?;
let parsed: ProjectsResponse = match serde_json::from_str(&text) {
Ok(parsed) => parsed,
Err(e) => {
debug(&format!(
"Failed to parse /projects response: {} | body: {}",
e, text
));
return Ok(None);
}
};
let parsed: ProjectsResponse = serde_json::from_str(&text).map_err(|e| {
debug(&format!(
"Failed to parse /projects response: {} | body: {}",
e, text
));
format!("Failed to parse /projects response: {}", e)
})?;

Comment thread src/utils/api.rs
return Ok(None);
}
};
let expected = repo_path.to_lowercase();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolver has discarded the remote host before this point, and repo_url_matches_path compares only the post-host path. Because the backend query is also hostless/partial, https://github.com/acme/api and https://gitlab.com/acme/api both pass this predicate; .find(...) then confirms whichever project the API happened to return first. list/wait can consequently expose scans or issues for the wrong project. Preserve the origin host through resolution and use it to select among exact path matches; if multiple path matches remain and none matches a known host, fail as ambiguous and direct the caller to --project-name.

Comment thread src/list.rs
println!();
if *sca_issues {
// SCA has no project parameter; this name is only error copy.
let project_name = utils::generic::determine_project_name(None);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new list --project-name / --repo flags are accepted with --sca-issues, but this branch discards both selectors and calls get_sca_issues with pagination/scan ID only. The SCA endpoint supports project and repo filters, so corgea list --sca-issues --project-name X currently succeeds while returning company-wide SCA findings instead of project X. Thread the explicit selector into get_sca_issues (skipping it only for the scan-ID route), or reject these flag combinations at clap parsing; add a request-target assertion proving the selected project/repo is sent.

Comment thread src/list.rs
} else if *issues {
// The --scan-id route hits /scan/{id}/issues and ignores the project.
let resolved = scan_id.is_none().then(|| {
utils::api::resolve_project_or_exit(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolved/user-supplied project name introduced here is passed to get_scan_issues, which still constructs src/utils/api.rs:479 with format!("...?project={}", project) and then appends &page=.... A valid explicit name such as foo&bar is therefore sent as project=foo&bar&page=1, so the server reads project foo and can return another project's issues. Build the /issues base URL first and pass project, page, and page_size via reqwest .query(...) (as query_scan_list already does), with a test asserting reserved characters are percent-encoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant