[NAE-2303] TaskRef Security Improvements #399
Conversation
- improve dataSet behavior check
- forbid setData for taskRef and caseRef fields on endpoint calls
- improve security
- add javadoc
- remove todo
- implement task controller tests
WalkthroughAdds a runStrict mode to data-setting that enforces per-field editability and skips TASK_REF/CASE_REF types; controllers now call dataService.setData per referenced task and fall back to resolving main outcomes via workflow when the task-level main outcome is null. Tests and a new Petri net were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as AbstractTaskController
participant DataService
participant Workflow as IWorkflowService
Client->>Controller: POST /tasks/{id}/setData(payload, params)
Controller->>Controller: extract referenced taskIds from payload (TASK_REF fields)
loop per referenced taskId
Controller->>Controller: create payloadCopy scoped to this taskId
Controller->>DataService: setData(task, payloadCopy, params, runStrict=true)
alt all fields accepted
DataService-->>Controller: SetDataEventOutcome (task-level)
else forbidden/validation issues
DataService-->>Controller: partial outcome or throws SetDataException
end
end
Controller->>Controller: aggregate outcomes, determine mainOutcome
alt mainOutcome is null
Controller->>Workflow: handleMainSetDataEventOutcome(caseId, taskId)
Workflow-->>Controller: derived SetDataEventOutcome
end
Controller-->>Client: HTTP response with resolved mainOutcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-10-20T11:44:44.907ZApplied to files:
📚 Learning: 2025-11-14T10:22:01.634ZApplied to files:
📚 Learning: 2025-12-18T19:20:32.658ZApplied to files:
📚 Learning: 2025-12-19T07:44:36.826ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/main/java/com/netgrif/application/engine/workflow/service/DataService.java(4 hunks)src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java(5 hunks)src/main/java/com/netgrif/application/engine/workflow/web/PublicTaskController.java(2 hunks)src/main/java/com/netgrif/application/engine/workflow/web/TaskController.java(2 hunks)src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy(10 hunks)src/test/resources/petriNets/task_controller_set_data.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-09-05T10:21:54.893Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:135-135
Timestamp: 2025-09-05T10:21:54.893Z
Learning: In ExportServiceTest.groovy, writing to src/test/resources is intentional to simulate production behavior where the working tree is mutated during file exports. This mirrors how the system works in production.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (12)
src/test/resources/petriNets/task_controller_set_data.xml (2)
134-136: Verify duplicate dataGroup ID is intentional.The dataGroup ID
data_0is used both here (line 135) and in thedatatransition (line 96). If this is intentional (sharing the same logical group), this is fine. Otherwise, consider using a unique ID liketestSetDataNonReferencedField_0for clarity.
1-154: LGTM!The test resource is well-structured with clear separation of test scenarios:
- Field type restrictions (TASK_REF, CASE_REF)
- Visible vs editable field behaviors
- Nested TaskRef restrictions
- Non-referenced field handling
This provides good coverage for the security improvements.
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy (2)
181-209: LGTM!The nested TaskRef restriction test properly validates:
- Setup of a chain: case1 → case2 → case3 via taskRef_0
- Rejection of updates to non-referenced nested tasks (case2's data transition)
- Acceptance of updates to properly referenced nested tasks (case3's data transition)
This effectively tests the security boundary for nested task references.
150-166: LGTM!The test effectively validates that TASK_REF and CASE_REF field types are blocked from being set via the controller API, verifying both:
- The response indicates no changes were applied
- The persisted field values remain empty
This is a key security test for the forbidden field types feature.
src/main/java/com/netgrif/application/engine/workflow/web/TaskController.java (1)
55-58: LGTM!The constructor properly injects
IWorkflowServiceand passes it to the superclass. This follows standard Spring dependency injection patterns and enables the workflow service integration for the security improvements.src/main/java/com/netgrif/application/engine/workflow/web/PublicTaskController.java (1)
53-59: LGTM!The constructor properly integrates
IWorkflowServicewhile maintaining the existing pattern of passingnullforsearchService(sincePublicTaskControllerdoesn't use Elasticsearch). The local field assignments fortaskService,dataService, anduserServiceare retained for direct usage in this controller's methods.src/main/java/com/netgrif/application/engine/workflow/service/DataService.java (3)
71-72: LGTM! Good security constants.The forbidden field types set correctly restricts TASK_REF and CASE_REF from being modified through the safe data path, which aligns with the security improvement objectives.
223-225: LGTM! Clean backward-compatible delegation.The existing
setDataoverload now delegates to the new implementation withrunSafe=false, preserving existing behavior for internal callers while enabling stricter validation for controller-initiated requests.
333-340: LGTM! Clean helper method with proper null safety.The
isDataFieldEditablemethod correctly validates editability by checking the behavior map and transition-specific behavior set. The null guards prevent NPEs when behavior is undefined.src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java (3)
54-63: LGTM! Constructor updated to wire IWorkflowService.The new dependency enables the controller to resolve cases when needed for outcome creation.
217-229: Good security filtering; verify API contract change is documented.The logic correctly restricts data modifications to the main task and tasks referenced via TASK_REF fields. This prevents unauthorized modification of arbitrary tasks.
However, the request body structure appears to have changed from field-keyed to task-keyed format:
// New format (inferred) { "taskId1": { "fieldId": {...} }, "taskId2": { "fieldId": {...} } }Ensure this API change is documented in migration guides, as existing clients will need to update their request payloads.
241-244: LGTM! Proper fallback when main task has no direct changes.The fallback correctly handles the case where
dataBodycontains changes only for referenced tasks but not for the main task itself, ensuring a valid response is always returned.
src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
Show resolved
Hide resolved
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
Show resolved
Hide resolved
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
Outdated
Show resolved
Hide resolved
- handle possible NPE in DataService
- rename parameter in IDataService
- handle possible NPE in AbstractTaskController - fix typo
- rename method in TaskControllerTest
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java (1)
39-40: LGTM!The new overloaded method signature cleanly extends the existing API to support the
runSafemode for enhanced security during data field updates.Note: A past review comment flagged parameter naming inconsistency between the interface (
runSafehere, but mentioned asapplyForbiddenTypesin the comment). Based on the current code, the parameter is consistently namedrunSafein both the interface and implementation, so this appears to have been addressed.src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy (1)
303-307: Consider renaming to clarify purpose.This helper creates a nested structure (
taskId -> fieldId -> values) different fromImportHelper.populateDataset(fieldId -> values). Consider renaming topopulateTaskDatasetor similar to clarify the distinction.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/netgrif/application/engine/workflow/service/DataService.java(4 hunks)src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java(9 hunks)src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy(10 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
📚 Learning: 2025-12-18T19:20:32.658Z
Learnt from: Smotanka
Repo: netgrif/application-engine PR: 401
File: src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java:57-66
Timestamp: 2025-12-18T19:20:32.658Z
Learning: When performing multiple string replacements in Java files (e.g., sanitization/escaping logic like ElasticsearchQuerySanitizer), prefer using StringUtils.replaceEach() over chaining multiple StringUtils.replace() calls. This avoids cascading replacements where the result of one replacement triggers another replacement and potentially corrupts the final output. Apply this pattern to other similar sanitization code across the codebase as appropriate.
Applied to files:
src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.javasrc/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.javasrc/main/java/com/netgrif/application/engine/workflow/service/DataService.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.javasrc/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-09-05T10:21:54.893Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:135-135
Timestamp: 2025-09-05T10:21:54.893Z
Learning: In ExportServiceTest.groovy, writing to src/test/resources is intentional to simulate production behavior where the working tree is mutated during file exports. This mirrors how the system works in production.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-11-14T10:22:01.634Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
🧬 Code graph analysis (1)
src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java (1)
src/main/java/com/netgrif/application/engine/workflow/domain/eventoutcomes/response/EventOutcomeWithMessageResource.java (1)
EventOutcomeWithMessageResource(7-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (11)
src/main/java/com/netgrif/application/engine/workflow/service/DataService.java (4)
71-72: LGTM!Immutable
Set.of()is appropriate for this static constant, and blockingTASK_REFandCASE_REFfield types aligns with the security improvements objective.
223-225: LGTM!Backward-compatible delegation with
runSafe = falsepreserves existing behavior for current callers.
238-264: LGTM!The
runSafemode implementation correctly:
- Validates field existence and throws for non-existent fields
- Silently skips forbidden field types (TASK_REF, CASE_REF)
- Enforces per-field editability based on transition behavior
The NPE concern from the past review has been addressed with the null check at line 253.
336-343: LGTM!The helper method is null-safe and correctly requires explicit
EDITABLEbehavior for the transition. This is the right security posture—fields are non-editable by default unless explicitly marked.src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java (2)
344-351: LGTM!The fallback ensures a valid
SetDataEventOutcomeis always returned, even when no changes were made to the main task. This prevents null responses to the client.
288-329: LGTM!Consistent application of
handleMainSetDataEventOutcomeacross file operations (deleteFile,saveFiles,deleteNamedFile) ensures uniform fallback behavior.src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy (5)
150-166: LGTM!Good test coverage for the forbidden field types feature. The test verifies both response structure (
changedFields.isEmpty()) and persisted state (field value unchanged).
168-179: LGTM!The test correctly validates that attempting to edit a non-editable field results in an error response. The TODO at line 178 has a tracking task as confirmed by the developer.
181-209: LGTM!Comprehensive test for nested TASK_REF restrictions. It correctly validates:
- Tasks not in the direct reference chain cannot be modified (lines 196-201)
- Tasks in the reference chain can be modified (lines 203-208)
211-221: LGTM!Good negative test validating that attempting to modify a non-referenced field results in an error.
246-254: LGTM!Clean separation of test nets for different test scenarios.
src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java
Show resolved
Hide resolved
- change the parameter type in TaskControllerTest
- rename parameter
Description
Security improvements
Implements NAE-2303
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manually and with updated unit tests
Test Configuration
Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.