Repository navigation
fix(core): keep API keys off other hosts and plain http, and time out silent endpoints - #356
Merged
Merged
Conversation
… silent endpoints A named provider without a key of its own inherited settings.api_key and OPENMAX_API_KEY, so a key configured for one server went to whatever host a providers.json entry named, and any key went out as a bearer header over plain http to any host. Separately, only connecting had a deadline: an endpoint that accepted a request and then sent nothing held the turn forever, so a headless or stdio run never ended. A provider now inherits the settings key only when its base_url has the same scheme, host, and port as settings.base_url. Any other server gets no Authorization header, and a 401 from it names how to give the provider its own key. A request that would carry a key or an Authorization header over http to a non-loopback host is refused before it is sent, naming https or a loopback address as the fix. An idle timeout (10 minutes by default, idle_timeout_secs per provider) covers the wait for response headers and every gap in the body, and SSE keepalive comments count as activity. Silence ends the attempt as a transport fault: resent before any reply text, a truncation after it, and a finished reply stands. The configuration docs and the providers, settings, and stdio specs describe both rules.
The 401 hint for a provider denied the settings key began "no key was sent", which is false for a provider that authenticates through its own headers entry, and sent its user toward the wrong fix. It now says the settings key was not sent, which holds however the provider authenticates. The configuration docs and the providers spec said a silent endpoint is resent whenever no reply text has arrived, but a one-shot JSON reply or an error body that goes silent after its headers fails at once. Both now say only a missing response or a stream silent before reply text is resent.
A base_url with userinfo, such as http://user:secret@192.168.1.5:8000/v1, still went out over plain http to another machine: the HTTP client turns URL userinfo into a Basic Authorization header, and the plain http check only looked at the key and the provider's headers. The check now counts a username or password in the base_url as a credential, and the refusal names it among the sources to remove. The --spec providers entry for idle_timeout_secs now matches the client: a silent one-shot reply is not resent, and 0 never disables the timeout.
…red server Only a header named Authorization counted as a credential, so a key a provider sends in X-API-Key, api-key, or a token header still went out over plain http to another machine. And the HTTP client followed any redirect: on a move to another host it drops Authorization and cookies but keeps every other header and, on a 307 or 308, the body, so an endpoint that redirected handed such a key and the transcript to a server nobody configured. A header now counts as a credential when its name contains auth, key, token, secret, password, credential, or cookie, so the plain-http refusal covers it. The shared HTTP client follows a redirect only on the scheme, host, and port the request was sent to; one to another server fails the request with an error naming that server, and nothing reaches it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A named provider with no key of its own silently inherited the
api_keyfrom settings.json (orOPENMAX_API_KEY), whatever server it pointed at. Switching to a second provider handed the first server's credential to a different host. Separately, a key, a credential header such asAuthorizationorX-API-Key, oruser:passwordinbase_urlwent out over plainhttp://to any host, so it crossed the network unencrypted, and a redirect from the endpoint carried the request, with any key in a custom header, on to whatever server it named. And a request had no deadline of any kind: an endpoint that accepted the request and then went quiet (a wedged server, a proxy holding a dead upstream) held the turn forever with no error and no retry.Summary
api_keyorapi_key_envuses the settings key only when itsbase_urlhas the same scheme, host, and port (default ports counted, path ignored) as the settingsbase_url. Any other server gets no key. A 401 from such a provider says the settings key was not sent and how to give the provider its own (addapi_key_env, or export the variable it already names).headers(any name containingauth,key,token,secret,password,credential, orcookie, such asAuthorizationorX-API-Key), oruser:passwordinbase_urloverhttp://fails before anything is sent, naming the host (never the secret) and the fix: use https, or a loopback address. Loopback (localhost,127.0.0.0/8,::1, IPv4-mapped loopback) and the unspecified address (0.0.0.0,::) stay allowed. A server that needs no key still works over http, routing headers such asX-Titleincluded.idle_timeout_secsin providers.json sets a different interval for one provider.--checkvalidates it and rejects 0; 0 never disables the timeout. No new settings.json key.docs/configuration.md,--spec providers,--spec settings(theapi_keyentry), the stdio spec,docs/stdio-protocol.md, and theAgentEvent::Retrydoc now describe the key and redirect rules and that a retry reason can be a stream that went silent. No model-visible prompt bytes changed.Test Plan
Red first: the key, plain http, credential header, redirect, silence, and keepalive tests below failed on the unmodified code and pass with the change.
providers::tests::a_settings_key_is_inherited_only_by_its_own_host: a provider on the settings host inherits the key; one on another host, scheme, or port does not, and carries the withheld-key hint.client::tests::a_settings_key_reaches_only_its_own_host: the key reaches only the matching server on the wire, and a 401 from another server includes the hint.client::tests::a_key_never_crosses_plain_http_to_another_machine: a key, anAuthorizationorX-API-Keyheader, oruser:password/:passwordin anhttp://base_urlto another host is refused with zero retries and nothing sent; the error names the credential source and never contains the secret.client::tests::a_credential_header_is_known_by_its_name:Authorization,Proxy-Authorization,X-API-Key,api-key, token, cookie, secret, and password headers count as credentials;X-Route,HTTP-Referer,X-Title,User-Agent, andAcceptdo not.client::tests::a_redirect_never_carries_a_request_to_another_server: a 307 to another port fails the request with no retry and the other server receives nothing (it received theX-API-Keyrequest before the fix); a 307 to another path on the same server is followed.client::tests::only_https_or_this_machine_may_carry_a_key: https, loopback (includingLOCALHOST,127.8.9.10,[::1], IPv4-mapped loopback) and0.0.0.0are allowed; other http hosts, including private addresses andlocalhost.example.com, are not.client::tests::a_silent_endpoint_is_resent_before_reply_text: no response headers, and a stream stalled before reply text, are both resent and then succeed.client::tests::a_stream_silent_after_reply_text_or_its_finish_is_not_resent: silence after reply text is reported truncated; silence after the finish keeps the reply.client::tests::keepalive_comments_hold_off_the_idle_timeout: a stream that sends only keepalive comments for longer than the interval still completes.client::tests::a_providers_idle_timeout_secs_sets_its_interval: the provider's value reaches the client.providers::tests::check_file_reports_valid_and_invalid_provider_documents:idle_timeout_secsis a known key and--checkrejects 0.cargo test --workspace --locked: exit 0.cargo +1.97.0 clippy --workspace --all-targets --locked -- -D warnings: exit 0.No outstanding finding blocks merging.
Summary
The PR limits settings-key inheritance to the configured server, refuses credential-bearing requests to remote plain-HTTP endpoints, adds an inactivity timeout, and documents these behaviors. The current code also recognizes credential-like custom headers and restricts redirects to the original server. No new actionable issue was supplied.
Reviews (2) · Last reviewed commit: "fix(core): guard credential headers and ..."