Skip to content

fix(#910): use repository_owner in WIF attribute conditions - #920

Merged
waynesun09 merged 2 commits into
mainfrom
agent/910-wif-repository-owner
May 14, 2026
Merged

fix(#910): use repository_owner in WIF attribute conditions#920
waynesun09 merged 2 commits into
mainfrom
agent/910-wif-repository-owner

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

The WIF attribute condition used assertion.repository scoped to the .fullsend repo, which rejected OIDC tokens from cross-repo dispatched workflows. When a shim workflow in another repo (e.g. org/my-app) calls .fullsend via workflow_call, GitHub sets the repository claim to the caller repo, causing GCP STS to return HTTP 400.

Changed buildAttributeCondition to use assertion.repository_owner instead of assertion.repository, scoping to the org level. Also updated the per-repo path in ProvisionWIF to use repository_owner with just the owner portion. This is safe because the mint's prevalidateOIDCToken already validates org membership, allowed workflow files, and workflow ref prefixes.

parseConditionOrgs retains backward compatibility with old assertion.repository conditions via the fullsendRepoSuffix check.

Added TestBuildAttributeCondition unit test covering single-org and multi-org cases.

Note: pre-commit could not run due to Go toolchain permission error in sandbox. All Go tests pass.


Closes #910

Post-script verification

  • Branch is not main/master (agent/910-wif-repository-owner)
  • Secret scan passed (gitleaks — d0b625254fe4044c99ae9f3b6e9ec5f5b0084e98..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

The WIF attribute condition used assertion.repository scoped to
the .fullsend repo, which rejected OIDC tokens from cross-repo
dispatched workflows. When a shim workflow in another repo (e.g.
org/my-app) calls .fullsend via workflow_call, GitHub sets the
repository claim to the caller repo, causing GCP STS to return
HTTP 400.

Changed buildAttributeCondition to use assertion.repository_owner
instead of assertion.repository, scoping to the org level. Also
updated the per-repo path in ProvisionWIF to use repository_owner
with just the owner portion. This is safe because the mint's
prevalidateOIDCToken already validates org membership, allowed
workflow files, and workflow ref prefixes.

parseConditionOrgs retains backward compatibility with old
assertion.repository conditions via the fullsendRepoSuffix check.

Added TestBuildAttributeCondition unit test covering single-org
and multi-org cases.

Note: pre-commit could not run due to Go toolchain permission
error in sandbox. All Go tests pass.

Closes #910
@github-actions

Copy link
Copy Markdown

fullsend review is working on this — view logs

@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown

Site preview

Preview: https://306b443d-site.fullsend-ai.workers.dev

Commit: 8325945671dd74adf7d2f64758958326974143f8

@fullsend-ai-review

fullsend-ai-review Bot commented May 14, 2026

Copy link
Copy Markdown

Review: #920

Head SHA: 8325945
Timestamp: 2026-05-14T12:00:00Z
Outcome: approve

Summary

This is a clean, well-scoped fix that correctly addresses the WIF attribute condition rejecting cross-repo OIDC tokens. The change widens the WIF CEL condition from assertion.repository == 'org/.fullsend' to assertion.repository_owner == 'org', which is safe because the mint's prevalidateOIDCToken independently validates org membership, workflow ref scope, and workflow file allowlist. Tests are comprehensive, backward compatibility is preserved in parseConditionOrgs, and no security concerns were found.

Findings

Low

  • [Intent alignment] PR description — The PR body states "Also updated the per-repo path in ProvisionWIF to use repository_owner with just the owner portion" but the diff does not include this change. Line 784 of provisioner.go is unchanged. This is a description inaccuracy rather than a missing code change — the per-repo path correctly uses assertion.repository since the OIDC repository claim matches p.cfg.Repo for per-repo installs.

Info

  • [Security] provisioner.go — The WIF scope widening from repo-level to org-level is validated as safe. prevalidateOIDCToken (mint/main.go:524-612) checks org membership via checkAllowedOrg, restricts job_workflow_ref to .fullsend or upstream workflows, and enforces an allowlist of workflow files. WIF is defense-in-depth, not the sole gate.

  • [Correctness] provisioner.go:675-691parseConditionOrgs correctly handles both the new assertion.repository_owner == 'org' format and the legacy assertion.repository == 'org/.fullsend' format, ensuring backward compatibility with existing WIF providers.

Footer

Outcome: approve
This review applies to SHA 8325945671dd74adf7d2f64758958326974143f8. Any push to the PR head clears this review and requires a new evaluation.

Previous run

Review: #920

Head SHA: 6bd13fc
Timestamp: 2026-05-14T12:00:00Z
Outcome: request-changes

Summary

The org-level buildAttributeCondition fix correctly addresses the cross-repo dispatch OIDC failure described in #910. However, the per-repo path in ProvisionWIF widens the WIF condition to org-scope while leaving the IAM principal binding repo-scoped, creating an inconsistency that either leaves the per-repo cross-repo dispatch case unfixed or unnecessarily widens the WIF gate. This needs clarification before merge.

Findings

High

  • [correctness] internal/dispatch/gcf/provisioner.go:784 — Per-repo WIF condition widened to org-level (repository_owner == 'acme') but the IAM binding on line 801 still uses attribute.repository/acme/widget. For cross-repo dispatch, a caller repo (e.g. acme/my-app) would authenticate via WIF but receive identity attribute.repository/acme/my-app, which has no IAM role binding. This is either an incomplete fix (IAM binding should also be updated) or an unnecessary WIF widening (if per-repo installs don't use cross-repo dispatch).
    Remediation: If per-repo installs require cross-repo dispatch, update the IAM binding to use an org-scoped principal. If not, keep the per-repo WIF condition as assertion.repository == '<repo>' and only widen the org-level path.

Medium

  • [style/conventions] internal/dispatch/gcf/provisioner.go:672-674 — The parseConditionOrgs doc comment labels the repository_owner format as "legacy org-scoped" when this PR makes it the primary format. The terminology is now inverted and will confuse future readers.
    Remediation: Update the comment to reflect that repository_owner is the current format and assertion.repository is the legacy format.

Footer

Outcome: request-changes
This review applies to SHA 6bd13fc5d1701fe066ff7b4f634dd8e5201cb4af. Any push to the PR head clears this review and requires a new evaluation.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

@github-actions

Copy link
Copy Markdown

fullsend fix is working on this — view logs

- Revert per-repo ProvisionWIF path to use assertion.repository == '<repo>'
  instead of assertion.repository_owner. Per-repo installs don't use
  cross-repo dispatch, and widening to org-level would be inconsistent
  with the repo-scoped IAM principal binding.
- Update parseConditionOrgs doc comment to label repository_owner as the
  current format and assertion.repository as legacy, matching the new
  org-level default from this PR.

Addresses review feedback on #920

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed both review findings: (1) reverted per-repo WIF condition to repo-scoped assertion.repository to match the IAM binding, (2) fixed parseConditionOrgs comment terminology to reflect repository_owner as current format.

Fixed (2):

  1. Per-repo WIF condition widened to org-level inconsistent with repo-scoped IAM binding (internal/dispatch/gcf/provisioner.go): Reverted per-repo ProvisionWIF path to use assertion.repository == '' instead of assertion.repository_owner. Per-repo installs don't use cross-repo dispatch, so the WIF condition should stay repo-scoped to match the IAM principal binding.
  2. parseConditionOrgs doc comment labels repository_owner as legacy when it is now the primary format (internal/dispatch/gcf/provisioner.go): Updated comment to label repository_owner as the current format and assertion.repository as the legacy format, matching the new org-level default.

Tests: passed

Decision points
  • Chose to revert per-repo path rather than update IAM binding to org-scoped (alternatives: Update IAM binding to org-scoped principal to match widened WIF condition; rationale: Per-repo installs don't need cross-repo dispatch (that's the org-level concern from WIF attribute condition rejects OIDC tokens from non-.fullsend repos, causing STS 400 #910). Keeping the per-repo path repo-scoped is the minimal, correct fix that avoids unnecessary permission widening.)

Updated by fullsend fix agent

@github-actions

Copy link
Copy Markdown

fullsend review is working on this — view logs

@waynesun09
waynesun09 added this pull request to the merge queue May 14, 2026
Merged via the queue into main with commit b3caf3f May 14, 2026
25 checks passed
@waynesun09
waynesun09 deleted the agent/910-wif-repository-owner branch May 14, 2026 15:42
waynesun09 added a commit that referenced this pull request May 14, 2026
- ADR 0033: document FULLSEND_PER_REPO_INSTALL guard variable in
  coexistence section, update migration steps, fix stale CLI flags
  (--gcp-auth-mode removed in PR #905), update repo variables list
- Installation guide: fix WIF attribute condition from
  assertion.repository == 'org/.fullsend' to
  assertion.repository_owner == 'org' (PR #920 fix), fix WIF
  principal from attribute.repository/org/.fullsend to
  attribute.repository_owner/org

Signed-off-by: Wayne Sun <gsun@redhat.com>
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.

WIF attribute condition rejects OIDC tokens from non-.fullsend repos, causing STS 400

1 participant