Harden safe-output field validation - #51133
Conversation
Allow only schema-declared fields through safe-output validation and move trusted transport metadata out of agent-controlled NDJSON. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c72be66-bc49-4422-8e20-5567e99015f1
|
|
|
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
No ADR enforcement needed: PR does not have the 'implementation' label and has 96 new lines of code in business logic directories (≤100 threshold). |
|
@copilot use w3c-spec-writer to update the safe output specification |
There was a problem hiding this comment.
Pull request overview
Hardens safe-output validation and moves privileged metadata derivation away from agent-controlled fields.
Changes:
- Strips undeclared fields and removes agent-controlled
comment_id. - Re-derives upload and patch metadata in privileged processing.
- Adds schema-consistency and security regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_validation_config.go |
Expands validation contracts. |
pkg/workflow/safe_output_validation_config_test.go |
Tests schema/config coverage. |
pkg/workflow/js/safe_outputs_tools.json |
Removes comment_id. |
actions/setup/js/upload_assets.test.cjs |
Tests trusted upload metadata. |
actions/setup/js/upload_assets.cjs |
Re-derives asset metadata. |
actions/setup/js/safe_outputs_tools.json |
Removes runtime comment_id. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Verifies hashed staging names. |
actions/setup/js/safe_outputs_handlers.cjs |
Prevents basename collisions. |
actions/setup/js/safe_output_type_validator.test.cjs |
Tests undeclared-field stripping. |
actions/setup/js/safe_output_type_validator.cjs |
Whitelists configured fields. |
actions/setup/js/push_to_pull_request_branch.test.cjs |
Tests trusted patch metadata and sizing. |
actions/setup/js/push_to_pull_request_branch.cjs |
Uses patch-embedded base metadata. |
actions/setup/js/generate_git_patch.cjs |
Embeds base commit in patches. |
actions/setup/js/create_pull_request.test.cjs |
Tests patch-embedded base handling. |
actions/setup/js/create_pull_request.cjs |
Reads trusted patch base metadata. |
actions/setup/js/commit_sha_helpers.test.cjs |
Tests metadata extraction. |
actions/setup/js/commit_sha_helpers.cjs |
Adds patch-header extraction. |
actions/setup/js/collect_ndjson_output.test.cjs |
Tests custom-field filtering. |
actions/setup/js/collect_ndjson_output.cjs |
Whitelists custom job inputs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 19/19 changed files
- Comments generated: 4
- Review effort level: Balanced
| const size = fileContent.length; | ||
| const githubServer = process.env.GITHUB_SERVER_URL || "https://github.com"; | ||
| const repo = process.env.GITHUB_REPOSITORY || "owner/repo"; | ||
| const url = `${githubServer}/${repo}/blob/${normalizedBranchName}/${targetFileName}?raw=true`; | ||
| processedAssets.push({ fileName, sha: computedSha, size, targetFileName, url }); |
There was a problem hiding this comment.
Fixed in 5be57975a0: upload_assets.cjs now uses the same host-specific URL construction as the producer, preserving GHES /raw/ URLs.
| // Key the staged file by its declared source path so same-basename assets | ||
| // cannot overwrite each other before the privileged publishing job. | ||
| const stagedFileName = `${crypto.createHash("sha256").update(filePath).digest("hex")}${fileExt}`; | ||
| const targetPath = path.join(assetsDir, stagedFileName); |
There was a problem hiding this comment.
Fixed in 5be57975a0: repeated upload_asset calls for the same resolved source path are now rejected before staging, preventing same-path snapshot collisions.
| @@ -455,21 +448,8 @@ async function main(config = {}) { | |||
| } | |||
| const bundleSizeKb = Math.ceil(bundleSizeBytes / 1024); | |||
|
|
|||
| const diffSizeBytesRaw = message.diff_size; | |||
| const haveDiffSize = typeof diffSizeBytesRaw === "number" && diffSizeBytesRaw >= 0; | |||
|
|
|||
| let sizeForCheckBytes; | |||
| let sizeLabel; | |||
| if (haveDiffSize) { | |||
| sizeForCheckBytes = diffSizeBytesRaw; | |||
| sizeLabel = "Incremental diff size"; | |||
| } else if (hasBundleFile) { | |||
| sizeForCheckBytes = bundleSizeBytes; | |||
| sizeLabel = "Bundle size"; | |||
| } else { | |||
| sizeForCheckBytes = patchSizeBytes; | |||
| sizeLabel = "Patch size"; | |||
| } | |||
| const sizeForCheckBytes = patchSizeBytes; | |||
| const sizeLabel = "Patch size"; | |||
There was a problem hiding this comment.
Fixed in 5be57975a0: after applying changes, the privileged checkout now enforces max_patch_size against the expanded changed file content size before pushing.
| "draft_title": {Type: "string", Sanitize: true, MaxLength: 256}, | ||
| "draft_body": {Type: "string", Sanitize: true, MaxLength: MaxBodyLength}, | ||
| "fields": {Type: "object"}, | ||
| "project": {Required: true, Type: "string", Sanitize: true, MaxLength: 512, Pattern: "^https://[^/]+/(orgs|users)/[^/]+/projects/\\d+", PatternError: "must be a full GitHub project URL (e.g., https://github.com/orgs/myorg/projects/42)"}, |
There was a problem hiding this comment.
Fixed in 5be57975a0: update_project.project validation now accepts temporary project IDs while retaining GHES-compatible project URL support.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly addresses agent-controlled field injection by rebuilding normalizedItem from only declared contract fields and moving base_commit to a trusted patch header. The approach is sound and the security boundary is well-considered.
Field stripping (safe_output_type_validator.cjs): normalizedItem = { type: item.type } + allowlist rebuild ensures undeclared fields never reach downstream handlers. The fieldValue fallback for declared fields that pass validation but produce no normalizedValue is correct.
Trusted patch header: base_commit is now embedded at patch-generation time and validated via normalizeCommitSHA on extraction, removing agent control over this value entirely.
Asset staging hardening: SHA256 path-keying prevents same-basename file collisions before the privileged publish job. The targetFileName !== path.basename(targetFileName) path-traversal guard is correct.
Size-check simplification: Always using uncompressed patch size removes agent influence over diff_size and makes the check deterministic.
Tests are comprehensive and verify the new strip-by-default behavior. One minor nit filed inline. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 90.4 AIC · ⊞ 5.5K
Comments that could not be inline-anchored
actions/setup/js/generate_git_patch.cjs:297
Nit: validate baseCommitSha before embedding
The value is injected into the patch header without passing through normalizeCommitSHA. extractPatchBaseCommit validates on the receiving end so this is not a security gap, but a malformed value (e.g. containing a newline) would silently produce an unparseable header and cause a silent fallback instead of an early warning at generation time.
Consider validating in embedBaseCommit before injecting:
function embedBaseCommit(patchCon…
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture and /tdd — requesting changes on a few targeted issues.
📋 Key Themes & Highlights
Key Themes
- Implicit caller–callee contracts: The removal of the null-guard on
validateItemWithSafeJobConfigis correct in intent, but the call site no longer documents/enforces the invariant thatsafeJobConfigis always non-null by the time it reaches the helper. - Partial field-copy in the validator: The new
else if (fieldValue !== undefined)branch insafe_output_type_validator.cjsmay silently copy raw values for fields whose validators return{ isValid: true }without anormalizedValue— the undeclared-field gate is now open to that edge case. embedBaseCommitmissing SHA validation: The function trusts the caller to pass a hex SHA; a non-hex value would be written verbatim into the patch header.- Asset URL computed before upload succeeds:
processedAssetsis populated before the git push, so a failed or skipped upload produces a dangling URL in the job summary. - Test coverage gaps for custom-schema stripping and CRLF patches: The Slack test doesn't assert that the extra
channelfield was stripped;extractPatchBaseCommithas no CRLF line-ending test.
Positive Highlights
- ✅ Excellent privilege separation: metadata (SHA, size,
targetFileName, base commit) is now re-derived or embedded by the privileged job, not trusted from agent output. - ✅ Staging collision prevention via
SHA256(declaredPath)is a clean solution. - ✅
comment_idremoval is well-justified and the PR description explains the ownership gap clearly. - ✅
extractPatchBaseCommitis a clean, testable unit with good boundary tests. - ✅ Test suite (729 JS, focused Go) gives strong regression confidence for the happy paths.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 79.4 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/collect_ndjson_output.cjs:392
[/improve-codebase-architecture] The old if (safeJobConfig && safeJobConfig.inputs) guard allowed items without a declared schema to bypass normalisation. Removing it is correct — but now validateItemWithSafeJobConfig(item, undefined, i+1) can be called when safeJobConfig is null/undefined, relying silently on the helper's early-return. The caller–callee contract is implicit and fragile.
<details>
<summary>💡 Suggestion</summary>
Add a guard to make the invariant explicit and prev…
actions/setup/js/safe_output_type_validator.cjs:624
[/improve-codebase-architecture] The new else if (fieldValue !== undefined) branch silently copies any non-undefined field value that didn't produce a normalizedValue from the validator. This means fields that fail validation with result.isValid === true but return no normalizedValue (e.g. no-op validators) still pass through the raw, unvalidated fieldValue. Verify that every field validator either sets result.normalizedValue or explicitly returns isValid: false; otherwise t…
actions/setup/js/commit_sha_helpers.cjs:121
[/tdd] extractPatchBaseCommit uses .split((r/redacted)?\n\r?\n/, 1)[0] to extract the first block, but the tests only cover Unix (\n\n) line endings. A CRLF-terminated patch (\r\n\r\n block separator) should be tested since the regex allows it in the line splitter but split on \r?\n\r?\n would match \r\n\r\n as a two-character pair — confirm the test covers this edge case.
<details>
<summary>💡 Suggested test</summary>
it('handles CRLF line endings in patch header', ()…
</details>
<details><summary>actions/setup/js/upload_assets.cjs:833</summary>
**[/improve-codebase-architecture]** The path traversal guard `targetFileName !== path.basename(targetFileName)` correctly rejects names containing `/`, but it only guards the *final* `targetFileName`. The `targetFileName` is computed as `asset.targetFileName || generatedTargetFileName` — if `asset.targetFileName` is agent-supplied and contains a directory component, the guard fires. However if `asset.targetFileName` is an empty string `""`, it falls back to `generatedTargetFileName` (safe) wit…
</details>
<details><summary>actions/setup/js/upload_assets.cjs:841</summary>
**[/improve-codebase-architecture]** The URL is constructed from `GITHUB_SERVER_URL` and `GITHUB_REPOSITORY` with a hard-coded `blob/<branch>/<targetFileName>` path before the file has been committed to the branch. If the upload later fails or is skipped (`missingAssetCount++`), `processedAssets` still contains a URL pointing to a file that may not exist. The summary is written after the upload loop, but a partial failure leaves dangling URLs in the job summary.
<details>
<summary>💡 Suggestio…
</details>
<details><summary>actions/setup/js/generate_git_patch.cjs:297</summary>
**[/tdd]** `embedBaseCommit` injects the `X-GH-AW-Base-Commit` header by inserting after the first newline. If `baseCommitSha` contains characters outside `[0-9a-fA-F]` (e.g. because the caller passes an unvalidated value), the injected header could contain unexpected content. There is no test asserting that `embedBaseCommit` rejects non-hex SHA values before writing them into the patch.
<details>
<summary>💡 Suggested test</summary>
```js
it('does not embed a non-hex baseCommitSha', () => {
…
</details>
<details><summary>actions/setup/js/collect_ndjson_output.cjs:114</summary>
**[/tdd]** `validateItemWithSafeJobConfig` now seeds `normalizedItem` with only `{ type: item.type }` — but there is no test asserting that a **custom** safe-job config item (non-built-in schema, via `GH_AW_SAFE_OUTPUTS` env config) also strips undeclared fields. The Slack test at line 264 now passes `channel` in the input but only asserts on `text`; it doesn't confirm that `channel` is absent from the output. Add an explicit assertion to prevent regression.
<details>
<summary>💡 Suggestion</s…
</details>
🧪 Test Quality Sentinel Report — PR #51133Score: 92/100 ✅ Excellent SummaryThis PR demonstrates exceptional test quality with 193+ test cases covering validation schemas, field enforcement, error handling, and complex integration scenarios. Tests are behavioral-focused, extensively cover edge cases, and maintain a healthy test-to-production code ratio of 1.73:1 (well under the 2:1 threshold). Key Metrics
Test Coverage by DomainInput Validation (commit_sha_helpers.test.cjs)
Field Validation Engine (safe_output_type_validator.test.cjs)
Validation Configuration Schema (safe_output_validation_config_test.go)
Asset Upload (upload_assets.test.cjs)
Patch Application (push_to_pull_request_branch.test.cjs)
Quality Checks
Score BreakdownRecommendation✅ APPROVE — Test quality exceeds expectations. Design-focused tests with comprehensive error and edge-case coverage. All guidelines met. Implementation ratio 6.7% (threshold: ≤30%).
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
|
@copilot Please address the remaining review feedback, refresh the branch if needed, and run the Outstanding review items:
Failed checks: Branch refresh was requested via GitHub update-branch.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR finisher pass. Addressed the in-scope review findings in |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Key Themes
- Undeclared-field stripping (core fix):
normalizedItem = { type: item.type }correctly closes the passthrough hole — excellent change. - Path-mode upload security: staging via sha256-keyed filenames and re-deriving metadata server-side are solid hardening steps.
comment_idallowlist: theparseAllowedCommentIds+validateAllowedAddCommentIdpattern is well-structured, but two issues need fixing (see inline comments).- Test coverage: the new regression suite is comprehensive; two missing edge cases flagged inline.
Positive Highlights
- ✅ Field stripping via allowlist is clean and the diagnostic error for unnormalized fields aids future maintenance.
- ✅
uploadedAssetPathsSet prevents duplicate source-path uploads within a single session. - ✅
buildAssetUrlcorrectly handles GHES vs. github.com URL shapes. - ✅ Parameterised
it.eachfor undeclared-field stripping gives good coverage with minimal boilerplate.
Issues Found
validateAllowedAddCommentIdmutatesentrydirectly — side-effectful validation is fragile (safe_outputs_handlers.cjs:226).- Path-mode uploads still accept agent-supplied
targetFileName— weakens the metadata-derivation hardening (upload_assets.cjs:141). - Missing tests for
comment_idrejection paths — non-*target and empty allowlist not covered (safe_outputs_handlers.test.cjs).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 110.8 AIC · ⊞ 7.1K
Comment /matt to run again
|
Smoke Test Results:
|
|
@copilot review and apply reviews in #51133 (review) |
Smoke Test: Copilot EnginePR: Harden safe-output field validation (#51133) ✅ github/gh-proxy, mcpscripts PR query, playwright, web-fetch, file/bash, discussion comment, build, artifact upload, discussion create, workflow dispatch, PR review tools, comment memory, check run, LSP function count Overall: FAIL (2/16 failed) Author: @pelikhan · Assignees: none Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test automated review pass.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · auto · 50.9 AIC · ⌖ 2.5 AIC · ⊞ 8.6K
Comment /smoke-copilot to run again
Add label smoke to run again
There was a problem hiding this comment.
Review details
Suppressed comments (2)
pkg/workflow/js/safe_outputs_tools.json:338
- The PR description explicitly says the agent-facing
comment_idis removed, but this schema continues to expose it for the agent to select from an allowlist. The implementation and documentation consistently describe allowlisted reuse, so either update the PR description to state that narrower contract or remove this field to match the stated security behavior.
actions/setup/js/upload_assets.cjs:13 - The new path-only entry shape is valid (
{ type, path }), but this typedef still marksfileName,sha,size, andtargetFileNameas required. That misstates the contract used below and will give incorrect static-analysis/IDE results for the newly supported input.
- Files reviewed: 192/192 changed files
- Comments generated: 1
- Review effort level: Balanced
| return await createProtectedFilesFallbackIssue(postApplyProtection.files); | ||
| } | ||
|
|
||
| const changedBlobSizeBytes = getChangedBlobSizeBytes(baseGitOpts, actualFiles); |
There was a problem hiding this comment.
Fixed in 9c43ee37d4: getBundlePreApplyFiles and the post-apply diff now use git diff --name-only -z and split on NUL without trimming, so filenames with leading/trailing whitespace or C-quoted characters are measured correctly instead of being silently dropped to zero size.
Co-authored-by: gh-aw-bot <4175913+gh-aw-bot@users.noreply.github.com>
Addressed in the latest commit:
|
…e re-derivation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Quick triage nudge for this PR. Please address the outstanding changes-requested feedback, update the safe output specification as requested, refresh the branch if needed, and run the Run: https://github.com/github/gh-aw/actions/runs/31225438989
|
…field-allowlist Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All review threads already have fixes applied and replied to in prior commits ( |
Summary
Hardens security-sensitive parts of the safe-output pipeline: strict field allowlisting during output normalization, a trusted allowlist for reusing existing PR/issue comments via
comment_id, and re-derivation of sensitive metadata (base commit SHA, upload filenames, patch sizes/filenames) from trusted runtime state instead of agent-supplied values.Key changes
safe_output_type_validator.cjs,collect_ndjson_output.cjs): normalized items now start with only thetypefield and add back only declared/validated fields — undeclared fields supplied by the agent are stripped rather than passed through. Validation now errors if a value is accepted but produces no normalized output (previously undetected class of bug).comment_idallowlist (add_comment.cjs,add_comment.go,safe_outputs_handlers.cjs,safe_outputs_handler_registry.go,safe_outputs_validation_config.go): agents may only reuse an existing comment ID whentarget: "*"and the ID is present in a newallows-comment-ids/AllowedCommentIDsworkflow-configured allowlist. Existing comment targets are fetched and validated before update, closing a path where an agent could edit/hide arbitrary comments.commit_sha_helpers.cjs,generate_git_patch.cjs,create_pull_request.cjs,push_to_pull_request_branch.cjs): base commit SHA is now embedded in and extracted from the generated patch header (extractPatchBaseCommit) rather than taken from the agent-controlled safe-output message (base_commit/normalizeCommitSHAremoved).push_to_pull_request_branch.cjs): git diff/pre-apply file listings switched to NUL-delimited output so non-ASCII/special-character filenames (e.g.,résumé.txt) can't bypass size/protection checks via shell quoting; patch size validation simplified to always check actual patch file size (droppeddiff_size/bundle-size branching) plus new post-apply content size enforcement.upload_assets.cjs): staged files are now keyed by a hash of the source path to prevent overwrite collisions; target filenames/metadata are re-derived rather than trusted from agent input; addedbuildAssetUrl()for GitHub.com/GHES URL generation.schemas/safe-outputs.jsonanddocs/.../safe-outputs-specification.md(bumped to v1.28.2) document thecomment_id/allows-comment-idscontract and that downstream payloads contain only declared fields. New job-type validation configs added inpkg/safe-output/validation.goforupload_artifact,push_repo_memory,create_check_run, plustemporary_idsupport (pattern^#?aw_[A-Za-z0-9_]{3,12}$) acrossupdate_project/create_project/upload_artifact, and aduplicate_offield oncreate_pull_request_review_comment.maps.Copy()refactor incodemod_bash_allowlist_unsupported_engine.go; comment/formatting/typo fixes inawf_command_builder.go,awf_env.go, and associated test files.Impact
add_comment,collect_ndjson_output,safe_output_type_validator,safe_outputs_handlers,upload_assets,push_to_pull_request_branch,generate_git_patch,commit_sha_helpers).Changed files (~29 files across Go and JS, all 7 diff chunks analyzed)
Full file list
.changeset/patch-harden-safe-output-field-validation.mdactions/setup/js/add_comment.cjs+ testactions/setup/js/collect_ndjson_output.cjs+ testactions/setup/js/commit_sha_helpers.cjs+ testactions/setup/js/create_pull_request.cjs+ testactions/setup/js/generate_git_patch.cjs+ testactions/setup/js/push_to_pull_request_branch.cjs+ integration test + testactions/setup/js/safe_output_type_validator.cjs+ testactions/setup/js/safe_outputs_handlers.cjs+ testactions/setup/js/safe_outputs_tools.jsonactions/setup/js/upload_assets.cjs+ testactions/setup/setup.shschemas/safe-outputs.jsondocs/src/content/docs/reference/safe-outputs.mddocs/src/content/docs/specs/safe-outputs-specification.mdpkg/cli/codemod_bash_allowlist_unsupported_engine.gopkg/workflow/add_comment.gopkg/workflow/awf_command_builder.go+ testpkg/workflow/awf_env.go+ testpkg/workflow/awf_feature_flags_test.gopkg/workflow/js/safe_outputs_tools.jsonpkg/workflow/safe_output_validation_config_test.gopkg/workflow/safe_outputs_config_generation_test.gopkg/workflow/safe_outputs_handler_registry.gopkg/workflow/safe_outputs_validation_config.gopkg/safe-output/validation.go