Skip to content

Admin settings secret resolution is asymmetric: nested-path secrets can't use the route-level resolve #1429

Description

@paullizer

Summary

Admin settings credentials are resolved in two different places depending on where the value is stored, and the split is invisible from the field declaration. A flat secret is resolved in the V2 route; a secret declared with paths cannot be, and is handled in the schema normalizer instead. Anyone adding a nested secret field has to know this, and nothing enforces it.

This is not currently a bug — both paths work — but it is the kind of asymmetry that rots. Filing it rather than unifying it now, because the fix touches shared code that several admin-settings PRs are queued against.

Why the two paths exist

resolve_admin_settings_secret_value(field_name, submitted, existing) in admin_settings_secret_utils.py swaps a submitted placeholder back for the stored credential, so an untouched secret survives a save. route_backend_v2.py applies it over the submitted keys in the PATCH.

That pass works by settings key. A field declaring paths is folded into its containing object by _apply_nested_paths in admin_settings_fields.py before the route sees the result, so it never arrives as a key of its own. The Web Search Foundry client secret is the live case:

Field key web_search_foundry_client_secret
Declared paths web_search_agent.other_settings.azure_ai_foundry.client_secret
What the route sees web_search_agent (a whole object)

So the route cannot reach it. Without something else handling it, an untouched placeholder would be folded into the container and written over the real credential — saving any unrelated Web Search setting would destroy the stored secret.

The current handling is in _normalize_field_value: a submitted placeholder is dropped only when the field declares paths. Dropping is sufficient because _apply_nested_paths rebuilds the container from stored settings, so the stored secret is carried through untouched. Flat secrets are left entirely to the route.

Why it's worth unifying

  1. The rule is undiscoverable. Nothing tells the author of the next nested secret field that declaring paths is what makes it safe. Declaring a nested secret without paths, or resolving it in the route by field key, silently does nothing.
  2. resolve_admin_settings_secret_value already supports it. It reads through get_nested_setting_value, which takes a dotted path. The helper is capable; only the caller lacks the path.
  3. Two overlapping redaction lists. The GET redacts through both ADMIN_SETTINGS_NESTED_SECRET_FIELDS (a hand-maintained tuple that already lists the Foundry path) and get_secret_storage_paths() (derived from the schema). Both cover this key today. The derived one generalizes; the tuple does not.

Suggested fix

Resolve secrets by storage location rather than by settings key, before _apply_nested_paths folds anything:

  • Have the route ask the schema for field key -> storage path for every declared secret.
  • Call resolve_admin_settings_secret_value(storage_path, submitted, settings) for each, so nested and flat secrets take the same route-level path.
  • Remove the paths exception from _normalize_field_value.
  • Consider deriving ADMIN_SETTINGS_NESTED_SECRET_FIELDS from the schema's declared storage paths instead of maintaining it by hand, so declaring a nested secret protects it on read without a second edit.

Then add a test asserting that every schema-declared secret, flat or nested, survives a save in which it was not touched. That is the property that actually matters and it currently has no single test covering both shapes.

Files involved

  • application/single_app/admin_settings_secret_utils.pyresolve_admin_settings_secret_value, ADMIN_SETTINGS_NESTED_SECRET_FIELDS, get_admin_settings_api_secret_fields
  • application/single_app/admin_settings_fields.py_normalize_field_value (the paths exception), _apply_nested_paths, get_secret_storage_paths
  • application/single_app/route_backend_v2.py_redact_admin_settings_for_v2, the PATCH resolve pass

Context

Found while merging #1425 (Knowledge admin settings V2) against the Security (#1421) and Chat (#1422) work, which introduced the shared mechanism. Discussed with the PR merge orchestration session, which agreed the unification is the better end state but should not happen inside a merge resolve with PRs queued behind it.

Refs #1425

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions