Skip to content

Fix safe-outputs YAML corruption when OTLP header masking is enabled #37068

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 5 commits into from
Jun 5, 2026
+160 −1
Merged

Fix safe-outputs YAML corruption when OTLP header masking is enabled #37068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5071ce1
Initial plan
Copilot Jun 5, 2026
cdbaf26
Fix safe-outputs GitHub App insertion when OTLP masking is enabled
Copilot Jun 5, 2026
1ecc9f8
Harden safe-outputs token insertion boundary logic
Copilot Jun 5, 2026
7cd1c84
Clarify safe-outputs insertion regression guards
Copilot Jun 5, 2026
4f9fa25
Address review feedback: add tests, warning log, and dynamic offset c…
Copilot Jun 5, 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
25 changes: 25 additions & 0 deletions pkg/workflow/compiler_safe_outputs_job.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ import (

var consolidatedSafeOutputsJobLog = logger.New("workflow:compiler_safe_outputs_job")

// stepNameLinePrefix matches the canonical YAML line emitted by this compiler for
// step starts in job.Steps (6-space indent + "- name: ").
const stepNameLinePrefix = " - name: "

// buildConsolidatedSafeOutputsJob builds a single job containing all safe output operations
// as separate steps within that job. This reduces the number of jobs in the workflow
// while maintaining observability through distinct step names, IDs, and outputs.
Expand Down Expand Up @@ -445,6 +449,15 @@ func (c *Compiler) buildSafeOutputsJobFromParts(
countParentSpanID := setupParentSpanNeedsExpr(constants.ActivationJobName)
insertIndex += len(c.generateSetupStep(data, setupActionRef, SetupActionDestination, data.SafeOutputs != nil && data.SafeOutputs.UploadArtifact != nil, countTraceID, countParentSpanID))
}
// Keep insertion index aligned with setup steps that may be injected between setup
// and artifact downloads. Count the entries each mask helper appends so the index
// stays self-consistent if either helper ever emits more than one step.
if isOTLPHeadersPresent(data) {
insertIndex += strings.Count(generateOTLPHeadersMaskStep(), stepNameLinePrefix)
}
if isOTLPAttributesPresent(data) {
insertIndex += strings.Count(generateOTLPAttributesMaskStep(), stepNameLinePrefix)
}
Comment on lines +455 to +460
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

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

[/diagnose] The insertIndex++ increments are load-bearing: each assumes the corresponding mask helper appends exactly one string entry to the steps slice. The comment documents this ("Each mask helper currently appends one string entry"), but if generateOTLPHeadersMaskStep() or generateOTLPAttributesMaskStep() ever grow to emit two entries (e.g. an added env-echo step), the arithmetic silently undercounts and the while-loop guard must compensate.

💡 More robust alternative

The offset could be derived from the actual number of entries appended, keeping the arithmetic self-consistent regardless of how many lines each helper emits:

if isOTLPHeadersPresent(data) {
    mask := generateOTLPHeadersMaskStep()
    // len == 1 today, but this survives future growth
    insertIndex += strings.Count(mask, "      - name: ")
}

Or expose a step-count constant alongside each generator. Either approach removes the silent undercounting risk.


// Add artifact download steps count
insertIndex += len(buildAgentOutputDownloadSteps(agentArtifactPrefix, c.getActionPin))
Expand All @@ -469,6 +482,18 @@ func (c *Compiler) buildSafeOutputsJobFromParts(

// Note: App token step must be inserted BEFORE shared checkout steps
// because those steps reference steps.safe-outputs-app-token.outputs.token
//
// The insertion index is line-oriented; if it lands in the middle of a
// multi-line run/with block, move it to the next step boundary.
for insertIndex < len(steps) && !strings.HasPrefix(steps[insertIndex], stepNameLinePrefix) {
insertIndex++
}
Comment on lines +488 to +490
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

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

[/diagnose] If the loop exhausts steps without finding a stepNameLinePrefix line, insertIndex equals len(steps) and the app-token block is silently appended at the very end — after checkout and safe-output steps that reference steps.safe-outputs-app-token.outputs.token. The YAML would be structurally valid but semantically broken, and there is no log message or error to surface it.

💡 Suggested guard

A short diagnostic log after the loop would make this case visible:

for insertIndex < len(steps) && !strings.HasPrefix(steps[insertIndex], stepNameLinePrefix) {
    insertIndex++
}
if insertIndex == len(steps) {
    consolidatedSafeOutputsJobLog.Printf(
        "WARN: app-token insertion reached end of steps slice (len=%d); step ordering may be incorrect",
        len(steps),
    )
}

This is a rare edge case today, but becomes more likely if future step sequences are ever added that push the final checkout step past the end of the current emission order.

if insertIndex == len(steps) {
consolidatedSafeOutputsJobLog.Printf(
"WARN: app-token insertion reached end of steps slice (len=%d); step ordering may be incorrect",
len(steps),
)
}

// Insert app token steps
var newSteps []string
Expand Down
136 changes: 135 additions & 1 deletion pkg/workflow/compiler_safe_outputs_job_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,7 +813,141 @@ func TestGitHubAppWithPushToPRBranch(t *testing.T) {
assert.Contains(t, stepsContent, "id: safe-outputs-app-token")
}

// TestJobWithGitHubAppWorkflowCallUsesTargetRepoNameFallback is a regression test verifying that
// TestGitHubAppTokenStepWithOTLPHeaders ensures the app token step insertion point
// stays aligned when OTLP header masking steps are present.
func TestGitHubAppTokenStepWithOTLPHeaders(t *testing.T) {
compiler := NewCompiler()
compiler.jobManager = NewJobManager()

workflowData := &WorkflowData{
Name: "Test Workflow",
Env: `"env":
OTEL_EXPORTER_OTLP_HEADERS: "Authorization=******"`,
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

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

[/tdd] The regression test only exercises the isOTLPHeadersPresent path (OTEL_EXPORTER_OTLP_HEADERS). The isOTLPAttributesPresent path (GH_AW_OTLP_ATTRIBUTES) has no coverage — but the fix also increments insertIndex for it.

💡 Suggested addition

Add a second test (or extend this one) with GH_AW_OTLP_ATTRIBUTES set in Env, repeating the same ordering assertions. Then add a third scenario with both env vars set to confirm insertIndex is incremented twice correctly.

Env: `"env":
  OTEL_EXPORTER_OTLP_HEADERS: "Authorization=******"
  GH_AW_OTLP_ATTRIBUTES: "{\"service.name\":\"test\"}"`

Without this, a regression in the attributes path would go undetected.

SafeOutputs: &SafeOutputsConfig{
GitHubApp: &GitHubAppConfig{
AppID: "${{ vars.ACTIONS_APP_ID }}",
PrivateKey: "${{ secrets.ACTIONS_PRIVATE_KEY }}",
},
AddComments: &AddCommentsConfig{},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
},
},
}

job, _, err := compiler.buildConsolidatedSafeOutputsJob(workflowData, string(constants.AgentJobName), "test.md")
require.NoError(t, err, "Should successfully build job")
require.NotNil(t, job, "Job should not be nil")

stepsContent := strings.Join(job.Steps, "")
maskIndex := strings.Index(stepsContent, "Mask OTLP telemetry headers")
downloadIndex := strings.Index(stepsContent, "Download agent output artifact")
setupEnvEchoIndex := strings.Index(stepsContent, "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json")
tokenIndex := strings.Index(stepsContent, "Generate GitHub App token")

require.NotEqual(t, -1, maskIndex, "OTLP headers mask step should be present")
require.NotEqual(t, -1, downloadIndex, "agent output download step should be present")
require.NotEqual(t, -1, setupEnvEchoIndex, "setup-agent-output-env run command should be present")
require.NotEqual(t, -1, tokenIndex, "GitHub App token step should be present")

assert.Less(t, maskIndex, downloadIndex, "masking should happen before artifact download")
assert.Less(t, setupEnvEchoIndex, tokenIndex, "app token step must be inserted after setup-agent-output-env run block")
// Regression guard: app token insertion must not split setup-agent-output-env and leave
// an `echo ...` run line nested under a previous `with:` block.
assert.NotContains(t, stepsContent, "with:\n echo \"GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json\" >> \"$GITHUB_OUTPUT\"",
"app token step must not split setup-agent-output-env into a nested echo under with:")
}

// TestGitHubAppTokenStepWithOTLPAttributes ensures the app token step insertion point
// stays aligned when the OTLP attributes masking step is present (GH_AW_OTLP_ATTRIBUTES path).
func TestGitHubAppTokenStepWithOTLPAttributes(t *testing.T) {
compiler := NewCompiler()
compiler.jobManager = NewJobManager()

workflowData := &WorkflowData{
Name: "Test Workflow",
Env: `"env":
GH_AW_OTLP_ATTRIBUTES: "{\"service.name\":\"test\"}"`,
SafeOutputs: &SafeOutputsConfig{
GitHubApp: &GitHubAppConfig{
AppID: "${{ vars.ACTIONS_APP_ID }}",
PrivateKey: "${{ secrets.ACTIONS_PRIVATE_KEY }}",
},
AddComments: &AddCommentsConfig{},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
},
},
}

job, _, err := compiler.buildConsolidatedSafeOutputsJob(workflowData, string(constants.AgentJobName), "test.md")
require.NoError(t, err, "Should successfully build job")
require.NotNil(t, job, "Job should not be nil")

stepsContent := strings.Join(job.Steps, "")
attrMaskIndex := strings.Index(stepsContent, "Mask OTLP custom attribute values")
downloadIndex := strings.Index(stepsContent, "Download agent output artifact")
setupEnvEchoIndex := strings.Index(stepsContent, "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json")
tokenIndex := strings.Index(stepsContent, "Generate GitHub App token")

require.NotEqual(t, -1, attrMaskIndex, "OTLP attributes mask step should be present")
require.NotEqual(t, -1, downloadIndex, "agent output download step should be present")
require.NotEqual(t, -1, setupEnvEchoIndex, "setup-agent-output-env run command should be present")
require.NotEqual(t, -1, tokenIndex, "GitHub App token step should be present")

assert.Less(t, attrMaskIndex, downloadIndex, "attributes masking should happen before artifact download")
assert.Less(t, setupEnvEchoIndex, tokenIndex, "app token step must be inserted after setup-agent-output-env run block")
assert.NotContains(t, stepsContent, "with:\n echo \"GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json\" >> \"$GITHUB_OUTPUT\"",
"app token step must not split setup-agent-output-env into a nested echo under with:")
}

// TestGitHubAppTokenStepWithOTLPHeadersAndAttributes ensures the app token step insertion
// point stays aligned when both OTLP header masking and attribute masking steps are present
// (insertIndex is incremented twice — highest-risk composition).
func TestGitHubAppTokenStepWithOTLPHeadersAndAttributes(t *testing.T) {
compiler := NewCompiler()
compiler.jobManager = NewJobManager()

workflowData := &WorkflowData{
Name: "Test Workflow",
Env: `"env":
OTEL_EXPORTER_OTLP_HEADERS: "Authorization=******"
GH_AW_OTLP_ATTRIBUTES: "{\"service.name\":\"test\"}"`,
SafeOutputs: &SafeOutputsConfig{
GitHubApp: &GitHubAppConfig{
AppID: "${{ vars.ACTIONS_APP_ID }}",
PrivateKey: "${{ secrets.ACTIONS_PRIVATE_KEY }}",
},
AddComments: &AddCommentsConfig{},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
},
},
}

job, _, err := compiler.buildConsolidatedSafeOutputsJob(workflowData, string(constants.AgentJobName), "test.md")
require.NoError(t, err, "Should successfully build job")
require.NotNil(t, job, "Job should not be nil")

stepsContent := strings.Join(job.Steps, "")
headerMaskIndex := strings.Index(stepsContent, "Mask OTLP telemetry headers")
attrMaskIndex := strings.Index(stepsContent, "Mask OTLP custom attribute values")
downloadIndex := strings.Index(stepsContent, "Download agent output artifact")
setupEnvEchoIndex := strings.Index(stepsContent, "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json")
tokenIndex := strings.Index(stepsContent, "Generate GitHub App token")

require.NotEqual(t, -1, headerMaskIndex, "OTLP headers mask step should be present")
require.NotEqual(t, -1, attrMaskIndex, "OTLP attributes mask step should be present")
require.NotEqual(t, -1, downloadIndex, "agent output download step should be present")
require.NotEqual(t, -1, setupEnvEchoIndex, "setup-agent-output-env run command should be present")
require.NotEqual(t, -1, tokenIndex, "GitHub App token step should be present")

assert.Less(t, headerMaskIndex, downloadIndex, "header masking should happen before artifact download")
assert.Less(t, attrMaskIndex, downloadIndex, "attributes masking should happen before artifact download")
assert.Less(t, setupEnvEchoIndex, tokenIndex, "app token step must be inserted after setup-agent-output-env run block")
assert.NotContains(t, stepsContent, "with:\n echo \"GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json\" >> \"$GITHUB_OUTPUT\"",
"app token step must not split setup-agent-output-env into a nested echo under with:")
}
// a safe-output job compiled for a workflow_call trigger uses
// needs.activation.outputs.target_repo_name (repo name only, no owner prefix) as the repositories
// fallback for the GitHub App token mint step, instead of the full target_repo slug.
Expand Down