[OPIK-7834] [BE] feat: mask trace content by field name at read time - #8043
[OPIK-7834] [BE] feat: mask trace content by field name at read time#8043thiagohora wants to merge 2 commits into
Conversation
Replaces the read-time regex redaction that shipped in #7973. Masking moves from the response serializer into the DAO row mappers, keyed on configured leaf field names rather than patterns, and carried on the reactive context the way workspaceId and visibility already are. What this removes: the Jackson module and its exemption list, the writer interceptor, the streamed-tree walk, the regex rule set, and MatchBudget with it. No regex means no ReDoS and no work bound to police, and no exemption list means no name-collision hazard in either direction - the structural fields the API addresses by (thread_id, project_name, model, id) are separate columns and were never candidates. Configuration becomes a YAML list of field names instead of JSON-in-a-scalar. What carries forward from the merged branch: permissions resolved from the workspace permissions API per credential type, the original_data_view rename, the opt-in permission model, the session-cookie coverage, and the config-test.yml keys. The Streamer no longer redacts, so the ProvisionException and cause-chain handling it needed is gone with it. Free-form SQL is now masked by shape rather than refused, since the caller chooses the projection; the dataset CSV export and attachment download are still refused, and the guard's javadoc is corrected to say so.
Runner jobs are read from Redis rather than through the masked DAOs, so moving masking into the DAOs left them returning stored content - a coverage regression against what the serializer approach covered by construction. Their inputs are the payload an agent was invoked with and their result is what it produced, which is the same caller content a trace carries. Masked in the resource, which is where the decision is readable: all three read paths already take workspaceId and userName from the request context, and nextJob captures the masker before subscribing so the reactor thread that resumes the long poll never touches the request scope. The two long-poll tests from the merged branch come back with it, restated for field masking - the configured field is replaced, the unnamed one survives, and a permitted caller still reads stored content.
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 41 skipped (no matching files changed)
|
Why the matching is hand-written and not Jayway JsonPathRecording this up front, because it was raised on #7973 and this PR is where the answer now lives. Short version: JsonPath is available and the instinct behind the suggestion was right, but it does not fit this specific job, for reasons already recorded in this codebase.
1. It needs the one construct this codebase bans. The mechanism is "mask the value of every leaf whose configured field name matches, at any depth", i.e. 2. It does not operate on the type we hold. In fairness, that note names 3. Two of the three operations have no JsonPath expression.
So the tree walk would not disappear — we would maintain it and a path grammar we then have to restrict. What the suggestion did get right is the level above the tool: declare what to mask rather than enumerate what to exempt. This PR makes exactly that change, and it is what removes the regex, Happy to be pushed back on — particularly on the provider caveat, which is testable if someone thinks it is worth the measurement. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| // Same path, same nonexistent job, caller who may see originals: anything but 403 proves the 403 above | ||
| // came from the masking decision rather than from authentication or the path itself. | ||
| try (var response = downloadExport(ADMIN_API_KEY)) { | ||
| assertThat(response.getStatus()).isNotEqualTo(HttpStatus.SC_FORBIDDEN); | ||
| } |
There was a problem hiding this comment.
Admin export regression remains undetected
The admin assertion only checks for a non-403 response on a nonexistent job ID, so unrelated 401/404/500 failures pass and the permitted download path remains untested — should we generate a real export, assert its successful status and content, and retain the member 403 assertion?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ReadTimeRedactionResourceTest.java
around lines 299-303, update `theRefusalIsTheMaskingDecision` so it exercises a real
generated export rather than a random nonexistent job. Assert the permitted admin
response’s expected success status and returned export content, not merely that it is
different from 403; retain the member test’s explicit 403 assertion.
| // deepCopy because the job may be served from a cached deserialisation, and mask rewrites in place. | ||
| return job.toBuilder() | ||
| .inputs(job.inputs() == null ? null : masker.mask(job.inputs().deepCopy())) | ||
| .result(job.result() == null ? null : masker.mask(job.result().deepCopy())) | ||
| .build(); |
There was a problem hiding this comment.
Masked callers receive raw runner content
LocalRunnersResource masks only inputs and result, so runner-supplied error and persisted LocalRunnerLogEntry.text returned by getJobLogs reach callers lacking original_data_view unchanged after response-wide redaction was removed — should we apply one explicit masking boundary to all returned runner job content?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/LocalRunnersResource.java
around lines 209-218, update the `masked` helper so caller redaction also masks the
`LocalRunnerJob.error` field instead of copying it unchanged. In the `getJobLogs` logic
around the reported lines 209-218, apply the same caller-specific masker to every
`LocalRunnerLogEntry.text` before returning logs, while preserving each entry’s
metadata and the existing no-op behavior. Ensure `listJobs`, `getJob`, and `getJobLogs`
all pass through this explicit masking boundary.
| return new AnalyticsQueryResponse(response.results().stream() | ||
| .map(result -> masker.maskEveryString(result.deepCopy())) | ||
| .toList()); |
There was a problem hiding this comment.
Doubles peak memory for large queries
result.deepCopy() creates a second JSON tree before maskEveryString traverses it, so large SQL responses briefly consume roughly twice the tree memory and incur a full extra traversal — should we mask in place when the response is unshared or during construction instead?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/internal/AnalyticsQueriesResource.java`
around lines 114-116, refactor `maskIfRequired` so each analytics result is not fully
duplicated with `deepCopy()` before `maskEveryString` traverses it. Determine whether
the service-owned response can be safely mutated and mask the existing nodes in place;
otherwise apply masking while constructing the results or use a streaming/structural
approach that avoids retaining both complete JSON trees. Preserve the current redaction
behavior and response immutability guarantees where required.
| // The file was generated by a background job with no caller, so it holds stored content whatever this | ||
| // caller may see. One artifact, many readers: it can only be withheld, not masked. | ||
| RedactionGuard.rejectUnmaskable(requestContext.get().isRedactResponse(), "Dataset CSV export download"); |
There was a problem hiding this comment.
Undocumented 403 breaks export clients
downloadDatasetExport now returns 403 when redaction is enabled and the caller lacks original_data_view, but @Operation advertises only 200, 400, and 404, so released clients classify this expected refusal as generic ApiError/OpikApiError and the frontend provides no permission-specific handling. Should we document the 403 and original_data_view requirement in the source operation and regenerate the API docs, leaving client/UI handling to the appropriate follow-up if generated artifacts are intentionally deferred?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DatasetsResource.java`
around lines 1010-1012, update `downloadDatasetExport` and its OpenAPI annotations to
document that redaction-enabled callers must have the `original_data_view` permission
and otherwise receive a 403 response. Add the 403 response to the operation metadata and
clearly describe the prerequisite, then regenerate and publish the API documentation
through the normal workflow; if generated client/UI artifacts are intentionally
deferred, record the corresponding follow-up handling.
| public static DatasetItem buildItemFromRow(Row row, RowMetadata rowMetadata, FieldMasker masker) { | ||
|
|
||
| Map<String, JsonNode> data = getData(row); | ||
| Map<String, JsonNode> data = getData(row, masker); |
There was a problem hiding this comment.
Dataset comparison leaks nested experiment content
buildItemFromRow applies the request FieldMasker only to top-level data, while getExperimentItems(...) parses experiment_items_array directly into ExperimentItem.input, output, and traceMetadata; those fields then reach ExperimentItem.View.Compare on /items/experiments/items, so callers without original_data_view receive unmasked nested content. Should we pass the masker through nested reconstruction and apply it to those JSON nodes before building each ExperimentItem?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemResultMapper.java
around lines 167-169, update `buildItemFromRow` so the request `FieldMasker` is also
passed into nested `experimentItems` reconstruction. Refactor `getExperimentItems` and
its callers to apply the masker to each `ExperimentItem`’s `input`, `output`, and
`traceMetadata` JSON nodes before constructing the objects, while preserving the
existing unmasked behavior when appropriate. Add or update tests to verify that requests
without `original_data_view` cannot receive nested experiment content.
| * The last is chosen because it is free and cannot be defeated by the input. If partial masking is ever required, | ||
| * write-time detection is the route — not a return to matching on read. | ||
| */ | ||
| public record FieldMasker(@NonNull Set<String> maskedFields, @NonNull String replacement) { |
There was a problem hiding this comment.
Redaction policy can drift at runtime
FieldMasker retains the caller-supplied mutable maskedFields reference, so later mutations can change a singleton's masking rules at runtime even though RedactionConfig.compile() currently uses Set.copyOf(...) — should we add a canonical constructor that snapshots maskedFields with Set.copyOf(...) and test that source-set mutations do not affect the record?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/redaction/FieldMasker.java
around line 40, update the `FieldMasker` record’s canonical construction so
`maskedFields` is defensively copied with `Set.copyOf(...)`, preventing caller mutations
from changing redaction behavior after construction. Add a focused test in the
corresponding FieldMasker test file that mutates the original set after creating the
record and verifies the record’s masking rules remain unchanged.
| // Numbers, booleans and nulls carry no free text and are left as stored. | ||
| return node; |
There was a problem hiding this comment.
Sensitive numeric values remain exposed
maskWholeValue leaves numeric and boolean scalar nodes unchanged, so configured sensitive fields remain exposed when redactResponse is true — should we replace matched scalars regardless of JSON type while preserving object/array structure?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/redaction/FieldMasker.java
around lines 152-153, fix the maskWholeValue logic so values under configured sensitive
fields cannot expose numeric, boolean, or other scalar content. Refactor the masking
helpers so matched fields replace every scalar with the replacement while preserving
object and array structure, and keep maskEveryString’s documented behavior separate if
it should continue leaving non-string values unchanged. Add or update tests covering
numeric and boolean sensitive fields.
| RequestContext current = requestContext.get(); | ||
| boolean redact = redactionService.shouldRedactFor(current.getPermissions()); | ||
|
|
||
| current.setRedactResponse(redact); | ||
| context.setProperty(REDACT_RESPONSE_PROPERTY, redact); | ||
| current.setRedactResponse(redactionService.shouldRedactFor(current.getPermissions())); | ||
| } |
There was a problem hiding this comment.
Dataset expansion leaks configured content fields
RedactionRequestFilter records the decision only on RequestContext, while DatasetExpansionService.buildDatasetItem copies parsed LLM fields into DatasetItem.data unchanged, so DatasetsResource.expandDataset returns generated_samples[].data.prompt (or content) verbatim to callers without original_data_view now that the global RedactionModule/RedactionWriterInterceptor is gone — should we mask generated samples before building the response or add an equivalent explicit response-path mask?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/redaction/RedactionRequestFilter.java
around lines 62-64, the `filter` method records the redaction decision only on
`RequestContext`, but the dataset expansion response does not consume it. Trace
`DatasetsResource.expandDataset` and `DatasetExpansionService.buildDatasetItem`, then
apply the same configured-field redaction to each generated sample’s `data` before
constructing the response, while honoring `original_data_view`. Add or reuse an explicit
response-path masking mechanism so callers without permission cannot receive
`generated_samples[].data.prompt` or `content` verbatim.`
1 participant
Details
Second iteration of read-time redaction, replacing the regex mechanism that shipped in #7973. Masking moves out of the Jackson serializer and into the DAO row mappers, keyed on configured leaf field names rather than patterns, and carried on the reactive context the way
workspaceIdandvisibilityalready are.Net 853 insertions, 2030 deletions — the mechanism swap removes far more than it adds, and it resolves several review threads from #7973 by construction rather than by argument:
MatchBudgetand its 199 lines of tests are gone, along with the per-character allowance, the ceiling and the output-growth bound.thread_id,project_name,model,id) are separate columns and were never candidates, so there is nothing to exempt and nothing to keep in sync. TheExperiment.promptVersions[].commitpaged-vs-streamed divergence disappears with it.redaction.maskFields) instead of a JSON array smuggled through a scalar.map('x', input),base64(input)); masking every string in the result cannot be, and it leaves counts and grouping over non-content dimensions working.Carried forward from #7973 unchanged: permissions resolved from the platform's workspace permissions API per credential type, the
original_data_viewrename, the opt-in permission model, and the session-cookie coverage. TheStreamerno longer redacts, so theProvisionExceptionnarrowing and cause-chain handling it needed are gone with it.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
main(mechanism swap, runner-job masking, config and test reconciliation), plus the commit messages and this description.Testing
mvn testinapps/opik-backend, local process mode, macOS, Testcontainers for MySQL/ClickHouse/Redis/Zookeeper:TracesResourceTest— 392 passed. The important one: every trace and span row mapper signature changed, so this is the regression surface for normal, unmasked reads.ReadTimeRedactionResourceTest— 12 passed. Covers masked vs stored per credential type (api key and session cookie), spans, the streamed/paged parity, the refusal paths, and the two long-poll runner-job cases.ReadTimeRedactionDisabledResourceTest— 2 passed. The feature off, end to end.RedactionConfigTest,FieldMaskerTest,RedactionGuardTest,RedactionServiceTest,RequestContextConstructionArchTest,RemoteAuthServiceTest(58),Local/RemoteWorkspacePermissionsServiceTest— 95 passed together.DatasetsResourceIntegrationTest— 2 passed.Coverage boundary, worth reviewing deliberately. Masking is applied where a content column becomes a
JsonNode, which means it is applied where it is wired rather than everywhere by construction — the opposite of the serializer approach. Currently wired: trace, span, thread, dataset-item, dataset-item-version and experiment-item DAOs, analytics queries, and runner jobs. Two consequences:inputsandresultreturning stored content — a regression against [OPIK-7834] [BE] feat: redact trace content on read for callers without the original-data permission #7973, which covered them by construction. Masked in the resource, with the masker captured beforenextJobsubscribes so the reactor thread that resumes the long poll never touches the request scope.A new endpoint or DAO is therefore unmasked by default. That is the trade for losing the exemption list, and it wants a convention or an ArchUnit rule if this stops being a PoC.
Not run: the full backend suite, and the frontend. Scope was the redaction mechanism, the auth/permissions path it reads from, and the DAOs it touches.
Two fixes to inconsistencies in the pre-port branch, found while reconciling: its
RedactionGuardjavadoc still listed free-form SQL as refused after its own final commit changed that to masking, and the analytics resource ended up carrying both the refusal and the masking.Documentation
config.ymldocumentsmaskFieldsandreplacementin full: why names rather than paths (the same field sits at different depths depending on the integration —messages[].content,messages[].kwargs.message.content,function.arguments), that the shipped seven names are a starting point and not a guarantee, that keys are never rewritten and therefore sensitive data must live in values, and that switching this on changes what a public project shows.config-test.ymlcarries the keys so the two cannot drift apart, including a comment on why the placeholder list entry exists (Dropwizard's indexed overrides only replace an index that already exists).Depends on
The platform counterpart, comet-ml/comet-backend#5650, must deploy first — it defines
original_data_viewand serves it from the workspace permissions API. Withredaction.enabled=falsethere is no permission traffic at all, so an older platform runs against this build unchanged. That PR still has the rename open for discussion, which should settle before either side merges.