Skip to content

feat(server): Add validation details to upstream auth logs [SAO-17523] - #270

Open
josjeon wants to merge 6 commits into
mainfrom
sao-17523-upstream-diagnostics
Open

josjeon wants to merge 6 commits into
mainfrom
sao-17523-upstream-diagnostics

Conversation

@josjeon

@josjeon josjeon commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Why

During the September 23 multitenant incident, Orbit rejected two authorization checks with 422 responses, and Agent Control returned 502. Agent Control's warning named the operation and status, but gave us no way to tell which field Orbit rejected. We still do not have the original caller payload, so the incident's root cause is unconfirmed.

This PR makes the next rejection easier to diagnose from the logs. It is the scoped change for SAO-17523.

Before and after

image image

What changed

When the upstream authorization service returns an unexpected 4xx, the warning now includes:

  • The operation and upstream status.
  • Whether target context was sent, plus the shape of target_type and target_id (for example, string:len=10 rather than the value).
  • Up to five validation errors, showing a recognized error kind and field path. The log also marks the summary as parsed, oversized, or unusable; the total count is unknown when it cannot be computed.

The details appear in the warning text and structured log fields, so they survive plain text, Agent Control JSON, and Orbit's host managed logging. The API response is unchanged: the same rejection still returns 502 with AUTH_UPSTREAM_REJECTED.

Illustrative outcomes (the original incident payload was not captured):

Scenario Client response Agent Control log
Invalid target rejected locally 422 VALIDATION_ERROR; Orbit is not called No upstream diagnostic warning
Orbit returns a parseable validation error 502 AUTH_UPSTREAM_REJECTED Field path and error type, without caller values
Orbit returns an oversized or unusable error body Same 502 response oversized or unusable; validation count is unknown

Keeping caller data out of logs

The summary drops validation messages, inputs, and context values. It logs only known error kinds and field names; unknown entries become other or <other>. Each field path is limited to four parts, and the error list is limited to five entries. Bodies over 64 KiB are marked oversized; deeply nested, malformed, or non JSON bodies are marked unusable. These cases produce an empty summary with an unknown count without changing the API response. A parsed empty list has a count of 0.

Verification and rollout

  • 156 auth framework and logging utility tests passed locally; one test that needs the usual setup fixture was deselected. Ruff and mypy passed.
  • PR CI passed on the pushed commit, including server checks, UI, TypeScript SDK, image build, and Codecov patch coverage.
  • The full local check could not start offline because splunk-ao==0.4.0 was not cached. The full server suite also needs PostgreSQL, which was unavailable locally.
  • No migration or configuration change is needed. Revert this PR to roll back. Multitenant staging validation still needs a deployable image after merge.

This change helps identify a future recurrence or equivalent reproduction. It does not establish or fix the original malformed payload. Local runtime token request validation is covered separately by #268.

Checklist

  • Linked issue: SAO-17523, a subtask of SAO-17418.
  • No API or configuration documentation change needed.
  • PR CI passed on the pushed commit.
  • Multitenant staging validation after merge.

AI Tool Assistance Usage Statement

  • AI assistance was used to draft parts of the implementation, which were subsequently modified and extended.
  • AI assistance was used in generating tests and documentation for this change.
  • AI assistance was used for optimizing and troubleshooting existing code in this change.
  • AI assistance was used to draft this entire change as is.

@codecov

codecov Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@josjeon
josjeon force-pushed the sao-17523-upstream-diagnostics branch from eb9dd54 to e069e65 Compare September 28, 2026 16:48
josjeon added a commit that referenced this pull request Sep 28, 2026
…SAO-17580] (#272)

## Summary

`make typecheck` currently fails on `main` and on every open PR. This
annotates the four call sites that SQLAlchemy 2.1.0 broke, unblocking
merges.

The failure belongs to no branch's changes.

## What happened

SQLAlchemy 2.1.0 no longer lets mypy infer the element type through
`result.scalars().all()`:

```
server/src/agent_control_server/services/control_bindings.py:282:
  error: Need type annotation for "rows" (hint: "rows: list[<type>] = ...")  [var-annotated]
Found 1 error in 1 file (checked 51 source files)
make: *** [Makefile:138: typecheck] Error 1
```

The repository lock pins 2.0.51, which still infers it, so the error
appears only in CI, where dependencies resolve fresh. Main's own run at
`bd7d91f` installs both and reports the error in the same log:

```
+ mypy==2.3.1
+ sqlalchemy==2.1.0
server/src/agent_control_server/services/control_bindings.py:282: error: Need type annotation for "rows"
```

## Evidence this is not branch-specific

| Branch | Date | CI |
| --- | --- | --- |
| `main` | 2026-09-22 | success |
| `main` | 2026-09-24 | failure |
| `fix/sao-17418-runtime-token-auth` | 2026-09-24 | success |
| `fix/sao-17418-runtime-token-auth` | 2026-09-28 | failure |

SQLAlchemy 2.1.0 was released between those dates, and the flagged file
is not in the diff of either affected PR. Re-running the job does not
clear it: CI resolves 2.1.0 again each time, so the failure is
deterministic rather than flaky.

## What changed

Four sites share the pattern. CI reports only the first, so fixing them
one at a time would surface the next on the following run.

| File | Annotation |
| --- | --- |
| `services/control_bindings.py:282` | `rows: list[ControlBinding]` |
| `services/controls.py:306` | `versions: list[ControlVersion]` |
| `services/controls.py:512` | `controls: list[Control]` |
| `endpoints/agents.py:463` | `agents: Sequence[Agent]` |

`agents.py` keeps `Sequence` rather than `list` because that call site
does not wrap the result in `list()` and only reads and slices it.
`Sequence` and `Agent` were already imported there.

## Scope

- Type annotations only. No runtime behavior changes, no control flow,
no queries touched.
- Deliberately out of scope: raising the SQLAlchemy floor in
`pyproject.toml` or regenerating the lock. That is a wider dependency
decision, and these annotations are correct under either version.

## Risk and Rollout

- Risk level: minimal. Annotations are erased at runtime.
- No migration or configuration change.
- Rollback plan: revert this PR.

## Testing

- [x] `mypy server/src` clean under the locked versions: 51 source
files, no issues.
- [x] `ruff check server/src` clean.
- [ ] Full server test suite: not run locally. The local Docker daemon
is unresponsive, so the Postgres-backed suite cannot start. CI covers it
here, and the change is annotations only.
- [ ] Reproduced the failure locally against CI's resolved versions: not
possible right now, the internal package index returns 401. The CI log
for `main` is cited above instead.

## Checklist

- [x] Linked issue:
[SAO-17580](https://splunk.atlassian.net/browse/SAO-17580).
- [x] Documentation/examples: no update required.
- [x] Unblocks #268 (approved, blocked only by this) and #270. Both
touch different files, so this merges independently of either.

# AI Tool Assistance Usage Statement

- [x] AI assistance was used to draft parts of the implementation, that
was subsequently modified and extended.
- [ ] 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.
- [ ] AI assistance was used to draft this entire change as is.
@josjeon
josjeon enabled auto-merge (squash) September 28, 2026 21:11
… [SAO-17523]

When the upstream authorization service rejected a check, the log recorded
only the operation and the status code. During the 2026-09-23 multitenant
incident that left the cause unknown: the pods showed Orbit returning 422 and
Agent Control translating it to 502, with nothing to say which field was
rejected.

Attach three things to that warning: the operation, the shape of the target
context that was sent, and the field path plus error kind from the upstream
validation body. Orbit answers with the standard FastAPI validation envelope,
where `type` and `loc` alone separate a malformed target_id (`uuid_parsing` at
body.context.target_id) from an unsupported target_type (`enum` at
body.context.target_type) from an unknown operation (`enum` at body.operation).

Caller data stays out. Only `type` and `loc` are taken from each entry;
`input`, `ctx`, and `msg` echo the caller's values and are dropped. Target
context is described by shape rather than value, so absent, null, empty, and
wrongly typed stay distinguishable without logging an identifier. The logged
list is capped, with the total reported separately so a truncated list never
reads as complete.

Nothing Orbit-specific is added. Target values remain opaque, with no checks
for `log_stream` or UUID format, and the parser never raises: an unexpected
rejection body degrades to an empty summary instead of turning a 502 into a
500.
@josjeon
josjeon force-pushed the sao-17523-upstream-diagnostics branch from e069e65 to 6396e25 Compare September 29, 2026 18:00
@josjeon
josjeon requested a review from wrisa September 29, 2026 18:56
@josjeon josjeon changed the title feat(server): Report sanitized diagnostics on upstream 4xx rejections [SAO-17523] feat(server): Add validation details to upstream auth logs [SAO-17523] Sep 29, 2026
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