Track auth-origin and Pro API key fingerprint in usage analytics#424
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds an authorization-origin telemetry signal ( ChangesAuthorization-origin telemetry attribution
Sequence Diagram(s)sequenceDiagram
participant Decorator as log_tool_invocation
participant Telemetry as telemetry.resolve_auth_signals
participant Context as pro_api_key_context.compute_auth_signals
participant Analytics as analytics.track_tool_invocation
participant Community as telemetry.send_community_usage_report
Decorator->>Telemetry: resolve_auth_signals(ctx)
Telemetry->>Context: compute_auth_signals(ctx)
Context-->>Telemetry: (auth_origin, api_key_fingerprint)
Telemetry-->>Decorator: (auth_origin, api_key_fingerprint)
Decorator->>Analytics: track_tool_invocation(..., auth_origin)
Analytics-->>Decorator: Mixpanel event with auth_origin
Decorator->>Community: send_community_usage_report(..., auth_origin, api_key_fingerprint)
Community-->>Decorator: POST payload with auth fields
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Both compute_auth_origin and compute_api_key_fingerprint independently extracted the ctx key state and re-implemented the same client→server precedence ladder, so the two signals could silently diverge if the precedence ever changed in one but not the other. Callers that need both (the tool decorator and the resource-read sink) also extracted the state twice per call. Introduce compute_auth_signals(ctx) -> tuple[AuthOrigin, str | None] as the single source of truth: one extraction pass, with the (origin, fingerprint) pair returned co-located per branch so they cannot disagree. compute_auth_origin / compute_api_key_fingerprint become thin views over it; the decorator and observability paths now unpack the pair from one call. The Mixpanel path keeps using compute_auth_origin (fingerprint stays out of the property bag). Add tests pinning the (origin, fingerprint) pair per precedence branch and asserting the thin wrappers stay in lockstep with the combined result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address PR #424 review findings: - De-duplicate auth-origin/fingerprint derivation on the hot path. The tool decorator and the resource-read observability path now compute compute_auth_signals(ctx) once and thread the origin into track_tool_invocation / track_resource_read (which fall back to deriving it themselves for direct callers), reusing the fingerprint for the community report. Previously each tracked call extracted the headers twice and ran two SHA-256 hashes (one discarded inside compute_auth_origin). Mirrors the existing "compute client_meta once and thread it through" pattern. - Document compute_api_key_fingerprint as an intentionally-retained public view kept for the deferred fingerprint consumer (no production caller yet). - Clarify the README privacy section so "we do not collect secrets/keys" no longer reads as contradicting the new one-way key fingerprint. Adds a regression test asserting the signals are derived once and the same values reach both sinks. Version intentionally not re-bumped (follow-up commit within the already-versioned PR). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…or, strengthen tests
Follow-up to the auth-origin / api-key-fingerprint telemetry work, applying
code-review findings:
- decorators.py: guard compute_auth_signals(ctx) in try/except with a
(None, None) fallback, mirroring the resource-read path in observability.py,
so this observability concern can never propagate into the tool body if the
"never raises" contract later changes.
- models.py / constants.py: drop the redundant max_length on the
api_key_fingerprint field (the anchored pattern ^[0-9a-f]{64}$ already fixes
the length to exactly 64) and remove the now-unused
PRO_API_KEY_FINGERPRINT_MAX_LENGTH constant.
- models.py: align auth_origin / api_key_fingerprint field descriptions with
API.md — "available to back the reported call", not "used for" — clarifying
the signals reflect the request's authorization context, not key consumption.
- tests: replace the vacuous auth_origin membership assertion with an exact
"client" check; rewrite the "derive once" regression guard to drive the real
analytics sink (only Mixpanel mocked) and spy on the real helpers so a dropped
auth_origin thread is actually caught; pin the exact server-key fingerprint at
the resource-read forwarding boundary instead of asserting merely non-null.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both observability paths (the tool decorator and the resource-read sink) duplicated the same guarded `compute_auth_signals(ctx)` call. Extract it into `telemetry.resolve_auth_signals`, the single derivation point shared by both, so the ctx extraction, the SHA-256, and the defensive guard are written once. The helper also short-circuits to (None, None) — without touching ctx — when no sink can consume the signals (not HTTP mode AND community telemetry disabled), avoiding a per-call SHA-256 of the configured server key. Behavior is unchanged: in that state both sinks already short-circuit on their own gates before reading these values. Tests: re-point the "derived once" regression guard at the new single derivation point; add unit coverage for the gate (short-circuit, both derive branches, never-raises). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to PR #424 (auth_origin + api_key_fingerprint in usage analytics, closes #423), applying the accepted review decisions: - Tolerate a malformed api_key_fingerprint instead of rejecting the report: a before-validator coerces any non-64-lowercase-hex value to null (report still 202), preserving the "present => valid" invariant; auth_origin stays strictly validated (422). - Gate the server-key fingerprint on its only consumer: derive it (and hash the server key) only when community reporting is enabled, threaded via resolve_auth_signals(include_server_fingerprint=...). - Remove compute_api_key_fingerprint; compute_auth_signals is the single source of truth for the (auth_origin, fingerprint) pair. - compute_auth_origin passes include_server_fingerprint=False so the origin-only path no longer computes a server-key SHA-256 it discards. - Drop the redundant ^/$ anchors from _FINGERPRINT_PATTERN (matched via fullmatch, behavior-identical). - Trim SPEC/API/AGENTS to one home-of-record: why in SPEC, field shape in API, search hint in AGENTS; mechanics live in code + tests. - Update and extend unit tests to pin all of the above. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make the four new auth-origin/fingerprint tests hermetic to the BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY environment variable. With community telemetry disabled and HTTP mode off, resolve_auth_signals short-circuits to (None, None), so an ambient env/.env value would fail the auth_origin assertions even though production is correct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The analytics sink's `auth_origin if not None else compute_auth_origin(ctx)` fallback was unreachable when HTTP mode is off (track_tool_invocation early-returns) and actively harmful in the one reachable case: when resolve_auth_signals swallowed a compute_auth_signals exception and returned None, re-deriving via compute_auth_origin(ctx) re-ran the same failing chain, was eaten by the except, and dropped the whole Mixpanel event — contradicting the "graceful degradation" the docstrings promised. Map None to AUTH_ORIGIN_UNKNOWN (mirroring track_community_usage) so the event is still sent, and remove the now-dead compute_auth_origin helper, its import, tests, and docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ingerprint
The include_server_fingerprint gate mirrored only one of send_community_usage_report's
two suppression conditions: it skipped the server-key SHA-256 on disable_community_telemetry
but not on the (is_http_mode_enabled() and mixpanel_token) early-return. On the official
server (HTTP + Mixpanel token + community telemetry enabled + server key set) the hash was
therefore recomputed on every tool call / resource read for a community report that is
guaranteed never to send.
Since config.pro_api_key is fixed for the process lifetime, memoize its fingerprint once
(functools.lru_cache) and remove the include_server_fingerprint parameter, its branch in
compute_auth_signals, the resolve_auth_signals gate argument, and the flag's docstring
sections. Client keys stay on the un-cached path so a per-request secret is never retained.
Replace the flag/spy tests with memoization coverage and repurpose the telemetry end-to-end
test to assert the full ("server", <hash>) pair.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Community usage reports arrive fire-and-forget from independently-versioned instances, and a rejection is silent on both ends (the sender ignores the response; the receiver returns 422 with no log). So a strictly-validated auth_origin meant any value this receiver did not recognize — a future enum member rolled out to reporters first, wrong capitalization, or junk — dropped the entire otherwise-valid report with no visibility. Mirror the existing api_key_fingerprint coercion: a mode="before" validator coerces any value outside AuthOrigin (plus null) to None, which track_community_usage already renders as the "unknown" bucket that legacy field-absent reports fall into. The coercion is logged at debug so version skew stays observable. This makes both forward-compat wire signals symmetric and honors issue #423's backward-compatibility goal under version skew. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
compute_auth_signals mirrored resolve_pro_api_key's client->server precedence with a second isinstance-over-state chain, coupled only by a docstring. A future precedence change (new key source, different malformed handling) would land in one chain and silently skip the other, so auth_origin would diverge from the key actually enforced — the exact consistency issue #423 exists to protect. Extract _apply_key_precedence(state) as the sole owner of the precedence, returning a discriminated decision (_UseClientKey / _RejectMalformed / _UseServerKey). resolve_pro_api_key now only applies raise/ContextVar semantics to that decision; compute_auth_signals only maps it to an (origin, fingerprint) pair. Neither re-decides precedence. The decision is richer than a bare (origin, effective_key | None): it keeps _RejectMalformed distinct from an absent-with-empty-server key so resolve still emits its two separate errors (malformed vs not-configured). Behavior of both public functions is unchanged. Add a parametrized coupling test asserting the reported auth_origin stays consistent with the key resolve_pro_api_key uses, so a future re-divergence of the two wrappers fails in CI rather than at review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
SPEC.md: the Authorization-context bullet restated the auth_origin/ fingerprint validation mechanics nearly verbatim from the ToolUsageReport validator docstrings and API.md, violating rule 1 (keep SPEC.md lean; details discoverable through code). Trim to the decision level — new dimension + fingerprint, privacy posture, and the version-skew rationale for tolerating unknown wire values — dropping the coercion mechanics (null coercion, invalid-value taxonomy, present-implies-valid invariant, downstream bucketing, skew logging). Point to API.md for the wire shape and to the validators for the coercion behavior. AGENTS.md: drop the 'runs outside @pro_api_key_scope' implementation detail from the compute_auth_signals() entry per rule 2 (search hint, not how-it-works prose); keep the brief consumer note at the altitude of neighboring entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review: extract the duplicated MCP-context builders into a shared tests/pro_api_key_helpers.py, and have the boundary tests (telemetry, observability, decorators) assert the forwarded fingerprint against the production _fingerprint_pro_api_key helper instead of hand-re-deriving prefix+sha256. A single independent scheme re-derivation (_expected_fingerprint) stays in the auth-signals unit tests, so a future scheme change (v1 -> HMAC pepper) touches exactly one place while wiring tests still catch a wrong forwarded hash. Also list the new shared helper in the AGENTS.md project-structure tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
.encode() defaults to UTF-8, so the explicit argument was byte-identical and only forced a UP012 suppression that obscured intent. Use .encode() and remove both noqas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `_tolerate_unknown_auth_origin` validator logged the rejected value verbatim (`value=%r`), while its sibling `_tolerate_malformed_fingerprint` deliberately logs only the type — because a field-swap bug in an independently-versioned reporter could land a raw key or fingerprint in `auth_origin`, and this codebase never logs the key. Log the value verbatim only when it is a short string (legitimate version-skew values are short enum members); anything longer — a 64-hex fingerprint, a raw key, or other junk — is logged by type/length only. Preserves version-skew visibility without leaking secret-shaped content into the central server's debug logs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two-branch debug logging in _tolerate_unknown_auth_origin (short value → verbatim, long → type/len) plus the _MAX_LOGGABLE_AUTH_ORIGIN_LEN constant guarded a threat model that does not hold: auth_origin is the one report field whose unrecognized value is destroyed by coercion to None and never leaves the process, while client_name/tool_args are forwarded to Mixpanel verbatim. The real "key never leaves the instance" guarantee lives on the sender side (compute_auth_signals never returns the raw key), and the cap did not even deliver its promise (client keys of 1-16 chars are legal and would print verbatim). Always log only the value's type and length, mirroring the neighboring _tolerate_malformed_fingerprint, and drop the overstated no-secret-in-logs reasoning from the docstring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address code-review finding on the auth-signal derivation gate: the `not http_mode and disable_community_telemetry` check sat outside the try (so the "Never raises" contract only covered compute_auth_signals), duplicated both sinks' gating knowledge inline, and was explained by a thrice-duplicated rationale comment. - Extract the condition into a single `is_any_telemetry_active()` predicate (one source of truth) and evaluate it inside the try, so no observability concern can propagate into the tool body. - Keep the derive-when-any-telemetry-active precondition: the fingerprint is forward-provisioned for Mixpanel distinct_id (per user/instance by deployment), so it must be derived whenever a sink is live, not only when community telemetry is on. Documented in the docstring + SPEC.md. - Correct the SPEC distinct_id note per deployment scenario (the public server's shared server key is deliberately not used as an identity). - Collapse the triple-duplicated rationale at both call-sites to a one-line pointer. - Add tests: gate-raises degradation and the predicate truth table. Behavior is unchanged in every telemetry configuration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- AGENTS.md: add the two new test modules to the structure tree - test_decorators.py: reuse ctx_with_header helper at the three sites that hand-rolled the same request-context shape - test_routes.py: add post_report() helper and dedup the six report-endpoint tests onto it Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_pro_api_key_context_auth_signals.py`:
- Around line 1-7: Clean up the module docstring in the test module so it no
longer contains the garbled “rule ``210``” placeholder text. Update the
rationale text near the top-level docstring in this file to use a clear,
human-readable explanation for why the tests live in a sibling module, and
remove any repeated or unresolved rule references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d393b9ea-7c32-440d-a5ab-0e9b79622fb5
📒 Files selected for processing (23)
AGENTS.mdAPI.mdREADME.mdSPEC.mdblockscout_mcp_server/__init__.pyblockscout_mcp_server/analytics.pyblockscout_mcp_server/constants.pyblockscout_mcp_server/models.pyblockscout_mcp_server/observability.pyblockscout_mcp_server/pro_api_key_context.pyblockscout_mcp_server/telemetry.pyblockscout_mcp_server/tools/decorators.pypyproject.tomlserver.jsontests/api/test_routes.pytests/pro_api_key_helpers.pytests/test_analytics.pytests/test_observability.pytests/test_pro_api_key_context.pytests/test_pro_api_key_context_auth_signals.pytests/test_telemetry.pytests/test_tool_usage_report.pytests/tools/test_decorators.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
1 participant
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.
Summary
Adds two new dimensions to usage analytics and community telemetry so we can tell how a request was authenticated without ever recording the secret itself:
auth_origin—"client"(caller supplied a valid Pro API key via context),"server"(no client key, but the server has one configured),"none"(neither), or"unknown"(legacy reports with no value). Derived purely from the requestctx— never viaresolve_pro_api_key(), so the two signals stay independent.api_key_fingerprint— an irreversible SHA-256 hash (bs-pro-key-v1:prefix + UTF-8 key bytes, 64-char lowercase hex) of the client-supplied Pro API key. Lets us correlate usage per key without ever transmitting or storing the key. The raw key is never emitted to Mixpanel or the community endpoint.Closes #423.
What changed (by layer)
ToolUsageReportgains optionalauth_origin/api_key_fingerprintfields (fingerprint validated against^[0-9a-f]{64}$);AuthOriginliteral type and supporting constants added.compute_auth_origin(ctx)andcompute_api_key_fingerprint(ctx)inpro_api_key_context.py, both built on the existing client-key extraction and with no fallback to the resolved server key.auth_originadded totrack_tool_invocation,track_resource_read, andtrack_community_usage(legacyNone→unknown); fingerprint deliberately kept out of the Mixpanel property bag.send_community_usage_report/send_community_resource_reportpayloads.422) and explicit-null payloads.0.17.0.dev2acrosspyproject.toml,__init__.py, andserver.json(mcpb manifests intentionally untouched).Testing
ruff check .andruff format --check .clean.Reviewer notes
tests/integration/files were touched. The live integration run above is a confirmation that the suite is unaffected.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation