fix(scorecard): filter SARIF into a runner-owned file to avoid root overwrite#267
Merged
Merged
Conversation
…verwrite ossf/scorecard-action runs in Docker and writes results.sarif as root; the filter step runs on the host as the runner user and could not overwrite it, so json.dump raised PermissionError, the (non-blocking) step no-opped, and the UNFILTERED SARIF was uploaded -- the posture noise never got dropped. Copy results.sarif to a runner-owned results.filtered.sarif first, filter that copy, and point the upload at it. If the filter fails, the copy is the unfiltered SARIF, so upload still succeeds (fail-safe). Refs nics-dp/TMDs#231 #265
Contributor
There was a problem hiding this comment.
Pull request overview
此 PR 修正 Scorecard SARIF posture-noise filter 在實務上未生效的問題：因 ossf/scorecard-action 於容器內產生 root 擁有的 results.sarif，導致 host 端（runner 使用者）嘗試覆寫同檔案時遇到權限錯誤而實際上傳了未過濾的 SARIF。調整為先複製到 runner-owned 的檔案後再過濾並上傳，以確保 filter 能真正影響上傳結果。
Changes:
- 在 filter 步驟先將
results.sarif複製為results.filtered.sarif，並改以該檔進行過濾寫回。 - 上傳步驟改為上傳
results.filtered.sarif（並相應調整hashFiles判斷檔案存在性）。
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…p upload Address Copilot review on #267: a bash 'cp' under set -e would abort the step before results.filtered.sarif exists, skipping the upload and breaking the fail-safe. Do the copy with shutil.copyfile inside the filter, then filter the copy in a try/except -- on any failure the file is left as the unfiltered copy so the upload still proceeds.
…e paths Address Copilot round 2 on #267: the comment overstated the fail-safe. Clarify that a copy failure uploads nothing, while only a filtering failure after a successful copy uploads the unfiltered copy. No behaviour change.
Address Copilot round 3 on #267: open(DST,'w') truncated results.filtered.sarif before json.dump, so a mid-write failure could leave an empty/partial SARIF that still gets uploaded. Write to a temp file then os.replace atomically; on any failure DST stays the intact unfiltered copy.
…uploaded Address Copilot round 4 on #267: shutil.copyfile could leave a partial results.filtered.sarif on a mid-copy failure, which the upload guard would still pick up. Copy to a temp file then os.replace, matching the filtered-write path: DST is only ever a complete file (absent on copy failure, so nothing is uploaded).
Address Copilot round 5 on #267: guarantee the results.filtered.sarif.tmp scratch file is removed via try/finally even if json.dump fails, so no half-written temp lingers in the workspace. Cosmetic (workspace is ephemeral, temp is never uploaded).
…duced none Address Copilot round 6 on #267: a Select step picks results.filtered.sarif when present, else the raw results.sarif, so a total filter-step failure still refreshes code scanning (non-blocking best effort) instead of silently uploading nothing. Uses a step output, no bash cp.
…back Address Copilot round 7 on #267: the round-6 fallback upload made the earlier comment and the copy-failure log stale (they said a copy failure uploads nothing). Reword both to state that a copy failure falls back to uploading the raw results.sarif. No behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fix the SARIF posture-noise filter (added in #266) so it actually takes effect.
Root cause
ossf/scorecard-actionruns in a Docker container and writesresults.sarifowned by root. The filter step runs on the host as the runner user, so
json.dump(open("results.sarif","w"))raisedPermissionError. Because the stepis
continue-on-error, the job continued and uploaded the unfiltered SARIF --so the TokenPermissions/Pinned noise was never dropped (confirmed via a manual
Scorecard run on dcf-access: alerts stayed open, job log showed
PermissionError: [Errno 13] Permission denied: 'results.sarif').Fix
Copy
results.sarifto a runner-ownedresults.filtered.sarif, filter that copy,and upload it. If the filter fails, the copy is just the unfiltered SARIF, so the
upload still succeeds (fail-safe). No sudo / ownership hacks needed.
Validated end-to-end against a real dcf-access SARIF: cp -> filter -> 32 -> 1
(only the genuine top-level write remained).
Refs nics-dp/TMDs#231 #265
Closes #268