From 5eebd653666f4967163857a30a034d4ec3fe624f Mon Sep 17 00:00:00 2001
From: Wouter Deconinck <wdconinc@gmail.com>
Date: Sun, 19 May 2024 09:30:19 -0500
Subject: [PATCH] audit: disallow
 github.com/org/repo/pull/n/commits/hash.patch?full_index=1 (#44212)

* audit: disallow github.com/org/repo/pull/n/commits/hash.patch?full_index=1

* [@spackbot] updating style on behalf of wdconinc

* audit: fix style

* audit: github.com/o/r/pull/n/commits/sha.patch -> sha.patch

* [@spackbot] updating style on behalf of wdconinc

* Revert "[@spackbot] updating style on behalf of wdconinc"

This reverts commit 2ecec9923829c9514f6e09885ee57cda3688b2f4.

* Revert "audit: github.com/o/r/pull/n/commits/sha.patch -> sha.patch"

This reverts commit 5bd7da2cadee9dae44ce14f35a4fb72323c0b98e.

* fix: modify audit message with suggested fix

* audit: github.com/o/r/pull/n/commits/sha.patch -> /o/r/commit/sha.patch?full_index=1

---------

Co-authored-by: wdconinc <wdconinc@users.noreply.github.com>
---
 lib/spack/spack/audit.py                      | 26 ++++++++++++++-----
 lib/spack/spack/test/audit.py                 |  2 ++
 .../package.py                                | 20 ++++++++++++++
 .../builtin/packages/dla-future/package.py    |  4 +--
 .../builtin/packages/miopen-hip/package.py    |  4 +--
 .../repos/builtin/packages/mlpack/package.py  |  4 +--
 .../packages/openscenegraph/package.py        |  4 +--
 .../builtin/packages/py-astor/package.py      |  4 +--
 .../repos/builtin/packages/umpire/package.py  |  8 +++---
 .../repos/builtin/packages/wrf/package.py     |  8 +++---
 10 files changed, 59 insertions(+), 25 deletions(-)
 create mode 100644 var/spack/repos/builtin.mock/packages/invalid-github-pull-commits-patch-url/package.py

diff --git a/lib/spack/spack/audit.py b/lib/spack/spack/audit.py
index 50eb8b8ec5d..3154723e162 100644
--- a/lib/spack/spack/audit.py
+++ b/lib/spack/spack/audit.py
@@ -421,6 +421,10 @@ def _check_patch_urls(pkgs, error_cls):
         r"^https?://(?:patch-diff\.)?github(?:usercontent)?\.com/"
         r".+/.+/(?:commit|pull)/[a-fA-F0-9]+\.(?:patch|diff)"
     )
+    github_pull_commits_re = (
+        r"^https?://(?:patch-diff\.)?github(?:usercontent)?\.com/"
+        r".+/.+/pull/\d+/commits/[a-fA-F0-9]+\.(?:patch|diff)"
+    )
     # Only .diff URLs have stable/full hashes:
     # https://forum.gitlab.com/t/patches-with-full-index/29313
     gitlab_patch_url_re = (
@@ -436,14 +440,24 @@ def _check_patch_urls(pkgs, error_cls):
                 if not isinstance(patch, spack.patch.UrlPatch):
                     continue
 
-                if re.match(github_patch_url_re, patch.url):
+                if re.match(github_pull_commits_re, patch.url):
+                    url = re.sub(r"/pull/\d+/commits/", r"/commit/", patch.url)
+                    url = re.sub(r"^(.*)(?<!full_index=1)$", r"\1?full_index=1", url)
+                    errors.append(
+                        error_cls(
+                            f"patch URL in package {pkg_cls.name} "
+                            + "must not be a pull request commit; "
+                            + f"instead use {url}",
+                            [patch.url],
+                        )
+                    )
+                elif re.match(github_patch_url_re, patch.url):
                     full_index_arg = "?full_index=1"
                     if not patch.url.endswith(full_index_arg):
                         errors.append(
                             error_cls(
-                                "patch URL in package {0} must end with {1}".format(
-                                    pkg_cls.name, full_index_arg
-                                ),
+                                f"patch URL in package {pkg_cls.name} "
+                                + f"must end with {full_index_arg}",
                                 [patch.url],
                             )
                         )
@@ -451,9 +465,7 @@ def _check_patch_urls(pkgs, error_cls):
                     if not patch.url.endswith(".diff"):
                         errors.append(
                             error_cls(
-                                "patch URL in package {0} must end with .diff".format(
-                                    pkg_cls.name
-                                ),
+                                f"patch URL in package {pkg_cls.name} must end with .diff",
                                 [patch.url],
                             )
                         )
diff --git a/lib/spack/spack/test/audit.py b/lib/spack/spack/test/audit.py
index 98e6ad83c85..0d2ca594f17 100644
--- a/lib/spack/spack/test/audit.py
+++ b/lib/spack/spack/test/audit.py
@@ -19,6 +19,8 @@
         (["missing-dependency"], ["PKG-DIRECTIVES", "PKG-PROPERTIES"]),
         # The package use a non existing variant in a depends_on directive
         (["wrong-variant-in-depends-on"], ["PKG-DIRECTIVES", "PKG-PROPERTIES"]),
+        # This package has a GitHub pull request commit patch URL
+        (["invalid-github-pull-commits-patch-url"], ["PKG-DIRECTIVES", "PKG-PROPERTIES"]),
         # This package has a GitHub patch URL without full_index=1
         (["invalid-github-patch-url"], ["PKG-DIRECTIVES", "PKG-PROPERTIES"]),
         # This package has invalid GitLab patch URLs
diff --git a/var/spack/repos/builtin.mock/packages/invalid-github-pull-commits-patch-url/package.py b/var/spack/repos/builtin.mock/packages/invalid-github-pull-commits-patch-url/package.py
new file mode 100644
index 00000000000..6038864c37b
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/invalid-github-pull-commits-patch-url/package.py
@@ -0,0 +1,20 @@
+# Copyright 2013-2024 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.package import *
+
+
+class InvalidGithubPullCommitsPatchUrl(Package):
+    """Package that has a GitHub pull request commit patch URL that fails auditing."""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/patch-1.0.tar.gz"
+
+    version("1.0", md5="0123456789abcdef0123456789abcdef")
+
+    patch(
+        "https://github.com/spack/spack/pull/1/commits/b4da28f71e2cef84c6e289afe89aa4bdf7936048.patch?full_index=1",
+        sha256="eae9035b832792549fac00680db5f180a88ff79feb7d7a535b4fd71f9d885e73",
+    )
diff --git a/var/spack/repos/builtin/packages/dla-future/package.py b/var/spack/repos/builtin/packages/dla-future/package.py
index 05a25abc61d..87164e6097f 100644
--- a/var/spack/repos/builtin/packages/dla-future/package.py
+++ b/var/spack/repos/builtin/packages/dla-future/package.py
@@ -128,8 +128,8 @@ class DlaFuture(CMakePackage, CudaPackage, ROCmPackage):
             depends_on(f"umpire cuda_arch={arch}", when=f"cuda_arch={arch}")
 
     patch(
-        "https://github.com/eth-cscs/DLA-Future/pull/1063/commits/efc9c176a7a8c512b3f37d079dec8c25ac1b7389.patch?full_index=1",
-        sha256="7f382c872d89f22da1ad499e85ffe9881cc7404c8465e42877a210a09382e2ea",
+        "https://github.com/eth-cscs/DLA-Future/commit/efc9c176a7a8c512b3f37d079dec8c25ac1b7389.patch?full_index=1",
+        sha256="f40e4a734650f56c39379717a682d00d6400a7a102d90821542652824a8f64cd",
         when="@:0.3 %gcc@13:",
     )
     # https://github.com/spack/spack/issues/41511
diff --git a/var/spack/repos/builtin/packages/miopen-hip/package.py b/var/spack/repos/builtin/packages/miopen-hip/package.py
index 5a9c1136e1c..b87e1b83c4d 100644
--- a/var/spack/repos/builtin/packages/miopen-hip/package.py
+++ b/var/spack/repos/builtin/packages/miopen-hip/package.py
@@ -58,8 +58,8 @@ class MiopenHip(CMakePackage):
     patch("0002-add-include-dir-miopen-hip-6.0.0.patch", when="@6.0")
     patch("0002-add-include-dir-miopen-hip-6.1.0.patch", when="@6.1")
     patch(
-        "https://github.com/ROCm/MIOpen/pull/2276/commits/f60aa1ff89f8fb596b4a6a4c70aa7d557803db87.patch?full_index=1",
-        sha256="c777d9f4cd2bbfec632b38620c0f70bb0cce8da1",
+        "https://github.com/ROCm/MIOpen/commit/f60aa1ff89f8fb596b4a6a4c70aa7d557803db87.patch?full_index=1",
+        sha256="7f382c872d89f22da1ad499e85ffe9881cc7404c8465e42877a210a09382e2ea",
         when="@5.7",
     )
 
diff --git a/var/spack/repos/builtin/packages/mlpack/package.py b/var/spack/repos/builtin/packages/mlpack/package.py
index d84fdc3bc18..d9d6b9f8045 100644
--- a/var/spack/repos/builtin/packages/mlpack/package.py
+++ b/var/spack/repos/builtin/packages/mlpack/package.py
@@ -64,8 +64,8 @@ class Mlpack(CMakePackage):
         depends_on("r-pkgbuild")
 
     patch(
-        "https://github.com/mlpack/mlpack/pull/3502/commits/183396e51a6771d5d2b43f22b0d2a9a91785e533.patch?full_index=1",
-        sha256="eaa1791ca874201cca5fb661f44e0038f9996b2d02dac6c71d42935eac56a2b4",
+        "https://github.com/mlpack/mlpack/commit/183396e51a6771d5d2b43f22b0d2a9a91785e533.patch?full_index=1",
+        sha256="bd726818a8932888f8d38548cab7f8dde15bacfbd8c58a36ce6a3be8d459578d",
         when="@4:4.2.0",
     )
 
diff --git a/var/spack/repos/builtin/packages/openscenegraph/package.py b/var/spack/repos/builtin/packages/openscenegraph/package.py
index c1ec2f50106..ce0af8cec36 100644
--- a/var/spack/repos/builtin/packages/openscenegraph/package.py
+++ b/var/spack/repos/builtin/packages/openscenegraph/package.py
@@ -83,8 +83,8 @@ class Openscenegraph(CMakePackage):
     # patch submitted for inclusion in OpenSceneGraph for extending compatibility
     # with ffmpeg from versions up to 4 to versions 5 & 6
     patch(
-        "https://github.com/openscenegraph/OpenSceneGraph/pull/1281/commits/759620a3b7b787c960a7e414ba26ab5497817d40.patch?full_index=1",
-        sha256="b8f588d1fba9361127a7d5127e0720a4d64f44ef021515d1d67d77dcacdef8fd",
+        "https://github.com/openscenegraph/OpenSceneGraph/commit/759620a3b7b787c960a7e414ba26ab5497817d40.patch?full_index=1",
+        sha256="1e6daf0d15e916b69d62519a0ca4f8a722fe2144cbdab7dd182eaffb141e3c1a",
         when="@3.6:",
     )
     patch("glibc-jasper.patch", when="@3.4%gcc")
diff --git a/var/spack/repos/builtin/packages/py-astor/package.py b/var/spack/repos/builtin/packages/py-astor/package.py
index 8b7703fc7eb..6664b48b429 100644
--- a/var/spack/repos/builtin/packages/py-astor/package.py
+++ b/var/spack/repos/builtin/packages/py-astor/package.py
@@ -25,7 +25,7 @@ class PyAstor(PythonPackage):
     # https://github.com/berkerpeksag/astor/issues/162
     # https://github.com/berkerpeksag/astor/pull/163
     patch(
-        "https://github.com/berkerpeksag/astor/pull/163/commits/30059dac4eb832e58ab2109db84508b294ba366d.patch?full_index=1",
-        sha256="edc5eeddabe153b08e938f52edaeb2d880ee3128082967f310db0f98510fe6e0",
+        "https://github.com/berkerpeksag/astor/commit/30059dac4eb832e58ab2109db84508b294ba366d.patch?full_index=1",
+        sha256="4993c8d7e36b7fbad7586ff49e57fd8e7abe79724936445db2eed2d91398e82d",
         when="@0.8.0",
     )
diff --git a/var/spack/repos/builtin/packages/umpire/package.py b/var/spack/repos/builtin/packages/umpire/package.py
index d7684c207a8..f741ddbefb0 100644
--- a/var/spack/repos/builtin/packages/umpire/package.py
+++ b/var/spack/repos/builtin/packages/umpire/package.py
@@ -157,15 +157,15 @@ class Umpire(CachedCMakePackage, CudaPackage, ROCmPackage):
 
     # https://github.com/LLNL/Umpire/pull/805
     patch(
-        "https://github.com/LLNL/Umpire/pull/805/commits/47ff0aa1f7a01a917c3b7ac618e8a9e44a10fd25.patch?full_index=1",
-        sha256="7ed5d2c315a3b31e339f664f6108e32d7cb4cb8e9f22e5c78a65ba02625ccc09",
+        "https://github.com/LLNL/Umpire/commit/47ff0aa1f7a01a917c3b7ac618e8a9e44a10fd25.patch?full_index=1",
+        sha256="802f074a05e1cb1f428e13d99c5fcb1435f86bd8f36a1ea2f7b6756e6625e0a0",
         when="@2022.10.0",
     )
 
     # https://github.com/LLNL/Umpire/pull/816
     patch(
-        "https://github.com/LLNL/Umpire/pull/816/commits/2292d1d6078f6d9523b7ad0886ffa053644569d5.patch?full_index=1",
-        sha256="0f43cad7cdaec3c225ab6414ab9f81bd405a1157abf5a508e515bcb6ca53326d",
+        "https://github.com/LLNL/Umpire/commit/2292d1d6078f6d9523b7ad0886ffa053644569d5.patch?full_index=1",
+        sha256="170dbcadb9ae36c7e211119c17a812695f11f4fe1be290b750f7af4fb4896192",
         when="@2022.10.0",
     )
 
diff --git a/var/spack/repos/builtin/packages/wrf/package.py b/var/spack/repos/builtin/packages/wrf/package.py
index 15f81f0aace..db4e6cac0ee 100644
--- a/var/spack/repos/builtin/packages/wrf/package.py
+++ b/var/spack/repos/builtin/packages/wrf/package.py
@@ -216,13 +216,13 @@ class Wrf(Package):
     )
     # Add ARM compiler support
     patch(
-        "https://github.com/wrf-model/WRF/pull/1888/commits/4a084e03575da65f254917ef5d8eb39074abd3fc.patch",
-        sha256="c522c4733720df9a18237c06d8ab6199fa9674d78375b644aec7017cb38af9c5",
+        "https://github.com/wrf-model/WRF/commit/4a084e03575da65f254917ef5d8eb39074abd3fc.patch?full_index=1",
+        sha256="2d06d709074ded9bd6842aa83c0dfdad5a4e4e2df99e2e5d4a82579f0486117e",
         when="@4.5: %arm",
     )
     patch(
-        "https://github.com/wrf-model/WRF/pull/1888/commits/6087d9192f7f91967147e50f5bc8b9e49310cf98.patch",
-        sha256="f82a18cf7334e0cbbfdf4ef3aa91ca26d4a372709f114ce0116b3fbb136ffac6",
+        "https://github.com/wrf-model/WRF/commit/6087d9192f7f91967147e50f5bc8b9e49310cf98.patch?full_index=1",
+        sha256="7c6487aefaa6cda0fff3976e78da07b09d2ba6c005d649f35a0f8f1694a0b2bb",
         when="@4.5: %arm",
     )