verify-action-build: tighten JS download heuristic

utils/tests/verify_action_build/test_security.py

@@ -1059,3 +1059,175 @@ def test_postJson_with_extract_in_same_file_not_exempt(self):
         findings = _find_binary_downloads_js(mixed)
         # Both the getJson and the downloadTool stay flagged.
         assert len(findings) == 2
+
+    def test_accept_application_json_marks_data_fetch(self):
+        # graalvm/setup-graalvm/src/gds.ts shape (lines 62, 97):
+        # ``http.get(url, { accept: 'application/json' })`` followed by
+        # ``JSON.parse(await response.readBody())``.  The accept header
+        # is the unambiguous data-vs-binary signal.
+        gds_metadata = """\
+const requestUrl = `${c.GDS_BASE}/artifacts?productId=${id}&...`
+const response = await http.get(requestUrl, { accept: 'application/json' })
+const artifactResponse = JSON.parse(await response.readBody())
+return artifactResponse.items[0]
+"""
+        assert _file_is_pure_data_fetch(gds_metadata) is True
+        assert _find_binary_downloads_js(gds_metadata) == []
+
+    def test_accept_application_json_double_quoted_also_works(self):
+        # Variant: double-quoted ``"accept"`` and ``"application/json"``.
+        content = """\
+const response = await http.get(url, { "accept": "application/json" })
+const data = JSON.parse(await response.readBody())
+"""
+        assert _file_is_pure_data_fetch(content) is True
+
+
+class TestFunctionDefinitionNotADownload:
+    """Regression: ``async function downloadTool(...)`` is a function
+    *definition*, not a call.  The download-pattern scanner must not
+    flag the function-name shadow that happens to match the regex.
+    Surfaced by graalvm/setup-graalvm/src/gds.ts (line 153).
+    """
+
+    def test_async_function_definition_skipped(self):
+        content = """\
+import * as tc from '@actions/tool-cache'
+
+async function downloadTool(url: string, headers?: OutgoingHttpHeaders): Promise<string> {
+  return await someInternalImplementation(url, headers)
+}
+"""
+        # Use a content that has a binary-handle marker so we don't
+        # short-circuit via _file_is_pure_data_fetch.
+        with_handler = content + "\nawait fs.writeFileSync('/tmp/bin', data)\n"
+        findings = _find_binary_downloads_js(with_handler)
+        # Pre-fix the function-definition line was flagged as a
+        # ``downloadTool(`` call.  Post-fix only the genuine
+        # writeFileSync-adjacent code is in scope, and that's not a
+        # download pattern — so zero findings.
+        assert findings == []
+
+    def test_function_definition_with_export_default_skipped(self):
+        content = """\
+import * as tc from '@actions/tool-cache'
+
+export default async function downloadTool(url) {
+  return tc.downloadTool(url)  // this IS a real call
+}
+await fs.writeFileSync('/tmp/x', data)
+"""
+        findings = _find_binary_downloads_js(content)
+        # Only the inner ``tc.downloadTool(url)`` call should be
+        # flagged; the ``export default async function downloadTool(``
+        # line must be skipped.
+        assert len(findings) == 1
+        # Returned tuple is (line_num, snippet); the snippet must be the
+        # ``tc.downloadTool(url)`` call, not the function declaration.
+        line_num, snippet = findings[0]
+        assert "tc.downloadTool(url)" in snippet
+        assert "function" not in snippet
+
+    def test_generator_function_definition_skipped(self):
+        # ``function*`` generators are a less common but valid syntax.
+        content = """\
+function* downloadTool(urls) {
+  for (const u of urls) yield u
+}
+await fs.writeFileSync('/tmp/x', data)
+"""
+        assert _find_binary_downloads_js(content) == []
+
+    def test_function_call_still_flagged_after_definition(self):
+        # The fix must not blanket-suppress *any* line containing the
+        # word ``function`` — only lines that are *themselves* function
+        # definitions.  A real call later in the file must still trip
+        # the scanner.
+        content = """\
+import * as tc from '@actions/tool-cache'
+
+// Helper definition (must be skipped):
+async function helper(url) { return null }
+
+// Real call (must be flagged):
+await tc.downloadTool('https://example.com/binary')
+await fs.writeFileSync('/tmp/bin', data)
+"""
+        findings = _find_binary_downloads_js(content)
+        assert len(findings) == 1
+        assert "tc.downloadTool" in findings[0][1]
+
+
+class TestVerificationPatternsRecognized:
+    """Verification patterns the scanner must accept as evidence that a
+    file with downloads also has integrity checks — so the binary-
+    download check downgrades from failure to warning."""
+
+    @staticmethod
+    def _has_verification(content: str) -> bool:
+        # Mirror the analyze_binary_downloads helper directly so the test
+        # exercises exactly the predicate used in production.
+        from verify_action_build.security import _JS_VERIFICATION_PATTERNS
+        return any(p.search(content) for p in _JS_VERIFICATION_PATTERNS)
+
+    def test_bare_create_hash_recognized(self):
+        # graalvm/setup-graalvm/src/utils.ts shape: ``createHash`` is
+        # imported via ``import { createHash } from 'crypto'`` and used
+        # bare without the ``crypto.`` prefix.  The original pattern
+        # ``crypto\.createHash\(`` missed this entirely.
+        content = """\
+import { createHash } from 'crypto'
+
+export function calculateSHA256(filePath) {
+  const hashSum = createHash('sha256')
+  hashSum.update(readFileSync(filePath))
+  return hashSum.digest('hex')
+}
+"""
+        assert self._has_verification(content) is True
+
+    def test_bare_create_hash_requires_sha_literal(self):
+        # An unrelated ``createHash`` identifier (or one called with a
+        # non-SHA algorithm) shouldn't false-positive.  Require the
+        # ``sha`` literal as the first argument so we only match
+        # genuine SHA-family hashing.
+        unrelated = """\
+const createHash = customLibrary.createHash('blake2')
+"""
+        # ``createHash('blake2')`` is a hash too but our concern is
+        # specifically SHA verification; the broader case is fine to
+        # miss here — better than over-matching arbitrary createHash
+        # identifiers.
+        assert self._has_verification(unrelated) is False
+
+    def test_calculate_sha256_function_name_recognized(self):
+        # Custom helper-function naming convention used by several
+        # actions: ``calculateSHA256``, ``calculateChecksum``, etc.
+        for func_name in (
+            "calculateSHA256",
+            "calculateSHA512",
+            "calculateSHA1",
+            "calculateChecksum",
+            "calculateDigest",
+        ):
+            content = f"const sha = {func_name}(downloadPath)\n"
+            assert self._has_verification(content) is True, func_name
+
+    def test_verify_hash_function_recognized(self):
+        # Whether named ``verifyHash`` or referenced inline.
+        for snippet in (
+            "if (!verifyHash(blob, expected)) throw new Error('bad hash')",
+            "const ok = computeChecksum(blob)",
+        ):
+            assert self._has_verification(snippet) is True, snippet
+
+    def test_existing_crypto_dotted_form_still_matches(self):
+        # Regression: the original ``crypto\.createHash\(`` matcher
+        # must still fire — existing actions that use the dotted form.
+        content = "const h = crypto.createHash('sha256')"
+        assert self._has_verification(content) is True
+
+    def test_sigstore_and_cosign_still_match(self):
+        # Other existing patterns kept.
+        for snippet in ("import * as sigstore from 'sigstore'", "cosign verify-blob"):
+            assert self._has_verification(snippet) is True, snippet

utils/verify_action_build/security.py

@@ -1144,6 +1144,11 @@ def analyze_action_metadata(
     # (``http.postJson<IdToken>(...)``); without it the regex misses the
     # exact call shape rubygems/configure-rubygems-credentials uses.
     re.compile(r"\.(?:get|post|put|patch|del|request)Json(?:\s*<[^>]*>)?\s*\("),
+    # An ``Accept: application/json`` header on the same line as an HTTP
+    # call is an unambiguous "I'm asking for JSON, not bytes" signal.
+    # graalvm/setup-graalvm's GDS metadata calls
+    # (``http.get(requestUrl, { accept: 'application/json' })``) hit this.
+    re.compile(r"['\"`]accept['\"`]\s*:\s*['\"`]application/json", re.IGNORECASE),
 ]
 
 # Markers indicating the response is treated as a binary or executable —
@@ -1173,6 +1178,20 @@ def analyze_action_metadata(
     re.compile(r"\bverifySignature\b"),
     re.compile(r"\bverifyChecksum\b"),
     re.compile(r"\bcomputeHash\b"),
+    # Bare ``createHash('sha…')`` — the function imported via
+    # ``import { createHash } from 'crypto'`` rather than dereferenced
+    # off the module object.  Common in TypeScript actions
+    # (e.g. graalvm/setup-graalvm's ``calculateSHA256``).  Require a
+    # ``sha`` literal so we don't false-positive on unrelated
+    # ``createHash`` identifiers from other libraries.
+    re.compile(r"\bcreateHash\s*\(\s*['\"`]sha", re.IGNORECASE),
+    # Custom helper-function names that imply a SHA computation or
+    # checksum comparison.  Catches ``calculateSHA256(...)``, ``verifyHash``,
+    # ``computeChecksum``, etc. when they're defined or called within
+    # the file under inspection.
+    re.compile(r"\bcalculate(?:SHA[\d]+|Checksum|Digest)\b", re.IGNORECASE),
+    re.compile(r"\bverifyHash\b"),
+    re.compile(r"\bcomputeChecksum\b"),
 ]
 
 _JS_SOURCE_EXTENSIONS = (".ts", ".js", ".mjs", ".cjs")
@@ -1210,12 +1229,23 @@ def _file_is_pure_data_fetch(content: str) -> bool:
     return not has_binary_handle
 
 
+# Function-definition shapes — the regex below matches any of these on
+# their own line, so the download-pattern scanner can skip them.  Without
+# this guard ``async function downloadTool(...)`` would be mis-flagged as
+# a download call simply because the regex sees ``downloadTool(``.
+_JS_FUNCTION_DEFINITION_RE = re.compile(
+    r"^\s*(?:export\s+)?(?:default\s+)?"
+    r"(?:async\s+)?function\s*\*?\s*\w+\s*[(<]"
+)
+
+
 def _find_binary_downloads_js(content: str) -> list[tuple[int, str]]:
     """Find lines in JS/TS source that fetch remote artifacts.
 
     Flags calls to ``tc.downloadTool`` / ``downloadTool``, bare ``fetch`` to
     an http(s) URL, node's ``http(s).get`` / ``.request``, ``axios.*``, and
-    ``new HttpClient()``. Skips comment-only lines.
+    ``new HttpClient()``. Skips comment-only lines and function definitions
+    (a ``function downloadTool(...)`` declaration isn't itself a download).
 
     Files that look like pure data fetches (response parsed as a value, not
     persisted or executed) return no findings — see ``_file_is_pure_data_fetch``
@@ -1228,6 +1258,8 @@ def _find_binary_downloads_js(content: str) -> list[tuple[int, str]]:
         stripped = line.strip()
         if not stripped or stripped.startswith("//") or stripped.startswith("*"):
             continue
+        if _JS_FUNCTION_DEFINITION_RE.match(line):
+            continue
         if any(p.search(line) for p in _JS_DOWNLOAD_PATTERNS):
             findings.append((i, stripped[:120]))
     return findings