Repository navigation
Commit 364ff45
authored
fix(server): Reject malformed runtime-token target context before authorization [SAO-17522] (#268)
## Summary
This PR hardens `POST /api/v1/auth/runtime-token-exchange` so malformed
target context is rejected by Agent Control with a local `422` before
any authorization provider is called.
It preserves the OSS contract: `target_type` and `target_id` are
required, non-empty strings, but their values remain opaque. Agent
Control does not enforce Orbit-specific values such as
`target_type="log_stream"` or a UUID-shaped `target_id`.
## What happened
In the multitenant environment, runtime-token exchange intermittently
returned `502`. Agent Control logs showed the upstream authorization
service returning `422` for `runtime.token_exchange`. The same incident
window also contained upstream `422` responses for
`control_bindings.read`.
The exact rejected runtime-token payload was not captured, so this PR
does **not** claim that missing target fields were the confirmed
production root cause.
Code inspection did identify a concrete boundary problem: the
authorization dependency builds its context before FastAPI binds and
validates `RuntimeTokenExchangeRequest`. The previous context builder
used `body.get(...)`, so malformed requests could reach the provider
with missing, `null`, empty, or incorrectly typed target fields. An
upstream `4xx` is then exposed by Agent Control as `502`, rather than
returning a useful local validation error.
## Goal
- Never call an authorization provider with missing or invalid runtime
target context.
- Return a deterministic, structured local `422` for malformed
runtime-token requests.
- Keep Agent Control generic and accept valid opaque target values.
- Avoid adding Orbit-specific `log_stream` or UUID validation to the OSS
service.
## What changed
- Validate that the exchange body is a JSON object before authorization.
- Validate only the two fields required by the authorization provider:
- `target_type`: required, string, non-empty;
- `target_id`: required, string, non-empty.
- Return field-level `ValidationErrorItem` entries whose codes and
messages match Pydantic v2 (`missing`, `string_type`, and
`string_too_short`).
- Do not echo invalid target values in validation errors.
- Pass valid target values to the provider unchanged.
- Keep unrelated body validation, including `extra="forbid"`, in the
existing Pydantic request model.
| Request | Result |
| --- | --- |
| Invalid JSON or a non-object body | Local `422`; authorization
provider is not called |
| Missing, `null`, empty, or non-string target field | Structured local
`422`; authorization provider is not called |
| Valid opaque target values | Passed through unchanged |
| Unknown non-target field | Provider receives only the valid target
context; the existing Pydantic model then returns `422` |
## Scope
- Changed files:
- `server/src/agent_control_server/endpoints/auth.py`
- `server/tests/test_runtime_token_exchange_endpoint.py`
- User-visible change: malformed runtime-token target context now
returns local `422` instead of reaching the upstream provider and
potentially surfacing as `502`.
- Valid requests are unchanged.
Out of scope for this PR:
- Changes to `control_bindings` or attachment context builders.
- New upstream `4xx` logging or diagnostic parsing. Tracked separately
in [SAO-17523](https://splunk.atlassian.net/browse/SAO-17523), which
touches `auth_framework/providers/http_upstream.py` and proceeds in
parallel.
- Identifying or changing the Orbit caller responsible for the observed
production `422`.
- Orbit's authorization contract. Orbit currently accepts only
`log_stream` plus a log-stream UUID; Agent Control intentionally does
not duplicate that validation.
- Project-scoped target support.
## Risk and Rollout
- Risk level: low. The behavior change is limited to malformed
runtime-token exchange requests before authorization.
- No migration or configuration change is required.
- Rollback plan: revert this PR.
## Testing
- [x] Added or updated automated tests.
- [x] Focused runtime-token endpoint suite: `26 passed`.
- [x] Focused endpoint statement and branch coverage: `100%`.
- [x] Ruff and mypy passed for the changed code.
- [x] Required GitHub checks passed for the current head: `ci`,
`validate`, `sdk-ts-ci`, `ui-ci`, `build-and-push-server`, and
`codecov/patch`.
- [x] Ran the `make check` suites locally against the rebased tree:
server tests `912 passed`, `make lint` clean across models/server/sdk,
and `mypy server/src` clean (51 files). Note `server/Makefile` pins
`AGENT_CONTROL_DB_PORT=5432` in `TEST_DB_ENV`, so the port must be
overridden when another Postgres holds 5432. `make typecheck` reports 3
pre-existing `opentelemetry.sdk.trace` import errors in `sdks/python`,
unrelated to this change and also present on `main`.
- [ ] Validated in multitenant staging: not possible from a PR branch.
`docker-publish.yml` only pushes images on `main` or a `v*` tag (`push:
github.event_name != 'pull_request'`), so there is no deployable image
until this merges. Tracked under the parent story.
## Checklist
- [x] Linked issue:
[SAO-17522](https://splunk.atlassian.net/browse/SAO-17522), a subtask of
[SAO-17418](https://splunk.atlassian.net/browse/SAO-17418).
- [x] Documentation/examples: no update required; valid API behavior and
the generic target contract are unchanged.
- [x] Follow-up boundary documented above: Orbit caller/root-cause
investigation and `control_bindings.read` are not changed here.
# AI Tool Assistance Usage Statement
- [x] AI assistance was used to draft parts of the implementation, that
was subsequently modified and extended.
- [x] AI assistance was used in generating tests/documentation/comments
for this change.
- [x] AI assistance was used for optimizing/troubleshooting/refactoring
existing code in this change.
- [x] AI assistance was used to draft this entire change as is.1 parent b7a76cf commit 364ff45
2 files changed
Lines changed: 259 additions & 29 deletions
File tree
- server
- src/agent_control_server/endpoints
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
20 | | - | |
| 20 | + | |
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | | - | |
| 31 | + | |
32 | 32 | | |
33 | 33 | | |
34 | 34 | | |
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
65 | 102 | | |
66 | | - | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
67 | 108 | | |
68 | 109 | | |
69 | 110 | | |
70 | 111 | | |
71 | 112 | | |
72 | 113 | | |
73 | | - | |
74 | | - | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
75 | 117 | | |
76 | | - | |
77 | | - | |
78 | | - | |
79 | | - | |
80 | | - | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
81 | 135 | | |
82 | 136 | | |
83 | 137 | | |
| |||
0 commit comments