fix(security-solution): preserve Painless scripts and fix group take-actions in bulk close - #289549
fix(security-solution): preserve Painless scripts and fix group take-actions in bulk close#289549jonwalstedt wants to merge 29 commits into
Conversation
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Pull request overview
Adds end-to-end support for preserving data view runtime field scripts during bulk alert status updates by introducing a runtime_mappings payload that is merged server-side and threaded through both “select all” and “group take-actions” flows.
Changes:
- Add
runtime_mappingsrequest param and server-side merge helper to preserve Painless scripts and stampon_script_error: 'continue'. - Update client bulk-close flows to forward runtime mappings (scripts + formats) via
toBulkCloseRuntimeMappings. - Add/adjust unit tests and OpenAPI schema to cover/describe the new parameter.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| x-pack/solutions/security/test/security_solution_api_integration/test_suites/detections_response/detection_engine/alerts/basic_license_essentials_tier/alert_status/alert_status.ts | Adds API integration regressions for closing by scripted/scriptless runtime mappings |
| x-pack/solutions/security/plugins/security_solution/server/lib/detection_engine/routes/signals/open_close_signals_route.ts | Accepts runtime_mappings, enforces a combined cap, merges synthesized + passthrough runtime fields |
| x-pack/solutions/security/plugins/security_solution/server/lib/detection_engine/routes/signals/bulk_close_runtime_mappings.ts | Adds mergeBulkCloseRuntimeMappings and shared type for passthrough mappings |
| x-pack/solutions/security/plugins/security_solution/server/lib/detection_engine/routes/signals/bulk_close_runtime_mappings.test.ts | Unit tests for merge precedence, stamping, and format/script preservation |
| x-pack/solutions/security/plugins/security_solution/public/detections/hooks/trigger_actions_alert_table/use_alert_actions.tsx | Switches select-all flow to send full runtime mappings via new helper |
| x-pack/solutions/security/plugins/security_solution/public/detections/hooks/trigger_actions_alert_table/use_alert_actions.test.tsx | Updates tests to assert script preservation and filtering unsupported types |
| x-pack/solutions/security/plugins/security_solution/public/detections/hooks/alerts_table/use_group_take_action_items.tsx | Threads runtime mappings into group take-actions status updates |
| x-pack/solutions/security/plugins/security_solution/public/detections/hooks/alerts_table/use_group_take_action_items.test.tsx | Adds tests asserting runtime mappings are forwarded in group take-actions |
| x-pack/solutions/security/plugins/security_solution/public/detections/containers/detection_engine/alerts/types.ts | Extends public API types/docs to include new runtimeMappings field |
| x-pack/solutions/security/plugins/security_solution/public/detections/containers/detection_engine/alerts/api.ts | Sends runtime_mappings to the bulk-close endpoint |
| x-pack/solutions/security/plugins/security_solution/public/detections/components/alerts_table/types.ts | Extends group take-action props to include runtime mappings |
| x-pack/solutions/security/plugins/security_solution/public/detections/components/alerts_table/alerts_sub_grouping.tsx | Forwards page-scoped runtime mappings into group take-actions callback |
| x-pack/solutions/security/plugins/security_solution/public/common/components/toolbar/bulk_actions/update_alerts.ts | Extends bulk action helper to accept and forward runtime mappings |
| x-pack/solutions/security/plugins/security_solution/public/common/components/toolbar/bulk_actions/runtime_mappings_for_bulk_close.ts | New helper to normalize/filter data view runtime mappings for the route schema |
| x-pack/solutions/security/plugins/security_solution/public/common/components/toolbar/bulk_actions/runtime_mappings_for_bulk_close.test.ts | Unit tests for normalization, filtering, and script/format preservation |
| x-pack/solutions/security/plugins/security_solution/docs/openapi/serverless/security_solution_detections_api_2023_10_31.bundled.schema.yaml | Documents new runtime_mappings param + schema |
| x-pack/solutions/security/plugins/security_solution/docs/openapi/ess/security_solution_detections_api_2023_10_31.bundled.schema.yaml | Documents new runtime_mappings param + schema |
| x-pack/solutions/security/plugins/security_solution/common/api/detection_engine/signals/set_signal_status/set_signals_status_route.schema.yaml | Adds schema definitions for RuntimeFieldMapping and runtime_mappings |
| x-pack/solutions/security/plugins/security_solution/common/api/detection_engine/signals/set_signal_status/set_signals_status_route.gen.ts | Regenerates Zod schema/types for runtime_mappings |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… raw sum The previous implementation summed Object.keys(runtime_fields).length + Object.keys(runtime_mappings).length, which double-counts any key that appears in both params. Since mergeBulkCloseRuntimeMappings lets the passthrough entry win on collision, the effective number of runtime mappings sent to ES is the size of the union of the two key sets, not the sum. Use a Set to count unique names so a shared key is only counted once and valid requests are not incorrectly rejected. Reviewed-at: elastic#289549 (comment)
…ration test The previous test used process.executable as the runtime field name, which is already indexed in the alerts index — the term filter would succeed even if runtime_mappings were ignored, making the test vacuous. Replace with a unique field name (exec_from_source_rt) and a script that reads process.executable from params._source, equivalent to what ES does internally for a scriptless runtime field. Since exec_from_source_rt is not in the alerts mapping, the term filter returns 0 docs when runtime_mappings is dropped. This makes the test fail under the old behavior and pass only when runtime_mappings reaches ES. Reviewed-at: elastic#289549 (comment)
…OpenAPI schema The previous description said the mapping was forwarded "verbatim/as-is", which was inaccurate. The server only forwards type, script.source, and format; it always stamps on_script_error: continue regardless of any value the caller supplies; and unknown keys are stripped. Update the description to explicitly document: - Which fields are forwarded (type, script.source, format) - That on_script_error is always set to continue by the server - That this affects error handling at the individual-alert level, not the whole request Regenerated bundled ESS and serverless OpenAPI schemas. Reviewed-at: elastic#289549 (comment)
…gs integration test
params._source in Painless is a nested Map — dotted field names like
process.executable are stored as { process: { executable: ... } }, not
as a literal dotted key at the top level. Accessing
params._source['process.executable'] always returns null, making the term
filter match 0 docs even when runtime_mappings is correctly forwarded.
Traverse the nested path explicitly: params._source['process']['executable'].
Reviewed-at: elastic#289549 (comment)
…of silently becoming scriptless
If field.script is an object without a source property (e.g. a stored-script
reference { id, params }), the previous code extracted source as undefined,
skipped the script block, and forwarded only { type } — silently changing the
entry to a scriptless runtime field that reads from _source[fieldName]. That
can match a different set of alerts than the user intended, or 0 alerts if the
field name is not in _source.
Switch from .map() to .flatMap() and return [] for entries whose script object
has no source property, dropping the entry entirely. This surfaces the mismatch
as 0 docs (no-op close) rather than a wrong-set close.
Reviewed-at: elastic#289549 (comment)
…wn-key stripping The test asserted that unknown keys on a keyword-type entry do not propagate, but the description claimed it was testing lookup-only key stripping. Rename to 'strips unknown keys from supported-type entries' so the description matches what the test body actually verifies. Reviewed-at: elastic#289549 (comment)
…ipt params or lang Address review feedback: the narrowing logic in toBulkCloseRuntimeMappings silently stripped script.params and lang from inline scripts, which changes their semantics — the script runs with params undefined and on_script_error:continue then skips each alert rather than matching it. Drop the entry entirely when the script object contains any property other than source, consistent with the existing stored-script-id handling. Reviewed-at: elastic#289549 (comment)
…ings route with 400 Address review feedback: the server schema accepted only script.source but silently stripped extra properties (params, lang) via Zod's default strip mode, meaning a parameterised script would evaluate with params undefined and on_script_error:continue would skip each alert without any error signal. Add additionalProperties: false to the RuntimeFieldMapping.script object in the OpenAPI schema so the generated Zod schema uses .strict() and returns an explicit 400 when unsupported properties are sent. Regenerate the gen.ts and rebundle both ESS and serverless OpenAPI docs. Update the server-side mergeBulkCloseRuntimeMappings JSDoc to document that params and lang are intentionally unsupported. Reviewed-at: elastic#289549 (comment)
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
68441c3 to
4b3e4c8
Compare
… raw sum The previous implementation summed Object.keys(runtime_fields).length + Object.keys(runtime_mappings).length, which double-counts any key that appears in both params. Since mergeBulkCloseRuntimeMappings lets the passthrough entry win on collision, the effective number of runtime mappings sent to ES is the size of the union of the two key sets, not the sum. Use a Set to count unique names so a shared key is only counted once and valid requests are not incorrectly rejected. Reviewed-at: elastic#289549 (comment)
…gs integration test
params._source in Painless is a nested Map — dotted field names like
process.executable are stored as { process: { executable: ... } }, not
as a literal dotted key at the top level. Accessing
params._source['process.executable'] always returns null, making the term
filter match 0 docs even when runtime_mappings is correctly forwarded.
Traverse the nested path explicitly: params._source['process']['executable'].
Reviewed-at: elastic#289549 (comment)
…of silently becoming scriptless
If field.script is an object without a source property (e.g. a stored-script
reference { id, params }), the previous code extracted source as undefined,
skipped the script block, and forwarded only { type } — silently changing the
entry to a scriptless runtime field that reads from _source[fieldName]. That
can match a different set of alerts than the user intended, or 0 alerts if the
field name is not in _source.
Switch from .map() to .flatMap() and return [] for entries whose script object
has no source property, dropping the entry entirely. This surfaces the mismatch
as 0 docs (no-op close) rather than a wrong-set close.
Reviewed-at: elastic#289549 (comment)
…wn-key stripping The test asserted that unknown keys on a keyword-type entry do not propagate, but the description claimed it was testing lookup-only key stripping. Rename to 'strips unknown keys from supported-type entries' so the description matches what the test body actually verifies. Reviewed-at: elastic#289549 (comment)
…ipt params or lang Address review feedback: the narrowing logic in toBulkCloseRuntimeMappings silently stripped script.params and lang from inline scripts, which changes their semantics — the script runs with params undefined and on_script_error:continue then skips each alert rather than matching it. Drop the entry entirely when the script object contains any property other than source, consistent with the existing stored-script-id handling. Reviewed-at: elastic#289549 (comment)
…ings route with 400 Address review feedback: the server schema accepted only script.source but silently stripped extra properties (params, lang) via Zod's default strip mode, meaning a parameterised script would evaluate with params undefined and on_script_error:continue would skip each alert without any error signal. Add additionalProperties: false to the RuntimeFieldMapping.script object in the OpenAPI schema so the generated Zod schema uses .strict() and returns an explicit 400 when unsupported properties are sent. Regenerate the gen.ts and rebundle both ESS and serverless OpenAPI docs. Update the server-side mergeBulkCloseRuntimeMappings JSDoc to document that params and lang are intentionally unsupported. Reviewed-at: elastic#289549 (comment)
…e mappings - Guard `on_script_error: 'continue'` to scripted entries only — omitting it from scriptless passthrough entries avoids sending a property that has no meaning without a script and may cause future ES validation errors - Add tests for the combined runtime_fields + runtime_mappings cap: 101 entries → 400, overlapping key counted once (union semantics), 100 → 200 - Use MappingRuntimeFields (ES client type) for runtimeMappings prop in alerts_sub_grouping instead of RunTimeMappings (timelines alias) — one type at this boundary, structurally identical - Add comment on by-IDs call explaining runtimeMappings is unused there Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… reduce Replace filter + flatMap + Object.fromEntries with a single reduce pass that builds the result object directly. Eliminates two intermediate arrays and the entry-tuple ceremony. Also simplify the format guard: 'format' in field check adds nothing since undefined is already falsy.
…nd ES Script type Reduce the number of type assertions from 3 to 1: - Add isSupportedType predicate so TypeScript narrows field.type after the membership check; no cast needed on the mapping construction. - Cast field.script to the correct ES Script type instead of the opaque Record<string, unknown>; source is then narrowed with typeof === 'string' rather than a second cast.
…meMappings not MappingRuntimeFields The runtimeMappings prop was typed as MappingRuntimeFields (the ES client type) but getAlertsGroupingQuery expects RunTimeMappings (the Kibana data-view type). Switch the prop and its import to RunTimeMappings, consistent with how alerts_grouping.tsx already casts the value before passing it down. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3557cbd to
2a1c4e4
Compare
PhilippeOberti
left a comment
There was a problem hiding this comment.
Nicely done, thanks for the fix!
|
@elasticmachine merge upstream |
florent-leborgne
left a comment
There was a problem hiding this comment.
LGTM for OAS. I left some suggestions to rephrase a little some of the descriptions. Feel free to review them and apply what makes sense.
Co-authored-by: Florent LB <florent.leborgne@elastic.co>
Co-authored-by: Florent LB <florent.leborgne@elastic.co>
Co-authored-by: Florent LB <florent.leborgne@elastic.co>
Co-authored-by: Florent LB <florent.leborgne@elastic.co>
Co-authored-by: Florent LB <florent.leborgne@elastic.co>
💛 Build succeeded, but was flaky
Failed CI Steps
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsshared chunks total size
total optimizer output size
warm start memory
Test Failures
History
cc @jonwalstedt |
Summary
Follow-up to #288946.
The problem
When a user adds a data view runtime field to the alerts table — particularly a scripted runtime field (one with a Painless expression, e.g. combining two source fields or lowercasing a value) — and then tries to bulk-close those alerts, they see:
The same failure occurs whether the user:
The root cause is that Elasticsearch needs the full runtime field definition (including the Painless script) to evaluate the filter at query time. Without it, the filter matches nothing.
Why it happens
Bulk closing by query sends a
_update_by_queryto Elasticsearch. The filter in that query can reference runtime fields — fields that don't exist in the alerts index mapping and must be evaluated on the fly by ES. For that to work, the runtime field's script must travel with the request.The merged fix in #288946 passed the runtime field names and types, but stripped the Painless scripts. The server synthesised a replacement script that reads the field value from
_source. That works for rule-source fields (where the alerting framework copies the value into_sourcewhen the alert is created), but not for data view runtime fields — ES computes those at query time from the raw document fields, and there is no copy in_source. The filter therefore always resolves to nothing.The group take-actions path had a separate problem: it never forwarded any runtime field information at all, so both scripted and scriptless data view fields failed there.
How it's fixed
The full runtime field definition — type, Painless script, format — is now forwarded to Elasticsearch as-is for data view fields. The older path that synthesises a
_sourcereader is preserved for the exceptions-flyout use case, which genuinely needs those semantics. The group take-actions path is now wired to the same runtime field data the rest of the alerts table already had available.A note on data integrity
This is worth backporting. Beyond the "0 alerts closed" UX failure there is a subtler correctness issue: if the runtime field transforms a value (e.g.
.toLowerCase()), the synthesised_sourcereader returns the untransformed value — so "select all → close" silently closes a different set of alerts than the grid showed. Alerts the user intended to close stay open; alerts they didn't intend to close may be closed.End-to-end request flow
User action: selects all alerts filtered on
display_name: "sudo-scripted"→ clicks "Mark as closed"Client (
toBulkCloseRuntimeMappings): reads the data view's runtime field definitions, strips unsupported types (composite,lookup) and unsupported script shapes (stored scripts, scripts withparams/lang), produces a clean{ name → { type, script.source, format } }map.HTTP request to Kibana server:
Zod (
.strict()on the script object): rejects any request that includes unsupported script properties (params,lang, etc.) with a 400 before the handler runs.Server handler (
mergeBulkCloseRuntimeMappings): merges anyruntime_fields(exceptions-flyout path, which synthesises a_sourcereader) with the newruntime_mappingspassthrough, and stampson_script_error: continueon each scripted entry so a single failing document doesn't abort the whole operation._update_by_queryto Elasticsearch:{ "script": { "source": "ctx._source['kibana.alert.workflow_status'] = 'closed'" }, "query": { "term": { "display_name": "sudo-scripted" } }, "runtime_mappings": { "display_name": { "type": "keyword", "on_script_error": "continue", "script": { "source": "emit(doc['process.name'].value + '-scripted')" } } } }Elasticsearch: defines
display_nameas a virtual field for this request, runs the Painless script per document to evaluate the filter, and updates matching documents in one pass. If the script throws on any document, that document is skipped and the rest proceed.Response:
{ updated: 47 }→ toast shows Successfully closed 47 alerts.The whole fix is ensuring the Painless script survives the journey from the data view definition all the way to ES, unchanged.
Backwards compatibility
This PR is fully backwards compatible. The fix adds a new optional body parameter (
runtime_mappings) to the existingPOST /api/detection_engine/signals/statusendpoint (API version2023-10-31). Callers that do not send it receive identical behaviour to today.The existing
runtime_fieldsparameter — used by the exceptions-flyout path — is untouched. Its behaviour is unchanged.How to reproduce the original issue
display_nameemit(doc['process.name'].value + '-scripted')in the script textareabulk-close-bug2.mov
Repeat steps 6–7 using Group alerts by Rule name → Take actions → Mark as closed on a group row to reproduce the second failure path.
bulk-close-bug-group.mov
How to verify the fix in this branch
_source-reader path that must remain intact..toLowerCase()), filter the grid on the transformed value, and select all → close. The closed count should equal the number of visible rows — not a superset or subset.Fixed
bulk-close-fixed2.mov
Security note — inline Painless scripts in a public API
The new
runtime_mappingsbody parameter accepts inline Painless scripts onPOST /api/detection_engine/signals/status(access: 'public'). A caller can supply an arbitrary Painless expression that Elasticsearch evaluates inside the_update_by_queryfilter context.The risk is equivalent to (and no greater than) the existing data view scripted-field feature, which already allows any authenticated user to write arbitrary Painless scripts that run in this same ES context. The key mitigations:
ALERTS_API_ALL(or the deprecated update privilege); scripted data view fields require only the user's data-view access.asCurrentUser— the_update_by_queryexecutes with the caller's own ES privileges; no privilege escalation occurs.script.sourceis capped at 10 000 characters;runtime_mappingsaccepts at most 100 unique field names combined withruntime_fields.Test plan
on_script_erroron scripted entries only, combined-cap validation (49 tests across 4 files)updated > 0;_source-reading field close returnsupdated > 0(both inalert_status.ts, running against a live ES stack with auditbeat data)Checklist
release_note:*label is applied per the guidelinesCloses: https://github.com/elastic/security-team/issues/19185
🤖 Generated with Claude Code