-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Security Solution] Register Workflows with Inbox #266256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
411f0cf
a3027b1
ea4b16b
0aab1a3
0dc86ee
12e3c1b
77a7e36
62778ee
dff988f
5c9b3c2
71a6ffc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,22 @@ export class WaitForInputStepImpl implements NodeImplementation { | |
| ) {} | ||
|
|
||
| async run(): Promise<void> { | ||
| // The step runtime's abort signal is how monitors (workflow-level timeout, | ||
| // cancellation) tell a step "you have already been settled — do not touch | ||
| // state". Without this guard a waitForInput that is resumed after the | ||
| // workflow has timed out would enter `tryEnterWaitUntil` with an in-memory | ||
| // status of FAILED (set by the monitor's failStep call), treat itself as | ||
| // "not already waiting", and re-write status back to WAITING_FOR_INPUT — | ||
| // leaving a zombie step that `listWaitingForInputSteps` keeps surfacing in | ||
| // the Inbox forever. | ||
| if (this.stepExecutionRuntime.abortController.signal.aborted) { | ||
| this.workflowLogger.logDebug( | ||
| `Step '${this.node.stepId}' run aborted before wait-entry; skipping`, | ||
| { event: { action: 'hitl:aborted' } } | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
|
Comment on lines
+26
to
+41
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @h88, this is one of the two (non-inbox-conditional) functional changes to workflows in this PR. This was surfaced as having stale items stuck in the Inbox views, and the fix makes sense to me, but please do confirm if this is an okay approach here. Corresponding tests are included as well. All other workflow changes in this PR are conditional on the Inbox plugin being enabled, for which it is currently off-by-default.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM |
||
| if (this.stepExecutionRuntime.tryEnterWaitUntil(undefined, ExecutionStatus.WAITING_FOR_INPUT)) { | ||
| // Store step config as input so the record is self contained | ||
| // consistent with every other step type & readable without a definition lookup | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import type { EsWorkflowStepExecution } from '@kbn/workflows'; | ||
| import { ExecutionStatus } from '@kbn/workflows'; | ||
| import { buildWorkflowSourceId, parseWorkflowSourceId, toInboxAction } from './to_inbox_action'; | ||
|
|
||
| const buildStep = (overrides: Partial<EsWorkflowStepExecution> = {}): EsWorkflowStepExecution => ({ | ||
| spaceId: 'default', | ||
| id: 'step-exec-1', | ||
| stepId: 'wait_approval', | ||
| stepType: 'waitForInput', | ||
| scopeStack: [], | ||
| workflowRunId: 'run-1', | ||
| workflowId: 'wf-1', | ||
| status: ExecutionStatus.WAITING_FOR_INPUT, | ||
| startedAt: '2026-04-24T12:00:00.000Z', | ||
| topologicalIndex: 0, | ||
| globalExecutionIndex: 0, | ||
| stepExecutionIndex: 0, | ||
| input: { | ||
| message: 'Approve isolation of host-42?', | ||
| schema: { | ||
| type: 'object', | ||
| properties: { approved: { type: 'boolean' } }, | ||
| required: ['approved'], | ||
| }, | ||
| }, | ||
| ...overrides, | ||
| }); | ||
|
|
||
| describe('buildWorkflowSourceId / parseWorkflowSourceId', () => { | ||
| it('round-trips a composite source id', () => { | ||
| const step = buildStep(); | ||
| const id = buildWorkflowSourceId(step); | ||
| expect(id).toBe('wf-1:run-1:step-exec-1'); | ||
| expect(parseWorkflowSourceId(id)).toEqual({ | ||
| workflowId: 'wf-1', | ||
| executionId: 'run-1', | ||
| stepExecutionId: 'step-exec-1', | ||
| }); | ||
| }); | ||
|
|
||
| it('returns null for malformed ids', () => { | ||
| expect(parseWorkflowSourceId('nope')).toBeNull(); | ||
| expect(parseWorkflowSourceId('a:b')).toBeNull(); | ||
| }); | ||
|
|
||
| it('re-joins trailing colons in the step execution id', () => { | ||
| expect(parseWorkflowSourceId('wf:run:step:with:colons')).toEqual({ | ||
| workflowId: 'wf', | ||
| executionId: 'run', | ||
| stepExecutionId: 'step:with:colons', | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('toInboxAction', () => { | ||
| it('populates the core InboxAction fields from a paused waitForInput step', () => { | ||
| const action = toInboxAction(buildStep()); | ||
|
|
||
| expect(action).toMatchObject({ | ||
| id: 'wf-1:run-1:step-exec-1', | ||
| source_app: 'workflows', | ||
| source_id: 'wf-1:run-1:step-exec-1', | ||
| status: 'pending', | ||
| title: 'Approve isolation of host-42?', | ||
| input_message: 'Approve isolation of host-42?', | ||
| input_schema: { | ||
| type: 'object', | ||
| properties: { approved: { type: 'boolean' } }, | ||
| required: ['approved'], | ||
| }, | ||
| created_at: '2026-04-24T12:00:00.000Z', | ||
| response_mode: 'pending', | ||
| }); | ||
| }); | ||
|
|
||
| it('falls back to a generated title when the step has no rendered message', () => { | ||
| const action = toInboxAction( | ||
| buildStep({ input: { schema: { type: 'object', properties: {} } } }) | ||
| ); | ||
| expect(action.title).toBe('Step "wait_approval" is waiting for input'); | ||
| expect(action.input_message).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('leaves input_schema undefined when the step input omits a schema', () => { | ||
| const action = toInboxAction(buildStep({ input: { message: 'Confirm?' } })); | ||
| expect(action.input_schema).toBeUndefined(); | ||
| expect(action.input_message).toBe('Confirm?'); | ||
| }); | ||
|
|
||
| it('leaves input_schema undefined when the step input is missing entirely', () => { | ||
| const action = toInboxAction(buildStep({ input: undefined })); | ||
| expect(action.input_schema).toBeUndefined(); | ||
| expect(action.input_message).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('rejects array-valued schema payloads (defensive)', () => { | ||
| const action = toInboxAction( | ||
| buildStep({ | ||
| input: { message: 'Weird', schema: ['not', 'an', 'object'] }, | ||
| }) | ||
| ); | ||
| expect(action.input_schema).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h88, this is the only other (non-inbox-conditional) functional change to workflows in this PR.
Perhaps this isn't necessary with the other previous fix for updating
event.actionon abort, so any confirmation here would be appreciated!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not harmful - I would say this is mainly guarding inconsistent execution docs / races because we have a status validation right before it (line # 1210) and any transition should update the status...