fix: visual tests workflow build var - #8860
Conversation
|
/visual-tests |
Visual regression tests (macos)❌ Visual tests failed.
Triggered via |
|
/visual-tests |
|
PR #8860, run #26292885644 Builds: Windows change, Windows baseline, macOS change, macOS baseline Framework 13 i7
|
|
Claude finished @pravusjif's task in 1m 28s —— View job Claude Security Review —
|
| Check | Status | Notes |
|---|---|---|
| W.1 prompt-from-checkout | N/A | No LLM in this workflow |
W.2 heredoc delimiter on $GITHUB_OUTPUT |
OK | Outputs use key=value form, no heredocs; HEAD_REF is a git ref so it cannot contain newlines |
| W.3 TOCTOU between trigger and run | OK for this dispatcher | gh api .../pulls/${PR_NUMBER} resolves the head at run time, but the head is only used to construct an artifact URL (no checkout/exec of PR code). Worst case a maintainer triggers a run against a newer commit than they read — annoying, not a secret-exfil channel |
| W.4 untrusted event text in prompt | N/A | comment.body is only matched with startsWith(...) in a GitHub expression; not shelled |
| W.5 destructive/exfil allowedTools | N/A | No allowedTools |
| W.6 actor gating | OK | OWNER|MEMBER|COLLABORATOR gate at L63–77 runs before any gh api call or reusable-workflow dispatch |
W.8 pull_request_target |
N/A | Trigger is issue_comment; no PR-code checkout |
| W.9 long-retention artifact | N/A | No upload-artifact here |
Pre-existing items (not introduced by this PR — noted for tracking, do not block)
- W.7a — secret-holding reusable workflow pinned to a mutable ref. L162 uses
decentraland/explorer-automation/.github/workflows/run-visual-suite.yml@maintogether withsecrets: inherit(passingALTTESTER_LICENSE,REPOS_READ_ONLY_TOKEN,DEV_EXPLORER_TEAM_S3_BUCKET, and the threeDEV_EXPLORER_TEAM_AWS_*secrets). Ifexplorer-automation'smainis ever compromised the next/visual-testsinvocation runs attacker code with all of those secrets in scope. Recommendation: pin to a 40-char commit SHA (and either Dependabot-manage it or document a manual review cadence). The inline comment at L159–161 explains the choice; SHA-pinning preserves the intent while removing the mutable-ref risk. - W.10 — possibly overbroad permissions. The job-level scopes are
contents: writeandpull-requests: write. The dispatcher itself only posts aneyesreaction (covered bypull-requests: write) and reads PR metadata.contents: writeis not exercised by anything in this file. It may be load-bearing for the reusable workflow, but reusable workflows receive permissions from their ownpermissions:block, not the caller's. Worth confirming and dropping if unused.
Neither item is caused by this PR's diff — flagging only because Step W asks me to surface them on any workflow touch.
Verdict
The variable rename is a benign config change; it repoints the dispatcher at a different S3 public-URL prefix and has no security impact on its own. The two pre-existing items above are worth a follow-up but should not block this PR.
DEPENDENCY_REVIEW: PASS
· branch test/trigger-visual-tests-2
Change env var used by the visual tests workflow so that it fetches the correct PR builds.