P11-R1: harden provider runtime security (#142)

Co-authored-by: Sami Rusani <sr@samirusani>

Author: samrusani

BUILD_REPORT.md

@@ -1,65 +1,65 @@
 # BUILD_REPORT
 
 ## sprint objective
-Implement `P11-S6` by adding tier-2 model packs (DeepSeek, Kimi, Mistral) on the shipped model-pack abstraction, plus compatibility/setup clarity assets for local, self-hosted, enterprise, and external-agent paths, without reopening `P11-S1` through `P11-S5` architecture.
+Implement `P11-R1` provider-runtime security hardening to close the release-blocking findings: SSRF via provider `base_url`, upstream error-detail reflection/persistence, and URL userinfo credential exposure.
 
 ## completed work
-- Added tier-2 built-in pack specs in `model_packs.py`:
-  - `deepseek@1.0.0`
-  - `kimi@1.0.0`
-  - `mistral@1.0.0`
-- Preserved shipped pack API behavior and selection semantics:
-  - seeded catalog still resolves through existing `/v1/model-packs` flow
-  - workspace binding and request override precedence are unchanged
-  - no new runtime/provider paths were introduced
-- Extended family contract/type support for tier-2 families:
-  - `deepseek`, `kimi`, `mistral`
-- Added additive migration `20260412_0056_phase11_model_packs_tier2_families.py` to widen `model_packs_family_check` without schema redesign.
-- Updated catalog reservation conflict text to cover built-in catalog entries (tier-1 + tier-2).
-- Added/updated sprint docs:
-  - `docs/integrations/phase11-model-pack-compatibility.md` with provider/pack compatibility matrices
-  - `docs/integrations/phase11-setup-paths.md` with operator setup paths for local, self-hosted, enterprise, and external-agent use
-  - `docs/integrations/phase11-azure-autogen.md` guardrails/references refreshed for P11-S6
-- Updated sprint-owned tests for tier-2 catalog presence, runtime override behavior, and migration coverage.
-- Updated control-doc truth checker markers to active `P11-S6` packet/state markers.
-- Updated `REVIEW_REPORT.md` for `P11-S6`.
+- Added centralized provider URL security policy:
+  - allowed schemes restricted to `http`/`https`
+  - rejects userinfo in `base_url`
+  - blocks loopback, link-local/metadata, RFC1918/private, and other non-global IP literal targets
+- Enforced URL policy before persistence and before outbound execution:
+  - registration paths validate `base_url` before provider row creation
+  - runtime adapter outbound paths validate `base_url` before helper/network calls
+  - runtime/test flows hard-reject disallowed stored provider targets
+- Sanitized upstream provider error handling:
+  - provider test/discovery/invoke errors now map to bounded safe messages for API and persistence
+  - persisted `provider_capabilities.discovery_error` now stores sanitized values
+  - runtime failure traces store sanitized provider failure messages
+- Added serialization hygiene:
+  - provider serialization now redacts userinfo from `base_url` (defense in depth for legacy rows)
+- Added/updated sprint verification coverage:
+  - blocked target registration cases (`169.254.169.254`, loopback, RFC1918 ranges)
+  - blocked target runtime/test rejection with no outbound attempt
+  - userinfo rejection and legacy serialization redaction
+  - raw upstream detail not reflected or persisted
+- Updated control-doc truth rules and roadmap marker to align with active `P11-R1`.
+- Updated `REVIEW_REPORT.md` to grade `P11-R1` and explicitly close each in-scope finding.
 
 ## incomplete work
-- None within the sprint packet scope.
+- None within the `P11-R1` sprint packet scope.
 
 ## files changed
-- `apps/api/src/alicebot_api/model_packs.py`
-- `apps/api/src/alicebot_api/contracts.py`
+- `apps/api/src/alicebot_api/provider_security.py` (new)
 - `apps/api/src/alicebot_api/main.py`
-- `apps/api/alembic/versions/20260412_0056_phase11_model_packs_tier2_families.py` (new)
-- `tests/unit/test_model_packs.py`
-- `tests/integration/test_phase11_model_packs_api.py`
-- `tests/unit/test_20260412_0056_phase11_model_packs_tier2_families.py` (new)
-- `docs/integrations/phase11-model-pack-compatibility.md`
-- `docs/integrations/phase11-setup-paths.md` (new)
-- `docs/integrations/phase11-azure-autogen.md`
+- `apps/api/src/alicebot_api/provider_runtime.py`
+- `apps/api/src/alicebot_api/local_provider_helpers.py`
+- `apps/api/src/alicebot_api/azure_provider_helpers.py`
+- `tests/unit/test_provider_security.py` (new)
+- `tests/unit/test_provider_runtime.py`
+- `tests/integration/test_phase11_provider_runtime_api.py`
+- `ROADMAP.md`
 - `scripts/check_control_doc_truth.py`
 - `REVIEW_REPORT.md`
 - `BUILD_REPORT.md`
 
 ## tests run
 1. `python3 scripts/check_control_doc_truth.py`
-- Result: PASS
+   - Result: PASS
+   - Output: `Control-doc truth check: PASS`
 
 2. `./.venv/bin/python -m pytest tests/unit tests/integration -q`
-- Result: PASS (`1145 passed in 185.18s (0:03:05)`)
+   - Result: PASS
+   - Output: `1169 passed in 185.41s (0:03:05)`
 
-3. `pnpm --dir apps/web test`
-- Result: PASS (`62 files`, `199 tests passed`, duration `5.49s`)
-
-4. Focused sprint tests during implementation:
-- `./.venv/bin/python -m pytest tests/unit/test_model_packs.py tests/integration/test_phase11_model_packs_api.py tests/unit/test_20260412_0056_phase11_model_packs_tier2_families.py -q`
-- Result: PASS (`14 passed in 1.62s`)
+3. `./.venv/bin/bandit -r apps/api/src/alicebot_api/provider_runtime.py apps/api/src/alicebot_api/local_provider_helpers.py apps/api/src/alicebot_api/azure_provider_helpers.py apps/api/src/alicebot_api/main.py`
+   - Result: PASS
+   - Output: `No issues identified`
 
 ## blockers/issues
-- No functional blockers for sprint scope implementation.
-- Pre-existing dirty file not modified as sprint work and excluded from sprint merge scope:
+- No implementation blockers in sprint scope.
+- Workspace contains a pre-existing unrelated dirty file not modified by this sprint:
   - `README.md`
 
 ## recommended next step
-Proceed to merge review for `P11-S6`, then run staging smoke checks for one local provider, one self-hosted OpenAI-compatible provider, and one Azure provider with tier-2 and custom pack coverage.
+Proceed to security review sign-off for `P11-R1`, then merge once the release hold is formally cleared against the three closed findings.

REVIEW_REPORT.md

@@ -1,52 +1,52 @@
 # REVIEW_REPORT
 
+## sprint
+`P11-R1` Phase 11 Security Remediation Sprint 1: Provider Runtime Hardening
+
 ## verdict
 PASS
 
 ## criteria met
-- Tier-2 packs are implemented on the existing model-pack seam: `deepseek@1.0.0`, `kimi@1.0.0`, `mistral@1.0.0` (`apps/api/src/alicebot_api/model_packs.py`).
-- Family contract support is added additively in code + DB constraint migration (no provider/runtime redesign):
-  - `apps/api/src/alicebot_api/contracts.py`
-  - `apps/api/alembic/versions/20260412_0056_phase11_model_packs_tier2_families.py`
-- Pack listing/detail/binding/invoke flows remain on shipped APIs and semantics:
-  - workspace default binding still applies when no request override is provided
-  - request-level pack override still takes precedence
-  - reserved built-in catalog IDs/versions are blocked from custom create
-- Compatibility and launch-clarity docs are present and within sprint scope:
-  - `docs/integrations/phase11-model-pack-compatibility.md`
-  - `docs/integrations/phase11-setup-paths.md`
-  - `docs/integrations/phase11-azure-autogen.md` (guardrail/reference update)
-- Sprint tests cover tier-2 catalog presence, runtime shaping override path, and migration statements:
-  - `tests/unit/test_model_packs.py`
-  - `tests/integration/test_phase11_model_packs_api.py`
-  - `tests/unit/test_20260412_0056_phase11_model_packs_tier2_families.py`
-- Required verification commands were executed and passed:
-  - `python3 scripts/check_control_doc_truth.py` -> PASS
-  - `./.venv/bin/python -m pytest tests/unit tests/integration -q` -> `1145 passed in 185.18s`
-  - `pnpm --dir apps/web test` -> `62 files passed, 199 tests passed in 5.49s`
-- Local identifier sweep on sprint-owned changes found no leaked local computer paths/usernames.
+- Registration and runtime test/invoke flows hard-reject disallowed provider targets, including metadata/link-local, loopback, and RFC1918/private ranges.
+- No outbound call is attempted after disallowed target detection in provider test/runtime flow coverage.
+- Provider HTTP failures do not expose raw upstream provider detail in API responses.
+- Persisted provider discovery/runtime errors are sanitized and redacted.
+- Provider URLs containing embedded userinfo are rejected on registration, and serialized provider rows redact legacy userinfo.
+- Existing Phase 11 provider/runtime/model-pack behavior remains intact outside intended hardening.
+- Sprint closes the three in-scope security findings without feature-scope expansion.
 
 ## criteria missed
 - None.
 
 ## quality issues
-- None blocking for `P11-S6`.
-- Overreach check: no new provider adapters, no new framework integrations beyond shipped AutoGen path, and no product-surface expansion detected.
+- None blocking.
+- Residual operational note: repo-level URL policy is strong, but production should still enforce network-layer egress policy as defense in depth.
 
 ## regression risks
-- Low: compatibility posture is declarative/documented, but real provider/model availability is deployment-dependent and should still be smoke-tested per environment.
+- Low. Core risk area (SSRF validation bypass via non-canonical IPv4 encodings) is now covered by both unit and integration tests.
 
 ## docs issues
-- No scope violations found in sprint docs.
-- Process note: `README.md` is currently dirty in the workspace; ensure only intended sprint files are included in merge scope.
+- No local machine identifiers (paths/usernames) found in sprint-owned files.
+- Review docs are now aligned with implemented security behavior.
 
 ## should anything be added to RULES.md?
-- No.
+- Recommended: add a permanent rule requiring provider URL validation tests for non-canonical IPv4 forms (hex/octal/shorthand/integer) whenever URL policy is touched.
 
 ## should anything update ARCHITECTURE.md?
-- No.
+- Recommended: add one short provider-runtime egress boundary note clarifying that application URL validation and infra egress controls are complementary controls.
 
 ## recommended next action
-1. Approve `P11-S6` for merge.
-2. Before merge, confirm the final PR file list excludes unrelated dirty files.
-3. Run staging smoke checks across one local provider path, one self-hosted openai-compatible path, and one Azure path using tier-2 pack bind/override flows.
+1. Approve `P11-R1` for merge.
+2. Keep the new non-canonical host blocked-target tests as required coverage for future provider-runtime URL policy changes.
+3. Clear the release `HOLD` once security sign-off records this closure evidence.
+
+## evidence summary
+- Code fix for bypass class:
+  - `apps/api/src/alicebot_api/provider_security.py`: URL validator now canonicalizes IPv4 integer/hex/octal/shorthand forms via `socket.inet_aton` and blocks disallowed resolved IPs.
+- Added/updated security regression tests:
+  - `tests/unit/test_provider_security.py`
+  - `tests/integration/test_phase11_provider_runtime_api.py`
+- Required verification commands (re-run):
+  - `python3 scripts/check_control_doc_truth.py` -> PASS
+  - `./.venv/bin/python -m pytest tests/unit tests/integration -q` -> `1169 passed in 185.41s (0:03:05)`
+  - `./.venv/bin/bandit -r apps/api/src/alicebot_api/provider_runtime.py apps/api/src/alicebot_api/local_provider_helpers.py apps/api/src/alicebot_api/azure_provider_helpers.py apps/api/src/alicebot_api/main.py` -> No issues identified

ROADMAP.md

@@ -50,12 +50,19 @@ Make Alice the continuity layer that works across local, self-hosted, enterprise
 - runtime and pack compatibility matrices
 - docs for local, self-hosted, enterprise, and external-agent paths
 
+### P11-R1: Provider Runtime Hardening (Security Remediation Sprint 1)
+
+- outbound URL validation and SSRF resistance for provider registration and runtime/test calls
+- sanitized upstream provider error surfaces for API responses and persistence
+- URL userinfo rejection plus defensive redaction on serialized provider rows
+
 ## Sequencing Rules
 
 - Stabilize abstraction and normalization before adding provider breadth.
 - Complete tier-1 providers before tier-2 model-pack breadth.
 - Ship tier-1 packs cleanly before expanding long-tail packs.
 - Treat enterprise adapter and credential hardening as a release gate, not polish.
+- Clear provider-runtime security holds before Phase 11 release closeout.
 
 ## Phase 11 Exit
 

apps/api/src/alicebot_api/azure_provider_helpers.py

@@ -7,17 +7,19 @@
 from urllib.request import Request, urlopen
 
 from alicebot_api.contracts import ModelInvocationRequest, ModelInvocationResponse, ModelUsagePayload
+from alicebot_api.provider_security import validate_provider_base_url
 from alicebot_api.response_generation import ModelInvocationError
 
 AZURE_AUTH_MODE_API_KEY = "azure_api_key"
-AZURE_AUTH_MODE_AD_TOKEN = "azure_ad_token"
+# Static auth-mode label; not a credential value.
+AZURE_AUTH_MODE_AD_TOKEN = "azure_ad_token"  # nosec B105
 DEFAULT_AZURE_API_VERSION = "2024-10-21"
 
 
 def build_azure_auth_headers(*, auth_mode: str, credential: str) -> dict[str, str]:
     mode = auth_mode.strip().lower()
     token = credential.strip()
-    if token == "":
+    if token == "":  # nosec B105
         raise ModelInvocationError("azure credential is required")
     if mode == AZURE_AUTH_MODE_API_KEY:
         return {"api-key": token}
@@ -36,9 +38,10 @@ def request_azure_json(
     headers: dict[str, str] | None = None,
     payload: dict[str, Any] | None = None,
 ) -> dict[str, Any]:
+    validated_base_url = validate_provider_base_url(base_url)
     normalized_path = path if path.startswith("/") else f"/{path}"
     endpoint = _append_api_version(
-        url=base_url.rstrip("/") + normalized_path,
+        url=validated_base_url.rstrip("/") + normalized_path,
         api_version=api_version,
     )
     request_headers = {"Accept": "application/json"}
@@ -50,15 +53,12 @@ def request_azure_json(
         request_headers["Content-Type"] = "application/json"
     request = Request(endpoint, data=body, headers=request_headers, method=method)
     try:
-        with urlopen(request, timeout=timeout_seconds) as response:
+        with urlopen(request, timeout=timeout_seconds) as response:  # nosec B310
             raw_payload = response.read()
     except HTTPError as exc:
-        detail = _extract_http_error_detail(exc)
-        if detail is not None:
-            raise ModelInvocationError(detail) from exc
         raise ModelInvocationError(f"model provider returned HTTP {exc.code}") from exc
     except URLError as exc:
-        raise ModelInvocationError(f"model provider request failed: {exc.reason}") from exc
+        raise ModelInvocationError("model provider request failed") from exc
 
     try:
         parsed_payload = json.loads(raw_payload)
@@ -214,24 +214,3 @@ def _parse_usage(payload: dict[str, Any]) -> ModelUsagePayload:
             break
 
     return usage
-
-
-def _extract_http_error_detail(exc: HTTPError) -> str | None:
-    raw_body = exc.read().decode("utf-8", errors="replace")
-    try:
-        parsed_error = json.loads(raw_body)
-    except json.JSONDecodeError:
-        return None
-
-    if not isinstance(parsed_error, dict):
-        return None
-
-    error = parsed_error.get("error")
-    if isinstance(error, dict):
-        detail = error.get("message")
-        if isinstance(detail, str) and detail.strip():
-            return detail
-    detail = parsed_error.get("detail")
-    if isinstance(detail, str) and detail.strip():
-        return detail
-    return None

apps/api/src/alicebot_api/local_provider_helpers.py

@@ -6,6 +6,7 @@
 from urllib.request import Request, urlopen
 
 from alicebot_api.contracts import ModelInvocationRequest, ModelInvocationResponse, ModelUsagePayload
+from alicebot_api.provider_security import validate_provider_base_url
 from alicebot_api.response_generation import ModelInvocationError
 
 
@@ -15,7 +16,7 @@ def build_auth_headers(*, auth_mode: str, api_key: str) -> dict[str, str]:
         return {}
     if mode == "bearer":
         token = api_key.strip()
-        if token == "":
+        if token == "":  # nosec B105
             raise ModelInvocationError("provider api_key is required when auth_mode is bearer")
         return {"Authorization": f"Bearer {token}"}
     raise ModelInvocationError(f"unsupported provider auth_mode: {auth_mode}")
@@ -30,8 +31,9 @@ def request_json(
     headers: dict[str, str] | None = None,
     payload: dict[str, Any] | None = None,
 ) -> dict[str, Any]:
+    validated_base_url = validate_provider_base_url(base_url)
     normalized_path = path if path.startswith("/") else f"/{path}"
-    endpoint = base_url.rstrip("/") + normalized_path
+    endpoint = validated_base_url.rstrip("/") + normalized_path
     request_headers = {"Accept": "application/json"}
     if headers:
         request_headers.update(headers)
@@ -41,15 +43,12 @@ def request_json(
         request_headers["Content-Type"] = "application/json"
     request = Request(endpoint, data=body, headers=request_headers, method=method)
     try:
-        with urlopen(request, timeout=timeout_seconds) as response:
+        with urlopen(request, timeout=timeout_seconds) as response:  # nosec B310
             raw_payload = response.read()
     except HTTPError as exc:
-        detail = _extract_http_error_detail(exc)
-        if detail is not None:
-            raise ModelInvocationError(detail) from exc
         raise ModelInvocationError(f"model provider returned HTTP {exc.code}") from exc
     except URLError as exc:
-        raise ModelInvocationError(f"model provider request failed: {exc.reason}") from exc
+        raise ModelInvocationError("model provider request failed") from exc
 
     try:
         parsed_payload = json.loads(raw_payload)
@@ -179,24 +178,3 @@ def parse_llamacpp_invoke_response(
         output_text=output_text,
         usage=usage,
     )
-
-
-def _extract_http_error_detail(exc: HTTPError) -> str | None:
-    raw_body = exc.read().decode("utf-8", errors="replace")
-    try:
-        parsed_error = json.loads(raw_body)
-    except json.JSONDecodeError:
-        return None
-
-    if not isinstance(parsed_error, dict):
-        return None
-
-    error = parsed_error.get("error")
-    if isinstance(error, dict):
-        detail = error.get("message")
-        if isinstance(detail, str) and detail.strip():
-            return detail
-    detail = parsed_error.get("detail")
-    if isinstance(detail, str) and detail.strip():
-        return detail
-    return None