fix(plugins): sanitize BigQuery analytics rows and build GCS paths per call (v1) - #6808
Open
GWeale wants to merge 4 commits into
Open
fix(plugins): sanitize BigQuery analytics rows and build GCS paths per call (v1)#6808GWeale wants to merge 4 commits into
GWeale wants to merge 4 commits into
Conversation
…er raises (v1) Before, if the configured `content_formatter` raised, the plugin logged a warning and wrote the original, unformatted payload to BigQuery. The formatter is the operator's redaction boundary, so a failing formatter published exactly the content it was installed to remove. Now a formatter that raises causes the sentinel string `[FORMATTER_FAILED]` to be written in place of the payload. The warning it logs is a constant message plus the event type; the exception text is deliberately not interpolated, because an exception message or traceback can itself quote the protected content. Behaviour change: rows whose formatter failed now carry `[FORMATTER_FAILED]` in the `content` column instead of the original payload.
…ics (v1) Before, a `file_data` part's URI was written into the `content_parts` column exactly as supplied. A signed GCS or HTTP URL carries its signature in the query string and a URI may carry `user:password@` userinfo, so the row stored a working bearer credential that anyone with read access to the table could replay. Now the URI goes through a sanitizer before it is recorded. A URI with userinfo, one longer than 8192 characters, or one that does not parse is replaced with `[REDACTED_SENSITIVE_URI]`. Otherwise the query string and fragment are dropped and the scheme, host and path are kept, which is what identifies the object. A URI with none of those components is stored unchanged. Behaviour change: URIs in `content_parts` are no longer resolvable by copy-paste when they were signed, and a row whose URI was altered now has `is_truncated` set.
Before, `_enrich_attributes` ran the redacting sanitizer over `usage_metadata`, `cache_metadata` and `session.state`, but copied `extra_attributes` and `custom_tags` in untouched and then serialized the result. A session state delta or a configured tag holding a key such as `api_key` or `refresh_token` therefore reached the `attributes` column in the clear. The assembled tree now goes through `_recursive_smart_truncate` once more, immediately before serialization, so every value in the column has seen the sensitive-key redaction regardless of which producer put it there. That pass walks objects the sanitizer never saw before, which exposed a second problem. `_recursive_smart_truncate` detects cycles by object id, and an object whose `model_dump`, `dict` or `to_dict` returns a freshly built wrapper defeats that, because the walk never sees the same id twice and keeps descending. It now stops at a depth of 50 and substitutes `[MAX_DEPTH_EXCEEDED]`. Depth is only half a bound, because it says nothing about width. An object that hands back two fresh children on every access fills the 50 levels beneath it with tens of millions of nodes, and one such value in a state delta held the event loop for over a minute in testing. The walk now also carries a budget of 100,000 nodes for the whole invocation and replaces the remainder with `[SANITIZE_BUDGET_EXCEEDED]`. A directly redacted key spends budget too, so a wide `temp:`-scoped mapping cannot slip past the bound, and each container loop stops at the budget rather than emitting one sentinel per remaining element. The `mock_agent` test fixture now returns itself from `root_agent`, matching the pattern already used elsewhere in the file, so `root_agent_name` is a real name instead of a bare mock object. Behaviour change: the `attributes` column can now contain `[REDACTED]` and truncation markers where it previously carried raw values, and `is_truncated` is set when the attributes pass alters anything. A value already truncated by `_enrich_attributes` gains a second `...[TRUNCATED]` marker.
…(v1) Before, `_log_event` assigned the event's trace and span ids onto the single shared `HybridContentParser` instance and then awaited the parse. The offload path was built from those instance fields after the await, so a second event arriving in the meantime replaced them and the first event's media was written under the second event's prefix. Object names were also built from the part index alone, so two messages in one request, or two events offloading at the same moment, produced the same name and one overwrote the other. `parse` and `_parse_content_object` now take the trace and span ids as keyword arguments, defaulting to the instance fields so existing callers are unaffected. Each `parse` call generates a unique id that goes into the object name, along with the index of the message within the request. `_log_event` passes the ids instead of assigning them, so the shared parser is no longer mutated. Behaviour change: offloaded GCS object names gain a unique component and a message index, so they are no longer predictable from the trace id, span id and part index.
GWeale
changed the title
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
2 participants
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.
Four changes to
BigQueryAgentAnalyticsPluginon thev1branch, one commit each, re-implemented against v1's code rather than cherry-picked.Content formatter failure —
7c96e6b8, from2919bf5bcontent_formatterraises holds[FORMATTER_FAILED]incontent.External URIs —
1ba04e1d, from9adf0113content_parts[].urikeeps scheme, netloc and path only.urlsplitrejects becomes[REDACTED_SENSITIVE_URI].Attributes tree —
ef4b10f1, from2919bf5battributesholds[REDACTED]for_SENSITIVE_KEYSandtemp:-scoped keys fromextra_attributesandcustom_tags.[MAX_DEPTH_EXCEEDED]) and 100,000 nodes per invocation ([SANITIZE_BUDGET_EXCEEDED]).GCS offload paths —
4c34f25f, from2919bf5b<date>/<trace_id>/<span_id>_<parse_uid>_c<content_ordinal>_p<part_index><ext>; existing objects are untouched.is_truncatedis set when the attributes or URI pass alters a value. There is no opt-out, so dashboards readingcontent,content_parts,attributesoris_truncatedmay need updating. The plugin is opt-in and is not re-exported fromgoogle.adk.plugins.The File Content Compliance check fails for a pre-existing reason; this PR adds no endpoint URL.