Add compilation warning for id-token: write permission#15026
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a compiler warning when a workflow frontmatter grants permissions: id-token: write, to highlight OIDC security considerations during workflow compilation.
Changes:
- Detect
id-token: writein compiled workflow permissions and emit a formatted compiler warning while incrementing warning count. - Add integration tests validating warning presence/absence across permission combinations and verifying message contents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/workflow/compiler.go | Adds detection of id-token: write and emits a compiler warning + increments warning count. |
| pkg/workflow/idtoken_write_warning_test.go | Introduces integration tests that compile sample workflows and assert warning output/count behavior. |
Comments suppressed due to low confidence (2)
pkg/workflow/idtoken_write_warning_test.go:200
os.Pipe()errors are ignored here (r, w, _ := os.Pipe()), which can lead to nil pipes and misleading test failures. Handle the returned error and fail the test if pipe creation fails.
r, w, _ := os.Pipe()
os.Stderr = w
pkg/workflow/idtoken_write_warning_test.go:210
- The read end of the pipe (
r) is never closed andio.Copyerrors are ignored. Closerand check theio.Copyreturn error to avoid FD leaks and silent read failures.
var buf bytes.Buffer
io.Copy(&buf, r)
stderrOutput := buf.String()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| warningMsg := `This workflow grants id-token: write permission | ||
| OIDC tokens can authenticate to cloud providers (AWS, Azure, GCP). | ||
| Ensure proper audience validation and trust policies are configured.` |
There was a problem hiding this comment.
The raw string literal for warningMsg includes leading spaces on the 2nd/3rd lines due to code indentation, which will render as extra indentation in the warning output and won’t match the example output in the PR description. Consider left-aligning the message text (or trimming indentation) so the emitted warning is clean and consistent.
| warningMsg := `This workflow grants id-token: write permission | |
| OIDC tokens can authenticate to cloud providers (AWS, Azure, GCP). | |
| Ensure proper audience validation and trust policies are configured.` | |
| warningMsg := "This workflow grants id-token: write permission\n" + | |
| "OIDC tokens can authenticate to cloud providers (AWS, Azure, GCP).\n" + | |
| "Ensure proper audience validation and trust policies are configured." |
| if workflowData.Permissions != "" { | ||
| permissions := NewPermissionsParser(workflowData.Permissions).ToPermissions() | ||
| if permissions != nil { |
There was a problem hiding this comment.
NewPermissionsParser(workflowData.Permissions).ToPermissions() is invoked again here even though permissions were already parsed earlier in this function for MCP toolset validation. Consider parsing once and reusing the result to avoid duplicated logic and keep warning/validation behavior consistent.
|
|
||
| // Capture stderr to check for warnings | ||
| oldStderr := os.Stderr | ||
| r, w, _ := os.Pipe() |
There was a problem hiding this comment.
os.Pipe() errors are ignored here (r, w, _ := os.Pipe()), which can lead to nil pipes and misleading test failures. Handle the returned error and fail the test if pipe creation fails.
This issue also appears on line 198 of the same file.
| r, w, _ := os.Pipe() | |
| r, w, pipeErr := os.Pipe() | |
| if pipeErr != nil { | |
| t.Fatalf("failed to create stderr pipe: %v", pipeErr) | |
| } |
| r, w, _ := os.Pipe() | ||
| os.Stderr = w | ||
|
|
||
| compiler := NewCompiler() | ||
| compiler.SetStrictMode(false) | ||
| err := compiler.CompileWorkflow(testFile) | ||
|
|
||
| // Restore stderr | ||
| w.Close() | ||
| os.Stderr = oldStderr | ||
| var buf bytes.Buffer | ||
| io.Copy(&buf, r) |
There was a problem hiding this comment.
The read end of the pipe (r) is never closed and io.Copy errors are ignored. Close r (typically with defer r.Close() after successful os.Pipe()) and check the io.Copy return error to avoid FD leaks and silent read failures in CI.
This issue also appears on line 208 of the same file.
| r, w, _ := os.Pipe() | |
| os.Stderr = w | |
| compiler := NewCompiler() | |
| compiler.SetStrictMode(false) | |
| err := compiler.CompileWorkflow(testFile) | |
| // Restore stderr | |
| w.Close() | |
| os.Stderr = oldStderr | |
| var buf bytes.Buffer | |
| io.Copy(&buf, r) | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| t.Fatalf("failed to create stderr pipe: %v", err) | |
| } | |
| defer r.Close() | |
| os.Stderr = w | |
| compiler := NewCompiler() | |
| compiler.SetStrictMode(false) | |
| err = compiler.CompileWorkflow(testFile) | |
| // Restore stderr | |
| w.Close() | |
| os.Stderr = oldStderr | |
| var buf bytes.Buffer | |
| if _, copyErr := io.Copy(&buf, r); copyErr != nil { | |
| t.Fatalf("failed to read from stderr pipe: %v", copyErr) | |
| } |
| // Emit warning if id-token: write permission is detected | ||
| log.Printf("Checking for id-token: write permission") | ||
| if workflowData.Permissions != "" { |
There was a problem hiding this comment.
log.Printf("Checking for id-token: write permission") will be written to stderr by the standard logger, and it contains the exact substring id-token: write. That makes stderr-based warning detection (including the new tests) flaky and also pollutes compiler output. Please remove this log line or gate it behind an explicit verbose/debug flag and avoid emitting id-token: write verbatim in logs.
See below for a potential fix:
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message))
c.IncrementWarningCount()
}
}
}
}
}
// Emit warning if id-token: write permission is detected
Workflows using
id-token: writepermission now emit a compilation warning to inform developers about OIDC authentication security considerations.Changes
pkg/workflow/compiler.go): Added detection forid-token: writepermission after GitHub MCP toolsets validationpkg/workflow/idtoken_write_warning_test.go): Coverage for warning presence/absence across permission combinationsExample Output
When compiling a workflow with
id-token: write:Produces:
The warning uses the standard compiler message format and increments the warning counter.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.