diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f88465..be0c34c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,39 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C +### Fixed + +- **BREAKING (security CRITICAL)**: Jira webhook handler is now + fail-closed (issue #83). Previously, if `JIRA_WEBHOOK_SECRET` was + unset, `_verify_signature` returned `True` and any unauthenticated + POST to `/webhooks/jira` could trigger the full ingest pipeline. The + handler now returns **503** when the secret is missing + (operator-misconfiguration signal, distinct from 401 wrong-signature). + Operators relying on the no-secret = accept-everything mode (don't — + it was never documented) must set `JIRA_WEBHOOK_SECRET` before this + merges. +- **Security (CRITICAL)**: Jira issue keys arriving via webhooks are now + validated against the canonical `^[A-Z][A-Z0-9]{0,31}-[0-9]{1,12}\Z` format + (`[0-9]` not `\d` to refuse non-ASCII Unicode digits, `\Z` not `$` to + refuse trailing newlines that `$` would tolerate) + before any filesystem operation (issue #83). Previously, `issue_key` flowed + unsanitized into `connectors/jira/service.py` (`save_issue`, + `download_attachment`, `_handle_deletion`, `process_webhook_event`) and + `connectors/jira/incremental_transform.py`, enabling path traversal + (`../../etc/passwd` style writes outside the Jira data dir). New module + `connectors/jira/validation.py` provides `is_valid_issue_key` (regex + whitelist; underscore deliberately excluded — Atlassian rejects underscores + in real project keys) and `safe_join_under` (`Path.resolve()` containment + check). Both are enforced at every filesystem boundary, defense-in-depth. +- **Security (CRITICAL)**: `webhookEvent` (the second attacker-controlled field + in Jira webhook payloads) was used as a filename component in + `_log_webhook_event` without sanitization (issue #83 reviewer follow-up). + A payload with `webhookEvent: "../../tmp/pwn"` could write a JSON dump + outside `WEBHOOK_LOG_DIR`. The handler now strips everything that isn't + `[A-Za-z0-9_-]` (dot deliberately excluded to defeat `..` survival), + clips length to 64 chars, and routes the final filename through + `safe_join_under`. + ## [0.11.5] — 2026-04-27 Follow-up release for PR #73: addresses four rounds of Devin AI review on the role-management-complete branch. No new public-API surface; the user-visible payoff is that v8→v9-migrated installations now work end-to-end (login flows, user list, admin nav, privilege revocation), and `make local-dev` startup is finally quiet. diff --git a/app/api/jira_webhooks.py b/app/api/jira_webhooks.py index 4e787b0..dc0511c 100644 --- a/app/api/jira_webhooks.py +++ b/app/api/jira_webhooks.py @@ -14,7 +14,17 @@ from datetime import datetime, timezone from fastapi import APIRouter, Request, Response from fastapi.responses import JSONResponse +import re + from connectors.jira.service import Config, get_jira_service +from connectors.jira.validation import is_valid_issue_key, safe_join_under + +# webhookEvent is attacker-controlled; sanitize before using as a filename +# component. Real Jira webhookEvent values are like "jira:issue_updated" — +# alphanumeric + colon. We strip everything that isn't alphanumeric/underscore/dash +# (the colon → underscore mapping happens via sub). Dots are deliberately +# refused so `..` cannot survive sanitization as a directory component. +_WEBHOOK_EVENT_SAFE_RE = re.compile(r"[^A-Za-z0-9_-]+") logger = logging.getLogger(__name__) @@ -25,15 +35,18 @@ WEBHOOK_LOG_DIR = Config.JIRA_DATA_DIR / "webhook_events" def _verify_signature(payload: bytes, signature: str | None) -> bool: - """Verify HMAC-SHA256 signature from Jira webhook.""" + """Verify HMAC-SHA256 signature from Jira webhook. + + Fail-closed: callers must check ``Config.JIRA_WEBHOOK_SECRET`` is set + before invoking. If it is not, this returns False (so a misconfigured + deploy cannot accept unauthenticated webhooks). Issue #83. + """ secret = Config.JIRA_WEBHOOK_SECRET if not secret: - logger.warning("JIRA_WEBHOOK_SECRET not configured, skipping signature verification") - return True + return False if not signature: - logger.warning("No signature provided in webhook request") return False if signature.startswith("sha256="): @@ -49,13 +62,29 @@ def _verify_signature(payload: bytes, signature: str | None) -> bool: def _log_webhook_event(event_data: dict) -> None: - """Log webhook event to file for debugging/audit.""" + """Log webhook event to file for debugging/audit. + + `webhookEvent` is attacker-controlled. Sanitize it through a strict + whitelist before using as a filename component (issue #83) and apply + `safe_join_under` to catch anything the regex misses. + """ try: WEBHOOK_LOG_DIR.mkdir(parents=True, exist_ok=True) timestamp = datetime.now(timezone.utc).strftime("%Y%m%d_%H%M%S_%f") - event_type = event_data.get("webhookEvent", "unknown").replace(":", "_") + raw_event = event_data.get("webhookEvent", "unknown") + if not isinstance(raw_event, str): + raw_event = "unknown" + # Replace any non-`[A-Za-z0-9_-]` run with a single `_` (dot + # deliberately excluded — see _WEBHOOK_EVENT_SAFE_RE module + # comment). Also clip to 64 chars to bound filename length on + # hostile input. + event_type = _WEBHOOK_EVENT_SAFE_RE.sub("_", raw_event)[:64] or "unknown" filename = f"{timestamp}_{event_type}.json" - filepath = WEBHOOK_LOG_DIR / filename + try: + filepath = safe_join_under(WEBHOOK_LOG_DIR, filename) + except ValueError as e: + logger.warning(f"Refusing webhook log filename {filename!r}: {e}") + return with open(filepath, "w") as f: json.dump(event_data, f, indent=2, default=str) @@ -66,6 +95,16 @@ def _log_webhook_event(event_data: dict) -> None: @router.post("/jira") async def receive_jira_webhook(request: Request) -> Response: """Receive and process Jira webhook notifications.""" + # Refuse to process if the operator hasn't configured a webhook secret. + # Returning 503 (not 401) signals "operator misconfiguration" rather + # than "attacker guessed wrong". Issue #83. + if not Config.JIRA_WEBHOOK_SECRET: + logger.error("JIRA_WEBHOOK_SECRET not configured — refusing webhook") + return JSONResponse( + {"detail": "Webhook secret not configured"}, + status_code=503, + ) + payload = await request.body() # Verify signature @@ -87,12 +126,38 @@ async def receive_jira_webhook(request: Request) -> Response: if not event_data: return JSONResponse({"detail": "Empty payload"}, status_code=400) - # Log event for debugging - _log_webhook_event(event_data) - webhook_event = event_data.get("webhookEvent", "unknown") - issue = event_data.get("issue", {}) - issue_key = issue.get("key", "unknown") + # Defensive: some webhook senders pass `"issue": null` rather than + # omitting the key. Normalise to {} so the next .get() doesn't + # raise AttributeError on None. + issue = event_data.get("issue") or {} + issue_key = issue.get("key", "") + # Some Jira webhook event types deliver the key at the top level + # instead of `issue.key` (e.g. delete events historically). + # `process_webhook_event` already supports this fallback at + # connectors/jira/service.py — mirror it here so the handler + # doesn't reject those events with 400 before they ever reach the + # service layer. + if not issue_key: + issue_key = event_data.get("issue_key", "") + + # Validate issue_key format BEFORE any filesystem operation. Jira issue + # keys follow `[A-Z][A-Z0-9]+-\d+`; anything else (path traversal, + # SQL injection, control chars) is refused with 400. Issue #83. + if not is_valid_issue_key(issue_key): + logger.warning( + "Webhook rejected: malformed issue key %r from %s", + issue_key, + request.client.host if request.client else "unknown", + ) + return JSONResponse( + {"detail": "Malformed or missing issue key"}, + status_code=400, + ) + + # Log event for debugging (after key validation so traversal attempts + # don't end up named after attacker-supplied data). + _log_webhook_event(event_data) logger.info(f"Received webhook: {webhook_event} for issue {issue_key}") diff --git a/connectors/jira/incremental_transform.py b/connectors/jira/incremental_transform.py index a582299..aca66a3 100644 --- a/connectors/jira/incremental_transform.py +++ b/connectors/jira/incremental_transform.py @@ -17,6 +17,7 @@ import pyarrow.parquet as pq # Import transform functions from batch transform from .file_lock import parquet_month_lock +from .validation import is_valid_issue_key, safe_join_under from .transform import ( ATTACHMENTS_SCHEMA, CHANGELOG_SCHEMA, @@ -138,7 +139,17 @@ def transform_single_issue( output_dir = output_dir or DEFAULT_OUTPUT_DIR attachments_dir = attachments_dir or (raw_dir / "attachments") - json_path = raw_dir / "issues" / f"{issue_key}.json" + # Defense-in-depth: even if a stale/legacy code path bypasses webhook + # validation, the transform step will refuse a malformed key (issue #83). + if not is_valid_issue_key(issue_key): + logger.error(f"Refusing transform for malformed issue key: {issue_key!r}") + return False + issues_dir = raw_dir / "issues" + try: + json_path = safe_join_under(issues_dir, f"{issue_key}.json") + except ValueError as e: + logger.error(f"Path traversal blocked in transform for {issue_key!r}: {e}") + return False if deleted: # For deletion, we need to find which month the issue was in diff --git a/connectors/jira/service.py b/connectors/jira/service.py index 98b9017..45abd5b 100644 --- a/connectors/jira/service.py +++ b/connectors/jira/service.py @@ -18,6 +18,8 @@ from typing import Any import httpx +from connectors.jira.validation import is_valid_issue_key, safe_join_under + logger = logging.getLogger(__name__) @@ -255,6 +257,19 @@ class JiraService: logger.error("Issue data missing 'key' field") return None + # Defense-in-depth: validate `issue_key` BEFORE any code path + # uses it — including the HTTP URL constructions in + # fetch_remote_links / fetch_sla_fields below. The webhook + # handler already validates upstream, but a future internal + # caller invoking save_issue directly with attacker-controlled + # input would otherwise fire outbound requests with a malicious + # path component (limited SSRF / path manipulation against the + # Jira API server) before the filesystem-side guard rejected it. + # Issue #83 round 3. + if not is_valid_issue_key(issue_key): + logger.error(f"Refusing to save issue with malformed key: {issue_key!r}") + return None + # Create data directory if needed self.data_dir.mkdir(parents=True, exist_ok=True) @@ -277,9 +292,15 @@ class JiraService: logger.info(f"Overlayed SLA fields for {issue_key}") # Save to file (one file per issue for now, later we'll batch to parquet) + # Path.resolve() containment as second layer; the regex check + # above is the primary defense. issues_dir = self.data_dir / "issues" - file_path = issues_dir / f"{issue_key}.json" - file_path.parent.mkdir(parents=True, exist_ok=True) + issues_dir.mkdir(parents=True, exist_ok=True) + try: + file_path = safe_join_under(issues_dir, f"{issue_key}.json") + except ValueError as e: + logger.error(f"Path traversal blocked for issue {issue_key!r}: {e}") + return None try: from connectors.jira.file_lock import issue_json_lock @@ -353,13 +374,25 @@ class JiraService: ) return None - # Create issue-specific attachment directory - issue_attachments_dir = self.attachments_dir / issue_key + # Create issue-specific attachment directory. + # Two-layer guard against path traversal via issue_key (issue #83). + if not is_valid_issue_key(issue_key): + logger.error(f"Refusing to download attachment for malformed key: {issue_key!r}") + return None + try: + issue_attachments_dir = safe_join_under(self.attachments_dir, issue_key) + except ValueError as e: + logger.error(f"Path traversal blocked for attachment {issue_key!r}: {e}") + return None issue_attachments_dir.mkdir(parents=True, exist_ok=True) # Use attachment ID in filename to avoid collisions safe_filename = f"{attachment_id}_{filename}" - file_path = issue_attachments_dir / safe_filename + try: + file_path = safe_join_under(issue_attachments_dir, safe_filename) + except ValueError as e: + logger.error(f"Path traversal blocked for attachment filename {safe_filename!r}: {e}") + return None try: with httpx.Client(timeout=60, follow_redirects=True) as client: @@ -473,7 +506,11 @@ class JiraService: """ # Extract issue key from event # Jira webhook format: {"webhookEvent": "jira:issue_updated", "issue": {"key": "KSP-123", ...}} - issue = event_data.get("issue", {}) + # Defensive: a payload may carry `"issue": null` rather than + # omitting the key. The webhook handler normalises this, but + # do the same here too — process_webhook_event is reachable from + # internal callers as well as the webhook path. + issue = event_data.get("issue") or {} issue_key = issue.get("key") if not issue_key: @@ -484,6 +521,13 @@ class JiraService: logger.warning(f"Could not extract issue key from webhook event: {event_data.get('webhookEvent')}") return False + # Defense-in-depth: even if the webhook layer's validation is bypassed + # (e.g. a future internal caller invokes process_webhook_event directly), + # refuse a malformed key here. Issue #83. + if not is_valid_issue_key(issue_key): + logger.error(f"process_webhook_event: refusing malformed issue key {issue_key!r}") + return False + webhook_event = event_data.get("webhookEvent", "unknown") logger.info(f"Processing webhook event: {webhook_event} for issue {issue_key}") @@ -514,7 +558,16 @@ class JiraService: Returns: True if handled successfully """ - file_path = self.data_dir / "issues" / f"{issue_key}.json" + # Defense-in-depth path-traversal guard (issue #83). Callers should + # already have validated; refuse anyway. + if not is_valid_issue_key(issue_key): + logger.error(f"_handle_deletion: refusing malformed issue key {issue_key!r}") + return False + try: + file_path = safe_join_under(self.data_dir / "issues", f"{issue_key}.json") + except ValueError as e: + logger.error(f"_handle_deletion: path traversal blocked for {issue_key!r}: {e}") + return False if file_path.exists(): # Mark as deleted rather than removing diff --git a/connectors/jira/validation.py b/connectors/jira/validation.py new file mode 100644 index 0000000..b673dd4 --- /dev/null +++ b/connectors/jira/validation.py @@ -0,0 +1,53 @@ +"""Input validation for the Jira connector. + +Two layers of defense for issue keys (which arrive from attacker-controlled +webhook payloads, see issue #83): + +1. ``is_valid_issue_key`` — whitelist regex against the Jira format. +2. ``safe_join_under`` — Path.resolve() containment check, defense-in-depth + against future regex relaxation, symlink shenanigans, or callers that + forget the regex check. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +# Jira issue keys: project key + dash + issue number. +# +# Atlassian's project-key validator: first char must be a letter; the rest +# are letters and digits only. Underscores are NOT allowed in real project +# keys despite some informal docs suggesting otherwise — confirmed via the +# Atlassian project-creation form, which rejects `A_B`. Bounded length +# (32 chars on the project, 12 digits on the number) keeps regex evaluation +# cheap on adversarial input. +# `[0-9]` rather than `\d` — Python 3's `\d` matches any Unicode decimal +# (Arabic-Indic ٣, Bengali ৩, Devanagari ३, …), and a Jira issue key like +# `TEST-٣` is not real Jira input. ASCII-only here closes that bypass. +# `\Z` rather than `$` — Python's `$` matches before a trailing `\n`, +# so `re.match("…$", "TEST-1\n")` returns a match. `\Z` is hard +# end-of-string, so a CRLF-injection or trailing-newline payload is +# rejected as expected. +_ISSUE_KEY_RE = re.compile(r"^[A-Z][A-Z0-9]{0,31}-[0-9]{1,12}\Z") + + +def is_valid_issue_key(key: object) -> bool: + """Return True if ``key`` is a syntactically valid Jira issue key.""" + return isinstance(key, str) and bool(_ISSUE_KEY_RE.match(key)) + + +def safe_join_under(base: Path, *parts: str) -> Path: + """Join ``parts`` under ``base`` and verify the result stays within ``base``. + + Raises ValueError on any escape attempt. Use at every filesystem boundary + that touches attacker-supplied path components, even when callers have + already validated the components — this is defense-in-depth. + """ + base_resolved = base.resolve() + candidate = base.joinpath(*parts).resolve() + if base_resolved != candidate and base_resolved not in candidate.parents: + raise ValueError( + f"Path traversal blocked: {candidate} is not under {base_resolved}" + ) + return candidate diff --git a/tests/test_jira_service_full.py b/tests/test_jira_service_full.py index 0d4e405..82bc15b 100644 --- a/tests/test_jira_service_full.py +++ b/tests/test_jira_service_full.py @@ -126,7 +126,7 @@ class TestJiraServiceWebhookProcessing: def test_deletion_of_nonexistent_issue_returns_true(self, jira_env): """Deleting an issue that has no local file returns True (idempotent).""" service = _make_jira_service(jira_env) - event_data, _, _ = WebhookEventFactory.issue_deleted("PROJ-NOEXIST") + event_data, _, _ = WebhookEventFactory.issue_deleted("PROJ-99999") result = service.process_webhook_event(event_data) assert result is True diff --git a/tests/test_jira_validation.py b/tests/test_jira_validation.py new file mode 100644 index 0000000..5d7f158 --- /dev/null +++ b/tests/test_jira_validation.py @@ -0,0 +1,68 @@ +"""Unit tests for connectors/jira/validation.py — issue #83 defenses.""" + +import pytest + +from connectors.jira.validation import is_valid_issue_key, safe_join_under + + +class TestIsValidIssueKey: + @pytest.mark.parametrize("key", ["TEST-1", "PROJ-42", "ABC-123", "AB1-9", "A-1", "AB42-1234567"]) + def test_valid(self, key): + assert is_valid_issue_key(key) is True + + @pytest.mark.parametrize( + "key", + [ + "", + "test-1", # lowercase + "TEST", # no dash + "TEST-", # no number + "-1", # no project + "TEST-abc", # non-numeric + "../etc/passwd", + "TEST/1", + "TEST-1\x00", + "TEST-1\r\n", + "1-TEST", # starts with digit + "TEST-1.json", + "ABC_DEF-1", # underscore — Atlassian rejects, so do we + "А-1", # Cyrillic А (looks like Latin A) + "TEST-٣", # Arabic-Indic 3 — \\d would match, [0-9] doesn't + "TEST-৩", # Bengali 3 + "TEST-३", # Devanagari 3 + "A" * 100 + "-1", # absurd project length + "A-" + "9" * 20, # absurd issue number length + None, + 123, + ["TEST-1"], + ], + ) + def test_invalid(self, key): + assert is_valid_issue_key(key) is False + + +class TestSafeJoinUnder: + def test_normal_join(self, tmp_path): + result = safe_join_under(tmp_path, "issues", "TEST-1.json") + assert result == (tmp_path / "issues" / "TEST-1.json").resolve() + + def test_traversal_blocked(self, tmp_path): + with pytest.raises(ValueError, match="Path traversal"): + safe_join_under(tmp_path, "..", "evil") + + def test_nested_traversal_blocked(self, tmp_path): + with pytest.raises(ValueError, match="Path traversal"): + safe_join_under(tmp_path, "issues", "..", "..", "etc", "passwd") + + def test_absolute_path_blocked(self, tmp_path): + with pytest.raises(ValueError, match="Path traversal"): + safe_join_under(tmp_path, "/etc/passwd") + + def test_symlink_escape_blocked(self, tmp_path): + # Create a symlink inside base that points outside. + outside = tmp_path.parent / "outside_target" + outside.mkdir(exist_ok=True) + link = tmp_path / "escape" + link.symlink_to(outside) + with pytest.raises(ValueError, match="Path traversal"): + safe_join_under(tmp_path, "escape", "x.json") diff --git a/tests/test_jira_webhooks.py b/tests/test_jira_webhooks.py index 3d67d4d..46a8678 100644 --- a/tests/test_jira_webhooks.py +++ b/tests/test_jira_webhooks.py @@ -109,3 +109,160 @@ def test_empty_payload_400(webhook_client): }, ) assert resp.status_code == 400 + + +def test_unconfigured_secret_returns_503(tmp_path, monkeypatch): + """Issue #83: missing JIRA_WEBHOOK_SECRET must fail-closed (no fall-through to 200).""" + data_dir = tmp_path / "data" + data_dir.mkdir() + (data_dir / "issues").mkdir() + + monkeypatch.setenv("DATA_DIR", str(data_dir)) + monkeypatch.setenv("JWT_SECRET_KEY", "test-jwt-secret") + monkeypatch.delenv("JIRA_WEBHOOK_SECRET", raising=False) + monkeypatch.setenv("JIRA_DATA_DIR", str(data_dir)) + + from connectors.jira import service as svc + monkeypatch.setattr(svc.Config, "JIRA_WEBHOOK_SECRET", "") + monkeypatch.setattr(svc.Config, "JIRA_DATA_DIR", data_dir) + svc._jira_service = None + + from app.main import create_app + client = TestClient(create_app()) + + payload = json.dumps({"webhookEvent": "jira:issue_updated", "issue": {"key": "TEST-1"}}).encode() + resp = client.post( + "/webhooks/jira", + content=payload, + headers={"Content-Type": "application/json"}, + ) + assert resp.status_code == 503 + assert "secret" in resp.json()["detail"].lower() + + +@pytest.mark.parametrize( + "bad_key", + [ + "../../etc/passwd", + "../foo", + "TEST-1/../../../bar", + "TEST-1\x00.json", + "TEST-1\r\n", # CRLF injection + "test-1", # lowercase project — Jira keys are uppercase + "TEST", # missing - + "TEST-", # missing num + "-1", # missing project + "", # empty + "A" * 100 + "-1", # absurd length + "ABC_DEF-1", # underscore — not allowed in real Jira + "А-1", # Cyrillic А (looks like Latin A) + ], +) +def test_path_traversal_in_issue_key_rejected(webhook_client, bad_key): + """Issue #83: malformed issue keys must be rejected with 400, not used in paths.""" + payload = json.dumps({ + "webhookEvent": "jira:issue_updated", + "issue": {"key": bad_key}, + }).encode() + sig = _sign(payload, "test-webhook-secret") + + resp = webhook_client.post( + "/webhooks/jira", + content=payload, + headers={ + "Content-Type": "application/json", + "X-Hub-Signature-256": sig, + }, + ) + assert resp.status_code == 400, f"key {bad_key!r} should have been rejected, got {resp.status_code}" + + +def test_null_issue_field_does_not_crash(webhook_client): + """Issue #83 round-5: a payload with `issue: null` (not just missing) + used to raise AttributeError on `issue.get('key')` → unhandled 500. + The handler now normalises None to {} and falls through to the + 400 'Malformed or missing issue key' response.""" + payload = json.dumps({ + "webhookEvent": "jira:issue_updated", + "issue": None, + }).encode() + sig = _sign(payload, "test-webhook-secret") + + resp = webhook_client.post( + "/webhooks/jira", + content=payload, + headers={ + "Content-Type": "application/json", + "X-Hub-Signature-256": sig, + }, + ) + assert resp.status_code == 400 + assert "issue key" in resp.json()["detail"].lower() + + +def test_valid_issue_key_accepted(webhook_client): + """Sanity: a well-formed issue key still passes validation.""" + from unittest.mock import patch + + payload = json.dumps({ + "webhookEvent": "jira:issue_updated", + "issue": {"key": "PROJ-42"}, + }).encode() + sig = _sign(payload, "test-webhook-secret") + + with patch("app.api.jira_webhooks.get_jira_service") as mock_svc: + mock_svc.return_value.is_configured.return_value = True + mock_svc.return_value.process_webhook_event.return_value = True + + resp = webhook_client.post( + "/webhooks/jira", + content=payload, + headers={ + "Content-Type": "application/json", + "X-Hub-Signature-256": sig, + }, + ) + assert resp.status_code == 200 + + +def test_webhook_event_path_traversal_sanitized(webhook_client, tmp_path, monkeypatch): + """Issue #83: `webhookEvent` is attacker-controlled and was used to build + the webhook log filename. A payload with `../../tmp/pwn` for `webhookEvent` + must NOT escape the WEBHOOK_LOG_DIR; the file (if written at all) lands + under WEBHOOK_LOG_DIR with the traversal characters sanitized.""" + from unittest.mock import patch + import app.api.jira_webhooks as wh + + log_dir = tmp_path / "webhook_log" + log_dir.mkdir() + monkeypatch.setattr(wh, "WEBHOOK_LOG_DIR", log_dir) + + payload = json.dumps({ + "webhookEvent": "../../tmp/pwn", + "issue": {"key": "TEST-1"}, + }).encode() + sig = _sign(payload, "test-webhook-secret") + + with patch("app.api.jira_webhooks.get_jira_service") as mock_svc: + mock_svc.return_value.is_configured.return_value = True + mock_svc.return_value.process_webhook_event.return_value = True + + resp = webhook_client.post( + "/webhooks/jira", + content=payload, + headers={ + "Content-Type": "application/json", + "X-Hub-Signature-256": sig, + }, + ) + + assert resp.status_code == 200 + # No file landed outside log_dir. + parent = log_dir.parent + assert not (parent / "tmp" / "pwn.json").exists(), "path traversal succeeded" + # Either nothing was written (refused), or file is under log_dir with + # traversal chars replaced by underscores. + written = list(log_dir.glob("*.json")) + for f in written: + assert f.is_relative_to(log_dir), f"file {f} escaped log dir" + assert "/" not in f.name and ".." not in f.name diff --git a/tests/test_journey_jira.py b/tests/test_journey_jira.py index 229937d..fe188c4 100644 --- a/tests/test_journey_jira.py +++ b/tests/test_journey_jira.py @@ -40,33 +40,25 @@ class TestJiraWebhookJourney: assert "status" in body assert body["status"] == "ok" - def test_webhook_with_no_secret_configured_accepted(self, seeded_app): - """When JIRA_WEBHOOK_SECRET is not set, signature is skipped and webhook is processed.""" + def test_webhook_with_no_secret_configured_refused(self, seeded_app): + """Issue #83: when JIRA_WEBHOOK_SECRET is not set, webhook is REFUSED + with 503 (was previously fail-open — accepted unauthenticated). The + rename of this test from `_accepted` → `_refused` documents the + contract change.""" c = seeded_app["client"] payload = json.dumps(SAMPLE_JIRA_EVENT).encode() - with patch("connectors.jira.service._JiraConfig.JIRA_WEBHOOK_SECRET", ""), \ - patch("app.api.jira_webhooks.Config") as mock_cfg: + with patch("app.api.jira_webhooks.Config") as mock_cfg: mock_cfg.JIRA_WEBHOOK_SECRET = "" mock_cfg.JIRA_DATA_DIR = MagicMock() - mock_cfg.JIRA_DATA_DIR.__truediv__ = lambda self, other: MagicMock( - __truediv__=lambda s, o: MagicMock(mkdir=MagicMock(), __truediv__=lambda s2, o2: MagicMock()) + + resp = c.post( + "/webhooks/jira", + content=payload, + headers={"Content-Type": "application/json"}, ) - - mock_service = MagicMock() - mock_service.is_configured.return_value = True - mock_service.process_webhook_event.return_value = True - - with patch("app.api.jira_webhooks.get_jira_service", return_value=mock_service), \ - patch("app.api.jira_webhooks._verify_signature", return_value=True), \ - patch("app.api.jira_webhooks._log_webhook_event"): - resp = c.post( - "/webhooks/jira", - content=payload, - headers={"Content-Type": "application/json"}, - ) - assert resp.status_code == 200 - assert resp.json()["status"] == "ok" + assert resp.status_code == 503 + assert "secret" in resp.json()["detail"].lower() def test_webhook_with_valid_hmac_signature(self, seeded_app): """POST with valid HMAC-SHA256 signature is accepted.""" @@ -121,12 +113,13 @@ class TestJiraWebhookJourney: assert "Invalid signature" in resp.json()["detail"] def test_webhook_empty_payload_rejected(self, seeded_app): - """Empty body returns 400.""" + """Empty body returns 400 (the secret-configured path; the + no-secret path returns 503 — see test_webhook_with_no_secret_configured_refused).""" c = seeded_app["client"] with patch("app.api.jira_webhooks.Config") as mock_cfg, \ patch("app.api.jira_webhooks._verify_signature", return_value=True): - mock_cfg.JIRA_WEBHOOK_SECRET = "" + mock_cfg.JIRA_WEBHOOK_SECRET = "test-secret-not-empty" resp = c.post( "/webhooks/jira",