Skip to content

fix(harness): route the tenant key through credentialFetch [SAP-4112] - #1257

Open
antoine-berger wants to merge 6 commits into
mainfrom
antoine-berger/sap-4112-harness-tenant-key-credential-fetch
Open

antoine-berger wants to merge 6 commits into
mainfrom
antoine-berger/sap-4112-harness-tenant-key-credential-fetch

Conversation

@antoine-berger

@antoine-berger antoine-berger commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Primary change type

  • Bug fix
  • Documentation
  • Feature
  • Tests
  • Dependency update
  • Maintenance or refactor

Problem and motivation

Since SAP-4106 every SDK client sends the tenant API key through credentialFetch, which refuses plain http to a non-loopback host and drops the key on a redirect to another origin. Agent Studio (@sapiom/harness) was left out: eight requests and the MCP config it writes for Claude Code and Codex still set the key header by hand, and scripts/credential-header-allowlist.json exempted four of them from the CI guard. A custom plain-http environment, or a redirect, could send the key in clear from Studio while the rest of the SDK refuses.

Summary and scope

  • @sapiom/analytics-core/credential: Credential.header accepts "authorization" (Bearer APIs; the caller's own Authorization is replaced and the header is dropped on a cross-origin hop, as for the other two). New credentialHeaders(url, credential, policy) checks a URL before a key is handed to another process.
  • @sapiom/harness: new harnessCredentialPolicy() (label @sapiom/harness, opt-in SAPIOM_ALLOW_INSECURE_HTTP=1, read per call).
    • Agents surface (x-sapiom-api-key): run-state.ts, definition-slug-resolver.ts (2 sites).
    • Core surface (authorization: Bearer): vault-secrets.ts, account-plan.ts, template-catalog.ts, definition-app-link.ts.
    • Assistant bridge (opencode-bridge.ts): same x-api-key, still redirect: "error".
    • mcp-config.ts: the remote MCP key is only written for an allowed URL; otherwise session start fails with the refusal message, which names the opt-in.
  • A refused request throws before any byte is sent and lands in each site's existing failure path (502, unavailable, fallback, NO_APP_LINK, VaultSecretError).
  • scripts/credential-header-allowlist.json is now [].

Out of scope: studio-credentials.ts and assistant-access.ts send the Studio OAuth token, not the tenant key. The harness analytics emitter already goes through credentialFetch. A Studio-specific opt-in (CLI flag or desktop setting) is not added: the env var is the only switch.

Plan: plans/credential-transport/harness-tenant-key-credential-fetch.md.

Related work

Related issue or discussion: SAP-4112 (follows SAP-4106, #1236)

Validation

pnpm --filter @sapiom/analytics-core exec jest --maxWorkers=2 src/credential.spec.ts — 82 passed
pnpm --filter @sapiom/analytics-core build — ok
pnpm --filter @sapiom/harness exec vitest run --maxWorkers=2 <8 touched test files> — 181 passed
pnpm credential-header:check — passed (empty allowlist)
node --test scripts/credential-header-check.test.mjs — 16 passed
pnpm --filter @sapiom/harness typecheck — ok
eslint on changed files — clean

Tests and documentation

  • credential.spec.ts: authorization credential replaces a caller header, is dropped cross-origin and kept same-origin; credentialHeaders for https, loopback, LAN http, a non-http scheme and the opt-in.
  • One refusal test per harness site (non-loopback http base URL: no fetch call, existing failure result); account-plan and template-catalog assert the logged message names the refusal; run-state, slug resolver and mcp-config also cover SAPIOM_ALLOW_INSECURE_HTTP=1.
  • Existing assertions now read the lowercase authorization header and objectContaining (credentialFetch adds redirect).

Compatibility and release impact

  • Breaking or externally visible changes: Studio users with a custom ~/.sapiom/credentials.json environment on plain http to a non-loopback host (Docker service name, LAN IP) now get a refused session start and unavailable Agents/core panels until they set SAPIOM_ALLOW_INSECURE_HTTP=1. Built-in environments are https and localhost is unchanged.
  • Changeset: Added (@sapiom/harness minor, @sapiom/analytics-core minor).

Security

  • I have not included secrets, credentials, private data, or unsanitized logs.
  • This pull request does not publicly disclose a suspected vulnerability. I
    will follow the
    Security Policy for
    private reporting.

AI assistance

  • I did not use AI assistance for this change.
  • I used AI assistance and have described it below.

Claude Code (Opus) wrote the plan, the code and the tests from SAP-4112; I reviewed the diff and the commands above were run locally.

Checklist

  • I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
  • This pull request addresses one focused problem and contains no unrelated cleanup.
  • I added or updated tests, or explained above why tests are not applicable.
  • I ran the relevant build, typecheck, lint, and test commands, or explained
    any N/A checks above.
  • I updated documentation for user-facing changes, or marked it N/A above.
  • I added a Changeset for a published-package change, or explained why it is not applicable.
  • I can explain and maintain every submitted change, including any AI-assisted work.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Security
    • Requests containing credentials now refuse plain HTTP to non-local hosts by default. Set SAPIOM_ALLOW_INSECURE_HTTP=1 to allow it on trusted networks.
    • Credentials are removed when a request redirects to a different origin; HTTPS and localhost behavior remain unchanged.
    • Harness requests to unsupported plain-HTTP hosts fail without sending credentials. Requests without credentials can still proceed.
  • New Features
    • Authorization credentials are supported alongside API-key credentials.

antoine-berger and others added 3 commits October 22, 2026 16:03
…[SAP-4112]

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ers [SAP-4112]

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…the allowlist [SAP-4112]

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 35081348-ee23-487d-a6dd-06701b75a95f
Commits

Reviewing files that changed from the base of the PR and between a3de8e6 and b8d9621.

Files selected for processing: 1
  • packages/harness/README.md
Files skipped from review as they are similar to previous changes: 1
  • packages/harness/README.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Harness network requests and MCP configuration now use shared credential-policy helpers. The policy refuses credential transport over non-loopback plaintext HTTP unless SAPIOM_ALLOW_INSECURE_HTTP=1. Analytics Core adds support for authorization credentials and URL-checked header construction.

Changes: Credential transport

Layer / File(s) Summary
Credential contract and policy
packages/analytics-core/src/credential.ts, packages/analytics-core/src/credential.spec.ts, packages/harness/src/core/credential-policy.ts, scripts/credential-header-allowlist.json, .changeset/harness-credential-fetch.md, plans/credential-transport/..., packages/harness/README.md
Analytics Core adds the "authorization" credential header and credentialHeaders(), which checks the URL before returning a credential header. Harness adds a policy resolved per call. Tests cover authorization-header replacement and redirect behavior, plus URL policy cases. The changeset and README document the transport policy.
Harness API request migration
packages/harness/src/core/account-plan*, packages/harness/src/core/definition-app-link*, packages/harness/src/core/definition-slug-resolver*, packages/harness/src/core/run-state*, packages/harness/src/core/template-catalog*, packages/harness/src/core/vault-secrets*, packages/harness/src/server/runs.test.ts
Harness readers and fetchers use credentialFetch with the harness policy. Tests cover refusal of non-loopback plaintext HTTP, opt-in behavior, and existing request handling.
MCP configuration and Assistant bridge
packages/harness/src/core/inject/mcp-config*, packages/harness/src/server/opencode-bridge.ts
MCP configuration uses credentialHeaders() for a present API key. Tests cover remote HTTP refusal, keyless configuration, loopback HTTP, and opted-in HTTP. The bridge passes the API key to credentialFetch instead of adding it to ordinary upstream request headers.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant HarnessRequest
  participant harnessCredentialPolicy
  participant credentialFetch
  participant fetchImpl
  HarnessRequest->>harnessCredentialPolicy: Resolve policy for request
  HarnessRequest->>credentialFetch: Pass URL, credential, and policy
  credentialFetch->>fetchImpl: Send request when URL policy permits
Loading
Merge Risk: Minimal · up to b8d96

No actionable issue is established for the documentation change. The PR is mergeable subject to normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 19 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the primary change: routing the harness tenant key through credentialFetch. It is concise, specific, and includes the related issue identifier.
Description check Passed The description follows the required template and covers the problem, scope, related work, validation commands, tests, documentation, compatibility impact, release changes, security declarations, AI a…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description follows the required template and covers the problem, scope, related work, validation commands, tests, documentation, compatibility impact, release changes, security declarations, AI assistance, and checklist items.

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 19 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit checks the path before the key can roam,
HTTPS or loopback leads it safely home.
Across new origins, the secret stays behind,
While opted-in HTTP follows policy’s line.
The harness and core now share the same design.

Comment @coderabbitai help to get the list of available commands.

@antoine-berger
antoine-berger marked this pull request as ready for review October 22, 2026 21:09
devin-ai-integration[bot]

This comment was marked as resolved.

antoine-berger and others added 2 commits October 22, 2026 16:24
…ter test [SAP-4112]

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… and desktop [SAP-4112]

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

…n sessions [SAP-4112]

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

1 participant