🐛 Trust pinned GitHub download URLs #3694

martincostello · 2023-11-27T16:03:51Z

What kind of change does this PR introduce?

Bug fix

PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

Files that are downloaded and executed from raw.githubusercontent.com, even if they are addressed by a Git SHA, are marked as downloadThenRun dependencies that are not pinned by hash.

What is the new behavior?

Files that are downloaded from raw.githubusercontent.com and are addressed by a Git SHA, and therefore immutable, are considered to be pinned by hash.

Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #3339

Special notes for your reviewer

Does this PR introduce a user-facing change?

Pinned-Dependencies: Files downloaded by Git SHA from GitHub and executed are no longer considered as not pinned by hash.

codecov · 2023-11-27T16:14:42Z

Codecov Report

Merging #3694 (d493588) into main (ce0b54e) will decrease coverage by 5.54%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3694      +/-   ##
==========================================
- Coverage   76.31%   70.78%   -5.54%     
==========================================
  Files         210      210              
  Lines       14371    14395      +24     
==========================================
- Hits        10967    10189     -778     
- Misses       2757     3614     +857     
+ Partials      647      592      -55

checks/raw/shell_download_validate.go

spencerschrock

Thanks for the well tested PR! Left a few small comments

checks/raw/pinned_dependencies_test.go

checks/raw/testdata/Dockerfile-curl-sh

checks/raw/testdata/Dockerfile-wget-bin-sh

checks/raw/shell_download_validate_test.go

spencerschrock · 2023-11-29T18:34:56Z

/scdiff generate Pinned-Dependencies

github-actions · 2023-11-29T18:35:11Z

Here's a link to the scdiff run

Trust files that are downloaded from `raw.githubusercontent.com` where the file's ref is a Git SHA and therefore immutable. Resolves #3339. Signed-off-by: martincostello <martin@martincostello.com>

- Add `hasUnpinnedURLs` function. - Add test cases for different URLs. Signed-off-by: martincostello <martin@martincostello.com>

Appease the linter. Signed-off-by: martincostello <martin@martincostello.com>

Suppress warning on three long URLs. Signed-off-by: martincostello <martin@martincostello.com>

Address peer review feedback. Signed-off-by: martincostello <martin@martincostello.com>

Fix lint warning. Signed-off-by: martincostello <martin@martincostello.com>

* Trust pinned GitHub download URLs Trust files that are downloaded from `raw.githubusercontent.com` where the file's ref is a Git SHA and therefore immutable. Resolves ossf#3339. Signed-off-by: martincostello <martin@martincostello.com> * Move logic to function - Add `hasUnpinnedURLs` function. - Add test cases for different URLs. Signed-off-by: martincostello <martin@martincostello.com> * Fix formatting Appease the linter. Signed-off-by: martincostello <martin@martincostello.com> * Suppress lint warnings Suppress warning on three long URLs. Signed-off-by: martincostello <martin@martincostello.com> * Address peer review Address peer review feedback. Signed-off-by: martincostello <martin@martincostello.com> * Fix lint warning Fix lint warning. Signed-off-by: martincostello <martin@martincostello.com> Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>

martincostello temporarily deployed to gitlab November 27, 2023 16:04 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 27, 2023 16:04 — with GitHub Actions Inactive

martincostello temporarily deployed to gitlab November 27, 2023 16:15 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 27, 2023 16:16 — with GitHub Actions Inactive

martincostello force-pushed the issue-3339 branch from 3d96833 to 3dfa68c Compare November 27, 2023 16:16

martincostello temporarily deployed to gitlab November 27, 2023 16:16 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 27, 2023 16:16 — with GitHub Actions Inactive

martincostello marked this pull request as ready for review November 27, 2023 16:27

martincostello requested a review from a team as a code owner November 27, 2023 16:27

martincostello requested review from raghavkaul and laurentsimon and removed request for a team November 27, 2023 16:27

laurentsimon reviewed Nov 27, 2023

View reviewed changes

checks/raw/shell_download_validate.go Outdated Show resolved Hide resolved

martincostello temporarily deployed to gitlab November 27, 2023 17:29 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 27, 2023 17:30 — with GitHub Actions Inactive

martincostello temporarily deployed to gitlab November 27, 2023 17:35 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 27, 2023 17:35 — with GitHub Actions Inactive

martincostello temporarily deployed to gitlab November 27, 2023 17:40 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 27, 2023 17:40 — with GitHub Actions Inactive

martincostello force-pushed the issue-3339 branch from aefb0dd to 06c9966 Compare November 27, 2023 18:11

martincostello temporarily deployed to gitlab November 27, 2023 18:11 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 27, 2023 18:11 — with GitHub Actions Inactive

martincostello force-pushed the issue-3339 branch from 06c9966 to 3b2ce51 Compare November 28, 2023 07:15

martincostello temporarily deployed to gitlab November 28, 2023 07:15 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 28, 2023 07:15 — with GitHub Actions Inactive

martincostello requested a review from laurentsimon November 28, 2023 09:32

martincostello force-pushed the issue-3339 branch from 3b2ce51 to 9232db8 Compare November 28, 2023 17:53

martincostello temporarily deployed to gitlab November 28, 2023 17:54 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 28, 2023 17:54 — with GitHub Actions Inactive

martincostello force-pushed the issue-3339 branch from 9232db8 to 8f63819 Compare November 29, 2023 07:49

martincostello temporarily deployed to gitlab November 29, 2023 07:49 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 29, 2023 07:50 — with GitHub Actions Inactive

spencerschrock approved these changes Nov 29, 2023

View reviewed changes

spencerschrock mentioned this pull request Nov 29, 2023

🌱 re-enable paralleltest linter #3705

Merged

2 tasks

martincostello temporarily deployed to gitlab November 30, 2023 09:22 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 30, 2023 09:22 — with GitHub Actions Inactive

martincostello requested a review from spencerschrock November 30, 2023 09:23

martincostello added 5 commits November 30, 2023 09:23

Trust pinned GitHub download URLs

dfa618e

Trust files that are downloaded from `raw.githubusercontent.com` where the file's ref is a Git SHA and therefore immutable. Resolves #3339. Signed-off-by: martincostello <martin@martincostello.com>

Move logic to function

e6e5e2b

- Add `hasUnpinnedURLs` function. - Add test cases for different URLs. Signed-off-by: martincostello <martin@martincostello.com>

Fix formatting

ecf0582

Appease the linter. Signed-off-by: martincostello <martin@martincostello.com>

Suppress lint warnings

6d0b106

Suppress warning on three long URLs. Signed-off-by: martincostello <martin@martincostello.com>

Address peer review

c88c741

Address peer review feedback. Signed-off-by: martincostello <martin@martincostello.com>

martincostello force-pushed the issue-3339 branch from ce6dee1 to c88c741 Compare November 30, 2023 09:23

martincostello temporarily deployed to gitlab November 30, 2023 09:23 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 30, 2023 09:24 — with GitHub Actions Inactive

Fix lint warning

d493588

Fix lint warning. Signed-off-by: martincostello <martin@martincostello.com>

martincostello temporarily deployed to gitlab November 30, 2023 09:33 — with GitHub Actions Inactive

martincostello temporarily deployed to integration-test November 30, 2023 09:33 — with GitHub Actions Inactive

spencerschrock approved these changes Nov 30, 2023

View reviewed changes

spencerschrock merged commit 0c40e14 into ossf:main Nov 30, 2023
38 checks passed

martincostello deleted the issue-3339 branch November 30, 2023 19:41

spencerschrock mentioned this pull request Dec 4, 2023

🌱 remove ununsed nolint directives #3713

Merged

2 tasks

bclark8923 mentioned this pull request Sep 6, 2024

Fix unpinned hashes in recent GHA workflow changes TraceMachina/nativelink#1297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Trust pinned GitHub download URLs #3694

🐛 Trust pinned GitHub download URLs #3694

martincostello commented Nov 27, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading

spencerschrock left a comment

spencerschrock commented Nov 29, 2023

github-actions bot commented Nov 29, 2023

🐛 Trust pinned GitHub download URLs #3694

🐛 Trust pinned GitHub download URLs #3694

Conversation

martincostello commented Nov 27, 2023

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

spencerschrock left a comment

Choose a reason for hiding this comment

spencerschrock commented Nov 29, 2023

github-actions bot commented Nov 29, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading