Skip to content

Commit 77fbde1

Browse files
Copilotpelikhan
Copilot
and
pelikhan
authored
fix: address copilot reviewer comments on github-app permissions
- parseAppConfig: log warnings for non-string permission values and non-map permissions entries instead of silently dropping them - compiler_github_mcp_steps: normalize permission level (trim+lowercase) and emit a warning for any level that isn't "read" or "none" before skipping it - validateGitHubMCPAppPermissionsNoWrite: broaden validation to reject all invalid levels (not just "write") after normalization; list both write-specific and generic invalid scopes with separate explanations in the error message - warnGitHubAppPermissionsUnsupportedContexts: new function that emits a compile-time warning when permissions is set in safe-outputs.github-app, on.github-app, or the top-level github-app fallback (where it has no effect); called from compiler.go after the no-write check - schema: keep permissions in $defs.github_app (allOf+additionalProperties:false conflict prevents scoping via allOf) but update its description to clarify it only takes effect for tools.github.github-app; scoping enforced at runtime via the new warning function - test: update write-rejection assertion strings to match the new error format Agent-Logs-Url: https://github.com/github/gh-aw/sessions/293c96f3-a7d6-461b-88c3-8ded806c935c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent 3335321 commit 77fbde1

6 files changed

Lines changed: 87 additions & 26 deletions

File tree

‎pkg/parser/schemas/main_workflow_schema.json‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3599,18 +3599,7 @@
35993599
},
36003600
"github-app": {
36013601
"$ref": "#/$defs/github_app",
3602-
"description": "GitHub App configuration for token minting. When configured, a GitHub App installation access token is minted at workflow start and used instead of the default token. This token overrides any custom github-token setting and provides fine-grained permissions matching the agent job requirements.",
3603-
"examples": [
3604-
{
3605-
"app-id": "${{ vars.APP_ID }}",
3606-
"private-key": "${{ secrets.APP_PRIVATE_KEY }}"
3607-
},
3608-
{
3609-
"app-id": "${{ vars.APP_ID }}",
3610-
"private-key": "${{ secrets.APP_PRIVATE_KEY }}",
3611-
"repositories": ["repo1", "repo2"]
3612-
}
3613-
]
3602+
"description": "GitHub App configuration for token minting. When configured, a GitHub App installation access token is minted at workflow start and used instead of the default token. This token overrides any custom github-token setting and provides fine-grained permissions matching the agent job requirements."
36143603
}
36153604
},
36163605
"additionalProperties": false,
@@ -9580,7 +9569,8 @@
95809569
}
95819570
},
95829571
"permissions": {
9583-
"$ref": "#/$defs/github_app_permissions"
9572+
"$ref": "#/$defs/github_app_permissions",
9573+
"description": "Optional extra GitHub App-only permissions to merge into the minted token. Only takes effect for tools.github.github-app; ignored in other github-app contexts."
95849574
}
95859575
},
95869576
"required": ["app-id", "private-key"],

‎pkg/workflow/compiler.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,9 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
156156
return formatCompilerError(markdownPath, "error", err.Error(), err)
157157
}
158158

159+
// Warn when github-app.permissions is set in contexts that don't support it
160+
warnGitHubAppPermissionsUnsupportedContexts(workflowData)
161+
159162
// Validate agent file exists if specified in engine config
160163
log.Printf("Validating agent file if specified")
161164
if err := c.validateAgentFile(workflowData, markdownPath); err != nil {

‎pkg/workflow/compiler_github_mcp_steps.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,13 @@ func (c *Compiler) generateGitHubMCPAppTokenMintingStep(yaml *strings.Builder, d
114114
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
115115
continue
116116
}
117-
permissions.Set(scope, PermissionLevel(val))
117+
level := strings.ToLower(strings.TrimSpace(val))
118+
if level != string(PermissionRead) && level != string(PermissionNone) {
119+
msg := fmt.Sprintf("Unknown permission level %q for scope %q in tools.github.github-app.permissions. Valid levels are: read, none.", val, key)
120+
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
121+
continue
122+
}
123+
permissions.Set(scope, PermissionLevel(level))
118124
}
119125
}
120126

‎pkg/workflow/github_app_permissions_validation.go‎

Lines changed: 68 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,12 @@ package workflow
22

33
import (
44
"errors"
5+
"fmt"
6+
"os"
57
"sort"
68
"strings"
9+
10+
"github.com/github/gh-aw/pkg/console"
711
)
812

913
var githubAppPermissionsLog = newValidationLogger("github_app_permissions")
@@ -109,11 +113,12 @@ func formatWriteOnAppScopesError(scopes []PermissionScope) error {
109113
return errors.New(strings.Join(lines, "\n"))
110114
}
111115

112-
// validateGitHubMCPAppPermissionsNoWrite validates that no scope in
113-
// tools.github.github-app.permissions is set to "write".
116+
// validateGitHubMCPAppPermissionsNoWrite validates that every scope in
117+
// tools.github.github-app.permissions is set to "read" or "none" (after trimming/lowercasing).
114118
// The schema allows "write" so that editors can offer it as a completion, but
115119
// the compiler must reject it because GitHub App-only scopes have no write-level
116120
// semantics in this context — write operations must go through safe-outputs.
121+
// Any other unrecognised level is also rejected here.
117122
func validateGitHubMCPAppPermissionsNoWrite(workflowData *WorkflowData) error {
118123
if workflowData.ParsedTools == nil ||
119124
workflowData.ParsedTools.GitHub == nil ||
@@ -125,32 +130,84 @@ func validateGitHubMCPAppPermissionsNoWrite(workflowData *WorkflowData) error {
125130
return nil
126131
}
127132

133+
var invalidScopes []string
128134
var writeScopes []string
129135
for scope, level := range app.Permissions {
130-
if level == string(PermissionWrite) {
131-
writeScopes = append(writeScopes, scope)
136+
normalized := strings.ToLower(strings.TrimSpace(level))
137+
switch normalized {
138+
case string(PermissionRead), string(PermissionNone):
139+
// valid
140+
default:
141+
invalidScopes = append(invalidScopes, scope+" (level: "+level+")")
142+
if normalized == string(PermissionWrite) {
143+
writeScopes = append(writeScopes, scope)
144+
}
132145
}
133146
}
134-
if len(writeScopes) == 0 {
147+
if len(invalidScopes) == 0 {
135148
return nil
136149
}
150+
sort.Strings(invalidScopes)
137151
sort.Strings(writeScopes)
138152

139153
var lines []string
140-
lines = append(lines, `"write" is not allowed in tools.github.github-app.permissions.`)
141-
lines = append(lines, "GitHub App-only scopes in this section must be declared as \"read\" or \"none\".")
142-
lines = append(lines, "Write operations must be performed via safe-outputs, not through declared permissions.")
154+
lines = append(lines, "Invalid permission levels in tools.github.github-app.permissions.")
155+
lines = append(lines, "Each permission level must be exactly \"read\" or \"none\".")
156+
if len(writeScopes) > 0 {
157+
lines = append(lines, "")
158+
lines = append(lines, `"write" is not allowed: write operations must be performed via safe-outputs.`)
159+
lines = append(lines, "")
160+
lines = append(lines, "The following scopes were declared with \"write\" access:")
161+
lines = append(lines, "")
162+
for _, s := range writeScopes {
163+
lines = append(lines, " - "+s)
164+
}
165+
}
143166
lines = append(lines, "")
144-
lines = append(lines, "The following scopes were declared with \"write\" access:")
167+
lines = append(lines, "The following scopes have invalid permission levels:")
145168
lines = append(lines, "")
146-
for _, s := range writeScopes {
169+
for _, s := range invalidScopes {
147170
lines = append(lines, " - "+s)
148171
}
149172
lines = append(lines, "")
150-
lines = append(lines, "Change the permission level to \"read\" for read-only access, or remove the entry.")
173+
lines = append(lines, "Change the permission level to \"read\" for read-only access, or \"none\" to disable the scope.")
151174
return errors.New(strings.Join(lines, "\n"))
152175
}
153176

177+
// warnGitHubAppPermissionsUnsupportedContexts emits a warning when
178+
// tools.github.github-app.permissions is set in contexts that do not support it.
179+
// The permissions field only takes effect for the GitHub MCP token minting step;
180+
// it is silently ignored if set on safe-outputs.github-app, on.github-app, or the
181+
// top-level github-app fallback.
182+
func warnGitHubAppPermissionsUnsupportedContexts(workflowData *WorkflowData) {
183+
type context struct {
184+
label string
185+
app *GitHubAppConfig
186+
}
187+
unsupported := []context{
188+
{"safe-outputs.github-app", safeOutputsGitHubApp(workflowData)},
189+
{"on.github-app", workflowData.ActivationGitHubApp},
190+
{"github-app (top-level fallback)", workflowData.TopLevelGitHubApp},
191+
}
192+
for _, ctx := range unsupported {
193+
if ctx.app != nil && len(ctx.app.Permissions) > 0 {
194+
msg := fmt.Sprintf(
195+
"The 'permissions' field under '%s' has no effect. "+
196+
"Extra GitHub App permissions only apply to tools.github.github-app.",
197+
ctx.label,
198+
)
199+
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
200+
}
201+
}
202+
}
203+
204+
func safeOutputsGitHubApp(workflowData *WorkflowData) *GitHubAppConfig {
205+
if workflowData.SafeOutputs == nil {
206+
return nil
207+
}
208+
return workflowData.SafeOutputs.GitHubApp
209+
}
210+
154211
func hasGitHubAppConfigured(workflowData *WorkflowData) bool {
155212
// Check tools.github.github-app
156213
if workflowData.ParsedTools != nil &&

‎pkg/workflow/github_mcp_app_token_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,7 @@ Test that write is rejected in tools.github.github-app.permissions.
525525

526526
err = compiler.CompileWorkflow(testFile)
527527
require.Error(t, err, "Compiler should reject write in tools.github.github-app.permissions")
528-
assert.Contains(t, err.Error(), `"write" is not allowed in tools.github.github-app.permissions`, "Error should mention that write is not allowed")
528+
assert.Contains(t, err.Error(), "Invalid permission levels in tools.github.github-app.permissions", "Error should mention invalid permission levels")
529+
assert.Contains(t, err.Error(), `"write" is not allowed`, "Error should mention that write is not allowed")
529530
assert.Contains(t, err.Error(), "members", "Error should mention the offending scope")
530531
}

‎pkg/workflow/safe_outputs_app_config.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,12 @@ func parseAppConfig(appMap map[string]any) *GitHubAppConfig {
7373
for key, val := range permsMap {
7474
if valStr, ok := val.(string); ok {
7575
appConfig.Permissions[key] = valStr
76+
} else {
77+
safeOutputsAppLog.Printf("Ignoring github-app.permissions[%q]: expected string value, got %T", key, val)
7678
}
7779
}
80+
} else {
81+
safeOutputsAppLog.Printf("Ignoring github-app.permissions: expected object, got %T", perms)
7882
}
7983
}
8084

0 commit comments

Comments
 (0)