From fb352e3160547bbf8d4297e5de236b6575eae3fc Mon Sep 17 00:00:00 2001 From: Jessica Smith <12jessicasmith34@gmail.com> Date: Sat, 5 Sep 2026 11:50:16 -0500 Subject: [PATCH 1/4] fix: client-side security hardening against API weaknesses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven protections added: 1. Path segment validation (validate_identifier) — rejects path traversal, slashes, null bytes, percent-encoding, and oversized strings in all user-supplied identifiers (booking_uuid, booking_id, class_uuid, class_id, studio_uuid, performance_summary_id). Applied at extraction choke points in utils.py and directly in studio_client.py and workout_client.py. 2. Credential redaction in exceptions — OtfRequestError now strips Authorization, x-amz-security-token, and x-amz-date headers from stored request objects, preventing token leakage through error reporting services (Sentry, Datadog) or exception logging. 3. Name field validation — update_member_name rejects control characters, HTML-like content (<>), empty/whitespace-only strings, and names over 50 characters before sending to the API's weak validation layer. 4. Response ownership verification — get_member_detail checks that the returned member_uuid matches the authenticated user's UUID, guarding against potential IDOR in the v1 API's explicit-UUID path pattern. 5. Response size cap — _handle_response rejects responses over 10 MB before JSON parsing to prevent OOM from unexpectedly large payloads. 6. TrendType enum enforcement — get_workout_stats no longer accepts raw strings, closing a path injection vector via the stats_key URL segment. 7. Cache directory permissions — cache directory created with 0700 and re-secured on each access, preventing other local users from reading cached Cognito tokens and device credentials. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc --- .gitignore | 1 + src/otf_api/api/client.py | 20 ++++++++ src/otf_api/api/members/member_api.py | 53 +++++++++++++++++++++- src/otf_api/api/studios/studio_client.py | 4 ++ src/otf_api/api/trends/trend_api.py | 2 +- src/otf_api/api/utils.py | 46 +++++++++++++++---- src/otf_api/api/workouts/workout_client.py | 3 ++ src/otf_api/cache.py | 25 ++++++++++ src/otf_api/exceptions.py | 29 +++++++++++- 9 files changed, 172 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index 4cc9e679..39eb8831 100644 --- a/.gitignore +++ b/.gitignore @@ -178,3 +178,4 @@ scratch*.py docs/reference/ .vscode .playwright-mcp/ +.claude/worktrees/ diff --git a/src/otf_api/api/client.py b/src/otf_api/api/client.py index fb621471..107fee84 100644 --- a/src/otf_api/api/client.py +++ b/src/otf_api/api/client.py @@ -29,6 +29,10 @@ CACHE = get_cache() LOGGER = getLogger(__name__) +# Maximum response body size (bytes) the library will attempt to parse. +# Protects against OOM from unexpectedly large API responses. +MAX_RESPONSE_SIZE = 10 * 1024 * 1024 # 10 MB + class OtfClient: """Client for interacting with the OTF API - generally to be used by the Otf class. @@ -233,6 +237,22 @@ def _handle_response(self, method: str, response: httpx.Response, request: httpx LOGGER.debug("No content returned from %s %s", method, response.url) return None + content_length = len(response.content) + if content_length > MAX_RESPONSE_SIZE: + LOGGER.error( + "Response from %s %s exceeds size limit (%d bytes, max %d)", + method, + response.url, + content_length, + MAX_RESPONSE_SIZE, + ) + raise exc.OtfRequestError( + f"Response too large ({content_length} bytes, max {MAX_RESPONSE_SIZE})", + original_exception=None, + response=response, + request=request, + ) + try: json_data = response.json() except JSONDecodeError as e: diff --git a/src/otf_api/api/members/member_api.py b/src/otf_api/api/members/member_api.py index fdaa2846..e3086005 100644 --- a/src/otf_api/api/members/member_api.py +++ b/src/otf_api/api/members/member_api.py @@ -2,6 +2,7 @@ from logging import getLogger from typing import Any +from otf_api import exceptions as exc from otf_api import models from .member_client import MemberClient @@ -114,6 +115,34 @@ def update_email_notification_settings( new_settings = self.get_email_notification_settings() return new_settings + @staticmethod + def _validate_name(value: str, field_name: str) -> str: + """Validate a name field before sending to the API. + + Guards against control characters, HTML injection, and unreasonable + lengths that the API's weak validation layer may accept. + + Args: + value: The name string to validate. + field_name: Human-readable field name for error messages. + + Returns: + The stripped, validated name string. + + Raises: + ValueError: If the name is invalid. + """ + value = value.strip() + if not value: + raise ValueError(f"{field_name} must not be empty after stripping whitespace") + if len(value) > 50: + raise ValueError(f"{field_name} is too long ({len(value)} chars, max 50)") + if any(c < " " and c not in ("\t",) for c in value): + raise ValueError(f"{field_name} contains control characters") + if "<" in value or ">" in value: + raise ValueError(f"{field_name} contains HTML-like characters") + return value + def update_member_name(self, first_name: str | None = None, last_name: str | None = None) -> models.MemberDetail: """Update the member's name. Will return the original member details if no names are provided. @@ -140,6 +169,9 @@ def update_member_name(self, first_name: str | None = None, last_name: str | Non if last_name is None: raise ValueError("Last name is required") + first_name = self._validate_name(first_name, "first_name") + last_name = self._validate_name(last_name, "last_name") + res = self.client.put_member_name(first_name, last_name) return models.MemberDetail.create(**res, api=self.otf) @@ -156,7 +188,26 @@ def get_member_detail(self) -> models.MemberDetail: home_studio_uuid = data["homeStudio"]["studioUUId"] data["home_studio"] = self.otf.studios.get_studio_detail(home_studio_uuid) - return models.MemberDetail.create(**data, api=self.otf) + member = models.MemberDetail.create(**data, api=self.otf) + + # Verify the API returned data for the authenticated user, not someone else. + # This guards against potential IDOR vulnerabilities in the v1 API which uses + # explicit member UUIDs in request paths rather than token-derived identity. + expected_uuid = self.client.member_uuid + if hasattr(member, "member_uuid") and member.member_uuid != expected_uuid: + LOGGER.error( + "API returned member data for %s but authenticated as %s — possible IDOR", + member.member_uuid, + expected_uuid, + ) + raise exc.OtfRequestError( + "API returned data for a different member than authenticated", + original_exception=None, + response=None, # type: ignore[arg-type] + request=None, # type: ignore[arg-type] + ) + + return member def get_member_membership(self) -> models.MemberMembership: """Get the member's membership details. diff --git a/src/otf_api/api/studios/studio_client.py b/src/otf_api/api/studios/studio_client.py index a5dd27c3..801232e4 100644 --- a/src/otf_api/api/studios/studio_client.py +++ b/src/otf_api/api/studios/studio_client.py @@ -3,6 +3,7 @@ from typing import Any from otf_api.api.client import CACHE, OtfClient +from otf_api.api.utils import validate_identifier LOGGER = getLogger(__name__) @@ -21,6 +22,7 @@ def __init__(self, client: OtfClient): @CACHE.memoize(expire=600, tag="studio_detail") def get_studio_detail(self, studio_uuid: str) -> dict: """Retrieve raw studio details.""" + studio_uuid = validate_identifier(studio_uuid, "studio_uuid") return self.client.default_request("GET", f"/mobile/v1/studios/{studio_uuid}")["data"] def _get_studios_by_geo( @@ -93,6 +95,7 @@ def get_favorite_studios(self) -> dict: def get_studio_services(self, studio_uuid: str) -> dict: """Retrieve raw studio services data.""" + studio_uuid = validate_identifier(studio_uuid, "studio_uuid") return self.client.default_request("GET", f"/member/studios/{studio_uuid}/services")["data"] def post_favorite_studio(self, studio_uuids: list[str]) -> dict: @@ -116,6 +119,7 @@ def get_studio_detail_threaded(self, studio_uuids: list[str]) -> dict[str, dict[ Returns: dict[str, dict[str, Any]]: A dictionary of studio details, keyed by studio UUID. """ + studio_uuids = [validate_identifier(uuid, "studio_uuid") for uuid in studio_uuids] studios_dict: dict[str, dict[str, Any]] = {} with ThreadPoolExecutor(max_workers=10) as pool: futures = {pool.submit(self.get_studio_detail, uuid): uuid for uuid in studio_uuids} diff --git a/src/otf_api/api/trends/trend_api.py b/src/otf_api/api/trends/trend_api.py index 05f83f0b..11f5968a 100644 --- a/src/otf_api/api/trends/trend_api.py +++ b/src/otf_api/api/trends/trend_api.py @@ -32,7 +32,7 @@ def __init__(self, otf: "Otf", otf_client: "OtfClient"): def get_workout_stats( self, - trend_type: TrendType | str, + trend_type: TrendType, start_date: date | str | None = None, end_date: date | str | None = None, ) -> WorkoutStatsResponse: diff --git a/src/otf_api/api/utils.py b/src/otf_api/api/utils.py index 97f0855c..db44fed4 100644 --- a/src/otf_api/api/utils.py +++ b/src/otf_api/api/utils.py @@ -1,3 +1,4 @@ +import re import typing from datetime import date, datetime, time, timedelta from json import JSONDecodeError @@ -16,6 +17,35 @@ MIN_TIME = datetime.min.time() +_UNSAFE_PATH_CHARS = re.compile(r"[/\\\x00%\s]") + + +def validate_identifier(value: str, name: str) -> str: + """Validate that a string is safe to use as a URL path segment. + + Rejects values containing path separators, traversal sequences, null bytes, + percent-encoding, and whitespace to prevent path injection. + + Args: + value: The identifier string to validate. + name: Human-readable name of the parameter (for error messages). + + Returns: + The validated string, unchanged. + + Raises: + ValueError: If the string is empty, too long, or contains unsafe characters. + """ + if not value: + raise ValueError(f"{name} must not be empty") + if len(value) > 200: + raise ValueError(f"{name} is too long ({len(value)} chars, max 200)") + if ".." in value: + raise ValueError(f"{name} contains path traversal sequence") + if _UNSAFE_PATH_CHARS.search(value): + raise ValueError(f"{name} contains unsafe characters for a URL path segment") + return value + def get_studio_uuid_list( home_studio_uuid: str, studio_uuids: list[str] | str | None, include_home_studio: bool = True @@ -142,10 +172,10 @@ def get_booking_uuid(booking_or_uuid: str | Booking) -> str: TypeError: If the input is not a string or Booking object. """ if isinstance(booking_or_uuid, str): - return booking_or_uuid + return validate_identifier(booking_or_uuid, "booking_uuid") if isinstance(booking_or_uuid, Booking): - return booking_or_uuid.booking_uuid + return validate_identifier(booking_or_uuid.booking_uuid, "booking_uuid") raise TypeError(f"Expected Booking or str, got {type(booking_or_uuid)}") @@ -163,10 +193,10 @@ def get_booking_id(booking_or_id: str | BookingV2) -> str: TypeError: If the input is not a string or BookingV2 object. """ if isinstance(booking_or_id, str): - return booking_or_id + return validate_identifier(booking_or_id, "booking_id") if isinstance(booking_or_id, BookingV2): - return booking_or_id.booking_id + return validate_identifier(booking_or_id.booking_id, "booking_id") raise TypeError(f"Expected BookingV2 or str, got {type(booking_or_id)}") @@ -186,12 +216,12 @@ def get_class_uuid(class_or_uuid: "str | OtfClass | BookingV2Class") -> str: """ if isinstance(class_or_uuid, str): - return class_or_uuid + return validate_identifier(class_or_uuid, "class_uuid") if hasattr(class_or_uuid, "class_uuid"): class_uuid = getattr(class_or_uuid, "class_uuid", None) if class_uuid: - return class_uuid + return validate_identifier(class_uuid, "class_uuid") raise ValueError("Class does not have a class_uuid") raise TypeError(f"Expected OtfClass, BookingV2Class, or str, got {type(class_or_uuid)}") @@ -210,10 +240,10 @@ def get_class_id(class_or_id: str | BookingV2Class) -> str: TypeError: If the input is not a string or BookingV2Class. """ if isinstance(class_or_id, str): - return class_or_id + return validate_identifier(class_or_id, "class_id") if isinstance(class_or_id, BookingV2Class): - return class_or_id.class_id + return validate_identifier(class_or_id.class_id, "class_id") raise TypeError(f"Expected BookingV2Class or str, got {type(class_or_id)}") diff --git a/src/otf_api/api/workouts/workout_client.py b/src/otf_api/api/workouts/workout_client.py index 675e45f5..9f7300b6 100644 --- a/src/otf_api/api/workouts/workout_client.py +++ b/src/otf_api/api/workouts/workout_client.py @@ -4,6 +4,7 @@ from typing import Any from otf_api.api.client import API_IO_BASE_URL, API_TELEMETRY_BASE_URL, CACHE, OtfClient +from otf_api.api.utils import validate_identifier LOGGER = getLogger(__name__) @@ -49,6 +50,7 @@ def get_performance_summaries(self, limit: int | None = None) -> dict: @CACHE.memoize(expire=600, tag="performance_summary") def get_performance_summary(self, performance_summary_id: str) -> dict: """Retrieve raw performance summary data.""" + performance_summary_id = validate_identifier(performance_summary_id, "performance_summary_id") return self.performance_summary_request("GET", f"/v1/performance-summaries/{performance_summary_id}") def get_hr_history_raw(self) -> dict: @@ -60,6 +62,7 @@ def get_hr_history_raw(self) -> dict: @CACHE.memoize(expire=600, tag="telemetry") def get_telemetry(self, performance_summary_id: str, max_data_points: int = 150) -> dict: """Retrieve raw telemetry data.""" + performance_summary_id = validate_identifier(performance_summary_id, "performance_summary_id") data = self.telemetry_request( "GET", "/v1/performance/summary", diff --git a/src/otf_api/cache.py b/src/otf_api/cache.py index 108c0440..4665be0f 100644 --- a/src/otf_api/cache.py +++ b/src/otf_api/cache.py @@ -1,5 +1,7 @@ +import stat from importlib.metadata import version from logging import getLogger +from pathlib import Path from diskcache import Cache from packaging.version import Version @@ -118,15 +120,38 @@ def get_cache_dir() -> str: return cache_dir +def _ensure_secure_directory(path: str) -> None: + """Create the cache directory with restrictive permissions (owner-only access). + + On systems that support it, sets the directory to mode 0700. On Windows + or other systems where chmod is a no-op, this is best-effort. + + Args: + path: The directory path to create and secure. + """ + Path(path).mkdir(mode=0o700, parents=True, exist_ok=True) + + # Ensure permissions are correct even if the directory already existed + # with more permissive settings (e.g., from a previous version). + try: + Path(path).chmod(stat.S_IRWXU) # 0700: owner read/write/execute only + except OSError: + LOGGER.warning("Could not set restrictive permissions on cache directory: %s", path) + + def get_cache() -> OtfCache: """Returns the cache instance, creating it if it does not exist. + Creates the cache directory with owner-only permissions (0700) to prevent + other local users from reading cached authentication tokens. + Returns: Cache: The cache instance. """ global _CACHE if _CACHE is None: cache_dir = get_cache_dir() + _ensure_secure_directory(cache_dir) LOGGER.debug("Using cache directory: %s", cache_dir) _CACHE = OtfCache(cache_dir) return _CACHE diff --git a/src/otf_api/exceptions.py b/src/otf_api/exceptions.py index a78a8e68..998733dd 100644 --- a/src/otf_api/exceptions.py +++ b/src/otf_api/exceptions.py @@ -33,11 +33,38 @@ class OtfRequestError(OtfError): response: "Response" request: "Request" + # Headers to redact from stored request objects to prevent credential leakage + # when exceptions are logged or sent to error-reporting services. + _SENSITIVE_HEADERS = frozenset({"authorization", "x-amz-security-token", "x-amz-date"}) + def __init__(self, message: str, original_exception: Exception | None, response: "Response", request: "Request"): super().__init__(message) self.original_exception = original_exception self.response = response - self.request = request + self.request = self._sanitize_request(request) + + @classmethod + def _sanitize_request(cls, request: "Request | None") -> "Request | None": + """Return a copy of the request with sensitive headers redacted. + + This prevents credential leakage when exceptions are logged or sent + to error-reporting services. Returns None if no request is provided. + """ + if request is None: + return None + + import httpx + + sanitized_headers = dict(request.headers) + for header in cls._SENSITIVE_HEADERS: + if header in sanitized_headers: + sanitized_headers[header] = "[REDACTED]" + + return httpx.Request( + method=request.method, + url=request.url, + headers=sanitized_headers, + ) class RetryableOtfRequestError(OtfRequestError): From 6cbe74a64a0e078b5fde63a186da5d7e25c61ab2 Mon Sep 17 00:00:00 2001 From: Jessica Smith <12jessicasmith34@gmail.com> Date: Sat, 5 Sep 2026 12:30:02 -0500 Subject: [PATCH 2/4] fix: address code review findings - Widen OtfRequestError type annotations to Response | None / Request | None, drop type: ignore at IDOR call site - Move httpx import to top of exceptions.py (no lazy imports rule) - Preserve request body in _sanitize_request (include content=) - Drop dead hasattr guard on MemberDetail.member_uuid (required field) - Add runtime isinstance check for TrendType enforcement - Fix MAX_RESPONSE_SIZE comment to accurately describe protection boundary - Add validate_identifier call in BookingApi.get_booking (missed gap) - Use ord(c) < 32 instead of c < ' ' for control char check clarity - Add 30 unit tests for all new security validation functions Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc --- .claude/agent-memory/code-reviewer/MEMORY.md | 2 + .../exceptions_optional_fields.md | 23 +++ .../integration-reviewer/MEMORY.md | 3 + .../identifier-validation-convention.md | 34 ++++ src/otf_api/api/bookings/booking_api.py | 2 + src/otf_api/api/client.py | 5 +- src/otf_api/api/members/member_api.py | 8 +- src/otf_api/api/trends/trend_api.py | 3 + src/otf_api/exceptions.py | 17 +- tests/test_api/test_security_hardening.py | 162 ++++++++++++++++++ 10 files changed, 248 insertions(+), 11 deletions(-) create mode 100644 .claude/agent-memory/code-reviewer/MEMORY.md create mode 100644 .claude/agent-memory/code-reviewer/exceptions_optional_fields.md create mode 100644 .claude/agent-memory/integration-reviewer/MEMORY.md create mode 100644 .claude/agent-memory/integration-reviewer/identifier-validation-convention.md create mode 100644 tests/test_api/test_security_hardening.py diff --git a/.claude/agent-memory/code-reviewer/MEMORY.md b/.claude/agent-memory/code-reviewer/MEMORY.md new file mode 100644 index 00000000..8e88775d --- /dev/null +++ b/.claude/agent-memory/code-reviewer/MEMORY.md @@ -0,0 +1,2 @@ +# Memory Index +- [OtfRequestError optional fields](exceptions_optional_fields.md) — response/request typed non-Optional but sometimes passed as None with type:ignore; crashes documented `e.request.method` usage. diff --git a/.claude/agent-memory/code-reviewer/exceptions_optional_fields.md b/.claude/agent-memory/code-reviewer/exceptions_optional_fields.md new file mode 100644 index 00000000..d9a43870 --- /dev/null +++ b/.claude/agent-memory/code-reviewer/exceptions_optional_fields.md @@ -0,0 +1,23 @@ +--- +name: exceptions-optional-fields +description: OtfRequestError.response/.request are typed non-Optional but code sometimes passes None +metadata: + type: project +--- + + +`src/otf_api/exceptions.py`'s `OtfRequestError` declares `response: "Response"` and +`request: "Request"` as non-Optional class attributes, but at least one call site +(`member_api.py`'s IDOR check, added 2026-09) passes `response=None, request=None` +with `# type: ignore[arg-type]` to suppress the mismatch. + +**Why it matters:** `docs/guides/error-handling.md` documents catching `OtfRequestError` +generically and accessing `e.request.method` / `e.request.url` — that crashes with +`AttributeError` on a None request. `_sanitize_request()` already null-checks +`request is None`, showing the author knew this could happen but didn't update the +type annotation to `Request | None`. + +**How to apply:** When reviewing new `raise exc.OtfRequestError(...)` call sites, check +whether `response`/`request` are being passed as `None`. If so, flag it — either the +field types need to become `Optional`, or the call site needs a real request/response +object. Don't let `# type: ignore[arg-type]` on these two fields pass silently. diff --git a/.claude/agent-memory/integration-reviewer/MEMORY.md b/.claude/agent-memory/integration-reviewer/MEMORY.md new file mode 100644 index 00000000..0e37c890 --- /dev/null +++ b/.claude/agent-memory/integration-reviewer/MEMORY.md @@ -0,0 +1,3 @@ +# Memory Index + +- [Identifier validation convention](identifier-validation-convention.md) — path-segment identifiers must be validated at the *Client layer before URL interpolation; check for gaps when reviewing future security diffs. diff --git a/.claude/agent-memory/integration-reviewer/identifier-validation-convention.md b/.claude/agent-memory/integration-reviewer/identifier-validation-convention.md new file mode 100644 index 00000000..e6213f84 --- /dev/null +++ b/.claude/agent-memory/integration-reviewer/identifier-validation-convention.md @@ -0,0 +1,34 @@ +--- +name: identifier-validation-convention +description: otf-api's convention for validating URL-path-segment identifiers (booking_uuid, class_uuid, studio_uuid, performance_summary_id) before interpolating into request paths +metadata: + type: project +--- + + +As of commit fb352e3 ("client-side security hardening against API weaknesses"), the established +pattern is: any identifier interpolated into an HTTP path segment must pass through +`otf_api.api.utils.validate_identifier()` (rejects path separators, `..`, null bytes, whitespace, +percent-encoding) before being used to build a URL. This is applied either directly in the +`*Client` class (e.g. `StudioClient.get_studio_detail`, `WorkoutClient.get_performance_summary`) +or via the shared `utils.get_booking_uuid`/`get_booking_id`/`get_class_uuid`/`get_class_id` helpers +that `*Api` classes call before delegating to the client. + +**Known gap as of fb352e3 (flagged, not yet fixed):** `BookingClient.get_booking` interpolates +`booking_uuid` into a path with no validation, and `BookingApi.get_booking` (a public, +directly-callable method) does not call `utils.get_booking_uuid` before forwarding. Check this +specific method when reviewing future diffs — if still unpatched, it's a real gap, not a false +positive. + +**Also flagged in the same diff:** `TrendClient.get_workout_stats` closes the same class of gap +by narrowing `TrendApi.get_workout_stats`'s `trend_type` param from `TrendType | str` to +`TrendType` instead of calling `validate_identifier` — a different (breaking) technique for the +same problem. Worth checking whether this was reconciled to the `validate_identifier` pattern or +left as an enum-only breaking change. + +**Also flagged:** `OtfRequestError.response`/`.request` are typed non-Optional and one existing +catch site (`booking_api.py` `post_class_rating`, `except OtfRequestError as e: e.response.status_code`) +relies on that being true. `MemberApi.get_member_detail`'s new IDOR guard raises +`OtfRequestError(response=None, request=None)`, violating that invariant. Check whether the type +was ever changed to `Response | None` / `Request | None` with consumers updated, or whether this +call site was given a synthetic Response/Request instead. diff --git a/src/otf_api/api/bookings/booking_api.py b/src/otf_api/api/bookings/booking_api.py index afd8466f..97658fcd 100644 --- a/src/otf_api/api/bookings/booking_api.py +++ b/src/otf_api/api/bookings/booking_api.py @@ -311,6 +311,8 @@ def get_booking(self, booking_uuid: str) -> models.Booking: if not booking_uuid: raise ValueError("booking_uuid is required") + utils.validate_identifier(booking_uuid, "booking_uuid") + data = self.client.get_booking(booking_uuid) return models.Booking.create(**data, api=self.otf) diff --git a/src/otf_api/api/client.py b/src/otf_api/api/client.py index 107fee84..2e79af1d 100644 --- a/src/otf_api/api/client.py +++ b/src/otf_api/api/client.py @@ -29,8 +29,9 @@ CACHE = get_cache() LOGGER = getLogger(__name__) -# Maximum response body size (bytes) the library will attempt to parse. -# Protects against OOM from unexpectedly large API responses. +# Maximum response body size (bytes) the library will attempt to parse as JSON. +# The response is already buffered by this point — this prevents wasting CPU on +# parsing an unexpectedly large payload, not the memory cost of the read itself. MAX_RESPONSE_SIZE = 10 * 1024 * 1024 # 10 MB diff --git a/src/otf_api/api/members/member_api.py b/src/otf_api/api/members/member_api.py index e3086005..863c9e03 100644 --- a/src/otf_api/api/members/member_api.py +++ b/src/otf_api/api/members/member_api.py @@ -137,7 +137,7 @@ def _validate_name(value: str, field_name: str) -> str: raise ValueError(f"{field_name} must not be empty after stripping whitespace") if len(value) > 50: raise ValueError(f"{field_name} is too long ({len(value)} chars, max 50)") - if any(c < " " and c not in ("\t",) for c in value): + if any(ord(c) < 32 and c != "\t" for c in value): raise ValueError(f"{field_name} contains control characters") if "<" in value or ">" in value: raise ValueError(f"{field_name} contains HTML-like characters") @@ -194,7 +194,7 @@ def get_member_detail(self) -> models.MemberDetail: # This guards against potential IDOR vulnerabilities in the v1 API which uses # explicit member UUIDs in request paths rather than token-derived identity. expected_uuid = self.client.member_uuid - if hasattr(member, "member_uuid") and member.member_uuid != expected_uuid: + if member.member_uuid != expected_uuid: LOGGER.error( "API returned member data for %s but authenticated as %s — possible IDOR", member.member_uuid, @@ -203,8 +203,8 @@ def get_member_detail(self) -> models.MemberDetail: raise exc.OtfRequestError( "API returned data for a different member than authenticated", original_exception=None, - response=None, # type: ignore[arg-type] - request=None, # type: ignore[arg-type] + response=None, + request=None, ) return member diff --git a/src/otf_api/api/trends/trend_api.py b/src/otf_api/api/trends/trend_api.py index 11f5968a..a85043aa 100644 --- a/src/otf_api/api/trends/trend_api.py +++ b/src/otf_api/api/trends/trend_api.py @@ -46,6 +46,9 @@ def get_workout_stats( Returns: WorkoutStatsResponse: The stat data with individual data points per workout. """ + if not isinstance(trend_type, TrendType): + raise TypeError(f"trend_type must be a TrendType enum member, got {type(trend_type).__name__}") + start = utils.ensure_date(start_date) or pendulum.today().subtract(days=90).date() end = utils.ensure_date(end_date) or pendulum.today().date() diff --git a/src/otf_api/exceptions.py b/src/otf_api/exceptions.py index 998733dd..0846fa1f 100644 --- a/src/otf_api/exceptions.py +++ b/src/otf_api/exceptions.py @@ -1,5 +1,7 @@ import typing +import httpx + if typing.TYPE_CHECKING: from httpx import Request, Response @@ -30,14 +32,20 @@ class OtfRequestError(OtfError): """Raised when an error occurs while making a request to the OTF API.""" original_exception: Exception | None - response: "Response" - request: "Request" + response: "Response | None" + request: "Request | None" # Headers to redact from stored request objects to prevent credential leakage # when exceptions are logged or sent to error-reporting services. _SENSITIVE_HEADERS = frozenset({"authorization", "x-amz-security-token", "x-amz-date"}) - def __init__(self, message: str, original_exception: Exception | None, response: "Response", request: "Request"): + def __init__( + self, + message: str, + original_exception: Exception | None, + response: "Response | None", + request: "Request | None", + ): super().__init__(message) self.original_exception = original_exception self.response = response @@ -53,8 +61,6 @@ def _sanitize_request(cls, request: "Request | None") -> "Request | None": if request is None: return None - import httpx - sanitized_headers = dict(request.headers) for header in cls._SENSITIVE_HEADERS: if header in sanitized_headers: @@ -64,6 +70,7 @@ def _sanitize_request(cls, request: "Request | None") -> "Request | None": method=request.method, url=request.url, headers=sanitized_headers, + content=request.content, ) diff --git a/tests/test_api/test_security_hardening.py b/tests/test_api/test_security_hardening.py new file mode 100644 index 00000000..ecbdfd59 --- /dev/null +++ b/tests/test_api/test_security_hardening.py @@ -0,0 +1,162 @@ +"""Tests for client-side security hardening.""" + +import os +import stat +import tempfile +from pathlib import Path + +import httpx +import pytest + +from otf_api.api.members.member_api import MemberApi +from otf_api.api.utils import validate_identifier +from otf_api.cache import _ensure_secure_directory +from otf_api.exceptions import OtfRequestError + + +class TestValidateIdentifier: + """Tests for the path segment validation helper.""" + + def test_valid_uuid(self): + result = validate_identifier("abc-123-def-456-ghi", "test") + assert result == "abc-123-def-456-ghi" + + def test_valid_alphanumeric(self): + result = validate_identifier("abc123", "test") + assert result == "abc123" + + def test_rejects_empty(self): + with pytest.raises(ValueError, match="must not be empty"): + validate_identifier("", "test_field") + + def test_rejects_path_traversal(self): + with pytest.raises(ValueError, match="path traversal"): + validate_identifier("../../admin", "test_field") + + def test_rejects_slash(self): + with pytest.raises(ValueError, match="unsafe characters"): + validate_identifier("abc/def", "test_field") + + def test_rejects_backslash(self): + with pytest.raises(ValueError, match="unsafe characters"): + validate_identifier("abc\\def", "test_field") + + def test_rejects_null_byte(self): + with pytest.raises(ValueError, match="unsafe characters"): + validate_identifier("abc\x00def", "test_field") + + def test_rejects_percent_encoding(self): + with pytest.raises(ValueError, match="unsafe characters"): + validate_identifier("abc%2Fdef", "test_field") + + def test_rejects_whitespace(self): + with pytest.raises(ValueError, match="unsafe characters"): + validate_identifier("abc def", "test_field") + + def test_rejects_oversized(self): + with pytest.raises(ValueError, match="too long"): + validate_identifier("a" * 201, "test_field") + + def test_accepts_max_length(self): + result = validate_identifier("a" * 200, "test") + assert len(result) == 200 + + def test_error_message_includes_field_name(self): + with pytest.raises(ValueError, match="booking_uuid"): + validate_identifier("", "booking_uuid") + + +class TestSanitizeRequest: + """Tests for credential redaction in OtfRequestError.""" + + def test_redacts_authorization_header(self): + req = httpx.Request("GET", "https://example.com", headers={"Authorization": "Bearer secret"}) + err = OtfRequestError("test", None, None, req) + assert err.request.headers["authorization"] == "[REDACTED]" + + def test_redacts_aws_security_token(self): + req = httpx.Request("GET", "https://example.com", headers={"x-amz-security-token": "tok"}) + err = OtfRequestError("test", None, None, req) + assert err.request.headers["x-amz-security-token"] == "[REDACTED]" + + def test_preserves_non_sensitive_headers(self): + req = httpx.Request("GET", "https://example.com", headers={"content-type": "application/json"}) + err = OtfRequestError("test", None, None, req) + assert err.request.headers["content-type"] == "application/json" + + def test_preserves_request_body(self): + req = httpx.Request("POST", "https://example.com", content=b'{"key": "value"}') + err = OtfRequestError("test", None, None, req) + assert err.request.content == b'{"key": "value"}' + + def test_does_not_modify_original_request(self): + req = httpx.Request("GET", "https://example.com", headers={"Authorization": "Bearer secret"}) + OtfRequestError("test", None, None, req) + assert req.headers["authorization"] == "Bearer secret" + + def test_handles_none_request(self): + err = OtfRequestError("test", None, None, None) + assert err.request is None + + def test_handles_none_response(self): + err = OtfRequestError("test", None, None, None) + assert err.response is None + + +class TestValidateName: + """Tests for name field validation in MemberApi.""" + + def test_rejects_empty(self): + with pytest.raises(ValueError, match="must not be empty"): + MemberApi._validate_name("", "first_name") + + def test_rejects_whitespace_only(self): + with pytest.raises(ValueError, match="must not be empty"): + MemberApi._validate_name(" ", "first_name") + + def test_rejects_too_long(self): + with pytest.raises(ValueError, match="too long"): + MemberApi._validate_name("A" * 51, "first_name") + + def test_rejects_control_characters(self): + with pytest.raises(ValueError, match="control characters"): + MemberApi._validate_name("abc\x01def", "first_name") + + def test_rejects_html_brackets(self): + with pytest.raises(ValueError, match="HTML-like"): + MemberApi._validate_name("", "first_name") + + def test_strips_whitespace(self): + result = MemberApi._validate_name(" Jessica ", "first_name") + assert result == "Jessica" + + def test_allows_valid_name(self): + result = MemberApi._validate_name("Jessica", "first_name") + assert result == "Jessica" + + def test_allows_hyphenated_name(self): + result = MemberApi._validate_name("Mary-Jane", "first_name") + assert result == "Mary-Jane" + + def test_allows_accented_characters(self): + result = MemberApi._validate_name("José", "first_name") + assert result == "José" + + +class TestEnsureSecureDirectory: + """Tests for cache directory permission hardening.""" + + def test_creates_directory_with_0700(self): + with tempfile.TemporaryDirectory() as tmp: + path = f"{tmp}/test_cache" + _ensure_secure_directory(path) + mode = stat.S_IMODE(Path(path).stat().st_mode) + assert mode == 0o700, f"Expected 0700, got {oct(mode)}" + + def test_tightens_existing_loose_permissions(self): + with tempfile.TemporaryDirectory() as tmp: + path = f"{tmp}/test_cache" + os.makedirs(path, mode=0o755) + _ensure_secure_directory(path) + mode = stat.S_IMODE(Path(path).stat().st_mode) + assert mode == 0o700, f"Expected 0700, got {oct(mode)}" From d792c91be7ded6e93686aee5d31900337e0798bc Mon Sep 17 00:00:00 2001 From: Jessica Smith <12jessicasmith34@gmail.com> Date: Sat, 5 Sep 2026 13:06:29 -0500 Subject: [PATCH 3/4] refactor: address clean code review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LLM patterns: - Remove HTML bracket check from validate_name (context-blind XSS defense in a JSON API client with no rendering surface) - Reframe IDOR comment as sanity-check, raise ValueError instead of OtfRequestError (not an HTTP error shape) - Revert OtfRequestError type widening — response/request stay non-Optional - Replace TrendType type narrowing with validate_identifier (consistent pattern, non-breaking API change) Deferred debt: - Add validate_identifier to BookingClient (delete_booking, get_booking, delete_booking_new) to match StudioClient/WorkoutClient convention Nitpick: - Extract _MAX_NAME_LENGTH, _MAX_IDENTIFIER_LENGTH, _CACHE_DIR_MODE constants - Drop underscore prefixes on ensure_secure_directory and validate_name (both directly tested, not behaving as private) - Unify parameter naming (field_name → name) - Remove redundant TYPE_CHECKING import block from exceptions.py - Remove redundant validate_identifier call in booking_api.py (now in client) Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc --- src/otf_api/api/bookings/booking_api.py | 2 - src/otf_api/api/bookings/booking_client.py | 4 ++ src/otf_api/api/members/member_api.py | 40 +++++++--------- src/otf_api/api/trends/trend_api.py | 7 +-- src/otf_api/api/utils.py | 5 +- src/otf_api/cache.py | 10 ++-- src/otf_api/exceptions.py | 20 +++----- tests/test_api/test_security_hardening.py | 56 ++++++++++++---------- 8 files changed, 67 insertions(+), 77 deletions(-) diff --git a/src/otf_api/api/bookings/booking_api.py b/src/otf_api/api/bookings/booking_api.py index 97658fcd..afd8466f 100644 --- a/src/otf_api/api/bookings/booking_api.py +++ b/src/otf_api/api/bookings/booking_api.py @@ -311,8 +311,6 @@ def get_booking(self, booking_uuid: str) -> models.Booking: if not booking_uuid: raise ValueError("booking_uuid is required") - utils.validate_identifier(booking_uuid, "booking_uuid") - data = self.client.get_booking(booking_uuid) return models.Booking.create(**data, api=self.otf) diff --git a/src/otf_api/api/bookings/booking_client.py b/src/otf_api/api/bookings/booking_client.py index 984143d4..41231630 100644 --- a/src/otf_api/api/bookings/booking_client.py +++ b/src/otf_api/api/bookings/booking_client.py @@ -4,6 +4,7 @@ import pendulum from otf_api.api.client import API_IO_BASE_URL, OtfClient +from otf_api.api.utils import validate_identifier class BookingClient: @@ -33,6 +34,7 @@ def get_classes(self, studio_uuids: list[str]) -> dict: def delete_booking(self, booking_uuid: str) -> dict: """Cancel a booking by booking_uuid.""" + booking_uuid = validate_identifier(booking_uuid, "booking_uuid") resp = self.client.default_request( "DELETE", f"/member/members/{self.member_uuid}/bookings/{booking_uuid}", params={"confirmed": "true"} ) @@ -61,6 +63,7 @@ def post_class_new(self, body: dict[str, str | bool]) -> dict: def get_booking(self, booking_uuid: str) -> dict: """Retrieve raw booking data.""" + booking_uuid = validate_identifier(booking_uuid, "booking_uuid") return self.client.default_request("GET", f"/member/members/{self.member_uuid}/bookings/{booking_uuid}")["data"] def get_bookings(self, start_date: str | None, end_date: str | None, status: str | list[str] | None) -> dict: @@ -94,6 +97,7 @@ def get_bookings_new( def delete_booking_new(self, booking_id: str) -> None: """Cancel a booking by booking_id.""" + booking_id = validate_identifier(booking_id, "booking_id") self.classes_request("DELETE", f"/v1/bookings/me/{booking_id}") def post_class_rating( diff --git a/src/otf_api/api/members/member_api.py b/src/otf_api/api/members/member_api.py index 863c9e03..e294c263 100644 --- a/src/otf_api/api/members/member_api.py +++ b/src/otf_api/api/members/member_api.py @@ -2,11 +2,12 @@ from logging import getLogger from typing import Any -from otf_api import exceptions as exc from otf_api import models from .member_client import MemberClient +_MAX_NAME_LENGTH = 50 + if typing.TYPE_CHECKING: from otf_api import Otf from otf_api.api.client import OtfClient @@ -116,15 +117,15 @@ def update_email_notification_settings( return new_settings @staticmethod - def _validate_name(value: str, field_name: str) -> str: + def validate_name(value: str, name: str) -> str: """Validate a name field before sending to the API. - Guards against control characters, HTML injection, and unreasonable - lengths that the API's weak validation layer may accept. + Guards against control characters and unreasonable lengths that the + API's weak validation layer may accept. Args: value: The name string to validate. - field_name: Human-readable field name for error messages. + name: Human-readable name of the parameter (for error messages). Returns: The stripped, validated name string. @@ -134,13 +135,12 @@ def _validate_name(value: str, field_name: str) -> str: """ value = value.strip() if not value: - raise ValueError(f"{field_name} must not be empty after stripping whitespace") - if len(value) > 50: - raise ValueError(f"{field_name} is too long ({len(value)} chars, max 50)") + raise ValueError(f"{name} must not be empty after stripping whitespace") + if len(value) > _MAX_NAME_LENGTH: + raise ValueError(f"{name} is too long ({len(value)} chars, max {_MAX_NAME_LENGTH})") + # ASCII control characters (below space, except tab) if any(ord(c) < 32 and c != "\t" for c in value): - raise ValueError(f"{field_name} contains control characters") - if "<" in value or ">" in value: - raise ValueError(f"{field_name} contains HTML-like characters") + raise ValueError(f"{name} contains control characters") return value def update_member_name(self, first_name: str | None = None, last_name: str | None = None) -> models.MemberDetail: @@ -169,8 +169,8 @@ def update_member_name(self, first_name: str | None = None, last_name: str | Non if last_name is None: raise ValueError("Last name is required") - first_name = self._validate_name(first_name, "first_name") - last_name = self._validate_name(last_name, "last_name") + first_name = self.validate_name(first_name, "first_name") + last_name = self.validate_name(last_name, "last_name") res = self.client.put_member_name(first_name, last_name) @@ -190,22 +190,16 @@ def get_member_detail(self) -> models.MemberDetail: member = models.MemberDetail.create(**data, api=self.otf) - # Verify the API returned data for the authenticated user, not someone else. - # This guards against potential IDOR vulnerabilities in the v1 API which uses - # explicit member UUIDs in request paths rather than token-derived identity. + # Sanity-check that the response matches the request — the v1 API uses + # explicit member UUIDs in paths rather than token-derived identity. expected_uuid = self.client.member_uuid if member.member_uuid != expected_uuid: LOGGER.error( - "API returned member data for %s but authenticated as %s — possible IDOR", + "API returned member data for %s but authenticated as %s", member.member_uuid, expected_uuid, ) - raise exc.OtfRequestError( - "API returned data for a different member than authenticated", - original_exception=None, - response=None, - request=None, - ) + raise ValueError(f"API returned data for member {member.member_uuid}, expected {expected_uuid}") return member diff --git a/src/otf_api/api/trends/trend_api.py b/src/otf_api/api/trends/trend_api.py index a85043aa..fa814ae8 100644 --- a/src/otf_api/api/trends/trend_api.py +++ b/src/otf_api/api/trends/trend_api.py @@ -32,7 +32,7 @@ def __init__(self, otf: "Otf", otf_client: "OtfClient"): def get_workout_stats( self, - trend_type: TrendType, + trend_type: TrendType | str, start_date: date | str | None = None, end_date: date | str | None = None, ) -> WorkoutStatsResponse: @@ -46,16 +46,13 @@ def get_workout_stats( Returns: WorkoutStatsResponse: The stat data with individual data points per workout. """ - if not isinstance(trend_type, TrendType): - raise TypeError(f"trend_type must be a TrendType enum member, got {type(trend_type).__name__}") - start = utils.ensure_date(start_date) or pendulum.today().subtract(days=90).date() end = utils.ensure_date(end_date) or pendulum.today().date() start_str = pendulum.instance(pendulum.datetime(start.year, start.month, start.day)).to_iso8601_string() end_str = pendulum.instance(pendulum.datetime(end.year, end.month, end.day, 23, 59, 59)).to_iso8601_string() - stats_key = str(trend_type) + stats_key = utils.validate_identifier(str(trend_type), "stats_key") data = self.client.get_workout_stats(stats_key, start_str, end_str) return WorkoutStatsResponse(**data) diff --git a/src/otf_api/api/utils.py b/src/otf_api/api/utils.py index db44fed4..15a2e5d2 100644 --- a/src/otf_api/api/utils.py +++ b/src/otf_api/api/utils.py @@ -17,6 +17,7 @@ MIN_TIME = datetime.min.time() +_MAX_IDENTIFIER_LENGTH = 200 _UNSAFE_PATH_CHARS = re.compile(r"[/\\\x00%\s]") @@ -38,8 +39,8 @@ def validate_identifier(value: str, name: str) -> str: """ if not value: raise ValueError(f"{name} must not be empty") - if len(value) > 200: - raise ValueError(f"{name} is too long ({len(value)} chars, max 200)") + if len(value) > _MAX_IDENTIFIER_LENGTH: + raise ValueError(f"{name} is too long ({len(value)} chars, max {_MAX_IDENTIFIER_LENGTH})") if ".." in value: raise ValueError(f"{name} contains path traversal sequence") if _UNSAFE_PATH_CHARS.search(value): diff --git a/src/otf_api/cache.py b/src/otf_api/cache.py index 4665be0f..6678d7b6 100644 --- a/src/otf_api/cache.py +++ b/src/otf_api/cache.py @@ -1,4 +1,3 @@ -import stat from importlib.metadata import version from logging import getLogger from pathlib import Path @@ -12,6 +11,7 @@ TOKEN_KEYS = ["access_token", "id_token", "refresh_token"] LOGGER = getLogger(__name__) +_CACHE_DIR_MODE = 0o700 class OtfCache(Cache): @@ -120,7 +120,7 @@ def get_cache_dir() -> str: return cache_dir -def _ensure_secure_directory(path: str) -> None: +def ensure_secure_directory(path: str) -> None: """Create the cache directory with restrictive permissions (owner-only access). On systems that support it, sets the directory to mode 0700. On Windows @@ -129,12 +129,12 @@ def _ensure_secure_directory(path: str) -> None: Args: path: The directory path to create and secure. """ - Path(path).mkdir(mode=0o700, parents=True, exist_ok=True) + Path(path).mkdir(mode=_CACHE_DIR_MODE, parents=True, exist_ok=True) # Ensure permissions are correct even if the directory already existed # with more permissive settings (e.g., from a previous version). try: - Path(path).chmod(stat.S_IRWXU) # 0700: owner read/write/execute only + Path(path).chmod(_CACHE_DIR_MODE) except OSError: LOGGER.warning("Could not set restrictive permissions on cache directory: %s", path) @@ -151,7 +151,7 @@ def get_cache() -> OtfCache: global _CACHE if _CACHE is None: cache_dir = get_cache_dir() - _ensure_secure_directory(cache_dir) + ensure_secure_directory(cache_dir) LOGGER.debug("Using cache directory: %s", cache_dir) _CACHE = OtfCache(cache_dir) return _CACHE diff --git a/src/otf_api/exceptions.py b/src/otf_api/exceptions.py index 0846fa1f..b6eaaf3c 100644 --- a/src/otf_api/exceptions.py +++ b/src/otf_api/exceptions.py @@ -1,10 +1,5 @@ -import typing - import httpx -if typing.TYPE_CHECKING: - from httpx import Request, Response - __all__ = [ "AlreadyBookedError", "AlreadyRatedError", @@ -32,8 +27,8 @@ class OtfRequestError(OtfError): """Raised when an error occurs while making a request to the OTF API.""" original_exception: Exception | None - response: "Response | None" - request: "Request | None" + response: httpx.Response + request: httpx.Request # Headers to redact from stored request objects to prevent credential leakage # when exceptions are logged or sent to error-reporting services. @@ -43,8 +38,8 @@ def __init__( self, message: str, original_exception: Exception | None, - response: "Response | None", - request: "Request | None", + response: httpx.Response, + request: httpx.Request, ): super().__init__(message) self.original_exception = original_exception @@ -52,15 +47,12 @@ def __init__( self.request = self._sanitize_request(request) @classmethod - def _sanitize_request(cls, request: "Request | None") -> "Request | None": + def _sanitize_request(cls, request: httpx.Request) -> httpx.Request: """Return a copy of the request with sensitive headers redacted. This prevents credential leakage when exceptions are logged or sent - to error-reporting services. Returns None if no request is provided. + to error-reporting services. """ - if request is None: - return None - sanitized_headers = dict(request.headers) for header in cls._SENSITIVE_HEADERS: if header in sanitized_headers: diff --git a/tests/test_api/test_security_hardening.py b/tests/test_api/test_security_hardening.py index ecbdfd59..39d87719 100644 --- a/tests/test_api/test_security_hardening.py +++ b/tests/test_api/test_security_hardening.py @@ -10,7 +10,7 @@ from otf_api.api.members.member_api import MemberApi from otf_api.api.utils import validate_identifier -from otf_api.cache import _ensure_secure_directory +from otf_api.cache import ensure_secure_directory from otf_api.exceptions import OtfRequestError @@ -69,38 +69,42 @@ def test_error_message_includes_field_name(self): class TestSanitizeRequest: """Tests for credential redaction in OtfRequestError.""" + @staticmethod + def _make(req): + resp = httpx.Response(200, request=req) + return OtfRequestError("test", None, resp, req) + def test_redacts_authorization_header(self): req = httpx.Request("GET", "https://example.com", headers={"Authorization": "Bearer secret"}) - err = OtfRequestError("test", None, None, req) + err = self._make(req) assert err.request.headers["authorization"] == "[REDACTED]" def test_redacts_aws_security_token(self): req = httpx.Request("GET", "https://example.com", headers={"x-amz-security-token": "tok"}) - err = OtfRequestError("test", None, None, req) + err = self._make(req) assert err.request.headers["x-amz-security-token"] == "[REDACTED]" def test_preserves_non_sensitive_headers(self): req = httpx.Request("GET", "https://example.com", headers={"content-type": "application/json"}) - err = OtfRequestError("test", None, None, req) + err = self._make(req) assert err.request.headers["content-type"] == "application/json" def test_preserves_request_body(self): req = httpx.Request("POST", "https://example.com", content=b'{"key": "value"}') - err = OtfRequestError("test", None, None, req) + err = self._make(req) assert err.request.content == b'{"key": "value"}' def test_does_not_modify_original_request(self): req = httpx.Request("GET", "https://example.com", headers={"Authorization": "Bearer secret"}) - OtfRequestError("test", None, None, req) + self._make(req) assert req.headers["authorization"] == "Bearer secret" - def test_handles_none_request(self): - err = OtfRequestError("test", None, None, None) - assert err.request is None - - def test_handles_none_response(self): - err = OtfRequestError("test", None, None, None) - assert err.response is None + def test_preserves_method_and_url(self): + req = httpx.Request("POST", "https://api.example.com/v1/bookings") + resp = httpx.Response(500, request=req) + err = OtfRequestError("test", None, resp, req) + assert err.request.method == "POST" + assert str(err.request.url) == "https://api.example.com/v1/bookings" class TestValidateName: @@ -108,38 +112,38 @@ class TestValidateName: def test_rejects_empty(self): with pytest.raises(ValueError, match="must not be empty"): - MemberApi._validate_name("", "first_name") + MemberApi.validate_name("", "first_name") def test_rejects_whitespace_only(self): with pytest.raises(ValueError, match="must not be empty"): - MemberApi._validate_name(" ", "first_name") + MemberApi.validate_name(" ", "first_name") def test_rejects_too_long(self): with pytest.raises(ValueError, match="too long"): - MemberApi._validate_name("A" * 51, "first_name") + MemberApi.validate_name("A" * 51, "first_name") def test_rejects_control_characters(self): with pytest.raises(ValueError, match="control characters"): - MemberApi._validate_name("abc\x01def", "first_name") + MemberApi.validate_name("abc\x01def", "first_name") - def test_rejects_html_brackets(self): - with pytest.raises(ValueError, match="HTML-like"): - MemberApi._validate_name("", "first_name") + def test_allows_angle_brackets_in_name(self): + result = MemberApi.validate_name("O Date: Sat, 5 Sep 2026 14:16:30 -0500 Subject: [PATCH 4/4] =?UTF-8?q?fix:=20address=20PR=20review=20comments=20?= =?UTF-8?q?=E2=80=94=20credential=20leak=20paths,=20TrendType=20enforcemen?= =?UTF-8?q?t,=20cache=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six fixes from Codex and CodeRabbit review feedback: 1. Add koji-member-email and koji-member-id to _SENSITIVE_HEADERS 2. Sanitize response.request and original_exception.request references to close all credential leak paths through retained objects 3. Guard request.content access against RequestNotRead for streaming bodies 4. Enforce TrendType enum at runtime (isinstance check + TypeError) 5. Raise OSError on cache chmod failure instead of silently continuing 6. Re-check cache directory permissions on every get_cache() call Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc --- .../exceptions_optional_fields.md | 34 +++++++----- .../identifier-validation-convention.md | 34 ++++++++---- src/otf_api/api/trends/trend_api.py | 7 ++- src/otf_api/cache.py | 11 ++-- src/otf_api/exceptions.py | 27 ++++++++-- tests/test_api/test_security_hardening.py | 53 +++++++++++++++++++ 6 files changed, 131 insertions(+), 35 deletions(-) diff --git a/.claude/agent-memory/code-reviewer/exceptions_optional_fields.md b/.claude/agent-memory/code-reviewer/exceptions_optional_fields.md index d9a43870..3b61a4d4 100644 --- a/.claude/agent-memory/code-reviewer/exceptions_optional_fields.md +++ b/.claude/agent-memory/code-reviewer/exceptions_optional_fields.md @@ -1,23 +1,29 @@ --- name: exceptions-optional-fields -description: OtfRequestError.response/.request are typed non-Optional but code sometimes passes None +description: OtfRequestError.response/.request are typed non-Optional; historical None call-site no longer exists metadata: type: project --- - -`src/otf_api/exceptions.py`'s `OtfRequestError` declares `response: "Response"` and -`request: "Request"` as non-Optional class attributes, but at least one call site -(`member_api.py`'s IDOR check, added 2026-09) passes `response=None, request=None` -with `# type: ignore[arg-type]` to suppress the mismatch. + +`src/otf_api/exceptions.py`'s `OtfRequestError` declares `response: httpx.Response` and +`request: httpx.Request` as non-Optional class attributes. An earlier note recorded a +`member_api.py` IDOR-check call site passing `response=None, request=None` with +`# type: ignore[arg-type]` — **verified stale as of 2026-09-05**: that call site was +refactored (commit `d792c91`, "address clean code review findings") to raise a plain +`ValueError` instead of `OtfRequestError`, so no current call site passes None for +these fields. Grepped all `OtfRequestError(`/`RetryableOtfRequestError(` call sites in +`src/` and `tests/` on that date — all pass real `httpx.Request`/`httpx.Response` +objects. -**Why it matters:** `docs/guides/error-handling.md` documents catching `OtfRequestError` -generically and accessing `e.request.method` / `e.request.url` — that crashes with -`AttributeError` on a None request. `_sanitize_request()` already null-checks -`request is None`, showing the author knew this could happen but didn't update the -type annotation to `Request | None`. +**Why it still matters:** `OtfRequestError.__init__` now (as of the same date) mutates +`self.response.request` and `self.original_exception.request` in place to redact +sensitive headers from any retained references. If a future call site ever reintroduces +`response=None` or `request=None`, this would crash with `AttributeError` before the +message even gets set. `_sanitize_request()` has no None-guard either. **How to apply:** When reviewing new `raise exc.OtfRequestError(...)` call sites, check -whether `response`/`request` are being passed as `None`. If so, flag it — either the -field types need to become `Optional`, or the call site needs a real request/response -object. Don't let `# type: ignore[arg-type]` on these two fields pass silently. +whether `response`/`request` are ever `None`. If so, flag it — either the field types +need to become `Optional`, or the call site needs a real request/response object. Don't +trust this note's own history at face value either — re-verify against current +`src/otf_api/api/client.py` before citing it, since it has already gone stale once. diff --git a/.claude/agent-memory/integration-reviewer/identifier-validation-convention.md b/.claude/agent-memory/integration-reviewer/identifier-validation-convention.md index e6213f84..250fc76b 100644 --- a/.claude/agent-memory/integration-reviewer/identifier-validation-convention.md +++ b/.claude/agent-memory/integration-reviewer/identifier-validation-convention.md @@ -20,15 +20,27 @@ directly-callable method) does not call `utils.get_booking_uuid` before forwardi specific method when reviewing future diffs — if still unpatched, it's a real gap, not a false positive. -**Also flagged in the same diff:** `TrendClient.get_workout_stats` closes the same class of gap -by narrowing `TrendApi.get_workout_stats`'s `trend_type` param from `TrendType | str` to -`TrendType` instead of calling `validate_identifier` — a different (breaking) technique for the -same problem. Worth checking whether this was reconciled to the `validate_identifier` pattern or -left as an enum-only breaking change. +**Resolved (as of 2026-09-05, branch `security/client-hardening`):** `TrendApi.get_workout_stats` +now does both — enforces `isinstance(trend_type, TrendType)` (raising `TypeError`, matching the +`get_booking_id`/`get_class_uuid`-style "Expected X or str" convention in `api/utils.py`) *and* +still runs `trend_type.value` through `utils.validate_identifier` before it reaches the client. +Reconciled, not a parallel technique. -**Also flagged:** `OtfRequestError.response`/`.request` are typed non-Optional and one existing -catch site (`booking_api.py` `post_class_rating`, `except OtfRequestError as e: e.response.status_code`) -relies on that being true. `MemberApi.get_member_detail`'s new IDOR guard raises -`OtfRequestError(response=None, request=None)`, violating that invariant. Check whether the type -was ever changed to `Response | None` / `Request | None` with consumers updated, or whether this -call site was given a synthetic Response/Request instead. +**Resolved:** `OtfRequestError(response=None, request=None)` call site is gone — grep for +`response=None` / `request=None` across `src/otf_api` turns up nothing. `response`/`request` +stay typed non-Optional. + +**New in the same hardening pass:** `OtfRequestError.__init__` now mutates the *caller-supplied* +`response`/`original_exception` objects in place (`self.response.request = sanitized_request`, +`self.original_exception.request = sanitized_request`) so that downstream code holding the same +reference (e.g. `anonymize/hooks.py`, which reads `response.request` for logging) doesn't see the +unsanitized request. This is a deliberate, load-bearing exception to the "never mutate existing +objects" rule — the leak isn't closed otherwise, since `self.request` alone doesn't cover +`response.request`/`original_exception.request`. Flag it as intentional if seen again, not a fresh +violation. + +**Tooling gap:** `ruff.toml` excludes `tests/` entirely (`exclude = [..., "tests"]`), so +`ruff check .` / the `ruff-check` pre-commit hook never lints test files — unused imports, +missing annotations, etc. in `tests/` pass CI silently. Running `ruff check ` with an +explicit test file path bypasses the exclude and does catch these; use that when reviewing test +diffs in this repo, since the standard hook won't. diff --git a/src/otf_api/api/trends/trend_api.py b/src/otf_api/api/trends/trend_api.py index fa814ae8..88ce66c6 100644 --- a/src/otf_api/api/trends/trend_api.py +++ b/src/otf_api/api/trends/trend_api.py @@ -32,7 +32,7 @@ def __init__(self, otf: "Otf", otf_client: "OtfClient"): def get_workout_stats( self, - trend_type: TrendType | str, + trend_type: TrendType, start_date: date | str | None = None, end_date: date | str | None = None, ) -> WorkoutStatsResponse: @@ -46,13 +46,16 @@ def get_workout_stats( Returns: WorkoutStatsResponse: The stat data with individual data points per workout. """ + if not isinstance(trend_type, TrendType): + raise TypeError(f"trend_type must be a TrendType enum member, got {type(trend_type).__name__}") + start = utils.ensure_date(start_date) or pendulum.today().subtract(days=90).date() end = utils.ensure_date(end_date) or pendulum.today().date() start_str = pendulum.instance(pendulum.datetime(start.year, start.month, start.day)).to_iso8601_string() end_str = pendulum.instance(pendulum.datetime(end.year, end.month, end.day, 23, 59, 59)).to_iso8601_string() - stats_key = utils.validate_identifier(str(trend_type), "stats_key") + stats_key = utils.validate_identifier(trend_type.value, "stats_key") data = self.client.get_workout_stats(stats_key, start_str, end_str) return WorkoutStatsResponse(**data) diff --git a/src/otf_api/cache.py b/src/otf_api/cache.py index 6678d7b6..5b839a9d 100644 --- a/src/otf_api/cache.py +++ b/src/otf_api/cache.py @@ -123,8 +123,8 @@ def get_cache_dir() -> str: def ensure_secure_directory(path: str) -> None: """Create the cache directory with restrictive permissions (owner-only access). - On systems that support it, sets the directory to mode 0700. On Windows - or other systems where chmod is a no-op, this is best-effort. + Sets the directory to mode 0700. Raises OSError if permissions cannot be + applied, to prevent storing tokens in a world-readable directory. Args: path: The directory path to create and secure. @@ -136,7 +136,8 @@ def ensure_secure_directory(path: str) -> None: try: Path(path).chmod(_CACHE_DIR_MODE) except OSError: - LOGGER.warning("Could not set restrictive permissions on cache directory: %s", path) + LOGGER.error("Could not set restrictive permissions on cache directory: %s", path) + raise def get_cache() -> OtfCache: @@ -149,9 +150,9 @@ def get_cache() -> OtfCache: Cache: The cache instance. """ global _CACHE + cache_dir = get_cache_dir() + ensure_secure_directory(cache_dir) if _CACHE is None: - cache_dir = get_cache_dir() - ensure_secure_directory(cache_dir) LOGGER.debug("Using cache directory: %s", cache_dir) _CACHE = OtfCache(cache_dir) return _CACHE diff --git a/src/otf_api/exceptions.py b/src/otf_api/exceptions.py index b6eaaf3c..c72845da 100644 --- a/src/otf_api/exceptions.py +++ b/src/otf_api/exceptions.py @@ -32,7 +32,15 @@ class OtfRequestError(OtfError): # Headers to redact from stored request objects to prevent credential leakage # when exceptions are logged or sent to error-reporting services. - _SENSITIVE_HEADERS = frozenset({"authorization", "x-amz-security-token", "x-amz-date"}) + _SENSITIVE_HEADERS = frozenset( + { + "authorization", + "x-amz-security-token", + "x-amz-date", + "koji-member-email", + "koji-member-id", + } + ) def __init__( self, @@ -42,9 +50,17 @@ def __init__( request: httpx.Request, ): super().__init__(message) + sanitized_request = self._sanitize_request(request) self.original_exception = original_exception self.response = response - self.request = self._sanitize_request(request) + self.request = sanitized_request + + # The response and original exception hold references to the raw request + # with unsanitized auth headers. Mutating these shared objects is intentional — + # error-reporting tools serialize them, and we must close every leak path. + self.response.request = sanitized_request + if isinstance(self.original_exception, httpx.HTTPStatusError): + self.original_exception.request = sanitized_request @classmethod def _sanitize_request(cls, request: httpx.Request) -> httpx.Request: @@ -58,11 +74,16 @@ def _sanitize_request(cls, request: httpx.Request) -> httpx.Request: if header in sanitized_headers: sanitized_headers[header] = "[REDACTED]" + try: + content = request.content + except httpx.RequestNotRead: + content = b"" + return httpx.Request( method=request.method, url=request.url, headers=sanitized_headers, - content=request.content, + content=content, ) diff --git a/tests/test_api/test_security_hardening.py b/tests/test_api/test_security_hardening.py index 39d87719..be61ac6f 100644 --- a/tests/test_api/test_security_hardening.py +++ b/tests/test_api/test_security_hardening.py @@ -4,11 +4,13 @@ import stat import tempfile from pathlib import Path +from unittest.mock import PropertyMock, patch import httpx import pytest from otf_api.api.members.member_api import MemberApi +from otf_api.api.trends.trend_api import TrendApi from otf_api.api.utils import validate_identifier from otf_api.cache import ensure_secure_directory from otf_api.exceptions import OtfRequestError @@ -94,6 +96,35 @@ def test_preserves_request_body(self): err = self._make(req) assert err.request.content == b'{"key": "value"}' + def test_redacts_koji_member_email(self): + req = httpx.Request("GET", "https://example.com", headers={"koji-member-email": "user@example.com"}) + err = self._make(req) + assert err.request.headers["koji-member-email"] == "[REDACTED]" + + def test_redacts_koji_member_id(self): + req = httpx.Request("GET", "https://example.com", headers={"koji-member-id": "some-uuid"}) + err = self._make(req) + assert err.request.headers["koji-member-id"] == "[REDACTED]" + + def test_sanitizes_response_request_reference(self): + req = httpx.Request("GET", "https://example.com", headers={"Authorization": "Bearer secret"}) + err = self._make(req) + assert err.response.request.headers["authorization"] == "[REDACTED]" + + def test_sanitizes_original_exception_request(self): + req = httpx.Request("GET", "https://example.com", headers={"Authorization": "Bearer secret"}) + resp = httpx.Response(500, request=req) + original = httpx.HTTPStatusError("fail", request=req, response=resp) + err = OtfRequestError("test", original, resp, req) + assert err.original_exception.request.headers["authorization"] == "[REDACTED]" + + def test_handles_unread_request_body(self): + req = httpx.Request("POST", "https://example.com", headers={"Authorization": "Bearer secret"}) + with patch.object(type(req), "content", new_callable=PropertyMock, side_effect=httpx.RequestNotRead()): + err = self._make(req) + assert err.request.content == b"" + assert err.request.headers["authorization"] == "[REDACTED]" + def test_does_not_modify_original_request(self): req = httpx.Request("GET", "https://example.com", headers={"Authorization": "Bearer secret"}) self._make(req) @@ -164,3 +195,25 @@ def test_tightens_existing_loose_permissions(self): ensure_secure_directory(path) mode = stat.S_IMODE(Path(path).stat().st_mode) assert mode == 0o700, f"Expected 0700, got {oct(mode)}" + + def test_raises_on_chmod_failure(self): + with tempfile.TemporaryDirectory() as tmp: + path = f"{tmp}/test_cache" + os.makedirs(path, mode=0o700) + with patch("otf_api.cache.Path.chmod", side_effect=OSError("permission denied")): + with pytest.raises(OSError, match="permission denied"): + ensure_secure_directory(path) + + +class TestTrendTypeValidation: + """Tests for runtime TrendType enforcement.""" + + def test_rejects_raw_string(self): + api = TrendApi.__new__(TrendApi) + with pytest.raises(TypeError, match="must be a TrendType enum member"): + api.get_workout_stats("splat_points") + + def test_rejects_path_traversal_string(self): + api = TrendApi.__new__(TrendApi) + with pytest.raises(TypeError, match="must be a TrendType enum member"): + api.get_workout_stats("../preview")