-
Notifications
You must be signed in to change notification settings - Fork 640
fix(runner): keep the run credential when the public api base is unconfigured #6281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| import { type AgentRunRequest, type ToolPermission } from "../../protocol.ts"; | ||
| import { claimSessionOwnership, REPLICA_ID } from "../../sessions/alive.ts"; | ||
| import { | ||
| configuredIngestBases, | ||
| isAgentaIngest, | ||
| platformAuthorizationProvider, | ||
| publicApiBaseConfigured, | ||
| resolveOtlpTraceEndpoint, | ||
| type AuthorizationProvider, | ||
| } from "../../tracing/otel.ts"; | ||
|
|
@@ -19,16 +21,65 @@ export function runCredential(request: AgentRunRequest): string { | |
| return (headers["authorization"] ?? headers["Authorization"] ?? "").trim(); | ||
| } | ||
|
|
||
| /** Endpoints already warned about, so a per-turn read warns once instead of every run. */ | ||
| const warnedEndpoints = new Set<string>(); | ||
|
|
||
| /** Test-only: forget which endpoints have warned, so a case can assert on its own warning. */ | ||
| export function resetPlatformCredentialWarnings(): void { | ||
| warnedEndpoints.clear(); | ||
| } | ||
|
|
||
| /** | ||
| * The legacy wire has one authorization header for two possible owners. Treat it as an Agenta | ||
| * platform credential only when the configured destination is Agenta ingest; for an external | ||
| * collector it belongs exclusively to that collector and must never enter platform calls. | ||
| * | ||
| * That attribution is only decidable once the runner knows its platform's PUBLIC api base | ||
| * (`AGENTA_API_URL`), because the public form is what a dispatched run carries: the API hands the | ||
| * SDK `https://<host>/api`, while the runner's own hop is usually the internal `http://api:8000`. | ||
| * A runner told ONLY its internal hop cannot tell its own API under its public name from a | ||
| * third-party collector. Refusing there fails closed on the wrong axis — it silently strips the | ||
| * credential from every run in an otherwise healthy self-hosted deployment, and the damage | ||
| * surfaces far away as a 401 on session persistence. So the strict check arms itself only when | ||
| * the operator has supplied the base that makes it decidable, and otherwise keeps the credential | ||
| * and says loudly what to configure. | ||
| */ | ||
| export function platformCredentialForRequest(request: AgentRunRequest): string { | ||
| export function platformCredentialForRequest( | ||
| request: AgentRunRequest, | ||
| log: Log = (message) => process.stderr.write(`${message}\n`), | ||
| ): string { | ||
| const endpoint = resolveOtlpTraceEndpoint( | ||
| request.telemetry?.exporters?.otlp?.endpoint, | ||
| ); | ||
| return isAgentaIngest(endpoint) ? runCredential(request) : ""; | ||
| if (isAgentaIngest(endpoint)) return runCredential(request); | ||
|
|
||
| const credential = runCredential(request); | ||
| if (!credential) return ""; | ||
|
|
||
| if (!publicApiBaseConfigured()) { | ||
| if (!warnedEndpoints.has(endpoint)) { | ||
| warnedEndpoints.add(endpoint); | ||
| log( | ||
| `[sessions] WARNING: trace endpoint ${endpoint} matches no configured Agenta ingest ` + | ||
| `base (${configuredIngestBases().join(", ")}), and AGENTA_API_URL is not set, so the ` + | ||
| `run credential cannot be attributed. Using it for platform calls anyway. Set ` + | ||
| `AGENTA_API_URL to this deployment's public api base (e.g. https://<host>/api) to ` + | ||
| `attribute it properly and to keep third-party collector credentials out of platform calls.`, | ||
|
Comment on lines
+63
to
+67
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win Redact the trace endpoint before writing it to stderr. These warnings log the raw endpoint. An OTLP endpoint can contain credentials in URL userinfo or query values. The runner then stores those credentials in its logs. Log a sanitized origin, or redact all URL credential-bearing components before interpolation. Proposed fix+function endpointForLog(endpoint: string): string {
+ try {
+ return new URL(endpoint).origin;
+ } catch {
+ return "<invalid endpoint>";
+ }
+}
+
- `[sessions] WARNING: trace endpoint ${endpoint} matches no configured Agenta ingest `
+ `[sessions] WARNING: trace endpoint ${endpointForLog(endpoint)} matches no configured Agenta ingest `
- `[sessions] trace endpoint ${endpoint} is not Agenta ingest `
+ `[sessions] trace endpoint ${endpointForLog(endpoint)} is not Agenta ingest `Also applies to: 76-80 |
||
| ); | ||
| } | ||
| return credential; | ||
| } | ||
|
|
||
| if (!warnedEndpoints.has(endpoint)) { | ||
| warnedEndpoints.add(endpoint); | ||
| log( | ||
| `[sessions] trace endpoint ${endpoint} is not Agenta ingest ` + | ||
| `(${configuredIngestBases().join(", ")}); dropping the run credential from platform ` + | ||
| `calls. Session persistence and history rebuild will fail with HTTP 401 if this ` + | ||
| `endpoint IS this deployment's api base.`, | ||
| ); | ||
| } | ||
| return ""; | ||
| } | ||
|
|
||
| export interface RunOtlpTarget { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the troubleshooting cause for the new fallback behavior.
When
AGENTA_API_URLis unset,platformCredentialForRequestkeeps the credential for platform calls. It does not withhold it and cause the stated HTTP 401 failures. The runner warns because it cannot prevent a third-party collector credential from reaching platform calls. Describe a missing or mismatched configured public base as the cause only when strict attribution drops a credential.