fix: client-side security hardening against API weaknesses - #145
fix: client-side security hardening against API weaknesses#145NodeJSmith wants to merge 4 commits into
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📝 WalkthroughWalkthroughThe changes add shared identifier validation, response-size limits, member validation, cache permission enforcement, and request-header redaction. The trend API now requires ChangesSecurity hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The hardening changes improve validation and redaction, but failed requests can still retain session or proxy credentials in exception-related request and response objects. Those credentials may reach logs or error-reporting consumers, so the remaining redaction gaps should be resolved before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 69 functions across 10 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb352e3160
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: f5fec942-0768-4378-9be7-cdabe514e3d6
📒 Files selected for processing (9)
.gitignoresrc/otf_api/api/client.pysrc/otf_api/api/members/member_api.pysrc/otf_api/api/studios/studio_client.pysrc/otf_api/api/trends/trend_api.pysrc/otf_api/api/utils.pysrc/otf_api/api/workouts/workout_client.pysrc/otf_api/cache.pysrc/otf_api/exceptions.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
- 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cbe74a64a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc
…forcement, cache hardening 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 013e3d56b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content_length = len(response.content) | ||
| if content_length > MAX_RESPONSE_SIZE: |
There was a problem hiding this comment.
The fresh narrowing of MAX_RESPONSE_SIZE to a JSON-parsing limit still misses every non-2xx response: in the inspected OtfClient.do() flow, raise_for_status() branches to get_json_from_response(e.response) at lines 147–149, which calls response.json(), and _handle_response() never runs. Thus a server returning a very large 4xx/5xx JSON body—especially a retryable 5xx—can still incur the parsing cost this cap is intended to prevent; enforce the limit before parsing the error response as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 4f1e8695-3909-4198-83b3-5f76c1e74f51
📒 Files selected for processing (12)
.claude/agent-memory/code-reviewer/MEMORY.md.claude/agent-memory/code-reviewer/exceptions_optional_fields.md.claude/agent-memory/integration-reviewer/MEMORY.md.claude/agent-memory/integration-reviewer/identifier-validation-convention.mdsrc/otf_api/api/bookings/booking_client.pysrc/otf_api/api/client.pysrc/otf_api/api/members/member_api.pysrc/otf_api/api/trends/trend_api.pysrc/otf_api/api/utils.pysrc/otf_api/cache.pysrc/otf_api/exceptions.pytests/test_api/test_security_hardening.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| --- | ||
|
|
||
| <!-- 2026-09-05 (updated) --> | ||
| `src/otf_api/exceptions.py`'s `OtfRequestError` declares `response: httpx.Response` and |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a top-level heading after the front matter.
markdownlint-cli2 reports MD041 because the first prose line is not a level-one heading. Add # OtfRequestError optional fields before this text.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 9-9: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
Source: Linters/SAST tools
| @@ -0,0 +1,2 @@ | |||
| # Memory Index | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line after the heading.
markdownlint-cli2 reports MD022 because the list starts immediately after the level-one heading.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
Source: Linters/SAST tools
| @@ -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. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the None call pattern as historical.
This index says response and request are “sometimes passed as None.” The linked memory file states that this call site was removed. Use past tense so future reviews do not treat the stale pattern as current behavior.
| type: project | ||
| --- | ||
|
|
||
| <!-- 2026-09-05 --> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a top-level heading after the YAML front matter.
markdownlint reports MD041 because the document body does not start with a level-one heading. Add # Identifier validation convention after the front matter.
Source: Linters/SAST tools
| "authorization", | ||
| "x-amz-security-token", | ||
| "x-amz-date", | ||
| "koji-member-email", | ||
| "koji-member-id", | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal · Exploitability: Moderate
Redact all retained request credentials.
_sanitize_request copies unlisted credential headers and readable request bodies. A failed request can retain Cookie, Proxy-Authorization, or body credentials in stored exception requests and error reports.
Redact standard credential headers, drop request bodies, and add tests for both cases.
Proposed fix
"authorization",
+ "cookie",
+ "proxy-authorization",
"x-amz-security-token",
"x-amz-date",
"koji-member-email",
"koji-member-id",
}
)
- try:
- content = request.content
- except httpx.RequestNotRead:
- content = b""
-
return httpx.Request(
method=request.method,
url=request.url,
headers=sanitized_headers,
- content=content,
+ content=b"",
)| self.response = response | ||
| self.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 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🤖 get_repo_knowledge executed:
get_repo_knowledge NodeJSmith/otf-api /tmp/coderabbit-repo-knowledge/nodejsmith-otf-api-52973f72/learnings /tmp/coderabbit-repo-knowledge/nodejsmith-otf-api-52973f72/conventions
Length of output: 3836
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- exceptions.py ---'
cat -n src/otf_api/exceptions.py | sed -n '20,95p'
printf '%s\n' '--- client call sites ---'
cat -n src/otf_api/api/client.py | sed -n '195,265p'
printf '%s\n' '--- response/error serialization references ---'
rg -n --glob '*.py' 'OtfRequestError|\.response\b|Set-Cookie|error.?report|capture_exception|sentry|serialize' src tests | head -200Repository: NodeJSmith/otf-api
Length of output: 11726
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal · Exploitability: Moderate
Sanitize response credential headers before retaining the response.
The constructor stores the raw httpx.Response. Replacing response.request does not redact response.headers. If an error response contains Set-Cookie: session=..., the credential remains in err.response.headers.
Store a sanitized response, or redact every Set-Cookie value before retaining it. Add a test with an error response that contains Set-Cookie.
| class TestValidateIdentifier: | ||
| """Tests for the path segment validation helper.""" | ||
|
|
||
| def test_valid_uuid(self): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ruff check tests/test_api/test_security_hardening.pyRepository: NodeJSmith/otf-api
Length of output: 50374
🤖 get_repo_knowledge executed:
get_repo_knowledge NodeJSmith/otf-api /tmp/coderabbit-repo-knowledge/nodejsmith-otf-api-52973f72/conventions /tmp/coderabbit-repo-knowledge/nodejsmith-otf-api-52973f72/learnings
Length of output: 2893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked config files ---'
git ls-files | rg '(^|/)(pyproject\.toml|ruff\.toml|\.ruff\.toml|setup\.cfg|tox\.ini)$'
printf '%s\n' '--- Ruff configuration references ---'
rg -n -C 3 'ruff|lint|D102|ANN201|ANN205|PTH103|SIM117|extend-select|select|ignore|per-file-ignores|unfixable' pyproject.toml ruff.toml .ruff.toml setup.cfg tox.ini 2>/dev/null || true
printf '%s\n' '--- target file structure and first 230 lines ---'
wc -l tests/test_api/test_security_hardening.py
sed -n '1,230p' tests/test_api/test_security_hardening.pyRepository: NodeJSmith/otf-api
Length of output: 11442
Make the test module pass the repository’s Ruff policy.
ruff.toml enables ANN and D, but this file has non-auto-fixable ANN201, ANN205, ANN001, and D102 findings. Add the required annotations and docstrings, or exclude these rules for test methods.
🧰 Tools
🪛 Ruff (0.16.3)
[warning] 22-22: Missing docstring in public method
(D102)
Sources: Coding guidelines, Linters/SAST tools
| def test_tightens_existing_loose_permissions(self): | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
| path = f"{tmp}/test_cache" | ||
| os.makedirs(path, mode=0o755) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Force loose permissions before testing permission tightening.
os.makedirs(path, mode=0o755) is filtered by the process umask. With umask 077, it creates mode 0700, so this test passes even if ensure_secure_directory() stops correcting existing directory permissions.
Proposed fix
- os.makedirs(path, mode=0o755)
+ Path(path).mkdir()
+ Path(path).chmod(0o755)
ensure_secure_directory(path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.makedirs(path, mode=0o755) | |
| Path(path).mkdir() | |
| Path(path).chmod(0o755) | |
| ensure_secure_directory(path) |
🧰 Tools
🪛 Ruff (0.16.3)
[warning] 194-194: os.makedirs() should be replaced by Path.mkdir(parents=True)
(PTH103)
1 participant
Security Hardening: Client-Side Protections Against API Weaknesses
Pentest of the OTF API surface revealed that the backend has weak input validation (MySQL errors leak through, endpoints accept "almost anything"). This PR adds client-side guardrails so the library protects users even when the API doesn't.
Changes
1. Path segment validation (
utils.py,studio_client.py,workout_client.py)validate_identifier()function rejects path traversal (../), slashes, null bytes, percent-encoding, whitespace, and oversized strings (>200 chars)get_booking_uuid(),get_booking_id(),get_class_uuid(),get_class_id(),get_studio_detail(),get_studio_services(),get_performance_summary(),get_telemetry()yarl.URL.build()silently normalizes../instead of rejecting it, so the library was constructing unintended request paths2. Credential redaction in exceptions (
exceptions.py)OtfRequestErrornow stripsAuthorization,x-amz-security-token, andx-amz-dateheaders from stored request objects3. Name field validation (
member_api.py)update_member_name()now rejects control characters, HTML-like content (<>), empty/whitespace-only strings, and names over 50 characters4. IDOR response ownership verification (
member_api.py)get_member_detail()now verifies the returnedmember_uuidmatches the authenticated user/me/)5. Response size cap (
client.py)_handle_response()rejects responses over 10 MB before attempting JSON parsing6. TrendType enum enforcement (
trend_api.py)get_workout_stats()parameter narrowed fromTrendType | strtoTrendType/users/me/workout-stats/{stats_key}7. Cache directory permissions (
cache.py)0700(owner-only access)0755Testing
🤖 Generated with Claude Code
https://claude.ai/code/session_01LCQ31EaGsi9Kxr8J588kfc
Summary by CodeRabbit
Bug Fixes
Security