fix(#191): validate credential-template provider so an unknown value can't silently inject nothing

[jgruberf5]

Summary

POST/PUT /api/credential-templates accepted any string as provider. Credential resolution only ever matches a small set of literals (aws, ibm, gcp, azure, ssh), so a natural misspelling like provider="ibmcloud" was stored, read back looking healthy, and then matched no branch in the resolver — the template silently injected nothing and the deploy fell through to global .env creds. The failure surfaced far away as an opaque Terraform "BearerToken property is required" error.

Root cause

provider was never validated at create/update, and the canonical set it must belong to was implicit — scattered across the resolver's if template.provider == ... branches.

Fix

• Add SUPPORTED_PROVIDERS = {aws, gcp, azure, ibm, ssh} as the single source of truth in credential_template_service.py, documented against each consumer that injects/resolves credentials (AWS/IBM env injection, GCP SA JSON, Azure engine_router, SSH tunnel). This is exactly the four cloud providers the UI offers plus the legacy ssh provider.
• Validate provider in create_template and update_template (service layer — covers every caller) → BadRequestError (400) with an enumerated message.
• Add matching Pydantic validation on the create/update route models so the API boundary returns a clean 422 naming the bad value and the supported set, before the service is reached. Update-time validation only fires when provider is actually being changed.

What the tests lock

• The exact ibmcloud misspelling from the report is rejected at both the service (400) and route (422) layers, with an enumerated message.
• Every canonical provider (aws, gcp, azure, ibm, ssh) is still accepted.
• An update cannot switch a healthy template onto a no-op provider; omitting provider on update leaves it untouched.
• A resolver-contract test documents that a pre-existing ibmcloud row injects no IBM credentials — the exact silent no-op the validation now prevents from being created.
• Mutation-tested: reverting the guards reds 5 tests.

Verification: pytest on the three affected test files → 108 passed; ruff check on all changed files → clean.

Closes #191

[jgruberf5]

Self-review (cold, adversarial) — verdict: no blocker, no major

An independent cold auditor reviewed this PR against issue #191, executing the code rather than eyeballing. Full affected suite: 108 passed; the guard tests were mutation-checked (neutralize validate_provider + the route validators → exactly 5 guard tests red, 7 survive — non-vacuous).

Held under attack:

• Canonical set is correct + complete — traced every consumer that branches on template.provider: aws/ibm → terraform-env injection (credentials_service), gcp → get_gcp_service_account_info, azure → engine_router AKS token, ssh → tunnel/test. SUPPORTED_PROVIDERS = {aws,gcp,azure,ibm,ssh} matches exactly — no valid template is wrongly rejected, no injecting provider is missing. UI TEMPLATE_PROVIDER_OPTIONS is a subset (no drift).
• Case/whitespace — template.provider is never lower/trimmed on read or write, so the guard (not in {...}) and the resolver (== 'ibm') agree; "IBM"/" ibm " are rejected and would inject nothing — consistent.
• No existing flow broken — no seed/route/migration creates an out-of-set provider; create_template is the single choke point.
• Error contract — messages enumerate sorted(SUPPORTED_PROVIDERS) (derived, not hardcoded → no drift).

Findings (all minor/informational):

• M3 (fixed, 79f08cf) — the SUPPORTED_PROVIDERS comment overstated azure/gcp as general "credential resolution"; they inject only a post-provision kubeconfig token, not terraform-env credentials. Corrected the comment and noted that credential-templates: provider is unvalidated, so provider="ibmcloud" silently injects no credentials #191's "looks healthy, injects nothing" class fully closes for aws/ibm (terraform-env) while azure/gcp are consumed only for cluster access.
• M1 (accepted, out of scope) — a pre-existing bad-provider row isn't self-healed on an update that doesn't touch provider. Cannot create new bad data; issue scope is create-time rejection.
• M2 (theoretical) — route returns 422 (Pydantic), direct service callers get 400; the two shapes never collide on the HTTP boundary.

Net: the fix is sound and non-vacuous; the one actionable item (M3 comment) is fixed.

[bonnyr-f5]

Review discipline pass — verdict: REVISE

Two independent cold audits (clean context, no prior review threads) plus an invariant sweep. Everything below was verified by execution, not inspection. Reviewed at 79f08cf.

What holds up

• The canonical set is right. Swept every consumer branching on template.provider: aws (credentials_service.py:190), ibm (:253), gcp (:373), azure (execution/engine_router.py:920), ssh (credential_template_service.py:381). No member lacks a consumer; no consumer sits outside the set. The ("ibm","ibmcloud","roks") tuple at cluster_management_service.py:578 is cluster.cloud_provider — a different vocabulary, not a conflict.
• Create really is a choke point. Two writers repo-wide, both gated.
• The guards are non-vacuous. Reproduced the mutation check independently: neutralizing validate_provider plus both route validators reds exactly 5 tests (103 pass). 108/108 green otherwise; ruff clean on all five files.
• No concurrent-writer collision with fix(#194): enqueue cluster inventory sync on registration and make a resync reliably triggerable #200 / fix(#195): stream opentofu task logs incrementally and expose log fields in the task list #201 / fix(#202): count Multus pods in the DaemonSet's namespace so OpenShift (openshift-multus) reports correctly #203; no migration in this PR.

Major

1. Constrain the field type instead of validating it in a model_validator.

A model-level ValueError raised from @model_validator(mode="after") causes Pydantic to attach the entire request body to the error, and no RequestValidationError handler is registered (backend/main.py:350-352). On a credential-template payload that body carries plaintext secrets. The reachable consumers make it visible: mcp-server/src/bnk_forge_mcp/client.py:184 falls through to json.dumps because detail is a list, and frontend-v2/src/lib/error-handler.ts:87 stringifies it into a toast.

To be clear about scope: this PR does not introduce that mechanism — the pre-existing IBM-key-required raise at backend/routes/credential_templates.py:78 already behaves this way on staging. What this PR adds is a new, high-frequency trigger on the one payload that always carries a key. Reproduction details and the affected-version assessment sent privately rather than posted here, since the mechanism is live outside this branch.

Typing the field closes it — verified: the error becomes field-scoped ("input":"ibmcloud", no body echo) and the message still enumerates the set.

2. INV-3: the constraint never reaches the API contract.

The repo's own type-generation strategy maps Literal[...] to an OpenAPI enum and on into the generated client. A model_validator produces no schema. From the committed spec:

field	backend/openapi.json
F5CredentialCreate.auth_type (f5_devices.py:73)	{"type":"string","enum":["password","token"]}
CredentialTemplateCreate.provider (this PR)	{"type":"string"}

routes/f5_devices.py:73/:82 is the exact create/update-pair idiom, in the same directory, for a constrained optional-on-update field.

Consequences: frontend-v2/src/types/api-generated.ts still declares provider: string; the frontend keeps a second hand-written list at frontend-v2/src/components/settings/CredentialTemplates.tsx:69-74 that is already divergent (four entries, no ssh); and the MCP tool signature still advertises provider: str (mcp-server/src/bnk_forge_mcp/tools/cloud_auth.py:216). Because this PR touches no openapi.json, the type-drift gate stays green precisely because the constraint never entered the schema.

Findings 1 and 2 share one fix, which deletes lines rather than adding them:

# CredentialTemplateBase
provider: Literal["aws", "azure", "gcp", "ibm", "ssh"]
# CredentialTemplateUpdate
provider: Literal["aws", "azure", "gcp", "ibm", "ssh"] | None = None

then regenerate openapi.json + api-generated.ts and type TEMPLATE_PROVIDER_OPTIONS against the generated union so the frontend list can no longer drift.

Minor

3. PUT {"provider": null} returns HTTP 500. Reproduced end to end. provider: str | None accepts null; the route guard's self.provider is not None skips; model_dump(exclude_unset=True) still includes {"provider": None}; the service guard's update_data.get("provider") is not None skips; the generic loop at credential_template_service.py:374-376 assigns it; flush() violates nullable=False.

The comment at credential_template_service.py:352 — "A None/absent provider leaves it unchanged" — is therefore inaccurate: None is assigned, not ignored. The predicate wants presence, not non-nullness:

if "provider" in update_data:
    validate_provider(update_data["provider"])

Worth noting the published contract advertises null as valid here: CredentialTemplateUpdate.provider is {"anyOf":[{"type":"string"},{"type":"null"}]}. POST with null, PUT with "ibmcloud", and PUT with "" all correctly return 422 — null-on-update is the only gap.

4. The rule is expressed three times and the messages have already diverged. routes/credential_templates.py:70-73 and :124-127 re-implement the membership test rather than calling validate_provider, and use different wording from credential_template_service.py:57-62. The service's better-worded BadRequestError is unreachable in production — the Pydantic model always rejects first, and the only non-test instantiation is routes/credential_templates.py:189/:198. Both message strings are now locked in by tests, so they will drift independently.

5. Issue #191's own row is neither repaired nor surfaced. No migration, backfill, or detection. A stored provider="ibmcloud" row still reads back healthy and still injects nothing. The operator cannot fix it through the UI: the provider Select is disabled={!!editingTemplate} (CredentialTemplates.tsx:425) and handleUpdate never sends provider. "Test" on such a row reports "only supported for AWS and SSH", which reads as an unsupported feature rather than a corrupt row. A one-line startup check (SELECT id, name, provider FROM cloud_credential_templates WHERE provider NOT IN (...)) or a serialized provider_supported: false flag would close the loop this PR opens.

6. Vacuous assertion. backend/tests/integration/test_routes_credential_templates.py:142: assert "ibm" in detail is satisfied by the offender "ibmcloud" itself, so it would still pass if the supported-set rendered empty. Assert on a non-substring such as "azure" or "ssh", as the service-level test correctly does.

Nits

• No case normalization: provider="AWS" now 422s, while the sibling cloud_provider namespace canonicalizes via the existing backend/utils/provider_config.py:15 helper. validate_provider(normalize_cloud_provider(p)) would close the case class too. Defensible either way — flagging because credential-templates: provider is unvalidated, so provider="ibmcloud" silently injects no credentials #191's root cause was a user typing the natural spelling.
• test_misspelled_ibmcloud_provider_injects_nothing passes unchanged with both guards deleted. Its docstring is honest that it locks the resolver contract, so this is only a note not to count it toward guard coverage.

Out of scope — worth its own issue

backend/core/errors.py:652-664: the IntegrityError -> ConflictError (409) mapping is dead code. The raise ConflictError(...) sits inside a try whose except (ImportError, AppError) catches it; e is still the original IntegrityError, so isinstance(e, AppError) is False and it falls through to a generic 500. That is why finding 3 surfaces as 500 rather than the 409 the docstring promises, and it affects every route using handle_route_errors.

---

Verdict REVISE, not BLOCK. The fix is correct, the tests are real, and #191's create path genuinely closes. Findings 1 and 2 collapse into a single type change that is already this codebase's idiom.

[jgruberf5]

Round-2 response @ 1ad1099 — REVISE items addressed

Findings 1 + 2 (Major — body-leak + INV-3 contract), one fix: replaced the provider check in the route @model_validators with a Literal field type — CredentialTemplateBase.provider: Literal["aws","azure","gcp","ibm","ssh"] and ...Update: Literal[...] | None = None (the region + IBM-key checks stay).

• Body-leak closed, mutation-verified: reverting to the model_validator-raise reproduced the leak — the 422 input carried the full body including ibmcloud_api_key: 'pltxt-…'; with the Literal the error is field-scoped (input:"ibmcloud", enumerates the set) and the secret is absent from the response (asserted in-test).
• INV-3 closed: provider is now {"type":"string","enum":[…]} in openapi.json (regenerated with a requirements.txt-only CI-matching venv; --check passes); api-generated.ts regenerated to the union; and TEMPLATE_PROVIDER_OPTIONS is now satisfies the generated provider union, so the frontend list can no longer drift (invalid entry = compile error). ssh stays intentionally out of the create UI but is type-permitted.

Minor 3 (PUT {"provider": null} → 500): the update guard now drops an explicit-null/absent provider before the assignment loop (leaves it unchanged, matching the corrected comment) so it can't write None into the non-nullable column; new test test_update_with_explicit_null_provider_leaves_it_unchanged (mutation-verified). Also de-vacuumed the enumerated-message assertion (now asserts non-substrings azure/ssh).

Verified: 109 passed, ruff clean, --check + tsc --noEmit green.

Consciously deferred (your lower Minors, acknowledged not missed): the provider set now lives as the schema Literal (the contract source of truth) with the service SUPPORTED_PROVIDERS kept in lockstep as defense for direct callers, and the FE list typed against the generated union — I did not further collapse the three into one symbol (finding 4). A pre-existing bad-provider row is still not self-repaired (finding 5) — create-time rejection is this PR's scope. The pre-existing IBM-key-required model_validator raise is unchanged (you noted it's live on staging, out of scope here). Re-requesting.

[jgruberf5]

CI status note

The code review above stands. The only red CI on this PR is the two repo-wide P4 security gates:

• P4 · Docker Build + Scan — Trivy CRITICAL CVE-2026-56854 (golang.org/x/crypto/ssh auth bypass) in the bundled helm 3.20.0 binary.
• P4 · Security Audit — gitpython 3.1.58 (PYSEC-2026-3785/86/87/88).

Both are environmental and repo-wide, not caused by this PR's code: the advisory/vuln DBs updated after staging last audited clean on 2026-08-24, so every open PR (and staging itself, if re-run) is red on them. Both are fixed in #215 (a documented .trivyignore suppression for the un-fixable-yet helm CVE + a gitpython patch bump). This PR goes green on P4 once #215 merges to staging and this branch rebases.

All P1/P2/P3 gates are green. Awaiting further review.

https://claude.ai/code/session_01UCsZXDxBsWV2s4kT47DwDW