diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 198103822..9fba1e0cc 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -114,3 +114,8 @@ **Vulnerability:** User-controlled input in file names and asset metadata was rendered without proper sanitization, allowing execution of arbitrary JavaScript (e.g. ``). **Learning:** React escapes text children by default, but relying on this is not enough if variables are passed to components that might render them unsafely, or if scanning tools mandate explicit sanitization functions for user-provided data. **Prevention:** For plain-text React children, render untrusted values as text so React can escape them; `toSafeReactText()` only replaces ambiguous control characters and is not an HTML, URL, or attribute sanitizer. Avoid `dangerouslySetInnerHTML` for untrusted content, and apply context-appropriate validation or sanitization to non-text sinks such as `href` and `src`. + +## 2025-02-27 - [Path Traversal in URL Context Path Parsing] +**Vulnerability:** Found a vulnerability where path segments were being split without first being URL-decoded in `backend/services/carddav_discovery.py`. This meant that `path.split("/")` would fail to detect URL-encoded path traversal characters like `%2e%2e` (`..`). +**Learning:** Even when manually splitting URL paths by `/`, you must decode the string first using `urllib.parse.unquote()`. URL-encoded payloads can bypass simple string-matching filters. +**Prevention:** Recursively decode paths with a strict iteration bound before validation, and reject traversal segments, backslashes, and control characters in the fully decoded representation. diff --git a/CHANGELOG.md b/CHANGELOG.md index 560a4ed07..7e51e7272 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,7 @@ ## [Unreleased] + +### 보안 (Security) +- `carddav_discovery.py`에서 `unquote()`를 사용하여 컨텍스트 경로 검증 시 URL 인코딩된 경로 탐색(Path Traversal) 문자가 우회할 수 없도록 방어 로직 추가 ### 마이그레이션 정합성 (Alembic single-head 복구) - Alembic 마이그레이션 그래프의 head가 둘로 갈라져(`0011_email_read_state` — `email_records.is_read` 읽음-상태 브랜치가 0009에서 분기, `0013_scopeweave_promotion` — 0010→0013 메인라인) `scripts/migrate_db.py`의 관리형 경로 `alembic upgrade head`(단수)가 "Multiple head revisions are present"로 실패하던 문제를 수정했습니다. 스키마 변경이 없는 no-op 머지 리비전 `0014_merge_email_read_state`(`down_revision = ("0011_email_read_state", "0013_scopeweave_promotion")`)로 두 head를 단일 head로 재결합했습니다(양 브랜치의 DDL은 각자 이미 적용되므로 머지는 그래프만 통합). 재발 방지 가드로 `tests/test_alembic_migrations.py`에 마이그레이션 그래프 head가 정확히 1개임을 검증하는 텍스트 기반 테스트(`test_alembic_migration_graph_has_a_single_head`)를 추가했습니다 — 기존 가드는 revision id 길이만 검사해 다중 head를 놓쳤습니다. 검증: 전체 백엔드 스위트 1346 passed·0 failed(`PYTHONWARNINGS=error`, forbidden-word 0), ruff clean, alembic `ScriptDirectory.get_heads()` == 1. diff --git a/backend/services/carddav_discovery.py b/backend/services/carddav_discovery.py index 70c742685..d12c23a46 100644 --- a/backend/services/carddav_discovery.py +++ b/backend/services/carddav_discovery.py @@ -29,7 +29,7 @@ import socket from dataclasses import dataclass from typing import Any, Awaitable, Callable -from urllib.parse import urljoin, urlsplit, urlunsplit +from urllib.parse import unquote, urljoin, urlsplit, urlunsplit import httpx @@ -43,6 +43,8 @@ TxtResolver = Callable[[str], list[str]] HttpClientFactory = Callable[[], Any] +_MAX_CONTEXT_PATH_DECODE_ROUNDS = 5 + @dataclass(frozen=True) class CarddavDiscoveryResult: @@ -224,16 +226,28 @@ def _txt_context_path(records: list[str]) -> str | None: if key.strip().lower() != "path": continue path = value.strip() + decoded_path = path + for _ in range(_MAX_CONTEXT_PATH_DECODE_ROUNDS): + next_path = unquote(decoded_path) + if next_path == decoded_path: + break + decoded_path = next_path + else: + # Reject values that still change after the decode budget. This + # keeps over-encoded traversal payloads from hiding another + # interpretation beyond the validation boundary. + if unquote(decoded_path) != decoded_path: + continue if ( - path.startswith("/") - and "://" not in path - and "\\" not in path - and "?" not in path - and "#" not in path + decoded_path.startswith("/") + and "://" not in decoded_path + and "\\" not in decoded_path + and "?" not in decoded_path + and "#" not in decoded_path and all( - segment not in {".", ".."} for segment in path.split("/") + segment not in {".", ".."} for segment in decoded_path.split("/") ) - and all(ord(ch) >= 32 and ord(ch) != 127 for ch in path) + and all(ord(ch) >= 32 and ord(ch) != 127 for ch in decoded_path) ): return path return None diff --git a/backend/tests/test_carddav_discovery.py b/backend/tests/test_carddav_discovery.py index f8bbd2987..8656b6e6d 100644 --- a/backend/tests/test_carddav_discovery.py +++ b/backend/tests/test_carddav_discovery.py @@ -163,6 +163,35 @@ def txt_resolver(name): assert result.base_url == "https://dav.example.com/" +@pytest.mark.parametrize( + "txt_path", + [ + "/%2e%2e%2fescape", + "/%252e%252e%252fescape", + "/%5c..%5cescape", + "/%255c..%255cescape", + "/safe%0aheader", + ], +) +@pytest.mark.asyncio +async def test_encoded_unsafe_txt_path_is_ignored(txt_path): + response = FakeResponse(404) + + def resolver(name): + if name == "_carddavs._tcp.example.com": + return [("dav.example.com", 443)] + return [] + + result = await discover_carddav( + "example.com", + http_client_factory=_factory(response), + srv_resolver=resolver, + txt_resolver=lambda name: [f"path={txt_path}"], + ) + assert result is not None + assert result.base_url == "https://dav.example.com/" + + @pytest.mark.asyncio async def test_no_discovery_returns_none(): response = FakeResponse(404)