Skip to content

Commit

Permalink
Add more detailed error messages to job_workflow_ref check (pypi#14319)
Browse files Browse the repository at this point in the history
  • Loading branch information
@di
di authored Aug 9, 2023
1 parent 5c666c7 commit 4b5327d
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 15 deletions.
90 changes: 78 additions & 12 deletions tests/unit/oidc/models/test_github.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,54 +236,115 @@ def test_github_publisher_verifies(self, monkeypatch, environment, missing_claim
)

@pytest.mark.parametrize(
("claim", "ref", "valid"),
("claim", "ref", "valid", "expected"),
[
# okay: workflow name, followed by a nonempty ref
(
"foo/bar/.github/workflows/baz.yml@refs/tags/v0.0.1",
"refs/tags/v0.0.1",
True,
None,
),
(
"foo/bar/.github/workflows/baz.yml@refs/pulls/6",
"refs/pulls/6",
True,
None,
),
("foo/bar/.github/workflows/baz.yml@refs/pulls/6", "refs/pulls/6", True),
(
"foo/bar/.github/workflows/baz.yml@refs/heads/main",
"refs/heads/main",
True,
None,
),
(
"foo/bar/.github/workflows/baz.yml@notrailingslash",
"notrailingslash",
True,
None,
),
# bad: workflow name, empty or missing ref
("foo/bar/.github/workflows/baz.yml@emptyref", "", False),
("foo/bar/.github/workflows/baz.yml@missingref", None, False),
(
"foo/bar/.github/workflows/baz.yml@emptyref",
"",
False,
"The ref claim is empty",
),
(
"foo/bar/.github/workflows/baz.yml@missingref",
None,
False,
"The ref claim is empty",
),
# bad: workflow name with various attempted impersonations
(
"foo/bar/.github/workflows/[email protected]@notrailingslash",
"notrailingslash",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@notrailingslash', got "
"'foo/bar/.github/workflows/[email protected]@notrailingslash'",
),
(
"foo/bar/.github/workflows/[email protected]@refs/pulls/6",
"refs/pulls/6",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@refs/pulls/6', got "
"'foo/bar/.github/workflows/[email protected]@refs/pulls/6'",
),
# bad: missing tail or workflow name or otherwise partial
("foo/bar/.github/workflows/baz.yml@", "notrailingslash", False),
("foo/bar/.github/workflows/@", "notrailingslash", False),
("foo/bar/.github/workflows/", "notrailingslash", False),
("baz.yml", "notrailingslash", False),
(
"foo/bar/.github/workflows/baz.yml@",
"notrailingslash",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@notrailingslash', got "
"'foo/bar/.github/workflows/baz.yml@'",
),
(
"foo/bar/.github/workflows/@",
"notrailingslash",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@notrailingslash', got "
"'foo/bar/.github/workflows/@'",
),
(
"foo/bar/.github/workflows/",
"notrailingslash",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@notrailingslash', got "
"'foo/bar/.github/workflows/'",
),
(
"baz.yml",
"notrailingslash",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@notrailingslash', got 'baz.yml'",
),
(
"foo/bar/.github/workflows/[email protected]@",
"notrailingslash",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@notrailingslash', got "
"'foo/bar/.github/workflows/[email protected]@'",
),
(
"foo/bar/.github/workflows/baz.yml@@",
"notrailingslash",
False,
"The claim does not match, expecting "
"'foo/bar/.github/workflows/baz.yml@notrailingslash', got "
"'foo/bar/.github/workflows/baz.yml@@'",
),
("foo/bar/.github/workflows/baz.yml@@", "notrailingslash", False),
("", "notrailingslash", False),
("", "notrailingslash", False, "The job_workflow_ref claim is empty"),
],
)
def test_github_publisher_job_workflow_ref(self, claim, ref, valid):
def test_github_publisher_job_workflow_ref(self, claim, ref, valid, expected):
publisher = github.GitHubPublisher(
repository_name="bar",
repository_owner="foo",
Expand All @@ -294,7 +355,12 @@ def test_github_publisher_job_workflow_ref(self, claim, ref, valid):
check = github.GitHubPublisher.__required_verifiable_claims__[
"job_workflow_ref"
]
assert check(publisher.job_workflow_ref, claim, {"ref": ref}) is valid
if valid:
assert check(publisher.job_workflow_ref, claim, {"ref": ref}) is True
else:
with pytest.raises(errors.InvalidPublisherError) as e:
check(publisher.job_workflow_ref, claim, {"ref": ref}) is True
assert str(e.value) == expected

@pytest.mark.parametrize(
("truth", "claim", "valid"),
Expand Down
12 changes: 9 additions & 3 deletions warehouse/oidc/models/github.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from sqlalchemy.orm import Query, mapped_column
from sqlalchemy.sql.expression import func, literal

from warehouse.oidc.errors import InvalidPublisherError
from warehouse.oidc.interfaces import SignedClaims
from warehouse.oidc.models._core import (
OIDCPublisher,
Expand All @@ -31,13 +32,18 @@ def _check_job_workflow_ref(ground_truth, signed_claim, all_signed_claims):
# Defensive: GitHub should never give us an empty job_workflow_ref,
# but we check for one anyways just in case.
if not signed_claim:
return False
raise InvalidPublisherError("The job_workflow_ref claim is empty")

ref = all_signed_claims.get("ref")
if not ref:
return False
raise InvalidPublisherError("The ref claim is empty")

if not (expected := f"{ground_truth}@{ref}") == signed_claim:
raise InvalidPublisherError(
f"The claim does not match, expecting {expected!r}, got {signed_claim!r}"
)

return f"{ground_truth}@{ref}" == signed_claim
return True


def _check_environment(ground_truth, signed_claim, all_signed_claims):
Expand Down

0 comments on commit 4b5327d

Please sign in to comment.