Feat: Record the HTTP method and path on session events - #967
Conversation
Traffic that no parser recognized reached the timeline carrying only a host. Issue rossoctl#906 shows the result: rows whose every informative column is an em dash, leaving no way to tell a token refresh from an object download, and no way for an implementor to see what Cortex was not monitoring. The verb and the path were already on pipeline.Context at every recording site, normalized across listener modes and query-stripped. They simply never made it onto the event, so they never reached /v1/sessions/{id}. Copy them across. Named HTTPMethod/HTTPPath rather than Method/Path because abctl already renders a METHOD column sourced from the protocol method (A2A "message/ stream", MCP "tools/call"), which is not an HTTP verb; an unprefixed Method would put two different meanings under one name. Both carry omitempty for the same skew reason as TunnelReason: an old abctl ignores unknown keys, and a new abctl against an older proxy sees "". Opaque tunnels keep an empty path on purpose. The bytes are never parsed as HTTP, so there is no request line to read one from, and the event pairs that empty path with a CONNECT verb — asserted by a test so the blank is not later "fixed" into something invented. This addresses the recording half of rossoctl#906. Displaying the fields in abctl when nothing else is known is left as follow-up work, so the issue stays open. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
|
Warning Review limit reachedNext included review available in 35 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe session event model now records HTTP method and query-stripped path metadata. Extproc, forward proxy, reverse proxy, and tunnel recording paths populate these fields. JSON serialization, tests, and event documentation cover the new metadata. ChangesSession HTTP metadata
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to HTTP method and query-free path metadata is consistently recorded and serialized, including opaque CONNECT tunnels. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 9 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Six review findings, all verified against the code first. The round-trip canary did not populate the new fields, so a field forgotten in UnmarshalJSON round-tripped clean and the test said nothing. Confirmed by deleting HTTPPath from UnmarshalJSON: the canary now fails, where before it passed. TestSessionEvent_MarshalJSON_OmitsEmpty has the same shape of gap — a hand-maintained field list — which left the omitempty half of the version-skew argument unasserted; extended with tunnel and tunnelReason too, since rossoctl#929 left the same hole. httpx.PathOnly had no test at all despite being the single chokepoint upholding the query-stripping invariant for four ext_proc sites and ext_authz — an invariant this PR surfaces to operators. Added a table test plus the property it rests on (a "?" never survives). One case pinned the opposite of my assumption: url.ParseRequestURI does not split fragments, because a client never sends one, so "#frag" stays in the path. Pinned as-is rather than "fixed" — net/http answers the proxy listeners identically and cross-mode parity is the point. Documented what the path can carry. A query string is stripped before recording, so query-borne credentials never reach the timeline, but a secret in a path SEGMENT survives — nothing distinguishes a bot token from a resource id. Same exposure Host already carried on a surface that serves request bodies, so this widens an existing one rather than adding a class; recorded because someone exporting these events off-box should know. transparent.go now sets Path: "" explicitly, matching handleConnect instead of relying on the zero value. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
Both keys serialize the request path and neither godoc mentioned the other, leaving a consumer to guess whether they can disagree. They cannot: every Invocation.Path assignment in the tree copies pctx.Path, and Record back-fills from c.Path when a plugin leaves it unset. What actually differs is when each is PRESENT, which is the useful half and the reason the new field is not redundant. httpPath is on every event recorded from a parsed HTTP request, whether or not a plugin ran; Invocation.Path appears only where some plugin recorded an invocation. An event can carry invocations and no httpPath at all — an opaque tunnel that ran a gate — so httpPath is what a consumer should read for "the path of this request", with Invocation.Path staying per-invocation context. Documented reciprocally on both fields and in the operator wire-key list. No behaviour change. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Tight, well-scoped change: the verb and path were already on pipeline.Context at every recording site and simply never copied onto the event.
I verified the completeness claim mechanically rather than taking it on trust — 14 is the whole set of non-test pipeline.SessionEvent{} construction sites (forwardproxy 3, extproc 6, reverseproxy 4, transparent 1), and the response-phase events inherit the populated context in all three listener modes (extproc threads the request-phase pctx into handleResponseHeaders/handleResponseBody; the two proxies reuse the same pctx), so there is no half-populated timeline row. Other claims checked against source and confirmed:
Invocation.Path"back-filled byRecord" →pipeline/context.go:355-357.- "explicit, as in
handleConnect" →forwardproxy/server.go:1096does setPath: "". - "ext_authz never populates
Method, though it records no session events either" → zeroSessionEvent/Sessions.Appendreferences there, and it sets onlyPath. - The
PathOnlytable matches realnet/urlsemantics, fragment retention and theForceQuery/ unparseable fallback branches included. - "No abctl display work needed" holds:
filterForDetailallowlists only the nesteda2a/mcp/inferenceobjects and dropsidentity; top-level keys pass through, so both new keys show up underenterand in yank output. - The naming rationale is real —
eventMethodValueinabctl/tui/events_pane.godoes source the METHOD column from the protocol method.
omitempty on both wire keys preserves the skew contract TunnelReason set, and folding tunnel/tunnelReason into the omit-when-zero test is a nice incidental tightening.
No blocking issues. The one substantive note is that the new producer tests guard 2 of the 14 sites, and the reflection guards can't cover the other 12 — details inline.
Areas reviewed: Go (authlib pipeline + 4 listeners), Go tests, docs (authbridge/CLAUDE.md), the unauthenticated session-API exposure, and abctl consumer impact. All 3 commits signed off; CI green.
| // reach the timeline carrying only a host, so an operator could not tell a | ||
| // token refresh from an object download. The verb and path were already on | ||
| // the pipeline context; these assertions pin that they now reach the event. | ||
| func TestRecordOutboundResponse_CarriesMethodAndPath(t *testing.T) { |
There was a problem hiding this comment.
suggestion — these two tests pin 2 of the 14 recording sites, and the reflection guards can't cover the rest: TestSessionEventWire_HasEveryDomainField catches "this field never reaches the wire", and ..._EveryFieldSerializes catches "MarshalJSON dropped it" — neither catches "a recording site forgot to copy pctx.Method/pctx.Path".
So the 6 extproc sites, the 4 reverseproxy sites, and any site added next quarter can omit both fields with a fully green suite. Cheapest closure: add the same two assertions to one existing extproc event test and one reverseproxy event test, or factor an assertCarriesMethodAndPath(t, ev) helper and call it wherever an event is already asserted. Not blocking — just noting that the guard you have is a serialization guard, not a population guard.
There was a problem hiding this comment.
Confirmed the hole before closing it: I deleted both lines from all six extproc recorders and the entire authlib suite passed. You were exactly right that what I had is a serialization guard, not a population guard.
Fixed in 819008d — all 14 sites are now covered:
extproc/httppath_test.go— table test over all six recorders, one subtest each so a failure names the site.reverseproxy/httppath_test.go— an end-to-end POST through the real proxy (asserting both the request and response event, so the response is proven to still carry the pair after the context is threaded throughmodifyResponse), plus a direct test forrecordInboundReject, which the allow path can't reach.- forwardproxy's 3 + transparent's 1 were already covered.
Re-verified by stripping population from all ten newly-covered sites: every one fails, and passes again on restore.
Writing the extproc table surfaced a gate I hadn't accounted for — recordOutboundResponseSession snapshots InvocationPhaseResponse, so my request-phase-only fixture left it recording nothing and the subtest failed. I fixed the fixture to carry both phases rather than loosening the assertion.
| // the listener left the context fields unset — ext_authz never populates | ||
| // Method, though it records no session events either. | ||
| // | ||
| // A query string is always stripped before the path gets here (see |
There was a problem hiding this comment.
suggestion — this paragraph is precise that the query is stripped, but silent on the fact that the path is percent-decoded. httpx.PathOnly returns u.Path (decoded), and r.URL.Path is decoded for the two proxy listeners, so /a%2Fb is recorded as /a/b — path_test.go pins exactly that as deliberate cross-mode parity, which I agree with.
Worth one sentence here anyway, because this PR is what puts the path in front of an operator: encoded-path evasion (traversal, encoded slash) is precisely the thing someone reads a timeline for, and a reader who trusts the godoc's care about query strings will reasonably assume the value is the raw request target. Saying "the decoded path, not the raw request target" costs a line and prevents a wrong conclusion during an investigation.
There was a problem hiding this comment.
Good catch, and agreed the security framing is the point. Verified: /x/..%2f..%2fetc/passwd is recorded as /x/../../etc/passwd.
Added to the godoc in 819008d — it now says the recorded value is the decoded path rather than the raw request target, gives that traversal example, notes the parity rationale, and explicitly warns not to infer from the value that no encoded-traversal attempt was made. The operator doc says "query-stripped and percent-decoded" too.
| } | ||
| } | ||
|
|
||
| func containsQuestion(s string) bool { |
There was a problem hiding this comment.
nit — containsQuestion is strings.Contains(s, "?"). Worth the extra import over a hand-rolled byte loop.
There was a problem hiding this comment.
Agreed — replaced with strings.Contains(got, "?") and dropped the helper in 819008d.
| - `invocations` — per-plugin invocation records for every plugin that ran on the pipeline pass. Structured as `{inbound: [...], outbound: [...]}`; each entry carries `plugin`, `action` (one of 5 values — see below), `reason` (machine-stable code), and optional plugin-specific context (expected issuer, target audience, cache-hit flag, path, etc.). abctl renders one row per invocation, so operators see an explicit per-plugin timeline. | ||
| - `plugins` — escape-hatch map for plugin-specific observability. Keys are plugin names; values are the raw JSON each plugin emitted. Unknown plugins render as opaque JSON in abctl. See [`docs/plugin-reference.md`](docs/plugin-reference.md#emitting-session-events) for the producer contract. | ||
| - `identity`, `host`, `statusCode`, `error`, `durationMs` — request-level context. | ||
| - `httpMethod`, `httpPath` — the HTTP verb and query-stripped path, so a request no parser recognized is still identifiable rather than showing only a host. Distinct from the `method` inside `a2a` / `mcp`, which is a protocol method name, not an HTTP verb. On an opaque tunnel `httpMethod` is `CONNECT` and `httpPath` is absent — opaque bytes carry no request line, so a blank path there is accurate rather than missing. Query strings are stripped before the path is recorded, so query-borne credentials never reach the timeline; a secret embedded in a path *segment* (a bot token, a webhook path) does survive, on the same unauthenticated surface that already carried `host` and request bodies — worth knowing before exporting events off-box. `httpPath` carries the same value as the `path` inside each `invocations` entry (both copy the request path), but the two differ in when they are present: `httpPath` is on every recorded HTTP event, while an `invocations` `path` exists only where a plugin recorded something — so read `httpPath` for the request's path. |
There was a problem hiding this comment.
nit — this bullet is a ~10-line single paragraph that restates the SessionEvent godoc nearly sentence-for-sentence. The parts an operator reading this file actually needs are the first two sentences plus the path-segment caveat; the httpPath-vs-invocations.path derivation is already a one-liner in extensions.go and doesn't need re-deriving here. Trimming it also keeps the two copies from drifting.
There was a problem hiding this comment.
Fair — it had grown by accretion as I answered earlier review points. Trimmed from 173 to 95 words in 819008d: kept the two lead sentences, the tunnel case and the path-segment caveat, and dropped the httpPath-vs-invocations.path derivation since extensions.go already carries it. Agreed that also removes the drift risk.
Review pointed out that the two producer tests covered 2 of the 14 recording sites, and that the reflection guards in pipeline cannot cover the rest: they catch "the field never reaches the wire" and "MarshalJSON dropped it", never "a recording site forgot to copy pctx.Method". I confirmed the hole before closing it — deleting both lines from all six extproc recorders left the entire authlib suite green. Now covered: a table test over all six extproc recorders, and for reverseproxy an end-to-end request through the real proxy (so the response event is proven to still carry the request's method and path after the context is threaded through modifyResponse) plus a direct test for the denial recorder the allow path cannot reach. Re-verified by stripping population from all ten sites: every one fails, and passes again on restore. Writing those found a real gate I had not accounted for: recordOutboundResponseSession snapshots InvocationPhaseResponse, so a request-phase-only fixture leaves it recording nothing. The fixture now carries both phases rather than the assertion being loosened. Also from review: say that the recorded path is percent-DECODED, not the raw request target — "/x/..%2f..%2fetc/passwd" is stored decoded, which is exactly what someone reads a timeline to investigate, so the godoc now says not to infer from it that no encoded-traversal attempt was made. Use strings.Contains over a hand-rolled byte loop. Trim the operator-doc bullet from 173 to 95 words, dropping the httpPath-vs-invocations.path derivation that extensions.go already carries so the two cannot drift. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Problem
#906 reports that traffic Cortex doesn't parse as A2A/MCP/inference arrives in the timeline carrying only a host — every informative column an em dash — so an operator cannot tell a token refresh from an object download, and an implementor cannot see what Cortex isn't monitoring.
Change
The HTTP verb and path were already on
pipeline.Contextat every recording site.Pathis query-stripped and consistent across listener modes per its godoc;Methodis populated by every listener that records session events (ext_authz leaves it"", but it records none, so nothing here is affected). They were just never copied onto the event, so they never reached/v1/sessions/{id}. Three changes:SessionEventgainsHTTPMethod/HTTPPath.sessionEventWire+MarshalJSON/UnmarshalJSONcarry them ashttpMethod/httpPath, bothomitempty.pctx(reverseproxy, forwardproxy, transparent, extproc).pipeline.Contextneeded no change.Naming
HTTPMethod/HTTPPath, notMethod/Path: abctl already renders a METHOD column sourced from the protocol method (eventMethodValue→A2A.Method/MCP.Method/Inference.Model), which is not an HTTP verb. An unprefixedMethodwould put two different meanings under one name.omitemptyfollows the precedentTunnelReasonset in #929 — an old abctl ignores unknown keys; a new abctl against an older proxy sees"".Opaque tunnels keep an empty path
Deliberate, and asserted by a test so it isn't later "fixed" into something invented: the bytes are never parsed as HTTP, so there is no request line to read a path from. Those events carry a
CONNECTverb and no path.Verification
Actual wire output from the recording path, matching the issue's screenshot:
{ "host": "s3.us-south.cloud-object-storage.appdomain.cloud", "httpMethod": "GET", "httpPath": "/bucket/object.json" } { "host": "api.us-east.bob.ibm.com:443", "tunnel": true, "tunnelReason": "passthrough-host", "httpMethod": "CONNECT" }authlibsuite +go vetpass;authbridge-proxy,authbridge-envoyandabctlall build.TestSessionEventWire_HasEveryDomainField,..._EveryFieldSerializes) are the primary correctness check. I confirmed the value-level guard genuinely fails when a field is dropped fromMarshalJSON, then restored it.Note on what a path can carry
Query strings are stripped before recording (
httpx.PathOnly, now under test), soquery-borne credentials never reach the timeline. A secret embedded in a path segment —
a bot token, a webhook path — does survive, since nothing distinguishes it from a resource
id. That is the same exposure
hostalready carried on this unauthenticated surface,which serves request bodies besides, so this widens an existing exposure rather than
adding a class. Documented in the field godoc and the operator doc for anyone exporting
events off-box.
Scope
Deliberately the recording side only, and no abctl display work is planned: the fields already reach an operator through the event-detail JSON (
enter) and yank-to-file ([y]), both of which render the whole event, so a dedicated PATH column is not needed for this to be useful. Whether that fully satisfies #906 is the maintainers' call. No refactors, renames, or adjacent fixes; pre-existinggofmt/ruffdebt in unrelated files was left untouched.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
CONNECTand leave the path empty.Documentation
Tests