Skip to content

fix(daemon): harden provider endpoints and document credential aggregation (A1/A3)#82

Open
sudhirverma wants to merge 4 commits into
mainfrom
AAP-82836
Open

fix(daemon): harden provider endpoints and document credential aggregation (A1/A3)#82
sudhirverma wants to merge 4 commits into
mainfrom
AAP-82836

Conversation

@sudhirverma

@sudhirverma sudhirverma commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes AAP-82836 (findings A1 / A3): credential aggregation blast radius and malicious/writable provider endpoint risk for Enterprise, Security-Conscious, and air-gapped operators.

Depends on auth + config-write hardening already on main (AAP-82788, AAP-82832, AAP-82833 / DR-030, DR-036, DR-037).

Finding A1 — Credential aggregation

  • Document blast radius vs per-extension storage and persona-specific operator guidance in docs/CONFIGURATION.md.
  • Keep secret APIs auth-gated; HTTP secret list returns presence (hasValue) only.
  • Emit [Audit] secret changed on set/delete (never log values).
  • DR-039 records deferral of per-secret encryption-at-rest / process isolation.

Finding A3 — Provider supply chain + malicious endpoints

  • @ai-sdk/* packages remain a fixed in-code allowlist (PROVIDER_LOADERS); unknown engine rejected on configure / config write.
  • Shared scheme/host policy for base_url (http/https only, no URL userinfo, http limited to loopback unless allowlisted / allow_insecure_provider_http).
  • Optional server.allowed_provider_hosts for enterprise/air-gapped gateways (also enforced on CoreState.addProvider).
  • Emit [Audit] provider endpoint changed on successful endpoint updates.
  • Anonymous provider/secret configure remains denied when HTTP auth is enabled.

Code / tests

  • New provider-endpoint policy helpers + unit tests.
  • Wired through HTTP configure / POST /api/config / discover-models and gRPC ConfigureProvider / UpdateConfig / DiscoverModels.
  • CoreState.addProvider honors server.allowed_provider_hosts + audits (core-add).
  • E2E: tests/integration/provider-endpoint-security.test.ts (unauth denied, bad endpoint rejected, allowed endpoint audited, unknown engine rejected).

Test plan

  • npm testprovider-endpoint / secrets-audit / config-schema / provider-endpoint-security / state-add-provider
  • npm run lint
  • npm test (full suite)
  • npm run ci:build
  • Manual: unauthenticated POST /api/provider/:id/configure → 401
  • Manual: authenticated configure with http://evil.example/... → 400
  • Manual: authenticated configure with https://… or loopback http → 200 and [Audit] provider endpoint changed in logs
  • Review operator docs: A1 aggregation section + A3 endpoint policy + DR-039

issue: https://redhat.atlassian.net/browse/AAP-82836

…ation (A1/A3)

Validate provider base_url and known engines on configure/config writes, audit
secret and endpoint changes, and record DR-038 for deferred encryption-at-rest.
@sudhirverma
sudhirverma requested a review from cidrblock July 20, 2026 03:10
Update engines.js vitest mocks so UpdateConfig can call
validateConfigProviderEngines after the A1/A3 hardening.
@sudhirverma
sudhirverma requested a review from sathyapramod July 20, 2026 15:29
@cidrblock

Copy link
Copy Markdown
Collaborator

Maintainer assist — agent-executable follow-up

Context: Closes AAP-82836 (findings A1 / A3 under epic AAP-80944): document credential-aggregation blast radius + audits, and block malicious/writable provider endpoints via engine allowlist + base_url scheme/host policy. CI is green; branch is 0 behind / 2 ahead of main. No prior review threads.

How to use this comment: Execute items in order. P0 is merge-blocking. Do not invent extra scope. Prefer fixing on branch AAP-82836 and pushing. When done, reply on this thread with commit SHAs and which options you chose. Do not resolve this comment as a “thread” (issue comment); just reply here.


P0-1 — Resolve DR-038 three-way collision

Problem: On current main, the last decision is DR-037. Three open PRs all add a different DR-038:

PR Claimed DR-038
#81 Stdio MCP spawn allowlist + operator approval (H6)
This PR (#82) Credential aggregation (A1) + provider endpoint policy (A3); defer encryption-at-rest
#83 Block MCP self-connections (H7)

Why it matters: Duplicate DR numbers break the decision log. Reviewers cannot cite “DR-038” unambiguously; merges will fight over docs/decisions.md.

What to do:

  1. Check merge order of fix(daemon): gate dynamic stdio MCP spawn (H6 / DR-038) #81 / fix(daemon): harden provider endpoints and document credential aggregation (A1/A3) #82 / feat: implement self-connection guard for MCP endpoints #83 against latest main.
  2. Whichever lands first keeps DR-038. This PR must take the next free ID after rebasing (likely DR-039 or DR-040 depending on what already merged).
  3. Update every self-reference in this branch:
    • docs/decisions.md heading
    • docs/CONFIGURATION.md, docs/ARCHITECTURE.md, docs/TESTING.md, and any other docs that say DR-038 for A1/A3
    • Code comments that say DR-038 for endpoint policy (e.g. provider-endpoint.ts header, config-schema.ts)
    • This PR body’s Test plan / Summary lines that cite DR-038 for A1/A3
  4. Re-read docs/decisions.md after rebase and confirm unique sequential IDs.

Acceptance:


P0-2 — Honor server.allowed_provider_hosts in CoreState.addProvider

Problem: HTTP/gRPC configure + POST /api/config / gRPC UpdateConfig correctly use endpointPolicyFromServer(loadConfig()?.server). But CoreState.addProvider() validates with the default policy only:

// packages/daemon/src/core/state.ts (this branch)
const endpoint = validateProviderEndpoint(options.baseUrl);
// no policy from server.allowed_provider_hosts / allow_insecure_provider_http

Why it matters: DR/A3 says when allowed_provider_hosts is non-empty, non-loopback hosts must match. Enterprise/air-gapped operators who set that allowlist can still get an in-memory provider pointed at https://evil.example via addProvider(), bypassing the control that HTTP/gRPC paths enforce. The audit source comment even lists core-add, but allowlist + audit are incomplete on this path.

What to do (preferred):

  1. Resolve policy the same way as other paths: endpointPolicyFromServer(this.configLoader?.() ?? loadConfig()?.server) (or pass ProviderEndpointPolicy into addProvider / read from the configured config loader already on CoreState).
  2. Call validateProviderEndpoint(options.baseUrl, policy).
  3. On successful base_url set, call auditProviderEndpointChange({ … source: 'core-add' }).
  4. On secret set via addProvider, call auditSecretChange({ key, op: 'set', source: 'core-add' }) (never log the value).
  5. Add a unit test: with allowed_provider_hosts: ['approved.example'], addProvider(..., { baseUrl: 'https://evil.example/v1' }) throws; approved host succeeds.

Acceptance:

  • addProvider rejects non-allowlisted hosts when allowlist is set
  • Loopback http / default https behavior still matches validateProviderEndpoint defaults when allowlist unset
  • Unit coverage for the allowlist bypass regression
  • Audit lines emitted for endpoint (and secret) mutations on this path

P1 — PR body polish (non-blocking but cheap)

Problem: Summary has typo unknown engine Iejected (should be rejected). Test plan still has unchecked boxes even though CI lint-and-test / build matrix / container are green.

What to do: Fix the typo; tick the local/CI items that already passed (npm run lint, full npm test, npm run ci:build if run — or note “covered by CI lint-and-test / build”). Keep manual items unchecked until done.

Acceptance:

  • No “Iejected” typo
  • Test plan checkboxes match reality

P1 — Residual risk (document only unless product wants fail-closed)

Problem: Default policy still allows any https: host when allowed_provider_hosts is unset. An authenticated writer can still point base_url at https://attacker.example and exfiltrate prompts/keys. That matches the DR (“optional allowlist for enterprise”), but it is the remaining A3 blast radius for non-enterprise defaults.

What to do: No code change required for merge if product accepts the DR. Optionally add one sentence in CONFIGURATION.md under A3: “Without allowed_provider_hosts, any https host is accepted; set the allowlist for Enterprise / air-gapped fleets.” (May already be present — verify and tighten if thin.)

Out of scope for this follow-up: Fail-closed default allowlist (product decision); encryption-at-rest (already deferred in the DR); filing separate GitHub issues unless a human asks.


Out of scope


Verification

# From repo root (abbenay uses npm workspaces, not tox):
npm test -w packages/daemon -- provider-endpoint secrets-audit config-schema provider-endpoint-security
npm run lint
npm test
# rebase if needed:
git fetch upstream && git rebase upstream/main

Confirm CI green after push (lint-and-test, build matrix, container, package-python).


Done definition for the executing agent

  1. P0-1 and P0-2 landed and pushed to AAP-82836.
  2. CI green.
  3. Reply on this thread with: commit SHAs, final DR number chosen, confirmation addProvider honors allowlist.
  4. P1 items included or explicitly skipped with reason.

What already looks solid (no action)

  • Shared validateProviderEndpoint / format vs host-policy split + Zod ConfigFileSchema.superRefine
  • Engine allowlist via PROVIDER_LOADERS / validateConfigProviderEngines on HTTP + gRPC config writes
  • HTTP configure / discover-models / gRPC ConfigureProvider + UpdateConfig wiring
  • [Audit] secret changed / [Audit] provider endpoint changed without values
  • Unit + E2E coverage for unauth deny, evil http, userinfo, https audit, unknown engine, config POST reject
  • Operator docs for A1 blast radius + A3 allowlist guidance
  • PR already has Summary / Changes / Test plan structure and links AAP-82836

@cidrblock cidrblock left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: needs work before merge.

Solid A1/A3 implementation and tests; CI green and up to date with main. Merge-blocked on:

  1. DR-038 collision with #81 and #83 (three different decisions all claim DR-038) — renumber after coordinating merge order.
  2. CoreState.addProvider ignores server.allowed_provider_hosts — enterprise allowlist bypass on the in-memory path.

Full agent-executable brief is in the PR comment above.

Renumber A1/A3 from colliding DR-038 to DR-039, apply server.allowed_provider_hosts
on CoreState.addProvider with audits, and document residual https default risk.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sudhirverma

Copy link
Copy Markdown
Contributor Author

Addressed the maintainer-assist follow-up on AAP-82836.

Choices

Commit

  • f0d9991fix(daemon): honor provider host allowlist in addProvider (DR-039)

Confirmation

  • addProvider rejects non-allowlisted hosts when server.allowed_provider_hosts is set; approved hosts succeed; loopback http / default https unchanged when allowlist unset.
  • Local verification: npm run lint, npm test, npm run ci:build green. Watching CI on the push.

Keep #83 MCP self-connection as DR-039; renumber A1/A3 provider
endpoint policy to DR-040 after the merge collision.
@sudhirverma
sudhirverma requested a review from cidrblock July 21, 2026 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants