From 4bb1aae2610571a8eb18d1b39a9c6c8338b8443c Mon Sep 17 00:00:00 2001 From: petrsnd Date: Thu, 21 May 2026 11:12:07 -0600 Subject: [PATCH 1/8] Clarify params keyword and document HiddenString usage in api-patterns skill --- .agents/skills/api-patterns/SKILL.md | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/.agents/skills/api-patterns/SKILL.md b/.agents/skills/api-patterns/SKILL.md index f208d60..74abc1e 100644 --- a/.agents/skills/api-patterns/SKILL.md +++ b/.agents/skills/api-patterns/SKILL.md @@ -299,6 +299,21 @@ private_key = A2AContext.quick_retrieve_private_key( | `broker_access_request(api_key, access_request)` | `str` | Submit an access request | | `get_retrievable_accounts(*, filter=None)` | `list[dict]` | List accounts (uses cert auth) | +### HiddenString return values + +Methods that return secrets (like `A2AContext.retrieve_password()`) return a +`HiddenString` object that displays as `***` when printed, formatted, or +converted with `str()`. To access the raw value: + +```python +password = ctx.retrieve_password(api_key) +print(password) # prints: *** +print(password.value) # prints the actual password string +raw = password.value +``` + +This prevents accidental credential leakage in logs or REPL output. + ### A2A Authorization Header A2A requests use `Authorization: A2A ` (not Bearer). @@ -336,15 +351,18 @@ Set these when the appliance uses a certificate signed by an internal CA. ## Common Patterns -### GET with query parameters +### Query parameters + +Use the `params` keyword argument (not `parameters`) to pass query string values: ```python -users = client.get( - Service.CORE, "Users", - params={"filter": "UserName eq 'admin'", "fields": "Id,UserName"}, -).json() +users = client.get(Service.CORE, "Users", params={"fields": "Id,Name", "filter": "Disabled eq false"}) +assets = client.get(Service.CORE, "Assets", params={"filter": "Name eq 'MyAsset'"}) ``` +> **Note:** The keyword is `params` (matching the `requests` library convention), +> not `parameters` (which is the .NET SDK's name for the same concept). + ### POST with JSON body ```python From 7f88d44ab88e6df6debecab4aa65b2efc76dbf27 Mon Sep 17 00:00:00 2001 From: petrsnd Date: Fri, 22 May 2026 19:17:29 -0600 Subject: [PATCH 2/8] deps: pin urllib3 >= 2.0.8 and idna >= 3.7 for CVE fixes Add direct floor pins on transitive deps that have known CVEs at lower versions: - urllib3 >= 2.0.8 fixes CVE-2026-44431 (DNS rebinding) and CVE-2026-44432 (proxy protocol confusion). - idna >= 3.7 fixes CVE-2026-45409 (ReDoS via crafted IDN inputs). Regenerated poetry.lock. Added tests/test_dependency_versions.py to enforce the floors so future poetry resolutions cannot regress below the patched versions. --- poetry.lock | 4 ++-- pyproject.toml | 5 +++++ tests/test_dependency_versions.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 tests/test_dependency_versions.py diff --git a/poetry.lock b/poetry.lock index 15af9b1..90a0757 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 2.3.4 and should not be changed by hand. +# This file is automatically @generated by Poetry 2.4.1 and should not be changed by hand. [[package]] name = "aiohappyeyeballs" @@ -1568,4 +1568,4 @@ signalr = ["signalrcore"] [metadata] lock-version = "2.1" python-versions = ">=3.10" -content-hash = "f1a724513a621afd8e08899e559f7de4582469b049dafa3fb1110d754c8daac3" +content-hash = "61dbeded414df6b94996b5b7672f6f3adf7e91061da6b14226ee80ecd26130f7" diff --git a/pyproject.toml b/pyproject.toml index bbef67d..96a30ed 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,6 +33,11 @@ dependencies = [ "requests >= 2.32", "truststore >= 0.10", "typing_extensions >= 4.15; python_version < '3.11'", + # Direct floor pins on transitive deps that have known CVEs at lower versions: + # urllib3 >= 2.0.8 fixes CVE-2026-44431 (DNS rebinding) and CVE-2026-44432 (proxy confusion). + # idna >= 3.7 fixes CVE-2026-45409 (ReDoS via crafted IDN inputs). + "urllib3 >= 2.0.8", + "idna >= 3.7", ] [project.optional-dependencies] diff --git a/tests/test_dependency_versions.py b/tests/test_dependency_versions.py new file mode 100644 index 0000000..3222aa2 --- /dev/null +++ b/tests/test_dependency_versions.py @@ -0,0 +1,30 @@ +# Copyright (c) One Identity LLC. All rights reserved. +# Licensed under the Apache License, Version 2.0. + +"""Regression tests pinning floor versions of security-sensitive transitive deps. + +These tests guard against poetry resolving urllib3 or idna below the floor that +fixes the CVEs referenced in the Phase 1 security review fix plan. +""" + +from __future__ import annotations + +from packaging.version import Version + + +def test_urllib3_minimum_version() -> None: + """urllib3 must be >= 2.0.8 (CVE-2026-44431 DNS rebinding, CVE-2026-44432 proxy confusion).""" + import urllib3 + + assert Version(urllib3.__version__) >= Version("2.0.8"), ( + f"urllib3 {urllib3.__version__} is below 2.0.8 (CVE-2026-44431/44432 fix)" + ) + + +def test_idna_minimum_version() -> None: + """idna must be >= 3.7 (CVE-2026-45409 ReDoS).""" + import idna + + assert Version(idna.__version__) >= Version("3.7"), ( + f"idna {idna.__version__} is below 3.7 (CVE-2026-45409 fix)" + ) From 65d48266a168034dd788ef247ac4fbd2408956bd Mon Sep 17 00:00:00 2001 From: petrsnd Date: Fri, 22 May 2026 19:26:16 -0600 Subject: [PATCH 3/8] samples: document verify=False in AnonymousExample and add TLS Verification README section The anonymous sample uses verify=False so it works out of the box against a dev appliance with a self-signed certificate. Make the insecurity intentional and traceable: - Inline WARNING comment in samples/AnonymousExample.py explaining when verify=False is appropriate and pointing back to the README. - New 'TLS Verification' section in README.md describing the verify= options and the production-safe REQUESTS_CA_BUNDLE / CA-bundle pattern. - Regression test that fails if either the inline guidance or the README section disappears. --- README.md | 23 ++++++++++++++++++ samples/AnonymousExample.py | 5 ++++ tests/test_samples_documentation.py | 36 +++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+) create mode 100644 tests/test_samples_documentation.py diff --git a/README.md b/README.md index 9dd5178..f5c7763 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,29 @@ following methods to establish trust. > The WEBSOCKET_CLIENT_CA_BUNDLE environment variable is only necessary > when working with SignalR. +## TLS Verification + +`SafeguardClient` accepts a `verify` argument that is forwarded to the underlying +`requests` library. It controls how TLS certificate verification is performed: + +- `verify=True` (default) — verify the appliance certificate against the system + trust store. **Use this in production.** +- `verify="/path/to/ca-bundle.pem"` — verify against an explicit CA bundle. Use + this in production when the appliance is signed by a private CA not present in + the system trust store. Equivalent to setting the `REQUESTS_CA_BUNDLE` + environment variable. +- `verify=False` — disable certificate verification entirely. **Never use this + in production.** It exposes the connection to man-in-the-middle attacks and + silently accepts any certificate, including expired or attacker-controlled + ones. + +The samples under [`samples/`](samples/) use `verify=False` so they work out of +the box against a dev/test appliance with a self-signed certificate. Each such +sample carries an inline `WARNING` comment pointing back to this section. When +adapting a sample for production, remove `verify=False` and configure trust via +`REQUESTS_CA_BUNDLE` (and `WEBSOCKET_CLIENT_CA_BUNDLE` if you use SignalR) or +pass an explicit CA bundle path to `verify`. + ## Getting Started > **Note:** Recent versions of Safeguard have Resource Owner Grant (ROG) diff --git a/samples/AnonymousExample.py b/samples/AnonymousExample.py index 06c7733..a942c60 100644 --- a/samples/AnonymousExample.py +++ b/samples/AnonymousExample.py @@ -9,6 +9,11 @@ host = "" print("Connecting anonymously") +# WARNING: verify=False disables TLS certificate verification. This sample uses it +# so it works out of the box against a dev appliance with a self-signed certificate. +# In production, omit `verify=False` and either trust the appliance's CA via the +# REQUESTS_CA_BUNDLE environment variable or pass `verify="/path/to/ca-bundle.pem"`. +# See the "TLS Verification" section in README.md for details. client = SafeguardClient(host, verify=False) print("Getting status") diff --git a/tests/test_samples_documentation.py b/tests/test_samples_documentation.py new file mode 100644 index 0000000..84fdbd5 --- /dev/null +++ b/tests/test_samples_documentation.py @@ -0,0 +1,36 @@ +# Copyright (c) One Identity LLC. All rights reserved. +# Licensed under the Apache License, Version 2.0. + +"""Verify sample code includes production-security guidance for insecure flags.""" + +from __future__ import annotations + +from pathlib import Path + +SAMPLES_DIR = Path(__file__).parent.parent / "samples" +README = Path(__file__).parent.parent / "README.md" + + +def test_anonymous_example_has_tls_warning() -> None: + """AnonymousExample.py uses verify=False but must explain why and how to fix in production.""" + example = SAMPLES_DIR / "AnonymousExample.py" + assert example.exists(), "samples/AnonymousExample.py not found" + content = example.read_text(encoding="utf-8") + + assert "verify=False" in content, "Sample must demonstrate verify=False for dev/test usage" + + # The sample must include at least one of these context cues near the insecure flag + # so a developer copying it knows it is intentional and what to do in production. + cues = ("WARNING", "production", "self-signed", "REQUESTS_CA_BUNDLE") + assert any(cue in content for cue in cues), f"Sample must include guidance about verify=False usage; expected one of {cues}" + + +def test_readme_has_tls_verification_section() -> None: + """README.md must include a TLS verification section that documents verify=.""" + assert README.exists(), "README.md not found" + content = README.read_text(encoding="utf-8") + assert "TLS Verification" in content, "README.md must contain a 'TLS Verification' section" + assert "verify=False" in content, "TLS Verification section must mention verify=False" + assert "REQUESTS_CA_BUNDLE" in content or "CA bundle" in content, ( + "TLS Verification section must reference CA bundle / REQUESTS_CA_BUNDLE for production guidance" + ) From 1839f3b40b0889820b3b6609e78f37915db9f987 Mon Sep 17 00:00:00 2001 From: petrsnd Date: Fri, 22 May 2026 19:33:21 -0600 Subject: [PATCH 4/8] errors: truncate response bodies in exception messages (D-013) Audit of exception/response-rendering paths found that ApiError.from_response, ApiError.from_async_response, and several SafeguardError raises in pkce.py interpolated the full upstream response body verbatim into the human-facing exception message. That body is the form that typically reaches logs, crash reporters, and SIEMs, so it is the leak surface that matters in practice. Per cross-cutting decision D-013 the chosen mitigation is **truncation**, not field-level redaction: substring or regex redaction would wrongly mask legitimate Safeguard payload fields such as PasswordRulesPolicyId, ApiKeyName, RequirePasswordChange, PasswordHistoryDepth, and PrivateKeyFormat. The full body remains available to callers via SafeguardError.response_body for diagnostic use. Changes: - errors.py: add _truncate_for_message helper with a 200-char cap and an explicit '... (truncated, N total chars)' marker. Apply it in both ApiError.from_response and ApiError.from_async_response. - pkce.py: route all four raise-SafeguardError sites that interpolated resp.text through _truncate_for_message. - tests/test_credential_redaction_audit.py: new regression suite covering truncation, marker presence, short-body passthrough, response_body preservation, a static guardrail against future raw {resp.text} interpolations in pkce.py, and the D-013 negative test ensuring legitimate API field names survive intact. --- src/pysafeguard/errors.py | 30 ++++++- src/pysafeguard/pkce.py | 10 +-- tests/test_credential_redaction_audit.py | 107 +++++++++++++++++++++++ 3 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 tests/test_credential_redaction_audit.py diff --git a/src/pysafeguard/errors.py b/src/pysafeguard/errors.py index 500b1c9..299cbff 100644 --- a/src/pysafeguard/errors.py +++ b/src/pysafeguard/errors.py @@ -18,6 +18,30 @@ from requests import Response +# D-013: cap how much of a response body we splice into an exception's human-facing +# message. The full body remains available to callers via :attr:`SafeguardError.response_body` +# for diagnostic use; this limit only bounds what lands in ``str(exc)`` (the form that +# typically reaches logs, crash reporters, and SIEMs). Truncation — not field-level +# redaction — is the chosen mitigation: it never wrongly masks legitimate Safeguard +# payload fields like ``PasswordRulesPolicyId``, ``ApiKeyName``, or +# ``RequirePasswordChange``. +_MAX_BODY_IN_MESSAGE = 200 + + +def _truncate_for_message(body: str | None, limit: int = _MAX_BODY_IN_MESSAGE) -> str: + """Bound a response body for inclusion in a human-readable exception message. + + Returns ``body`` unchanged if it is already at or under ``limit`` characters, + otherwise returns the first ``limit`` characters followed by a + ``... (truncated, N total chars)`` marker so the reader knows it was elided. + """ + if body is None: + return "" + if len(body) <= limit: + return body + return f"{body[:limit]}... (truncated, {len(body)} total chars)" + + class SafeguardError(Exception): """Base exception for all PySafeguard errors. @@ -81,9 +105,9 @@ class ApiError(SafeguardError): @classmethod def from_response(cls, resp: Response) -> ApiError: """Create an ApiError from a sync ``requests.Response``.""" - message = f"{resp.status_code} {resp.reason}: {resp.request.method} {resp.url}\n{resp.text}" - status_code = resp.status_code body = resp.text + message = f"{resp.status_code} {resp.reason}: {resp.request.method} {resp.url}\n{_truncate_for_message(body)}" + status_code = resp.status_code subclass = _STATUS_MAP.get(status_code, cls) return subclass(message, status_code=status_code, response_body=body) @@ -96,7 +120,7 @@ def from_async_response(cls, resp: ClientResponse, body: str) -> ApiError: :param body: The response body text (must be read by the caller with ``await resp.text()`` before calling this method). """ - message = f"{resp.status} {resp.reason}: {resp.method} {resp.url}\n{body}" + message = f"{resp.status} {resp.reason}: {resp.method} {resp.url}\n{_truncate_for_message(body)}" status_code = resp.status subclass = _STATUS_MAP.get(status_code, cls) diff --git a/src/pysafeguard/pkce.py b/src/pysafeguard/pkce.py index 136456a..38d68c5 100644 --- a/src/pysafeguard/pkce.py +++ b/src/pysafeguard/pkce.py @@ -18,7 +18,7 @@ from requests import Response, Session -from .errors import SafeguardError +from .errors import SafeguardError, _truncate_for_message DEFAULT_TIMEOUT = 300 @@ -93,7 +93,7 @@ def get_pkce_token( claims_resp = _rsts_request(session, pkce_base_url + _STEP_GENERATE_CLAIMS, form_data) if claims_resp.status_code != 200: raise SafeguardError( - f"Failed to generate claims: {claims_resp.text}", + f"Failed to generate claims: {_truncate_for_message(claims_resp.text)}", status_code=claims_resp.status_code, response_body=claims_resp.text, ) @@ -164,7 +164,7 @@ def _rsts_request(session: Session, url: str, form_data: dict[str, str]) -> Resp if not (200 <= status < 300): error_message = resp.text.strip() if resp.text.strip() else str(status) raise SafeguardError( - f"rSTS authentication error: {error_message}", + f"rSTS authentication error: {_truncate_for_message(error_message)}", status_code=status, response_body=resp.text, ) @@ -375,7 +375,7 @@ def _post_authorization_code(session: Session, appliance: str, code: str, code_v if not resp.ok: raise SafeguardError( - f"Failed to exchange authorization code: {resp.status_code} {resp.text}", + f"Failed to exchange authorization code: {resp.status_code} {_truncate_for_message(resp.text)}", status_code=resp.status_code, response_body=resp.text, ) @@ -400,7 +400,7 @@ def _post_login_response(session: Session, appliance: str, rsts_token: str, api_ if not resp.ok: raise SafeguardError( - f"Failed to exchange RSTS token: {resp.status_code} {resp.text}", + f"Failed to exchange RSTS token: {resp.status_code} {_truncate_for_message(resp.text)}", status_code=resp.status_code, response_body=resp.text, ) diff --git a/tests/test_credential_redaction_audit.py b/tests/test_credential_redaction_audit.py new file mode 100644 index 0000000..db40ac8 --- /dev/null +++ b/tests/test_credential_redaction_audit.py @@ -0,0 +1,107 @@ +# Copyright (c) One Identity LLC. All rights reserved. +# Licensed under the Apache License, Version 2.0. + +"""Regression tests for D-013 credential / response-body leak audit. + +Per cross-cutting decision D-013 (see security-triage.md §3), the SDK must not +interpolate full upstream response bodies into exception messages. The full +body is still available to callers via :attr:`SafeguardError.response_body`, +but the human-facing ``str(exc)`` rendering — which is what typically lands +in logs, crash reporters, and SIEMs — must be truncated to keep accidental +leakage of secrets, PII, or large bodies bounded. + +This module asserts the truncation on every code path that wraps an HTTP +response into a ``SafeguardError`` or ``ApiError``. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock + +from pysafeguard.errors import ApiError, _MAX_BODY_IN_MESSAGE + + +def _make_response( + status_code: int = 500, reason: str = "Internal Server Error", method: str = "POST", url: str = "https://host/service/core/v4/Endpoint", text: str = "" +) -> MagicMock: + resp = MagicMock() + resp.status_code = status_code + resp.reason = reason + resp.url = url + resp.text = text + resp.request = SimpleNamespace(method=method) + return resp + + +def test_message_truncated_when_body_exceeds_limit() -> None: + """ApiError.from_response message must not contain the body verbatim past the limit.""" + long_body = "A" * (_MAX_BODY_IN_MESSAGE * 4) + err = ApiError.from_response(_make_response(text=long_body)) + rendered = str(err) + # Full body must NOT appear in the human message. + assert long_body not in rendered + # But the underlying response_body attribute keeps it intact for callers. + assert err.response_body == long_body + + +def test_truncated_message_includes_ellipsis_marker() -> None: + """The truncated message should carry an explicit marker so users know it was elided.""" + long_body = "B" * (_MAX_BODY_IN_MESSAGE * 2) + err = ApiError.from_response(_make_response(text=long_body)) + rendered = str(err) + assert "truncated" in rendered.lower() or "..." in rendered + + +def test_short_body_passed_through_verbatim() -> None: + """Bodies at or below the limit must not be molested — they often carry the real error text.""" + short_body = '{"Code": 123, "Message": "Bad input"}' + assert len(short_body) <= _MAX_BODY_IN_MESSAGE + err = ApiError.from_response(_make_response(status_code=400, text=short_body)) + assert short_body in str(err) + + +def test_response_body_attribute_is_full_body() -> None: + """Truncation is a display concern; the raw body is still on the exception.""" + body = "X" * 5000 + err = ApiError.from_response(_make_response(text=body)) + assert err.response_body == body + assert err.has_response + + +def test_pkce_failures_do_not_leak_full_body() -> None: + """The PKCE / RSTS helpers also wrap response bodies; they must use the same truncation.""" + from pysafeguard import pkce as pkce_mod + + # Inspect the source of pkce.py for any unbounded `{...text}` interpolation that + # would bypass the truncation helper. This is a static guardrail. + import inspect + + src = inspect.getsource(pkce_mod) + # Any place that interpolates a raw .text body into an f-string message + # is suspect. The fix funnels through _truncate_for_message, so the only + # acceptable forms are ones that call that helper or use response_body= + # alone (the helper truncates when building messages). + forbidden_patterns = ( + "{resp.text}", + "{claims_resp.text}", + "{primary_resp.text}", + "{mfa_resp.text}", + "{init_resp.text}", + ) + for pat in forbidden_patterns: + assert pat not in src, f"pkce.py interpolates `{pat}` into a message verbatim; route through errors._truncate_for_message instead to honor D-013." + + +def test_truncation_helper_preserves_legitimate_field_names() -> None: + """D-013 negative test: truncation must NOT mask legitimate API body field names. + + The helper is allowed to truncate length, but it must never substring-redact + field names that happen to contain 'password', 'apikey', etc. Those are real + Safeguard payload fields and must round-trip unchanged when the body is short. + """ + legitimate = '{"PasswordRulesPolicyId": 5, "ApiKeyName": "svc", "RequirePasswordChange": true, "PasswordHistoryDepth": 10, "PrivateKeyFormat": "OpenSsh"}' + err = ApiError.from_response(_make_response(status_code=400, text=legitimate)) + rendered = str(err) + for needle in ("PasswordRulesPolicyId", "ApiKeyName", "RequirePasswordChange", "PasswordHistoryDepth", "PrivateKeyFormat"): + assert needle in rendered, f"{needle} must pass through the message unchanged (D-013)" From 33516ded8405f504bb5fd3cac07f925f4c44c074 Mon Sep 17 00:00:00 2001 From: petrsnd Date: Tue, 26 May 2026 14:57:50 -0600 Subject: [PATCH 5/8] test(d013): tripwire exact Password/ApiKey/PrivateKey field-name passthrough (C-PySafeguard-001) --- tests/test_credential_redaction_audit.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/tests/test_credential_redaction_audit.py b/tests/test_credential_redaction_audit.py index db40ac8..cf6c355 100644 --- a/tests/test_credential_redaction_audit.py +++ b/tests/test_credential_redaction_audit.py @@ -100,8 +100,23 @@ def test_truncation_helper_preserves_legitimate_field_names() -> None: field names that happen to contain 'password', 'apikey', etc. Those are real Safeguard payload fields and must round-trip unchanged when the body is short. """ - legitimate = '{"PasswordRulesPolicyId": 5, "ApiKeyName": "svc", "RequirePasswordChange": true, "PasswordHistoryDepth": 10, "PrivateKeyFormat": "OpenSsh"}' + preserved_field_names = ( + "PasswordRulesPolicyId", + "ApiKeyName", + "RequirePasswordChange", + "PasswordHistoryDepth", + "PrivateKeyFormat", + "Password", + "ApiKey", + "PrivateKey", + ) + legitimate = ( + '{"PasswordRulesPolicyId":1,"ApiKeyName":"a","RequirePasswordChange":true,' + '"PasswordHistoryDepth":1,"PrivateKeyFormat":"x","Password":"s",' + '"ApiKey":"k","PrivateKey":"-----BEGIN PRIVATE KEY-----..."}' + ) + assert len(legitimate) <= _MAX_BODY_IN_MESSAGE err = ApiError.from_response(_make_response(status_code=400, text=legitimate)) rendered = str(err) - for needle in ("PasswordRulesPolicyId", "ApiKeyName", "RequirePasswordChange", "PasswordHistoryDepth", "PrivateKeyFormat"): + for needle in preserved_field_names: assert needle in rendered, f"{needle} must pass through the message unchanged (D-013)" From 166884629306c5ead68fbc242b6d4dfa71577ef5 Mon Sep 17 00:00:00 2001 From: petrsnd Date: Tue, 26 May 2026 16:30:03 -0600 Subject: [PATCH 6/8] chore(release): bump version to 8.0.3 for security review --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 96a30ed..7af127e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "poetry.core.masonry.api" [project] name = "pysafeguard" description = "One Identity Safeguard Python Package" -version = "8.0.2" +version = "8.0.3" readme = { file = "README.md", content-type = "text/markdown" } keywords = ["safeguard", "oneidentity"] license = "Apache" From f3e59a2c30a4bbfdce5a1787ee54d70c98a33802 Mon Sep 17 00:00:00 2001 From: petrsnd Date: Wed, 27 May 2026 14:00:52 -0600 Subject: [PATCH 7/8] rollback on some fixes that aren't needed or worth it --- src/pysafeguard/errors.py | 29 +----- src/pysafeguard/pkce.py | 10 +- tests/test_credential_redaction_audit.py | 122 ----------------------- tests/test_dependency_versions.py | 30 ------ tests/test_samples_documentation.py | 36 ------- 5 files changed, 8 insertions(+), 219 deletions(-) delete mode 100644 tests/test_credential_redaction_audit.py delete mode 100644 tests/test_dependency_versions.py delete mode 100644 tests/test_samples_documentation.py diff --git a/src/pysafeguard/errors.py b/src/pysafeguard/errors.py index 299cbff..9a26968 100644 --- a/src/pysafeguard/errors.py +++ b/src/pysafeguard/errors.py @@ -18,29 +18,6 @@ from requests import Response -# D-013: cap how much of a response body we splice into an exception's human-facing -# message. The full body remains available to callers via :attr:`SafeguardError.response_body` -# for diagnostic use; this limit only bounds what lands in ``str(exc)`` (the form that -# typically reaches logs, crash reporters, and SIEMs). Truncation — not field-level -# redaction — is the chosen mitigation: it never wrongly masks legitimate Safeguard -# payload fields like ``PasswordRulesPolicyId``, ``ApiKeyName``, or -# ``RequirePasswordChange``. -_MAX_BODY_IN_MESSAGE = 200 - - -def _truncate_for_message(body: str | None, limit: int = _MAX_BODY_IN_MESSAGE) -> str: - """Bound a response body for inclusion in a human-readable exception message. - - Returns ``body`` unchanged if it is already at or under ``limit`` characters, - otherwise returns the first ``limit`` characters followed by a - ``... (truncated, N total chars)`` marker so the reader knows it was elided. - """ - if body is None: - return "" - if len(body) <= limit: - return body - return f"{body[:limit]}... (truncated, {len(body)} total chars)" - class SafeguardError(Exception): """Base exception for all PySafeguard errors. @@ -105,9 +82,9 @@ class ApiError(SafeguardError): @classmethod def from_response(cls, resp: Response) -> ApiError: """Create an ApiError from a sync ``requests.Response``.""" - body = resp.text - message = f"{resp.status_code} {resp.reason}: {resp.request.method} {resp.url}\n{_truncate_for_message(body)}" + message = f"{resp.status_code} {resp.reason}: {resp.request.method} {resp.url}\n{resp.text}" status_code = resp.status_code + body = resp.text subclass = _STATUS_MAP.get(status_code, cls) return subclass(message, status_code=status_code, response_body=body) @@ -120,7 +97,7 @@ def from_async_response(cls, resp: ClientResponse, body: str) -> ApiError: :param body: The response body text (must be read by the caller with ``await resp.text()`` before calling this method). """ - message = f"{resp.status} {resp.reason}: {resp.method} {resp.url}\n{_truncate_for_message(body)}" + message = f"{resp.status} {resp.reason}: {resp.method} {resp.url}\n{body}" status_code = resp.status subclass = _STATUS_MAP.get(status_code, cls) diff --git a/src/pysafeguard/pkce.py b/src/pysafeguard/pkce.py index 38d68c5..136456a 100644 --- a/src/pysafeguard/pkce.py +++ b/src/pysafeguard/pkce.py @@ -18,7 +18,7 @@ from requests import Response, Session -from .errors import SafeguardError, _truncate_for_message +from .errors import SafeguardError DEFAULT_TIMEOUT = 300 @@ -93,7 +93,7 @@ def get_pkce_token( claims_resp = _rsts_request(session, pkce_base_url + _STEP_GENERATE_CLAIMS, form_data) if claims_resp.status_code != 200: raise SafeguardError( - f"Failed to generate claims: {_truncate_for_message(claims_resp.text)}", + f"Failed to generate claims: {claims_resp.text}", status_code=claims_resp.status_code, response_body=claims_resp.text, ) @@ -164,7 +164,7 @@ def _rsts_request(session: Session, url: str, form_data: dict[str, str]) -> Resp if not (200 <= status < 300): error_message = resp.text.strip() if resp.text.strip() else str(status) raise SafeguardError( - f"rSTS authentication error: {_truncate_for_message(error_message)}", + f"rSTS authentication error: {error_message}", status_code=status, response_body=resp.text, ) @@ -375,7 +375,7 @@ def _post_authorization_code(session: Session, appliance: str, code: str, code_v if not resp.ok: raise SafeguardError( - f"Failed to exchange authorization code: {resp.status_code} {_truncate_for_message(resp.text)}", + f"Failed to exchange authorization code: {resp.status_code} {resp.text}", status_code=resp.status_code, response_body=resp.text, ) @@ -400,7 +400,7 @@ def _post_login_response(session: Session, appliance: str, rsts_token: str, api_ if not resp.ok: raise SafeguardError( - f"Failed to exchange RSTS token: {resp.status_code} {_truncate_for_message(resp.text)}", + f"Failed to exchange RSTS token: {resp.status_code} {resp.text}", status_code=resp.status_code, response_body=resp.text, ) diff --git a/tests/test_credential_redaction_audit.py b/tests/test_credential_redaction_audit.py deleted file mode 100644 index cf6c355..0000000 --- a/tests/test_credential_redaction_audit.py +++ /dev/null @@ -1,122 +0,0 @@ -# Copyright (c) One Identity LLC. All rights reserved. -# Licensed under the Apache License, Version 2.0. - -"""Regression tests for D-013 credential / response-body leak audit. - -Per cross-cutting decision D-013 (see security-triage.md §3), the SDK must not -interpolate full upstream response bodies into exception messages. The full -body is still available to callers via :attr:`SafeguardError.response_body`, -but the human-facing ``str(exc)`` rendering — which is what typically lands -in logs, crash reporters, and SIEMs — must be truncated to keep accidental -leakage of secrets, PII, or large bodies bounded. - -This module asserts the truncation on every code path that wraps an HTTP -response into a ``SafeguardError`` or ``ApiError``. -""" - -from __future__ import annotations - -from types import SimpleNamespace -from unittest.mock import MagicMock - -from pysafeguard.errors import ApiError, _MAX_BODY_IN_MESSAGE - - -def _make_response( - status_code: int = 500, reason: str = "Internal Server Error", method: str = "POST", url: str = "https://host/service/core/v4/Endpoint", text: str = "" -) -> MagicMock: - resp = MagicMock() - resp.status_code = status_code - resp.reason = reason - resp.url = url - resp.text = text - resp.request = SimpleNamespace(method=method) - return resp - - -def test_message_truncated_when_body_exceeds_limit() -> None: - """ApiError.from_response message must not contain the body verbatim past the limit.""" - long_body = "A" * (_MAX_BODY_IN_MESSAGE * 4) - err = ApiError.from_response(_make_response(text=long_body)) - rendered = str(err) - # Full body must NOT appear in the human message. - assert long_body not in rendered - # But the underlying response_body attribute keeps it intact for callers. - assert err.response_body == long_body - - -def test_truncated_message_includes_ellipsis_marker() -> None: - """The truncated message should carry an explicit marker so users know it was elided.""" - long_body = "B" * (_MAX_BODY_IN_MESSAGE * 2) - err = ApiError.from_response(_make_response(text=long_body)) - rendered = str(err) - assert "truncated" in rendered.lower() or "..." in rendered - - -def test_short_body_passed_through_verbatim() -> None: - """Bodies at or below the limit must not be molested — they often carry the real error text.""" - short_body = '{"Code": 123, "Message": "Bad input"}' - assert len(short_body) <= _MAX_BODY_IN_MESSAGE - err = ApiError.from_response(_make_response(status_code=400, text=short_body)) - assert short_body in str(err) - - -def test_response_body_attribute_is_full_body() -> None: - """Truncation is a display concern; the raw body is still on the exception.""" - body = "X" * 5000 - err = ApiError.from_response(_make_response(text=body)) - assert err.response_body == body - assert err.has_response - - -def test_pkce_failures_do_not_leak_full_body() -> None: - """The PKCE / RSTS helpers also wrap response bodies; they must use the same truncation.""" - from pysafeguard import pkce as pkce_mod - - # Inspect the source of pkce.py for any unbounded `{...text}` interpolation that - # would bypass the truncation helper. This is a static guardrail. - import inspect - - src = inspect.getsource(pkce_mod) - # Any place that interpolates a raw .text body into an f-string message - # is suspect. The fix funnels through _truncate_for_message, so the only - # acceptable forms are ones that call that helper or use response_body= - # alone (the helper truncates when building messages). - forbidden_patterns = ( - "{resp.text}", - "{claims_resp.text}", - "{primary_resp.text}", - "{mfa_resp.text}", - "{init_resp.text}", - ) - for pat in forbidden_patterns: - assert pat not in src, f"pkce.py interpolates `{pat}` into a message verbatim; route through errors._truncate_for_message instead to honor D-013." - - -def test_truncation_helper_preserves_legitimate_field_names() -> None: - """D-013 negative test: truncation must NOT mask legitimate API body field names. - - The helper is allowed to truncate length, but it must never substring-redact - field names that happen to contain 'password', 'apikey', etc. Those are real - Safeguard payload fields and must round-trip unchanged when the body is short. - """ - preserved_field_names = ( - "PasswordRulesPolicyId", - "ApiKeyName", - "RequirePasswordChange", - "PasswordHistoryDepth", - "PrivateKeyFormat", - "Password", - "ApiKey", - "PrivateKey", - ) - legitimate = ( - '{"PasswordRulesPolicyId":1,"ApiKeyName":"a","RequirePasswordChange":true,' - '"PasswordHistoryDepth":1,"PrivateKeyFormat":"x","Password":"s",' - '"ApiKey":"k","PrivateKey":"-----BEGIN PRIVATE KEY-----..."}' - ) - assert len(legitimate) <= _MAX_BODY_IN_MESSAGE - err = ApiError.from_response(_make_response(status_code=400, text=legitimate)) - rendered = str(err) - for needle in preserved_field_names: - assert needle in rendered, f"{needle} must pass through the message unchanged (D-013)" diff --git a/tests/test_dependency_versions.py b/tests/test_dependency_versions.py deleted file mode 100644 index 3222aa2..0000000 --- a/tests/test_dependency_versions.py +++ /dev/null @@ -1,30 +0,0 @@ -# Copyright (c) One Identity LLC. All rights reserved. -# Licensed under the Apache License, Version 2.0. - -"""Regression tests pinning floor versions of security-sensitive transitive deps. - -These tests guard against poetry resolving urllib3 or idna below the floor that -fixes the CVEs referenced in the Phase 1 security review fix plan. -""" - -from __future__ import annotations - -from packaging.version import Version - - -def test_urllib3_minimum_version() -> None: - """urllib3 must be >= 2.0.8 (CVE-2026-44431 DNS rebinding, CVE-2026-44432 proxy confusion).""" - import urllib3 - - assert Version(urllib3.__version__) >= Version("2.0.8"), ( - f"urllib3 {urllib3.__version__} is below 2.0.8 (CVE-2026-44431/44432 fix)" - ) - - -def test_idna_minimum_version() -> None: - """idna must be >= 3.7 (CVE-2026-45409 ReDoS).""" - import idna - - assert Version(idna.__version__) >= Version("3.7"), ( - f"idna {idna.__version__} is below 3.7 (CVE-2026-45409 fix)" - ) diff --git a/tests/test_samples_documentation.py b/tests/test_samples_documentation.py deleted file mode 100644 index 84fdbd5..0000000 --- a/tests/test_samples_documentation.py +++ /dev/null @@ -1,36 +0,0 @@ -# Copyright (c) One Identity LLC. All rights reserved. -# Licensed under the Apache License, Version 2.0. - -"""Verify sample code includes production-security guidance for insecure flags.""" - -from __future__ import annotations - -from pathlib import Path - -SAMPLES_DIR = Path(__file__).parent.parent / "samples" -README = Path(__file__).parent.parent / "README.md" - - -def test_anonymous_example_has_tls_warning() -> None: - """AnonymousExample.py uses verify=False but must explain why and how to fix in production.""" - example = SAMPLES_DIR / "AnonymousExample.py" - assert example.exists(), "samples/AnonymousExample.py not found" - content = example.read_text(encoding="utf-8") - - assert "verify=False" in content, "Sample must demonstrate verify=False for dev/test usage" - - # The sample must include at least one of these context cues near the insecure flag - # so a developer copying it knows it is intentional and what to do in production. - cues = ("WARNING", "production", "self-signed", "REQUESTS_CA_BUNDLE") - assert any(cue in content for cue in cues), f"Sample must include guidance about verify=False usage; expected one of {cues}" - - -def test_readme_has_tls_verification_section() -> None: - """README.md must include a TLS verification section that documents verify=.""" - assert README.exists(), "README.md not found" - content = README.read_text(encoding="utf-8") - assert "TLS Verification" in content, "README.md must contain a 'TLS Verification' section" - assert "verify=False" in content, "TLS Verification section must mention verify=False" - assert "REQUESTS_CA_BUNDLE" in content or "CA bundle" in content, ( - "TLS Verification section must reference CA bundle / REQUESTS_CA_BUNDLE for production guidance" - ) From aec7cf4b3534ca0f40b89a1ce0e3d0f10cbd457e Mon Sep 17 00:00:00 2001 From: petrsnd Date: Wed, 27 May 2026 14:02:57 -0600 Subject: [PATCH 8/8] fix: remove stray blank line in errors.py --- src/pysafeguard/errors.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pysafeguard/errors.py b/src/pysafeguard/errors.py index 9a26968..500b1c9 100644 --- a/src/pysafeguard/errors.py +++ b/src/pysafeguard/errors.py @@ -18,7 +18,6 @@ from requests import Response - class SafeguardError(Exception): """Base exception for all PySafeguard errors.