Skip to content

Prevent invalid id-token: read emission when merging permissions: all: read with explicit overrides #43297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jul 4, 2026
+13 −0
Merged

Prevent invalid id-token: read emission when merging permissions: all: read with explicit overrides #43297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff48290
Initial plan
Copilot Jul 4, 2026
2c0adce
Fix all:read expansion to skip id-token in Set path
Copilot Jul 4, 2026
8b91607
Merge remote-tracking branch 'origin/main' into copilot/fix-id-token-…
Copilot Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/workflow/permissions_operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ func (p *Permissions) Set(scope PermissionScope, level PermissionLevel) {
// Expand all permissions to explicit permissions first
for _, s := range GetAllPermissionScopes() {
if _, exists := p.permissions[s]; !exists {
// id-token does not support the read level
if s == PermissionIdToken && p.allLevel == PermissionRead {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] The guard is correct and well-placed. One minor note: the comment says id-token does not support the read level — it would be more precise to say it supports only write and none (not any non-write level), which matches how GitHub Actions actually validates permissions.

💡 Suggested comment wording 
// id-token only supports "write" and "none"; emitting "read" causes GitHub Actions to reject
// the workflow. Skip it entirely when expanding from all: read.

This mirrors the comment in permissions_rendering.go (if one exists) and makes the rule concrete for future readers — clarifying that this isn't a gh-aw policy but a GitHub Actions constraint.

@copilot please address this.

continue
}
p.permissions[s] = p.allLevel

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PermissionDiscussions has the same gap this PR just closed for PermissionIdToken: Set() expands all: read into an explicit map but does not skip discussions, causing discussions: read to materialize in the explicit map where the RenderToYAML guard no longer fires.

💡 Explanation and suggested fix

When Set() is called on an all: read Permissions, it expands all scopes into p.permissions. This PR correctly skips id-token during that expansion. However, RenderToYAML also suppresses discussions: read (GHE compatibility) — but only when rendering through the hasAll branch. Once Set() has materialized discussions: read into the explicit map, the RenderToYAML guard on the hasAll branch never runs, and the scope leaks into the final YAML.

Add the matching guard alongside the id-token one in the hasAll block:

// id-token does not support the read level
if s == PermissionIdToken && p.allLevel == PermissionRead {
    continue
}
// discussions: read is suppressed for GHE compatibility (only include if explicitly set)
if s == PermissionDiscussions && p.allLevel == PermissionRead {
    continue
}

The test added by this PR should also assert that discussions is absent from the converted map.

}
}
Expand Down
9 changes: 9 additions & 0 deletions pkg/workflow/permissions_operations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ func TestPermissionsSet(t *testing.T) {
if !exists || level != PermissionWrite {
t.Errorf("expected issues: write, got %v (exists: %v)", level, exists)
}

p3 := NewPermissionsAllRead()

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The new test case is embedded flat inside TestPermissionsSet, while the existing test for the shorthand conversion case (TestPermissionsSetPreservesShorthandPermissions) uses a structured table-driven pattern. Embedding p3 inline makes it harder to see what scenario is being tested.

💡 Suggested refactor

Either move this case into TestPermissionsSetPreservesShorthandPermissions as a new table row, or at minimum add a descriptive comment block:

// all: read + explicit write scope must not expand id-token to any level
p3 := NewPermissionsAllRead()
p3.Set(PermissionCopilotRequests, PermissionWrite)

Even better, consider adding a parallel entry in TestPermissionsSetPreservesShorthandPermissions that tests the all: read case with an assertion for PermissionIdToken absent, keeping all permission-preservation tests in one place.

@copilot please address this.

p3.Set(PermissionCopilotRequests, PermissionWrite)
if _, exists := p3.Get(PermissionIdToken); exists {
t.Error("expected id-token to be excluded when converting all: read to explicit map")
}
if yaml := p3.RenderToYAML(); strings.Contains(yaml, "id-token: read") {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The YAML assertion only guards against id-token: read, but the intent is that id-token should be entirely absent — any other accidental level (e.g. none) would slip through.

💡 Stronger assertion

Change the assertion to check for any id-token: entry, not just the read level:

if yaml := p3.RenderToYAML(); strings.Contains(yaml, "id-token:") {
    t.Errorf("RenderToYAML() should not contain any id-token entry, got:\n%s", yaml)
}

This matches the intent of the fix — id-token should be completely absent from the output when starting from all: read.

@copilot please address this.

t.Errorf("RenderToYAML() should not contain id-token: read, got:\n%s", yaml)
}

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test does not assert the boundary case: all: write still correctly expands id-token: write: The test only verifies that id-token is absent after all: read conversion, but does not verify the complementary case — that the guard is not applied when allLevel == PermissionWrite, which could silently regress.

💡 Suggested additional assertion

Add a case verifying the guard is not over-applied:

// Guard must NOT fire at write level — id-token: write is valid
p4 := &Permissions{hasAll: true, allLevel: PermissionWrite, permissions: make(map[PermissionScope]PermissionLevel)}
p4.Set(PermissionCopilotRequests, PermissionWrite)
if level, exists := p4.Get(PermissionIdToken); !exists || level != PermissionWrite {
    t.Errorf("expected id-token: write after all: write expansion, got level=%q exists=%v", level, exists)
}

Without this, a future change that over-broadly guards PermissionIdToken for all levels would pass the current test suite.

}

// TestPermissionsSetPreservesShorthandPermissions verifies that calling Set() on a Permissions
Expand Down
Loading