fix(#7140): map hyphens to underscores in role identifiers - #7215
fix(#7140): map hyphens to underscores in role identifiers#7215rh-hemartin wants to merge 1 commit into
Conversation
PR Summary by QodoNormalize hyphenated role identifiers across integration surfaces
AI Description
Diagram
High-Level Assessment
Files changed (23)
|
Custom harness roles such as ci-check produced invalid bash identifiers in Setup agent environment. Role-prefixed env vars, GitHub Actions secrets/variables, and FOREIGN allow-list names now share mintcore.RoleIdentifier: uppercase the role and replace hyphens with underscores. github status prints FOREIGN variable names without a reconstructed role; CI_CHECK is not uniquely ci-check or ci_check. Assisted-By: grok-4.6(pi) Signed-off-by: Hector Martinez <hemartin@redhat.com>
02cb454 to
d7fb5a7
Compare
|
🤖 Review · Commit: |
Code Review by Qodo
1. Workflow changes bypass guide review
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # GitHub Actions / bash identifiers cannot contain hyphens. | ||
| # Map the role the same way mintcore.RoleIdentifier does: uppercase | ||
| # and replace '-' with '_' so ci-check becomes CI_CHECK. | ||
| ROLE_UPPER=$(echo "${MATRIX_ROLE}" | tr '[:lower:]' '[:upper:]' | tr '-' '_') |
There was a problem hiding this comment.
1. Workflow changes bypass guide review 📘 Rule violation § Compliance
.github/workflows/reusable-dispatch.yml changes secret-derived environment setup without an inline reference to docs/contributing/ci-workflows.md. The PR description also omits any statement that the guide was consulted, so this workflow change reaches review without the required security and context check.
Agent Prompt
## Issue description
The reusable workflow was modified, but neither the workflow comment nor the PR description records consultation of `docs/contributing/ci-workflows.md` as required for GitHub Actions changes.
## Fix Focus Areas
- .github/workflows/reusable-dispatch.yml[1737-1740]
## Recommended Fix
Review `docs/contributing/ci-workflows.md`, verify the changed environment and identifier handling follows its security and context guidance, then add a concise inline comment referencing that guide or update the PR description to explicitly record the review.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
🤖 Finished Review · ✅ Success · Started 3:18 PM UTC · Completed 3:37 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.88 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // ForeignVariableName returns the org variable name for cross-org allowlist policy. | ||
| func ForeignVariableName(role string) string { | ||
| return foreignVarPrefix + strings.ToUpper(role) + foreignVarSuffix | ||
| return foreignVarPrefix + RoleIdentifier(role) + foreignVarSuffix |
There was a problem hiding this comment.
[medium] role-escalation
The new mintcore.RoleIdentifier helper (internal/mintcore/patterns.go:56) is not injective: RolePattern (^[a-z][a-z0-9_-]*$) still permits both hyphens and underscores in role names, and every admission path (RegisterCustomRolePermissions/RegisterCustomRoleLevels/validateCustomRoleLevels, per-repo config Validate, harness discovery loadKnownSlugs) only rejects exact string duplicates, not identifier collisions. Two distinct, individually valid roles differing only by hyphen vs underscore (e.g. ci-check and ci_check) now collapse onto the same identifier (CI_CHECK) and therefore the same GitHub Actions secret/variable names: FULLSEND_FOREIGN_CI_CHECK_REPOS (a FOREIGN cross-org/cross-repo mint allow-list grant for one role now authorizes the other), FULLSEND_CI_CHECK_APP_PRIVATE_KEY / FULLSEND_CI_CHECK_CLIENT_ID (SecretsLayer.Install is last-write-wins on the non-OIDC path, so one role credential can silently overwrite the other), and the harness-run _FULLSEND_MODEL override prefix. This is a regression on the GHA/FOREIGN path specifically: before this PR, ForeignVariableName/secretName/variableName used strings.ToUpper(role) with no hyphen folding, so ci-check and ci_check produced different (and, for ci-check, GitHub-Actions-invalid) strings — they did not collide. GCF Secret Manager IDs (fullsend-%s-app-pem, built from PemSecretRole only) remain distinct, which makes the FOREIGN-allow-list aliasing worse on the OIDC path: two GitHub Apps with different PEMs/permission envelopes can end up sharing one allow-list variable. Triggering this requires org-admin-level configuration of two RolePattern-valid custom roles (not an external/untrusted-input path), but it is a new authorization-surface invariant this PR introduces without any uniqueness check, despite the docs (standalone-mint.md) explicitly acknowledging the alias.
Suggested fix: Keep RoleIdentifier for producing GitHub-Actions-safe names, but reject identifier collisions wherever roles are admitted: mintcore.RegisterCustomRolePermissions/RegisterCustomRoleLevels/validateCustomRoleLevels, per-repo config role lists, harness discovery (loadKnownSlugs), SecretsLayer.Install, admin app setup, and CF StoreAgentPEM. Compare mintcore.RoleIdentifier(role) across the full configured role set (and against built-in role names) and error when two distinct role strings map to the same identifier.
1 participant
Summary
Custom harness roles with hyphens (
ci-check) failed in Setup agent environment because the dispatch job uppercasedmatrix.roleand exported names likeCI-CHECK_TARGET_REPO_DIR, which bash rejects. Role-prefixed identifiers now sharemintcore.RoleIdentifier: uppercase the role and replace hyphens with underscores (ci-check→CI_CHECK).Related Issue
Fixes #7140
Changes
reusable-dispatch.yml) soci-checkbecomesCI_CHECK_TARGET_REPO_DIR/CI_CHECK_.FULLSEND_<ROLE>_APP_PRIVATE_KEY,FULLSEND_<ROLE>_CLIENT_ID), FOREIGN allow-lists (FULLSEND_FOREIGN_<ROLE>_REPOS), and CF PEM names.fullsend github statusprints FOREIGN variable names without a reconstructed role in parentheses (CI_CHECKis not uniquelyci-checkorci_check).Testing
make lintpasses (stage changes first, then run)RoleIdentifier, hyphenated foreign allow,github statusoutput, dispatch hyphen mapping pin)Checklist
!for breaking changes)