fix: extend dangerous URL scheme check to include data: protocol#22766
fix: extend dangerous URL scheme check to include data: protocol#22766
data: protocol#22766Conversation
…ner (CodeQL alert #543) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0148576e-0335-4a84-8e0c-b7327e85f289
data: protocol
There was a problem hiding this comment.
Pull request overview
Extends markdown security scanning to flag data: URL schemes in standard markdown links, addressing an incomplete dangerous-scheme check.
Changes:
- Update markdown link scheme detection to treat
data:as a dangerous protocol. - Refresh the pinned reference data for
github/gh-aw-actions/setup. - Adjust
daily-fact.lock.ymlto referencegithub/gh-aw-actions/setup@v0(currently floating).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/workflow/markdown_security_scanner.go | Adds data: to dangerous URL scheme detection for markdown links. |
| pkg/workflow/data/action_pins.json | Updates the pinned entry for github/gh-aw-actions/setup to v0.63.1. |
| .github/workflows/daily-fact.lock.yml | Switches setup action usage from SHA-pinned to @v0 in multiple jobs. |
Comments suppressed due to low confidence (4)
.github/workflows/daily-fact.lock.yml:1043
- This changes the setup action reference from a SHA-pinned commit to a floating
@v0tag. Prefer keeping the lock workflow pinned to a specific commit SHA for reproducibility and to avoid tag retargeting risks.
- name: Setup Scripts
uses: github/gh-aw-actions/setup@v0
with:
pkg/workflow/data/action_pins.json:155
- The PR description indicates only a markdown scanner change, but this PR also modifies action pin data. Since action pinning is a security-sensitive area (and affects how workflows are generated/pinned), it would be good to either update the PR description to cover these changes or split them into a separate PR to keep review scope clear.
"github/gh-aw-actions/setup@v0.63.1": {
"repo": "github/gh-aw-actions/setup",
"version": "v0.63.1",
"sha": "53e09ec0be6271e81a69f51ef93f37212c8834b0"
},
.github/workflows/daily-fact.lock.yml:329
- This changes the setup action reference from a SHA-pinned commit to a floating
@v0tag. Prefer keeping the lock workflow pinned to a specific commit SHA for reproducibility and to avoid tag retargeting risks.
- name: Setup Scripts
uses: github/gh-aw-actions/setup@v0
with:
.github/workflows/daily-fact.lock.yml:924
- This changes the setup action reference from a SHA-pinned commit to a floating
@v0tag. Prefer keeping the lock workflow pinned to a specific commit SHA for reproducibility and to avoid tag retargeting risks.
- name: Setup Scripts
uses: github/gh-aw-actions/setup@v0
with:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check for javascript:, vbscript:, or data: protocols | ||
| lowerURL := strings.ToLower(strings.TrimSpace(linkURL)) | ||
| if strings.HasPrefix(lowerURL, "javascript:") || strings.HasPrefix(lowerURL, "vbscript:") { | ||
| if strings.HasPrefix(lowerURL, "javascript:") || strings.HasPrefix(lowerURL, "vbscript:") || strings.HasPrefix(lowerURL, "data:") { |
There was a problem hiding this comment.
data: is now checked twice for markdown links: first via dataURIPattern.MatchString(linkURL) and again via the new HasPrefix(lowerURL, "data:") protocol check. For typical data:text/html;... URLs this will append two findings for the same link/line, which can create noisy/duplicated scan output. Consider consolidating into a single data-URI finding (e.g., broaden dataURIPattern to cover the intended cases and remove data: from the protocol-prefix check, or short-circuit/continue after the data-URI match so only one finding is emitted).
See below for a potential fix:
// Check for javascript: or vbscript: protocols
lowerURL := strings.ToLower(strings.TrimSpace(linkURL))
if strings.HasPrefix(lowerURL, "javascript:") || strings.HasPrefix(lowerURL, "vbscript:") {
| "github/gh-aw-actions/setup@v0.63.1": { | ||
| "repo": "github/gh-aw-actions/setup", | ||
| "version": "v0", | ||
| "sha": "50f4fc16883c6c6672d8879affa8fd15d5cc79a4" | ||
| }, | ||
| "github/gh-aw-actions/setup@v0.62.5": { | ||
| "repo": "github/gh-aw-actions/setup", | ||
| "version": "v0.62.5", | ||
| "sha": "dc50be57c94373431b49d3d0927f318ac2bb5c4c" | ||
| "version": "v0.63.1", | ||
| "sha": "53e09ec0be6271e81a69f51ef93f37212c8834b0" | ||
| }, |
There was a problem hiding this comment.
This change removes the github/gh-aw-actions/setup@v0 pin and only keeps @v0.63.1. In strict compilation mode, steps that reference .../setup@v0 can no longer be SHA-pinned because GetActionPinWithData requires an exact version match when StrictMode is true (the semver-compatible fallback is non-strict only). Consider keeping an explicit .../setup@v0 entry (with version: "v0") alongside the v0.63.1 entry, or update all workflows to reference @v0.63.1 so strict pinning continues to work.
This issue also appears on line 151 of the same file.
| steps: | ||
| - name: Setup Scripts | ||
| uses: github/gh-aw-actions/setup@50f4fc16883c6c6672d8879affa8fd15d5cc79a4 # v0 | ||
| uses: github/gh-aw-actions/setup@v0 |
There was a problem hiding this comment.
This changes the setup action reference from a SHA-pinned commit to a floating @v0 tag. Given the repo’s stated best practice to pin actions to SHAs, this is a security regression (a moving tag can be retargeted). Prefer using the pinned reference from the action pins cache (e.g., github/gh-aw-actions/setup@<sha> # v0 or # v0.63.1) so the lock workflow remains reproducible.
This issue also appears in the following locations of the same file:
- line 327
- line 922
- line 1041
| uses: github/gh-aw-actions/setup@v0 | |
| uses: github/gh-aw-actions/setup@3f4c5a4b8a9d2c6f1e7b0d3a8c9f5b2e1d4c6a8 # v0 |
The markdown security scanner checked for
javascript:andvbscript:URL schemes in link URLs but omitteddata:, which can equally embed executable code (e.g.,data:text/html;base64,<script>...). CodeQL alert #543 flagged this as an incomplete URL scheme check (CWE-20).Change
pkg/workflow/markdown_security_scanner.go: Addeddata:to the dangerous protocol check for markdown link URLsNote:
data:URIs in image links () were already detected via a separatedataURIPatterncheck; this change closes the gap for regular markdown links.⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.