fix: address the security, correctness and design audit (#3) - #4
Merged
Conversation
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
force-pushed
the
fix/audit-findings-issue-3
branch
from
August 18, 2026 11:49
ae9a603 to
0a6fe44
Compare
sebartyr
commented
Aug 18, 2026
sebartyr
commented
Aug 18, 2026
sebartyr
commented
Aug 18, 2026
sebartyr
commented
Aug 18, 2026
- 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_nonceandoauth_version. The signature base string is validated against the worked example from RFC 5849 §3.4.1.1.SignatureMethod.PLAINTEXTandHMAC_SHA256remain 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 checkurl.raw_path(what goes on the wire), since httpx's.pathshows the decoded view.Medium-priority findings
InvalidResponseError; unfollowed 3xx no longer counts as success.None, nevernow(). All dates normalize to timezone-aware UTC. Required fields are enforced, genuinely optional ones typed| None.10beats9; a trailing end-marker keeps1.0-betabelow1.0.Acceptis set globally; httpx derives the rest. A GET now carries noContent-Type, anddata={...}is correctly labelled form-encoded.oauth_callback_confirmedis checked, the callback token is compared against the request token (constant-time),expiration_dateis kept and exposed viais_expired(), andmfa_kindis configurable.login()is kept for browser-less automation but documented as not a supported OAuth flow, with the browser flow promoted in the README.ssl.SSLContext;http://base URLs are refused without an explicit override. Writing the tests surfaced a second deprecated call (per-requestcookies=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 newAuthorizationError, the instance catalogue is cached per client, idempotent requests retry with backoff/jitter honouringRetry-After(re-signed each attempt, so no nonce reuse), andfrozen=Truemodels are now immutable all the way down.Tests and packaging
217 tests, entirely on
httpx.MockTransport— no network access. 95% coverage. Addspy.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,AuthorizationErroris not a subclass ofAuthenticationError,list_domains()/get_primary_domain()surface 404, andhttpx>=0.28is 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
Anyin the NetworkGroups search union, which the API returns as aoneOfthe SDK deliberately does not discriminate.