Fix hunter-agent findings batch #1 - #551
Conversation
Resolves 13 SonarCloud "hunter-agent" business-logic/security findings, mostly about the reset command's blast radius, a permission-escalation path, and several silent-data-loss / trust-boundary gaps. All 13 were previously marked WONTFIX/ACCEPTED in SonarCloud; #550 explicitly asks to resolve them, superseding that earlier triage. Reset command hardening: - Add --organization (anchored regex, mirrors transfer's --project_key) to scope reset to a subset of mapped orgs before the confirm/--yes prompt, and --dry-run to print the plan without any destructive call. - ConfirmedOrgs now fails closed: an empty/nil scope errors out instead of defaulting to "reset every mapped org". Config-file-driven callers can now supply confirmed_orgs directly. - resetPermissionTemplates hard-fails instead of silently proceeding when the built-in "Default Template" can't be found by name (no isBuiltIn flag exists for permission templates in the real API), and deleteTemplates additionally refuses to delete anything that's currently an org's default template, independent of name — closing the "renamed built-in gets wiped" gap. Permission handling: - The sonar-users -> Members built-in group alias can no longer auto-grant admin org-wide; the grant is skipped with a loud warning requiring manual review, while other permissions still apply. - Permission-grant tasks now return an error when every attempted grant failed (previously always reported success even at 0% success rate), so the migration DAG halts instead of silently proceeding as if permissions were in place. Data integrity: - --default_organization is now validated against the live API before being written to organizations.csv, so a bad first attempt no longer locks a corrected retry out via the "already mapped" check. Same fix applied to the sync-issues entry point, which had the identical bug. - Numeric-looking organization/identifier keys (e.g. "12345") are no longer silently coerced to float64 by the CSV loader and dropped from the migration. - Fresh project creation now treats an empty returned project key as a failure instead of silently propagating it downstream. Trust boundaries: - Extract run directories must match the real YYYY-MM-DD-NNNN shape before being considered "latest", closing a directory-poisoning path left open by the #543 ordering fix's string-compare fallback. - The wizard now verifies each phase's real on-disk prerequisites before dispatching into it, rather than trusting a resumed .wizard_state.json's Phase field verbatim — closes the path where a tampered or corrupted state file could skip straight into a migration phase against an arbitrary target. Also hardens the shared migrate test fixtures: replaces hand-typed copies of the built-in permission template name with a direct reference to the production constant, and documents the two easily-confused mock Cloud server constructors in testutil_test.go, after an incorrect edit to the wrong one surfaced this exact drift during review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- cmd/reset.go: confirmResetOrgs grew to complexity 43 (vs 15 allowed) after adding --organization scoping and config-file presetOrgs support. Split it into filterOrgsByPattern, resolveOrgSelection (a shared dedupe-and-classify helper that removes real duplication between the interactive and presetOrgs paths), confirmResetOrgsAutoYes, and confirmResetOrgsInteractive. - internal/migrate/reset.go: RunReset grew to 16 after adding the fail-closed ConfirmedOrgs check and dry-run branch. Extracted the target-task selection loop into resetTargets and the confirmed-orgs set construction into a small toSet helper. - internal/migrate/tasks_create.go: runCreateProjects grew to 17 after adding the empty-key defensive check. Split the already-exists and fresh-create branches into handleExistingProject/handleFreshProject. - internal/migrate/tasks_create_test.go: removed an unnecessary variable declaration flagged by godre:S8193. - internal/structure/csv_test.go: TestCoerceCSVValue's added numeric case pushed its type-switch assertion to complexity 18. Replaced the 5-branch type switch with reflect.DeepEqual. No behavior changes; go build/vet/test all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Gitar's review of the #550 hunter-agent fixes surfaced 7 issues, the most serious being a real regression this batch introduced: a single project lacking a cloud key (createProjects' "failed" record) caused every grant on it to fail, which aborted the shared errgroup context and cancelled permission grants for every other project in the run. - Critical: skip createProjects records with status:"failed" before granting migration-user permissions, instead of attempting (and failing) all of them and aborting the whole task. - Important: resetPermissionTemplates now fails only the affected org when the built-in "Default Template" can't be found by name, rather than hard-failing every org's reset. - Important: IsValidRunID accepts both the current ISO run-ID shape and the legacy pre-#108 MM-DD-YYYY-N shape still present in export directories on disk, instead of silently dropping the latter. - Important: runSetOrgGroupPermissions / runSetProfileGroupPermissions / runSetTemplateGroupPermissions now accumulate per-item total- failure errors and report one aggregated error at the end, instead of returning per-item errors that cancel the errgroup and abort every other item. - Minor: fixed a false total-failure in runSetTemplateGroupPermissions by tracking attempted/succeeded per (cloudTemplate, group) pair across both extract feeds, rather than per individual apply() call. - Minor: deliberate admin-escalation skips (sonar-users -> Members alias) no longer count as counter.Fail() - they're a security decision, not a failure. - Minor: cmd/reset.go's --dry-run now prints a preview message and skips the interactive confirmation prompt instead of always showing the deletion warning. Adds a regression test for the critical fix (TestRunGrantMigrationUserProjectPermissionsSkipsFailedRecords) and updates the one test whose assertion inverted with the resetPermissionTemplates fix.
✅ Code review updated (blocking issues remain unresolved).
Gitar's follow-up review found the previous round's fixes incomplete in three ways: - runGrantMigrationUserProjectPermissions still returned a hard error from inside the forEachMigrateItem closure on 100% grant failure, which cancels the shared errgroup context and aborts every other project still in flight. Switched to the same accumulate-then-report pattern already used by the three sibling permission tasks. - The "skip createProjects records with status:failed" guard only existed at the one call site Gitar's first review flagged (grantMigrationUserProjectPermissions). Every other createProjects consumer had no such filter, so a project that failed to create (cross-org key collision #525, or empty key #550) would still be processed by 12 other tasks across tasks_associate.go, tasks_hotspotsync.go, tasks_issuesync.go, tasks_read.go, tasks_portfolios.go, tasks_projectdata.go, and tasks_setglobalsettings.go, issuing API calls against a project that doesn't exist. Factored the check into a shared isFailedMigrateRecord helper and applied it at every createProjects consumer. - resetPermissionTemplates skipping an org (built-in template renamed) left deleteTemplates free to run against that org anyway. Its isCurrentDefaultTemplate net only protects whatever IS currently the org's default, which doesn't cover a renamed built-in that migration already demoted in favor of a promoted custom template. Reset now writes an explicit failed record for that org, and deleteTemplates reads it to skip template deletion entirely for orgs where the built-in couldn't be confirmed. Adds regression tests for all three: one proving a genuinely-failing project no longer aborts a sibling project's grants, one proving deleteTemplates skips an org whose built-in reset failed, and one proving a representative createProjects consumer (syncHotspotMetadata) skips a failed record.
|
| continue | ||
| } | ||
| for _, item := range items { | ||
| if isFailedMigrateRecord(item) { |
There was a problem hiding this comment.
⚠️ Bug: Reset now leaves empty-key-created projects behind undeleted
handleFreshProject writes a status:"failed" record when Create succeeded but returned an empty key — the project genuinely exists in the target org under requestedKey, which is what the record stores as cloud_project_key (tasks_create.go:150-170). Adding the blanket isFailedMigrateRecord filter to runGetCreatedProjects and MigrateCreatedProjectCounts means reset now silently skips that real project: it is never fed to deleteProjects and it is not counted in the #381 confirmation prompt, so a migration-created project is left behind on SonarQube Cloud forever. Only the #525 cross-org-collision failure describes a project that isn't ours; distinguish the two (e.g. an error_kind/cross_org marker on the record, or filter on the collision case only) so the empty-key case still gets cleaned up.
Tag the cross-org collision failure and let reset still delete the project created with an empty returned key.:
// tasks_create.go: mark which failures leave nothing of ours behind
result := common.EnrichRaw(item, map[string]any{
"cloud_project_key": cloudKey,
"sonarcloud_org_key": orgKey,
"status": "failed",
"failure_kind": "cross_org_key_collision", // not our project
"error": msg,
})
// tasks_read.go (both sites): only skip projects that aren't ours
if isFailedMigrateRecord(item) && extractField(item, "failure_kind") == "cross_org_key_collision" {
continue
}
Was this helpful? React with 👍 / 👎
| if builtIn == nil { | ||
| e.Logger.Warn("resetPermissionTemplates: no built-in \"Default Template\" found; deleteTemplates may fail to delete the current default", | ||
| // Fail this org, not the whole run: forEachMigrateItem fans | ||
| // out over an errgroup, so returning an error here would | ||
| // cancel every other confirmed org's reset too — one | ||
| // renamed built-in shouldn't block the rest. | ||
| // | ||
| // isCurrentDefaultTemplate only protects whichever template | ||
| // is *currently* the org's default for some qualifier — a | ||
| // renamed built-in that migration already demoted (because | ||
| // a custom template was promoted to default) matches | ||
| // neither that check nor isBuiltInPermissionTemplate, so it | ||
| // is NOT safe against deleteTemplates on its own (#551). | ||
| // Write an explicit failed record so runDeleteTemplates can | ||
| // skip template deletion entirely for this org instead. | ||
| e.Logger.Error("resetPermissionTemplates: no built-in \"Default Template\" found; skipping template-default reset for this org", |
There was a problem hiding this comment.
⚠️ Bug: deleteTemplates still runs when reset's search_templates call failed
runResetPermissionTemplates writes the new status:"failed" marker only in the builtIn == nil branch; when searchPermissionTemplates itself errors (line 663-666) it just calls failAPI and returns nil with no record. runDeleteTemplates skips only orgs present in unresetOrgs, so for that org it proceeds to list and delete templates even though the built-in was never confirmed or promoted — a renamed built-in that migration had already demoted matches neither isBuiltInPermissionTemplate nor isCurrentDefaultTemplate and gets destroyed, which is exactly the loss this commit set out to prevent. Write the same failed record on the listing-error path.
Emit the skip marker when the template listing fails, not only when the built-in is missing.:
templates, defaults, err := searchPermissionTemplates(ctx, e, orgKey)
if err != nil {
failAPI(counter, e.Logger, "resetPermissionTemplates: listing templates failed", err, "org", orgKey)
// #551: the built-in could not be confirmed for this org, so
// deleteTemplates must skip it entirely (same as the
// no-built-in-found branch below).
result, _ := json.Marshal(map[string]any{
"sonarcloud_org_key": orgKey,
"status": "failed",
"error": fmt.Sprintf("listing permission templates failed: %v", err),
})
return w.WriteOne(result)
}
Was this helpful? React with 👍 / 👎
| counter := TaskCounterFromContext(ctx) | ||
| var failMu sync.Mutex | ||
| var totalFailures []string |
There was a problem hiding this comment.
💡 Quality: Aggregated grant error can concatenate one entry per project
totalFailures accumulates one line per project and every line is joined into a single error string. The sibling tasks that established this pattern iterate groups/profiles (tens of items), whereas grantMigrationUserProjectPermissions iterates every created project — if the migration token lacks admin instance-wide, all N projects land in the list and the returned error (which gets logged and surfaced in the run report) becomes tens of thousands of lines. Cap the detail, e.g. list the first 10 and append "and N more".
Truncate the aggregated detail so the error stays readable at migration scale.:
if err == nil && len(totalFailures) > 0 {
sort.Strings(totalFailures)
shown := totalFailures
suffix := ""
if len(shown) > 10 {
shown, suffix = shown[:10], fmt.Sprintf(" (and %d more)", len(totalFailures)-10)
}
err = fmt.Errorf("grantMigrationUserProjectPermissions: %d project(s) received no permissions at all: %s%s",
len(totalFailures), strings.Join(shown, "; "), suffix)
}
Was this helpful? React with 👍 / 👎
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
3 participants
Summary
Closes #550. Resolves all 13 SonarCloud "hunter-agent" business-logic/security findings, previously marked WONTFIX/ACCEPTED, per the issue's explicit request to resolve them.
--organizationscoping (anchored regex, mirrorstransfer's--project_key),--dry-run, and a fail-closed default forConfirmedOrgs(an empty scope now errors instead of wiping every mapped org). Config-file-driven callers can supplyconfirmed_orgsdirectly.resetPermissionTemplateshard-fails when the built-in "Default Template" can't be found by name instead of silently proceeding;deleteTemplatesalso refuses to delete anything that's currently an org's default template, independent of name.sonar-users→Membersalias can no longer auto-grantadminorg-wide. Permission-grant tasks now surface an error when every attempted grant failed, instead of always reporting success.--default_organizationis validated against the live API before being persisted toorganizations.csv(closes a bug where a bad first attempt permanently locked out a corrected retry — also fixed in thesync-issuesentry point, which had the same bug). Numeric-looking org/identifier keys no longer get silently coerced tofloat64and dropped from the migration. Fresh project creation treats an empty returned key as a failure rather than propagating it.migratetest fixtures (a hand-typed template-name literal had silently drifted out of sync with production code during review; replaced with a direct reference to the real constant, plus clarifying docs on two easily-confused mock server constructors).Test plan
go build ./.../go vet ./...cleango test ./... -count=1— all packages pass--default_organizationapplies after a failed first attempt, numeric org key survives as a string, rogue run-ID directory excluded from "latest", total permission-grant failure surfaces an error, fabricated wizard state rejected before entering migrate)🤖 Generated with Claude Code