Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. `<img src=x onerror=alert(1)>`).
**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.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
30 changes: 22 additions & 8 deletions backend/services/carddav_discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -43,6 +43,8 @@
TxtResolver = Callable[[str], list[str]]
HttpClientFactory = Callable[[], Any]

_MAX_CONTEXT_PATH_DECODE_ROUNDS = 5


@dataclass(frozen=True)
class CarddavDiscoveryResult:
Expand Down Expand Up @@ -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
Expand Down
29 changes: 29 additions & 0 deletions backend/tests/test_carddav_discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading