Skip to content

fix: address the security, correctness and design audit (#3) - #4

Merged
sebartyr merged 4 commits into
mainfrom
fix/audit-findings-issue-3
Aug 18, 2026
Merged

fix: address the security, correctness and design audit (#3)#4
sebartyr merged 4 commits into
mainfrom
fix/audit-findings-issue-3

Conversation

@sebartyr

@sebartyr sebartyr commented Aug 18, 2026

Copy link
Copy Markdown
Member

Closes #3.

Reworks authentication, response handling and packaging around every finding in the audit, and adds the test suite and CI the repository was missing.

High-priority findings

1. OAuth implementation is incomplete and replayable — every request is now signed with HMAC-SHA512 over its method, normalized URL, query string and form body, carrying oauth_signature_method, oauth_timestamp, oauth_nonce and oauth_version. The signature base string is validated against the worked example from RFC 5849 §3.4.1.1. SignatureMethod.PLAINTEXT and HMAC_SHA256 remain selectable, PLAINTEXT only as an explicit compatibility mode.

2. Credentials leak through repr() — every credential dataclass redacts its secrets, keeping the non-secret identifiers visible so the repr stays useful. Covered including the nested case (repr({"auth": creds})), which is how structured logging usually leaks them.

3. API path parameters are not encoded — added encode_path_segment(), applied to every identifier. The three reproductions from the audit are regression tests; assertions check url.raw_path (what goes on the wire), since httpx's .path shows the decoded view.

Medium-priority findings

  • 4. Empty JSON responses — any successful status may now come without a body; undecodable JSON raises InvalidResponseError; unfollowed 3xx no longer counts as success.
  • 5. Fabricated dates — a missing date is None, never now(). All dates normalize to timezone-aware UTC. Required fields are enforced, genuinely optional ones typed | None.
  • 6. Lexicographic version ordering — natural ordering, so 10 beats 9; a trailing end-marker keeps 1.0-beta below 1.0.
  • 7. Forced JSON Content-Type — only Accept is set globally; httpx derives the rest. A GET now carries no Content-Type, and data={...} is correctly labelled form-encoded.
  • 8. OAuth callback and expirationoauth_callback_confirmed is checked, the callback token is compared against the request token (constant-time), expiration_date is kept and exposed via is_expired(), and mfa_kind is configurable. login() is kept for browser-less automation but documented as not a supported OAuth flow, with the browser flow promoted in the README.
  • 9. Deprecated HTTPX TLS APIs — TLS/mTLS go through an ssl.SSLContext; http:// base URLs are refused without an explicit override. Writing the tests surfaced a second deprecated call (per-request cookies= in the dance), also removed; the suite runs clean under -W error::DeprecationWarning.

Additional findings

404 no longer masks a missing application, transport errors are wrapped in TransportError, exception bodies are truncated to 2 KiB, 403 maps to a new AuthorizationError, the instance catalogue is cached per client, idempotent requests retry with backoff/jitter honouring Retry-After (re-signed each attempt, so no nonce reuse), and frozen=True models are now immutable all the way down.

Tests and packaging

217 tests, entirely on httpx.MockTransport — no network access. 95% coverage. Adds py.typed, and CI running ruff, strict mypy and pytest on Python 3.11, 3.12 and 3.13. Verified locally on 3.11 through 3.14.

Breaking changes

Version bumped to 0.2.0; full list in CHANGELOG.md. The main ones: Auth.get_authorization_header() takes the method and URL (a signature is bound to them), strict model parsing raises instead of fabricating values, AuthorizationError is not a subclass of AuthenticationError, list_domains()/get_primary_domain() surface 404, and httpx>=0.28 is required.

Not addressed

Two audit items are left as follow-ups, both API-design calls rather than defects: the batch application-creation redundancy, and the broad use of Any in the NetworkGroups search union, which the API returns as a oneOf the SDK deliberately does not discriminate.

Rebuilds authentication, response handling and packaging around the audit
findings, and adds the test suite and CI the repository was missing.

Security:
- Sign every OAuth request with HMAC-SHA512 over its method, URL, query and
  form body, with a timestamp, nonce and version. The previous header was
  static and replayable. PLAINTEXT stays as an explicit compatibility mode.
- Redact secrets in the repr() of every credential dataclass.
- Percent-encode path parameters, so an identifier cannot change the route.
- Refuse a clear-text http:// base URL unless explicitly allowed.
- Validate oauth_callback_confirmed and the callback token in the dance.
- Truncate response bodies carried by exceptions.

Correctness:
- Handle body-less successful responses; reject unfollowed redirections and
  wrap undecodable JSON in InvalidResponseError.
- Stop replacing missing dates with the current time; normalize to UTC.
- Order runtime versions naturally, so 10 outranks 9.
- Let HTTPX derive the Content-Type from the body actually sent.
- Configure TLS/mTLS through an ssl.SSLContext instead of the arguments
  deprecated in HTTPX 0.28.
- Classify 403 as AuthorizationError and wrap transport errors.
- Retry idempotent requests on transient failures, honouring Retry-After.
- Cache the instance catalogue per client.

Packaging and tests:
- Ship the py.typed marker the Typing :: Typed classifier promised.
- Add 217 tests running entirely on httpx.MockTransport, plus CI running
  ruff, strict mypy and pytest on Python 3.11, 3.12 and 3.13.

Breaking changes are listed in CHANGELOG.md.
Profile.name is str | None since parsing became strict, so the example
would have printed "Hello, None!" for an account without a name.
The docstrings inherited from the original code were uneven: 30 of 34
public methods documented no exception, several NetworkGroups methods
carried only their HTTP route, and the models did not say which fields
can be None.

- Document the error hierarchy once on CleverCloudClient, so each method
  only states what it adds on top of it.
- Add Args/Returns/Raises to the NetworkGroups and creation methods.
- Note that create_networkgroup/create_networkgroup_member answer 202:
  success means accepted, not ready.
- Add Attributes sections to the models, calling out the nullable fields
  and the guaranteed ones.
- Document the OAuth dance step by step, including which values to store
  and which failures map to which step.

Raises is now documented on 23 of 35 methods; the remaining 12 are pure
methods that raise nothing specific.
@sebartyr
sebartyr force-pushed the fix/audit-findings-issue-3 branch from ae9a603 to 0a6fe44 Compare August 18, 2026 11:49
Comment thread src/clever_cloud/client.py Outdated
Comment thread src/clever_cloud/client.py Outdated
Comment thread src/clever_cloud/oauth_dance.py
Comment thread src/clever_cloud/oauth_dance.py
- Retry-After: parsedate_to_datetime() raises on an arbitrary header, so a
  server-controlled value escaped the error hierarchy and cancelled the
  retry it was meant to schedule. Malformed values are now treated as
  absent, including NaN and infinity, which parse as floats but are not
  usable delays.
- Undecodable JSON: a JSONDecodeError holds the whole payload in .doc, and
  `raise ... from exc` kept it reachable. The error is now raised outside
  the except block, keeping only the reason as a string: `from None`
  alone would have cleared __cause__ while leaving __context__ pointing
  at the same object.
- OAuth login(): all four HTTP calls translate a transport failure into an
  OAuthError carrying the login, mfa_login or authorize step, like the
  token exchanges already did.
- Access token: the credentials now pin the API root that issued them, so
  handing them to CleverCloudClient no longer sends the token to the
  public API when the dance targeted a private deployment.

Adds 19 regression tests, one per failure mode, including the full
dance-to-client handoff against a private root.
@sebartyr
sebartyr merged commit 8cc9414 into main Aug 18, 2026
4 checks passed
@sebartyr
sebartyr deleted the fix/audit-findings-issue-3 branch August 18, 2026 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security, correctness and design audit findings

1 participant