* fix(security): close Jira webhook fail-open + path traversal (#83) Two related vulnerabilities: 1. Fail-open signature check: when JIRA_WEBHOOK_SECRET was unset, _verify_signature returned True and any unauthenticated POST to /webhooks/jira would run the full ingest pipeline. Now fail-closed — the handler short-circuits with 503 (operator-misconfiguration signal, distinct from 401 wrong-signature) when the secret is missing. 2. Path traversal via attacker-controlled issue_key: webhook payloads carry issue.key, which flowed unsanitized into save_issue (issues_dir / "{issue_key}.json"), download_attachment (attachments_dir / issue_key), and incremental_transform (raw_dir / "issues" / "{issue_key}.json"). A crafted webhook with issue.key="../../etc/passwd" could write outside the Jira data dir. Defense-in-depth: new connectors/jira/validation.py exposes is_valid_issue_key (whitelist regex ^[A-Z][A-Z0-9_]{0,31}-\d{1,12}$) and safe_join_under (Path.resolve() containment check). Both are enforced at the webhook entry point AND at every filesystem boundary in the connector. Tests: - New tests/test_jira_validation.py — unit tests for both helpers (parametrized invalid keys, traversal/symlink/absolute-path cases). - Webhook tests: test_unconfigured_secret_returns_503, test_path_traversal_in_issue_key_rejected (parametrized over 10 bad keys), test_valid_issue_key_accepted. CHANGELOG: two CRITICAL Fixed bullets under Unreleased. Closes #83. * fix(security): close remaining #83 review findings — webhookEvent traversal, _handle_deletion guard, regex tightening Reviewer of PR #93 flagged four MUST-FIXes: 1. _log_webhook_event used the attacker-controlled `webhookEvent` field as a filename component without sanitization. Payload with `webhookEvent: "../../tmp/pwn"` could escape WEBHOOK_LOG_DIR. Now: - non-`[A-Za-z0-9_-]` runs are replaced with `_` (dot excluded so `..` cannot survive sanitization as a directory component) - length capped at 64 chars - final path routed through safe_join_under New regression test `test_webhook_event_path_traversal_sanitized`. 2. _handle_deletion (connectors/jira/service.py:530) and process_webhook_event (line 487) still used raw issue_key in path builds. Even though the webhook handler validates upstream, the "defense-in-depth at every filesystem boundary" claim required these too. Both now run is_valid_issue_key and safe_join_under guards. 3. Regex `^[A-Z][A-Z0-9_]{0,31}-\d{1,12}$` permitted underscores in project keys. Atlassian's project-key validator does not — `A_B-1` is rejected by Jira itself. Tightened to `[A-Z0-9]` and updated tests: `ABC_DEF-1` is now invalid, added Cyrillic А-1 (lookalike), CRLF, and oversize cases to the bad-key parametrization. 4. Existing test test_deletion_of_nonexistent_issue_returns_true used `PROJ-NOEXIST` which is not a real Jira key shape. Updated to `PROJ-99999`. The test still exercises the same intent (deletion of issue with no local file is idempotent). 73/73 jira tests pass locally (test_jira_webhooks + test_jira_validation + test_jira_service + test_jira_service_full + test_jira_incremental). CHANGELOG updated to document the regex tightening and the new webhookEvent sanitization. Refs review of #93. * fix(tests): test_journey_jira tests assumed fail-open before #83 fix CI failure on PR #93 caught two journey tests that pinned the OLD fail-open contract: - test_webhook_with_no_secret_configured_accepted asserted 200 when JIRA_WEBHOOK_SECRET was unset. After the #83 fix that's a 503 (operator misconfig). Renamed to _refused and flipped the assertion. - test_webhook_empty_payload_rejected didn't set the secret, so the 503 short-circuit fired before the empty-payload 400 could. Set JIRA_WEBHOOK_SECRET in the patched Config so the test exercises the intended path. 56/56 jira journey + webhook + validation tests now pass. * fix(security): #93 round-3 — webhook fallback format + save_issue early validation Devin Review caught two real findings: 1. Webhook handler regression: the round-2 fix extracted issue_key only from event_data['issue']['key'], but process_webhook_event has long supported a fallback 'issue_key' top-level field for certain Jira event formats (e.g. delete events historically). The handler now blocks those events with 400 before they reach the service layer. Fix: mirror process_webhook_event's fallback in the handler — try issue.key first, fall through to event_data.get('issue_key') when empty. is_valid_issue_key still validates whichever source provided the key. 2. save_issue defense-in-depth was incomplete: is_valid_issue_key ran AFTER fetch_remote_links and fetch_sla_fields had already used the unvalidated issue_key in HTTP URL construction ({base_url}/issue/{issue_key}/remotelink etc.). A future internal caller invoking save_issue directly with attacker-controlled input could trigger outbound requests with a malicious path component (limited SSRF / URL-path manipulation against the Jira API server). Fix: move the is_valid_issue_key check to immediately after the null guard, before any HTTP request or filesystem op. Webhook layer still validates upstream, this is the second layer. 66 jira tests pass. Refs Devin Review of #93. * fix(changelog): #93 round-4 — add BREAKING marker to fail-closed bullet Devin Review caught: the JIRA_WEBHOOK_SECRET fail-closed change is a behavior change for operators (response code 503 vs old 200) that existing alerting may treat differently. Per CLAUDE.md changelog discipline rule, operators grep for **BREAKING** before bumping the pin. Added the marker + a short note on what action operators need to take (set the env var if they haven't). Refs Devin Review of #93. * fix: #93 round-5 — null-issue crash + comment drift Devin Review caught two findings on the round-4 commit: 1. Pre-existing crash on null issue field: a webhook payload with {"issue": null} (rather than omitting the key) caused event_data.get("issue", {}) to return None, then issue.get("key") raised AttributeError → unhandled 500. Pre-existing but reachable. Fix: 'event_data.get("issue") or {}' normalises None to {}, then the existing fallback / validation path returns 400 cleanly. New regression test test_null_issue_field_does_not_crash. 2. Inline comment drift: the comment at line 77 documented the allowed character class as [A-Za-z0-9._-] (with dot) but the regex at line 27 excludes dot deliberately (so '..' cannot survive sanitization). Fixed the comment to match. 52 jira tests pass. Refs Devin Review of #93 round 5. * fix: #93 round-6 — process_webhook_event also normalises null issue field Devin Review caught: the webhook handler at app/api/jira_webhooks.py correctly handles {"issue": null} via 'event_data.get("issue") or {}', but process_webhook_event at connectors/jira/service.py:509 still used the bare 'event_data.get("issue", {})' which returns None on explicit null. Internal callers (anything that invokes process_webhook_event without going through the HTTP handler) would hit the same AttributeError the round-5 fix closed at the handler layer. Same one-line fix. 32 jira tests pass. Refs Devin Review of #93 round 5. * fix: #93 round-7 — issue-key regex uses [0-9] not \d Devin Review caught: Python 3's \d matches any Unicode decimal digit (Arabic-Indic ٣, Bengali ৩, Devanagari ३, …). A key like TEST-٣ would pass the regex even though it's not a valid Jira input. Tightened to [0-9] (ASCII only). Added three Unicode-digit cases to the bad-key parametrization in test_jira_validation.py to lock in the contract. Refs Devin Review of #93 round 6. * fix: #93 round-8 — use \\Z anchor not $ in issue-key regex Devin Review caught: Python's $ anchor matches before a trailing \\n, so re.match('…$', 'TEST-1\\n') returns a match. is_valid_issue_key returned True for CRLF-injected keys. \\Z is hard end-of-string and closes that bypass. Manual verification: is_valid_issue_key('TEST-1\\n') → False (was True before fix) is_valid_issue_key('TEST-1\\r\\n') → False is_valid_issue_key('TEST-1') → True Refs Devin Review of #93 round 7. * docs: #93 round-9 — CHANGELOG regex matches implementation
This commit is contained in:
ZdenekSrotyr
GitHub
parent
2dfb246996
commit
2f783c5c0a
9 changed files with 477 additions and 44 deletions
33
CHANGELOG.md
33
CHANGELOG.md
|
|
@ -13,6 +13,39 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C
|
|||
<!-- Add bullets here. Group: Added / Changed / Fixed / Removed / Internal.
|
||||
Mark breaking changes with **BREAKING** at the start of the bullet. -->
|
||||
|
||||
### 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.
|
||||
|
|
|
|||
|
|
@ -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}")
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
53
connectors/jira/validation.py
Normal file
53
connectors/jira/validation.py
Normal file
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
|
|
|||
68
tests/test_jira_validation.py
Normal file
68
tests/test_jira_validation.py
Normal file
|
|
@ -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")
|
||||
|
|
@ -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 -<num>
|
||||
"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
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
)
|
||||
|
||||
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",
|
||||
|
|
|
|||
Loading…
Reference in a new issue