fix(wif): preserve GitHub canonical case in attributeCondition and IAM principals

[waynesun09]

Summary

Fixes #3678 — WIF attributeCondition forces lowercase org/repo, rejecting auth for mixed-case GitHub orgs.

The WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g. RHEcosystemAppEng/sdlc-plugins) permanently failed authentication with unauthorized_client: The given credential is rejected by the attribute condition.

Changes

• parseConditionOrgs — preserves case from existing CEL conditions instead of lowercasing
• ensureWIFPoolAndProvider — case-insensitive dedup using lowered map keys, preserving canonical-case values; installing orgs take precedence when casing differs
• ProvisionWIF repo path — split into parts (original case for CEL/IAM) and partsLower (for BuildRepoProviderID and validation)
• ProvisionWIF org path — preserves original case in orgs slice while deduping case-insensitively
• EnsureOrgInWIFCondition — case-insensitive dedup preserving canonical case
• RemoveOrgFromWIFCondition — uses strings.EqualFold for case-insensitive matching
• IAM grant helpers — removed defensive strings.ToLower since attribute.repository maps directly from the assertion without normalization
• Provision — stopped mutating p.cfg.GitHubOrgs entries to lowercase

What stays lowered (correct behavior)

• BuildRepoProviderID — GCP resource IDs require lowercase
• EnsureOrgInMint / RegisterPerRepoWIF — env var bookkeeping (ALLOWED_ORGS, PER_REPO_WIF_REPOS)
• RemoveOrgFromMint / RemoveRepoFromMint — env var bookkeeping

Test plan

• Updated TestProvisionWIF_PreservesOrgCase — condition preserves original case
• Updated TestProvisionWIF_RepoScoped_PreservesRepoCase — condition and IAM binding preserve case, provider ID still lowered
• Updated TestParseConditionOrgs — mixed case preserved in parsed output
• Updated TestEnsureOrgInWIFCondition_AddsOrgAndStripsPlaceholder — preserves case
• Added TestProvisionWIF_OrgScoped_PreservesOrgCase — mixed-case org in condition and IAM
• Added TestProvisionWIF_OrgScoped_MergeDedupsCase — installing org case wins on merge
• Added TestRemoveOrgFromWIFCondition_CaseInsensitiveMatch — removes by case-insensitive match
• Full go test ./internal/dispatch/gcf/ passes
• go vet clean

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:23 PM UTC · Completed 5:38 PM UTC
Commit: 0438979 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Fix WIF: preserve GitHub canonical case in CEL conditions and IAM principals

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Preserve mixed-case GitHub org/repo strings in WIF CEL attributeCondition literals.
• Keep provider ID generation/validation lowercase while deduping orgs case-insensitively.
• Update and add tests to prevent regressions for org- and repo-scoped WIF.

Diagram

graph TD
  GH{{"GitHub OIDC token"}} --> P["Provisioner"] --> CEL["CEL attributeCondition"] --> GCP[("GCP WIF Provider")]
  P --> IAM["IAM principalSet member"] --> GCP
  P --> DEDUP["Case-insensitive dedup"] --> CEL
  subgraph Legend
    direction LR
    _ext{{"External"}} ~~~ _svc["Service"] ~~~ _plat[("GCP resource")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make CEL checks case-insensitive (lower(assertion.*) comparisons)

• ➕ Users can enter org/repo in any case and still authenticate
• ➕ Avoids relying on stored canonical casing in conditions
• ➖ Depends on available CEL string functions and their exact semantics in Google STS
• ➖ Harder to read/debug conditions; more complex expressions
• ➖ May create subtle mismatches if only one side is normalized

2. Resolve canonical casing via GitHub API before writing conditions

• ➕ Conditions always use GitHub’s canonical org/repo casing regardless of user input
• ➖ Adds network dependency and failure modes to provisioning
• ➖ Requires auth/rate-limit handling; more operational complexity

Recommendation: Keep the PR’s approach: preserve the exact canonical case in security-critical CEL literals and IAM principalSet paths, while doing internal bookkeeping (dedup/provider IDs) case-insensitively. This aligns with Google STS byte-for-byte comparisons and avoids introducing CEL-function or external-API dependencies.

Files changed (2) +91 / -50

Bug fix (1) +33 / -43

provisioner.goPreserve org/repo casing for WIF CEL conditions and IAM principalSet members +33/-43

Preserve org/repo casing for WIF CEL conditions and IAM principalSet members

• Stops lowercasing GitHub org/repo names when constructing WIF provider attributeCondition strings and IAM principalSet member paths. Introduces case-insensitive dedup/merge using lowercased map keys while preserving canonical-case values, and keeps repo provider ID generation lowercased via separate lowercase parts.

internal/dispatch/gcf/provisioner.go

Tests (1) +58 / -7

provisioner_test.goAdd/adjust tests for canonical-case preservation and case-insensitive org removal +58/-7

Add/adjust tests for canonical-case preservation and case-insensitive org removal

• Renames and updates existing tests to assert that org/repo casing is preserved in attributeCondition and IAM bindings while provider IDs remain lowercased. Adds new coverage for org-scoped case preservation, merge dedup precedence when casing differs, and case-insensitive org removal from existing conditions.

internal/dispatch/gcf/provisioner_test.go

[github-actions]

Site preview

Preview: https://5b5138cf-site.fullsend-ai.workers.dev

Commit: 9a935bdc0aa3df905778e6b999f93271bc44277e

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 WIF attributeCondition forces lowercase org/repo, rejecting auth for mixed-case GitHub orgs

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. EnsureOrg flips existing case 🐞 Bug ≡ Correctness

Description

EnsureOrgInWIFCondition/ensureWIFPoolAndProvider case-insensitively deduplicates orgs by lowercasing
the key but still overwrites the stored org string when the caller/installer provides a
different-case variant, which rewrites the emitted CEL condition literals. Because the provider’s
attributeMapping is a direct pass-through and CEL string comparisons are case-sensitive, this casing
rewrite changes which assertions match and can unintentionally revoke access that previously matched
the original casing.

Code

internal/dispatch/gcf/provisioner.go[R1281-1293]

+	merged := make(map[string]string)
	for _, o := range existingOrgs {
		if o != PlaceholderOrg {
-			merged[o] = true
+			merged[strings.ToLower(o)] = o
		}
	}
-	merged[org] = true
+	merged[strings.ToLower(org)] = org

	allOrgs := make([]string, 0, len(merged))
-	for o := range merged {
+	for _, o := range merged {
		allOrgs = append(allOrgs, o)
	}
	sort.Strings(allOrgs)

Relevance

⭐ Low

PR #1113 normalized org casing (lowercased) in WIF condition parsing/merge; team didn’t treat casing
changes as breakage.

PR-#1113

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cited logic constructs a merged map keyed by strings.ToLower(org) and then unconditionally
assigns merged[key] = org, so if an org already exists in the provider condition and a later call
provides the same org with different casing, the map value (the casing that will be emitted) is
rewritten. The rebuilt condition is produced via buildAttributeCondition, which emits
case-sensitive CEL string equality / membership literals, while the WIF provider attributeMapping
passes GitHub assertion fields through without normalizing case (no .lower()), so changing the
literal casing directly changes which assertion strings satisfy the condition.

internal/dispatch/gcf/provisioner.go[1280-1304]
internal/dispatch/gcf/provisioner.go[1099-1112]
internal/dispatch/gcf/gcp.go[239-260]
internal/dispatch/gcf/provisioner.go[1194-1220]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`EnsureOrgInWIFCondition` / `ensureWIFPoolAndProvider` is intended to be an idempotent “ensure/merge” operation, but its case-insensitive dedup currently still rewrites an existing org’s casing when the caller supplies a different-case string. Because the resulting `AttributeCondition` uses case-sensitive CEL string literals and the provider `attributeMapping` does not normalize assertion values, this implicit casing rewrite changes condition semantics and can unintentionally revoke access.

## Issue Context
This code path reads an existing WIF provider, parses orgs out of the existing condition, merges in installing/caller orgs, and rebuilds the provider’s `AttributeCondition`. The merge uses a lowercased key for deduplication, but currently overwrites the stored value for that key, allowing later inputs to change the canonical casing used in the emitted CEL condition.

## Fix Focus Areas
- internal/dispatch/gcf/provisioner.go[1194-1220]
- internal/dispatch/gcf/provisioner.go[1280-1304]

## Suggested change
- Use the lowercased key only for presence detection, but do not overwrite an existing value for that key (preserve the first-seen value, typically the existing provider’s casing): e.g., `k := strings.ToLower(org); if _, ok := merged[k]; !ok { merged[k] = org }`.
- If intentionally changing casing is a requirement, introduce an explicit override mode/flag or a separate operation (e.g., `SetOrgCasingInWIFCondition`) rather than mutating casing implicitly during ensure/merge.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Outcome: Approve

Re-review: Head advanced from 043897993b → 9a935bdc0a. Prior review was approve (provenance: app-verified). The incremental change is a single test-only commit addressing the prior review's low finding.

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Incremental Change

The new commit (9a935bdc0a) adds TestEnsureOrgInWIFCondition_ReEnrollmentInstallingCaseWins, which directly addresses the prior review's low finding about merge precedence during re-enrollment. The test verifies that when an org re-enrolls with different casing ("ACME" over existing 'acme'), the installing org's case wins — documenting the intended "installing orgs take precedence" behavior.

Reviewed Dimensions

Dimension	Result
Correctness	✅ Production code unchanged from prior approved review. New test correctly verifies re-enrollment case precedence. No test weakening detected — all assertion changes reflect the intentional behavior change (case preservation), not assertion loosening.
Security	✅ No injection risk — GitHubOrgPattern (^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$) prevents CEL metacharacters. IAM principals correctly use canonical case to match GitHub OIDC token claims. Case-insensitive dedup via lowered map keys prevents bypass. RemoveOrgFromWIFCondition uses strings.EqualFold — no fail-open.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). New commit stays within authorized scope — test addition per review feedback. fix(wif): prefix correct. Not a breaking change.
Style & conventions	✅ New test follows existing Test<FunctionName>_<ScenarioDescription> naming pattern, uses the standard NewFakeGCFClient(WithFakeWIFProvider(...)) setup, and matches assertion style of sibling tests.
Documentation	✅ Test-only incremental change — no documentation staleness.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Test Assessment

The prior review's low finding has been fully addressed. Test coverage now includes: case preservation for org-scoped and repo-scoped provisioning, case-insensitive merge dedup, case-insensitive removal, provider ID lowering, and the re-enrollment precedence scenario. The test suite exercises the critical invariants that the provider ID remains lowered (GCP requirement) while CEL conditions and IAM bindings preserve canonical case (GitHub OIDC requirement).

Previous run

Review

Outcome: Approve

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Reviewed Dimensions

Dimension	Result
Correctness	✅ Logic is sound. Merge dedup uses lowered map keys with case-preserved values. Validation checks correctly use partsLower for pattern matching while parts preserves original case for CEL/IAM. BuildRepoProviderID still receives lowercase (GCP resource IDs require it).
Security	✅ No injection risk — GitHubOrgPattern and githubRepoSlugPattern prevent CEL metacharacters. No fail-open — PlaceholderOrg fallback preserved. IAM principal paths use validated values only.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). fix(wif): prefix is correct per COMMITS.md. Not a breaking change — all-lowercase orgs are unaffected; mixed-case orgs go from broken to working.
Style & conventions	✅ map[string]string for case-insensitive dedup, strings.EqualFold for matching, copy() for array duplication — all idiomatic Go and consistent with existing codebase patterns.
Documentation	✅ No stale documentation references found.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Findings

[low] Merge precedence during re-enrollment could cause condition case change — In EnsureOrgInWIFCondition and ensureWIFPoolAndProvider, the installing org's case always overwrites the existing condition's case (by design: "Installing orgs take precedence"). This enables fixing broken conditions via re-provisioning, but also means re-enrollment with wrong case (e.g., user types acmecorp when canonical name is AcmeCorp) will change a working condition. Consider adding a test for this scenario (e.g., EnsureOrgInWIFCondition("ACME") when 'acme' already exists in the condition) to document the intended behavior. (internal/dispatch/gcf/provisioner.go, EnsureOrgInWIFCondition merge logic)

Test Assessment

Test coverage is strong: 7 tests updated or added covering case preservation for org-scoped, repo-scoped, merge dedup, case-insensitive removal, and provider ID lowering. The test TestProvisionWIF_RepoScoped_PreservesRepoCase correctly verifies the critical invariant that the provider ID is lowered while the condition and IAM binding preserve original case.

---

Labels: PR modifies WIF provisioning in the dispatch component

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/dispatch/gcf/provisioner.go	90.47%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:50 PM UTC · Completed 7:57 PM UTC
Commit: 9a935bd · View workflow run →