fix(security): Require a webhook signature and keep workflows in the payload's repo (#126)

* fix(security): Require a webhook signature and keep workflows in the payload's repo

Without GH_WEBHOOK_SECRET the app accepted unsigned payloads, so anyone could
make it mint a token for any installation and read any GitHub API path with it.
Reject every payload when the secret is unset, and treat a missing signature
header as invalid instead of raising.

Also reject payloads whose run_url, or workflow_run repository, points outside
the payload's repository, or whose owner doesn't match the repository. The DSN
comes from the owner's .sentry repo, so a mismatch would send one repo's data
to another org's Sentry project.

Co-Authored-By: Armen Zambrano Gasparnian <44410+armenzg@users.noreply.github.com>

* fix(security): Reject repository names that hold a path

Only the owner part of repository.full_name was checked, so a name like
getsentry/../victim/private passed and ended up in the API URL.

---------

Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com>
Co-authored-by: Armen Zambrano Gasparnian <44410+armenzg@users.noreply.github.com>

Author: sentry-junior[bot]

docs/deployment.md

@@ -13,7 +13,7 @@ A Sentry project tracks errors of the backend itself. Its DSN is `APP_DSN`.
 | Variable | Purpose |
 | --- | --- |
 | `APP_DSN` | Optional. Reports the app's own errors and performance to Sentry. |
-| `GH_WEBHOOK_SECRET` | Secret shared with the GitHub webhook. When unset, the app accepts unsigned requests. Generate one with `python3 -c 'import secrets; print(secrets.token_urlsafe(20))'`. |
+| `GH_WEBHOOK_SECRET` | Required. Secret shared with the GitHub webhook. The app rejects requests that aren't signed with it, and rejects every request when it's unset. Generate one with `python3 -c 'import secrets; print(secrets.token_urlsafe(20))'`. |
 | `GH_APP_ID` | GitHub App ID, shown on the GitHub App page. Setting it turns on GitHub App mode. |
 | `GH_APP_PRIVATE_KEY` | Base64-encoded GitHub App private key. On Cloud Run (`K_SERVICE` set) without this variable, the app reads the key from Google Secret Manager instead. See `get_gh_app_private_key` in `src/web_app_handler.py`. |
 | `GH_TOKEN` | Optional personal access token. The app uses it when `GH_APP_ID` is unset or the private key fails to load. |

docs/development.md

@@ -37,7 +37,7 @@ The argument must be a job URL. For a job in the `getsentry` org, you can set `I
 1. Start the app with `flask run -p 5001`.
 2. Start ngrok with `ngrok http 5001` and note the URL.
 3. Create a separate GitHub App for development, configured as in [deployment.md](deployment.md#configure-the-github-app), with its webhook URL set to the ngrok URL.
-4. Generate a private key for it and set `GH_APP_ID` and `GH_APP_PRIVATE_KEY` locally. Put secrets in `.env`, which `.envrc` loads.
+4. Generate a private key for it and set `GH_APP_ID`, `GH_APP_PRIVATE_KEY`, and the app's webhook secret as `GH_WEBHOOK_SECRET` locally. Put secrets in `.env`, which `.envrc` loads.
 5. Delete the private key from the GitHub App when you finish.
 
 Fixtures can carry old timestamps. Sentry then shows "Clock drift detected in SDK" on the transaction. You can ignore it.

src/web_app_handler.py

@@ -4,19 +4,24 @@
 import hmac
 import logging
 import os
+import re
 from typing import NamedTuple
 
 from sentry_sdk import metrics
 from sentry_sdk.utils import BadDsn
 
 from .github_app import GithubAppToken
+from .github_sdk import GITHUB_API_URL
 from .github_sdk import GithubClient
 from src.sentry_config import fetch_config_for_github_org
+from src.sentry_config import GITHUB_OWNER_PATTERN
 
 LOGGING_LEVEL = os.environ.get("LOGGING_LEVEL", logging.INFO)
 logger = logging.getLogger(__name__)
 logger.setLevel(LOGGING_LEVEL)
 
+GITHUB_REPO_NAME_PATTERN = re.compile(r"[A-Za-z0-9._-]+")
+
 
 def commit_traces_enabled() -> bool:
     # Off by default: it costs one GitHub API call per successful workflow run
@@ -42,7 +47,9 @@ def handle_event(self, data, headers):
                 reason = "Skipped jobs are not traced."
             elif not self.dry_run:
                 reason, http_code = self._send(
-                    data, lambda client, _: client.send_trace(data["workflow_job"])
+                    event,
+                    data,
+                    lambda client, _: client.send_trace(data["workflow_job"]),
                 )
         elif event == "workflow_run":
             if not commit_traces_enabled():
@@ -55,6 +62,7 @@ def handle_event(self, data, headers):
                 reason = "Only successful workflow runs can turn a commit green."
             elif not self.dry_run:
                 reason, http_code = self._send(
+                    event,
                     data,
                     lambda client, config: client.send_commit_trace(
                         data["workflow_run"], config.commit_workflows
@@ -65,7 +73,7 @@ def handle_event(self, data, headers):
 
         return reason, http_code
 
-    def _send(self, data, send) -> tuple[str, int]:
+    def _send(self, event, data, send) -> tuple[str, int]:
         # Only GitHub App webhooks carry an installation; repository webhooks don't
         installation_id = (data.get("installation") or {}).get("id")
         org = data["repository"]["owner"]["login"]
@@ -74,6 +82,8 @@ def _send(self, data, send) -> tuple[str, int]:
                 "The payload has no installation. Send it from the GitHub App's webhook.",
                 400,
             )
+        if not _targets_payload_repository(event, data):
+            return "The payload's workflow doesn't belong to its repository.", 400
 
         try:
             # We are executing in Github App mode
@@ -97,16 +107,44 @@ def _send_with_token(self, token, installation_id, org, send):
         send(client, config)
 
     def valid_signature(self, body, headers):
+        # Without a secret anyone could send a payload, and the app would act on it
+        # with the token of whichever installation the payload names
         if not self.config.gh.webhook_secret:
-            return True
-        else:
-            signature = headers["X-Hub-Signature-256"].replace("sha256=", "")
-            body_signature = hmac.new(
-                self.config.gh.webhook_secret.encode(),
-                msg=body,
-                digestmod="sha256",
-            ).hexdigest()
-            return hmac.compare_digest(body_signature, signature)
+            return False
+        signature = headers.get("X-Hub-Signature-256", "").replace("sha256=", "")
+        body_signature = hmac.new(
+            self.config.gh.webhook_secret.encode(),
+            msg=body,
+            digestmod="sha256",
+        ).hexdigest()
+        return hmac.compare_digest(body_signature, signature)
+
+
+def _targets_payload_repository(event: str, data: dict) -> bool:
+    """Whether the workflow the app reads with the token is in the payload's repository.
+
+    The DSN comes from the repository owner's .sentry repo, so a workflow from another
+    repository would send that repository's data to the owner's Sentry project.
+    """
+    repo = data["repository"]["full_name"]
+    owner, _, name = repo.partition("/")
+    if owner.lower() != data["repository"]["owner"]["login"].lower():
+        return False
+    # The name ends up in API URLs, so it can't hold another path (e.g. `../other/repo`)
+    if (
+        not GITHUB_OWNER_PATTERN.fullmatch(owner)
+        or not GITHUB_REPO_NAME_PATTERN.fullmatch(name)
+        or name in (".", "..")
+    ):
+        return False
+    if event == "workflow_job":
+        run_url = (
+            rf"{re.escape(GITHUB_API_URL)}/repos/{re.escape(repo)}/actions/runs/\d+"
+        )
+        return bool(
+            re.fullmatch(run_url, data["workflow_job"]["run_url"], re.IGNORECASE)
+        )
+    return data["workflow_run"]["repository"]["full_name"].lower() == repo.lower()
 
 
 class GithubAppConfig(NamedTuple):
@@ -166,11 +204,15 @@ def init_config():
             "We have failed to load the private key, however, we will fallback to the PAT method.",
         )
 
+    webhook_secret = os.environ.get("GH_WEBHOOK_SECRET")
+    if not webhook_secret:
+        logger.error("GH_WEBHOOK_SECRET is not set, so every webhook will be rejected.")
+
     return Config(
         gh_app,
         GitHubConfig(
             # This token is a PAT
             token=os.environ.get("GH_TOKEN"),
-            webhook_secret=os.environ.get("GH_WEBHOOK_SECRET"),
+            webhook_secret=webhook_secret,
         ),
     )

tests/test_web_app_handler.py

@@ -71,6 +71,7 @@ def gh_app_env(monkeypatch):
     monkeypatch.delenv("K_SERVICE", raising=False)
     monkeypatch.setenv("GH_APP_ID", "1")
     monkeypatch.setenv("GH_APP_PRIVATE_KEY", base64.b64encode(pem).decode())
+    monkeypatch.setenv("GH_WEBHOOK_SECRET", "fake_secret")
 
 
 @responses.activate
@@ -82,7 +83,10 @@ def test_skipped_job_makes_no_api_calls(gh_app_env, skipped_workflow):
         data={
             "action": "completed",
             "installation": {"id": 1},
-            "repository": {"owner": {"login": "getsentry"}},
+            "repository": {
+                "full_name": "getsentry/sentry",
+                "owner": {"login": "getsentry"},
+            },
             "workflow_job": skipped_workflow,
         },
         headers={"X-GitHub-Event": "workflow_job"},
@@ -110,7 +114,10 @@ def test_invalid_dsn_is_a_warning(mock_metrics, gh_app_env, jobA_job, caplog):
         data={
             "action": "completed",
             "installation": {"id": 1},
-            "repository": {"owner": {"login": "getsentry"}},
+            "repository": {
+                "full_name": "getsentry/sentry",
+                "owner": {"login": "getsentry"},
+            },
             "workflow_job": jobA_job,
         },
         headers={"X-GitHub-Event": "workflow_job"},
@@ -143,10 +150,21 @@ def test_missing_workflow_job(monkeypatch):
     assert msg == "workflow_job"
 
 
-def test_valid_signature_no_secret(monkeypatch):
+def test_no_secret_rejects_every_payload(monkeypatch, webhook_event):
     monkeypatch.delenv("GH_WEBHOOK_SECRET", raising=False)
     handler = WebAppHandler()
-    assert handler.valid_signature(body={}, headers={}) == True
+    assert not handler.valid_signature(
+        body=json.dumps(webhook_event["payload"]).encode(),
+        headers={"X-Hub-Signature-256": f"sha256={valid_signature}"},
+    )
+
+
+def test_missing_signature_is_invalid(monkeypatch, webhook_event):
+    monkeypatch.setenv("GH_WEBHOOK_SECRET", "fake_secret")
+    handler = WebAppHandler()
+    assert not handler.valid_signature(
+        body=json.dumps(webhook_event["payload"]).encode(), headers={}
+    )
 
 
 def test_valid_signature(monkeypatch, webhook_event):
@@ -222,7 +240,10 @@ def test_repository_webhook_in_github_app_mode_is_rejected(gh_app_env, jobA_job)
     reason, http_code = handler.handle_event(
         data={
             "action": "completed",
-            "repository": {"owner": {"login": "getsentry"}},
+            "repository": {
+                "full_name": "getsentry/sentry",
+                "owner": {"login": "getsentry"},
+            },
             "workflow_job": jobA_job,
         },
         headers={"X-GitHub-Event": "workflow_job"},
@@ -248,7 +269,10 @@ def test_repository_webhook_uses_the_token(mock_metrics, monkeypatch, jobA_job):
     reason, http_code = handler.handle_event(
         data={
             "action": "completed",
-            "repository": {"owner": {"login": "getsentry"}},
+            "repository": {
+                "full_name": "getsentry/sentry",
+                "owner": {"login": "getsentry"},
+            },
             "workflow_job": jobA_job,
         },
         headers={"X-GitHub-Event": "workflow_job"},
@@ -257,3 +281,71 @@ def test_repository_webhook_uses_the_token(mock_metrics, monkeypatch, jobA_job):
     assert reason == "The DSN in the org's .sentry/sentry_config.ini is invalid."
     assert http_code == 200
     assert responses.calls[0].request.headers["Authorization"] == "token pat"
+
+
+@responses.activate
+@pytest.mark.parametrize(
+    "owner,run_url",
+    (
+        # The DSN would come from attacker's .sentry repo
+        ("attacker", "https://api.github.com/repos/getsentry/sentry/actions/runs/1"),
+        ("getsentry", "https://api.github.com/repos/getsentry/private/actions/runs/1"),
+        (
+            "getsentry",
+            "https://api.github.com/repos/getsentry/sentry/actions/runs/1/../../../private/actions/runs/1",
+        ),
+        ("getsentry", "https://api.github.com/repos/getsentry/sentry/actions/secrets"),
+    ),
+)
+def test_job_outside_the_payload_repository_is_rejected(
+    gh_app_env, jobA_job, owner, run_url
+):
+    reason, http_code = WebAppHandler().handle_event(
+        data={
+            "action": "completed",
+            "installation": {"id": 1},
+            "repository": {"full_name": "getsentry/sentry", "owner": {"login": owner}},
+            "workflow_job": {**jobA_job, "run_url": run_url},
+        },
+        headers={"X-GitHub-Event": "workflow_job"},
+    )
+    assert reason == "The payload's workflow doesn't belong to its repository."
+    assert http_code == 400
+    assert len(responses.calls) == 0
+
+
+@responses.activate
+@pytest.mark.parametrize(
+    "full_name", ("getsentry/../victim/private", "getsentry/..", "getsentry/a/b")
+)
+def test_repository_name_with_a_path_is_rejected(gh_app_env, jobA_job, full_name):
+    reason, http_code = WebAppHandler().handle_event(
+        data={
+            "action": "completed",
+            "installation": {"id": 1},
+            "repository": {"full_name": full_name, "owner": {"login": "getsentry"}},
+            "workflow_job": {
+                **jobA_job,
+                "run_url": f"https://api.github.com/repos/{full_name}/actions/runs/1",
+            },
+        },
+        headers={"X-GitHub-Event": "workflow_job"},
+    )
+    assert reason == "The payload's workflow doesn't belong to its repository."
+    assert http_code == 400
+    assert len(responses.calls) == 0
+
+
+@responses.activate
+def test_run_outside_the_payload_repository_is_rejected(monkeypatch, gh_app_env):
+    with open("tests/fixtures/commit/webhook_payload.json") as f:
+        payload = json.load(f)
+    monkeypatch.setenv("COMMIT_TRACES_ENABLED", "true")
+    payload["workflow_run"]["repository"]["full_name"] = "getsentry/private"
+
+    reason, http_code = WebAppHandler().handle_event(
+        data=payload, headers={"X-GitHub-Event": "workflow_run"}
+    )
+    assert reason == "The payload's workflow doesn't belong to its repository."
+    assert http_code == 400
+    assert len(responses.calls) == 0