fix(skit): add missing oneshot + plugin allowlist checks to batch API#294
fix(skit): add missing oneshot + plugin allowlist checks to batch API#294streamer45 merged 6 commits intomainfrom
Conversation
Both handle_validate_batch and handle_apply_batch were missing two validation checks that handle_add_node enforces: 1. Oneshot-only marker node rejection: nodes with kind streamkit::http_input or streamkit::http_output must be rejected on the dynamic control plane. 2. Plugin allowlist check: nodes with kind starting with plugin:: must be checked against perms.is_plugin_allowed(). Add both checks to the batch handlers, mirroring the exact same error messages and logic from handle_add_node for consistency. Add integration tests verifying the batch API correctly rejects oneshot-only nodes and allows valid node types. Closes #287 Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
handle_validate_batch was ignoring the session_id parameter, meaning it performed no session existence or ownership check unlike handle_apply_batch. This allowed users to validate operations against sessions they don't own. Add session lookup and can_access_session check to handle_validate_batch, mirroring handle_apply_batch's authorization semantics. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Address review feedback: 1. Extract validate_add_node_op() as the single source of truth for AddNode validation (oneshot check, node allowlist, plugin allowlist, file path checks). All three call sites (handle_add_node, handle_validate_batch, handle_apply_batch) now use this shared function instead of duplicated logic. 2. Fix test_validate_batch_rejects_disallowed_plugin to actually test rejection by configuring a server with an empty plugin allowlist. 3. Add test_validate_batch_rejects_nonexistent_session covering the session existence check added in the previous commit. 4. Add ApplyBatch tests for mixed-batch and plugin rejection parity with ValidateBatch tests. 5. Add type aliases (WsWriter, WsReader) to reduce verbose WebSocket type signatures in the test file. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…ult, and ownership tests - Add duplicate node_id pre-validation to handle_validate_batch, mirroring the simulation in handle_apply_batch (Add/Remove sequence). - Return structured ValidationResult with ValidationError entries instead of opaque ResponsePayload::Error for validation failures. All errors are now collected and reported at once instead of failing on the first. - Session-not-found and ownership-denied remain as pre-flight Error responses (they prevent any validation from running). - Add 5 new integration tests: - test_validate_batch_rejects_duplicate_node_id - test_validate_batch_reports_all_errors - test_validate_batch_rejects_cross_role_ownership - test_apply_batch_rejects_nonexistent_session - test_apply_batch_rejects_cross_role_ownership - Update existing ValidateBatch tests to expect ValidationResult with structured error entries including node_id context. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| // Validate all AddNode operations against permission and security rules. | ||
| for op in operations { | ||
| if let streamkit_api::BatchOperation::AddNode { node_id, kind, params, .. } = op { | ||
| if let Some(message) = | ||
| validate_add_node_op(kind, params.as_ref(), perms, &app_state.config.security) | ||
| { | ||
| errors.push(ValidationError { | ||
| error_type: ValidationErrorType::Error, | ||
| message, | ||
| node_id: Some(node_id.clone()), | ||
| connection_id: None, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| info!(operation_count = operations.len(), "Validated batch operations"); | ||
| ResponsePayload::ValidationResult { errors: Vec::new() } | ||
| info!( | ||
| operation_count = operations.len(), | ||
| error_count = errors.len(), | ||
| "Validated batch operations" | ||
| ); | ||
| ResponsePayload::ValidationResult { errors } |
There was a problem hiding this comment.
🚩 Behavioral change: ValidateBatch now returns ValidationResult instead of Error for invalid operations
The old handle_validate_batch short-circuited on the first failing AddNode operation and returned ResponsePayload::Error { message }. The new code accumulates all errors into a Vec<ValidationError> and returns ResponsePayload::ValidationResult { errors } with a non-empty list. This changes the wire-level response shape from {"action": "error", "message": "..."} to {"action": "validationresult", "errors": [...]} for operation-level failures. Clients that were matching on the error action for validation failures would need updating. This is arguably the correct behavior for a validation endpoint (returning structured results), but it is a breaking change in the API contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Acknowledged — this is an intentional behavioral change requested in code review. The reviewer asked for ValidateBatch to return structured ValidationResult { errors } instead of opaque Error { message }, so that (a) the response shape is consistent between success and failure paths, and (b) all errors are reported at once instead of failing on the first.
Pre-flight checks (session-not-found, ownership-denied, permission-denied) still return ResponsePayload::Error since they prevent any validation from running.
No existing clients consume ValidateBatch error responses today (the batch API is new), so the migration risk is minimal.
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
2 participants
Summary
Both
handle_validate_batchandhandle_apply_batchwere missing two validation checks thathandle_add_nodeenforces:Oneshot-only marker node rejection: Nodes with kind
streamkit::http_inputorstreamkit::http_outputmust be rejected on the dynamic control plane (they are only valid in oneshot HTTP pipelines).Plugin allowlist check: Nodes with kind starting with
plugin::must be checked againstperms.is_plugin_allowed().Additionally,
handle_validate_batchhad several gaps compared tohandle_apply_batch:Error { message }instead of structuredValidationResult { errors }Changes:
validate_add_node_op()as the single source of truth for AddNode validation. All three call sites (handle_add_node,handle_validate_batch,handle_apply_batch) now use this shared function.handle_validate_batch, mirroringhandle_apply_batch.handle_validate_batch, mirroringhandle_apply_batch's Add/Remove simulation.handle_validate_batchto return structuredValidationResult { errors }withValidationErrorentries (includingnode_idcontext) instead of opaqueError { message }. All errors are now collected and reported at once instead of failing on the first. Session-not-found and ownership-denied remain as pre-flightErrorresponses.ValidateBatchandApplyBatch.Closes #287
Review & Testing Checklist for Human
validate_add_node_op()correctly consolidates all per-node checks (oneshot, node allowlist, plugin allowlist, file_reader, file_writer, script_path)handle_validate_batchmirrorshandle_apply_batchValidationResultwire format change is acceptable for clients (previouslyError, nowValidationResultwith structured errors for operation-level failures)ValidateBatchwith two invalid nodes and confirm both errors are returnedNotes
ValidationResultresponse change is intentional: the success path already returnedValidationResult { errors: [] }, so the failure path now consistently returns the same shape with populated errors.Errorsince they prevent any validation from running.role_headerconfig + custom WS headers to simulate multi-role access.Link to Devin session: https://staging.itsdev.in/sessions/d053d3e7034c445eb190ae2b9193974e
Requested by: @streamer45