[NAE-2303] TaskRef Security Improvements #400
Conversation
- improve dataSet behavior check
- forbid setData for taskRef and caseRef fields on endpoint calls
- improve security
- add javadoc
- implement task controller tests
- fix cherry picking - fix jupiter dependency
|
Warning Rate limit exceeded@Retoocs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a runSafe mode for data updates (forbids TASK_REF/CASE_REF edits, enforces per-transition editability), wires IWorkflowService into task controllers, adds a null-safety check in TaskService.getMainOutcome, and introduces tests and a PetriNet fixture for these behaviors. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AbstractTaskController
participant TaskService
participant DataService
participant WorkflowService
Client->>AbstractTaskController: setData(taskId, values, params)
AbstractTaskController->>AbstractTaskController: extract TASK_REF targets from values
AbstractTaskController->>TaskService: findOne(targetTaskId)
TaskService-->>AbstractTaskController: Task
loop per-target (or main task)
AbstractTaskController->>DataService: setData(task, valuesSubset, params, runSafe=true)
DataService->>DataService: for each field: validate existence in PetriNet
alt field type is forbidden (TASK_REF/CASE_REF)
DataService-->>AbstractTaskController: skip field (no change)
else
DataService->>DataService: isDataFieldEditable(field, transition)
alt not editable
DataService-->>AbstractTaskController: throw IllegalArgumentException
else editable
DataService->>DataService: apply update & update lastModified
DataService-->>AbstractTaskController: SetDataEventOutcome
end
end
end
alt main outcome null
AbstractTaskController->>WorkflowService: fetch case for task
WorkflowService-->>AbstractTaskController: Case
AbstractTaskController->>AbstractTaskController: build main outcome from case
end
AbstractTaskController-->>Client: HTTP response (outcomes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
added
breaking change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
application-engine/pom.xml(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/PublicTaskController.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/TaskController.java(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy(9 hunks)application-engine/src/test/resources/petriNets/task_controller_set_data.xml(1 hunks)
💤 Files with no reviewable changes (1)
- application-engine/pom.xml
🧰 Additional context used
🧠 Learnings (9)
📓 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-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:
application-engine/src/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:
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-12-12T13:40:00.786Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Applied to files:
application-engine/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:
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-12-12T12:37:58.852Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy:4-4
Timestamp: 2025-12-12T12:37:58.852Z
Learning: In tests under application-engine/src/test, GroupService.getDefaultSystemGroup() lazily creates the default system group if it doesn't exist. Tests can rely on this behavior and omit explicit existence/setup checks for the default system group.
Applied to files:
application-engine/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:
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-12-12T12:36:06.651Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".
Applied to files:
application-engine/src/test/resources/petriNets/task_controller_set_data.xml
📚 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:
application-engine/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). (6)
- 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
- GitHub Check: task-list-completed
🔇 Additional comments (17)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java (1)
979-981: LGTM! Null safety check prevents NPE.This defensive check correctly handles the case where no outcome exists for the given task ID or any key in the map, preventing a NullPointerException on the subsequent
addOutcomescall. The callers (e.g.,AbstractTaskController.setData) have been updated to handle the null return appropriately.application-engine/src/test/resources/petriNets/task_controller_set_data.xml (1)
1-25: LGTM! Well-structured test resource for security validation.The PetriFlow definition correctly sets up the necessary data fields (
taskRef_0,caseRef_0,text_0,text_1) and transitions to test the new TaskRef security improvements. ThedefaultRoleandanonymousRolesettings enable proper test access.application-engine/src/main/java/com/netgrif/application/engine/workflow/web/TaskController.java (1)
56-62: LGTM! Constructor wiring correctly includes IWorkflowService.The constructor change properly injects
IWorkflowServiceand passes it to the superclass, enabling the enhancedsetDatalogic inAbstractTaskControllerto construct outcomes when needed.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java (1)
44-45: LGTM! New setData overload enables forbidden type enforcement.The new method signature with
applyForbiddenTypesparameter provides a clean API extension for the security improvements, allowing callers to opt into the TASK_REF/CASE_REF modification restrictions when needed.application-engine/src/main/java/com/netgrif/application/engine/workflow/web/PublicTaskController.java (1)
56-64: LGTM! Constructor properly wires IWorkflowService for public API.The constructor change correctly injects
IWorkflowServiceand passes it to the superclass while maintaining the existing pattern of passingnullforIElasticTaskService(which is not used in public endpoints).application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java (1)
58-73: LGTM! Constructor and field wiring for IWorkflowService.The new
IWorkflowServicefield and constructor parameter are correctly wired, enabling workflow-level operations needed to constructSetDataEventOutcomewhen no outcomes exist for the main task.application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy (6)
3-4: LGTM! Jackson imports support new test functionality.The ObjectMapper and ObjectNode imports are appropriately added to support the new
populateDatasethelper method used in the new test cases.
92-94: LGTM! Dual-net test structure supports comprehensive test coverage.Separating
allDataNetandsetDataNetallows testing different scenarios: the original all-data functionality and the new setData security restrictions.
170-181: LGTM! Test correctly validates non-editable field rejection.The test verifies that attempting to set a field that is only visible (not editable) returns an error response. The TODO comment appropriately flags that additional tests for taskRef parent visibility behavior are needed.
183-211: LGTM! Comprehensive nested TaskRef security test.The test thoroughly validates the nested TaskRef restriction logic:
- Creates a 3-level case structure with taskRef_0 linking case1 → case2 → case3
- Verifies that data cannot be set on non-directly-referenced nested tasks
- Confirms that data can be set on directly referenced nested tasks
213-223: LGTM! Non-referenced field test validates access control.The test appropriately verifies that attempting to set data on a field not referenced by the current task returns an error.
152-168: Verify that TaskController invokes setData with runSafe=true.The test correctly validates that TASK_REF and CASE_REF fields cannot be modified via setData and that changedFields remains empty. TaskController calls
dataService.setData()with therunSafeparameter set totrue, ensuring this behavior.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java (5)
74-74: LGTM! Clear constant definition for forbidden field types.The
setDataForbiddenFieldTypesset clearly defines TASK_REF and CASE_REF as the field types that cannot be modified via setData when safety checks are enabled.
225-226: LGTM! Backward compatibility preserved.The existing overload correctly delegates to the new one with
runSafe = false, ensuring existing callers continue to work without restrictions.
228-240: LGTM! Clear documentation of new safety parameter.The Javadoc clearly explains the purpose of the
runSafeparameter and its impact on validation behavior.
252-257: LGTM! Forbidden type check silently skips restricted fields.When
runSafeis true, the code appropriately skips TASK_REF and CASE_REF fields without throwing an exception, allowing the operation to continue processing other fields.
260-262: LGTM! Editability enforcement with clear error message.The check appropriately throws
IllegalArgumentExceptionwith a descriptive message when attempting to modify a non-editable field in safe mode.
...cation-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java
Outdated
Show resolved
Hide resolved
...ion-engine/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: 3
♻️ Duplicate comments (2)
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy (1)
305-309: Consider strengthening the type signature.The method signature declares
Map<String, Map<String, Map<String, String>>>, but callers pass non-String values (e.g., lists on line 157). While Groovy's dynamic typing and JSON serialization make this work, the signature is misleading.💡 Consider using a more flexible signature
-static ObjectNode populateNestedDataset(Map<String, Map<String, Map<String, String>>> data) { +static ObjectNode populateNestedDataset(Map<String, Map<String, Map<String, Object>>> data) { ObjectMapper mapper = new ObjectMapper() String json = mapper.writeValueAsString(data) return mapper.readTree(json) as ObjectNode }application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java (1)
336-343: Add transition-level behavior fallback toisDataFieldEditablemethod.The method only checks case-level behavior and returns
falseif not found. However,isForbidden(lines 199-205) andgetData(lines 160-178) implement a two-tier resolution pattern where transition-level behavior serves as fallback. Without this fallback, fields defined as editable in the transition will be rejected duringsetDataoperations if they lack case-level runtime behavior.🔎 Proposed fix with transition-level fallback
- private boolean isDataFieldEditable(DataField dataField, String transId) { + private boolean isDataFieldEditable(DataField dataField, String transId, Transition transition, String fieldId) { Map<String, Set<FieldBehavior>> behaviorMap = dataField.getBehavior(); - if (behaviorMap == null) { - return false; - } - Set<FieldBehavior> behaviorSet = behaviorMap.get(transId); - return behaviorSet != null && behaviorSet.contains(FieldBehavior.EDITABLE); + if (behaviorMap != null && behaviorMap.containsKey(transId)) { + Set<FieldBehavior> behaviorSet = behaviorMap.get(transId); + return behaviorSet != null && behaviorSet.contains(FieldBehavior.EDITABLE); + } + // Fallback to transition-level behavior + DataFieldLogic dataFieldLogic = transition.getDataSet().get(fieldId); + return dataFieldLogic != null && dataFieldLogic.isEditable(); }Note: The method signature and call site at line 263 would need to be updated to pass the transition and fieldId.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java(9 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy(9 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
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.
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.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:
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java
📚 Learning: 2025-12-12T13:40:00.786Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Applied to files:
application-engine/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:
application-engine/src/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:
application-engine/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:
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
📚 Learning: 2025-12-12T12:37:58.852Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy:4-4
Timestamp: 2025-12-12T12:37:58.852Z
Learning: In tests under application-engine/src/test, GroupService.getDefaultSystemGroup() lazily creates the default system group if it doesn't exist. Tests can rely on this behavior and omit explicit existence/setup checks for the default system group.
Applied to files:
application-engine/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:
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
⏰ 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). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (15)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IDataService.java (1)
44-45: LGTM! NewrunSafeoverload follows existing interface patterns.The new method signature aligns with the existing overloads and provides an explicit flag for enabling additional security validations during data updates.
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy (7)
3-4: LGTM! Imports support the new test utilities.
92-94: LGTM! Clear separation of test nets.Using separate
allDataNetandsetDataNetfields allows focused testing of different PetriNet configurations for existing and new security scenarios.
114-114: LGTM!
152-168: LGTM! Test properly validates forbidden field type restrictions.The test verifies that
TASK_REFandCASE_REFfields are silently skipped duringsetDataoperations withrunSafe=true, confirming both the response structure and the persistence layer remain unchanged.
183-211: LGTM! Comprehensive nested taskRef restriction test.The test effectively validates that:
- Updates to tasks not in the direct TASK_REF reference chain are blocked (case2's task accessed from case1)
- Updates to tasks within the nested reference chain succeed (case3's task accessed via case1→case2→case3)
This covers the security boundary enforcement for nested taskRef traversal.
213-223: LGTM! Test validates non-referenced field rejection.Properly verifies that attempting to modify a field not available on the current transition results in an error response.
248-256: LGTM! Clean net initialization.application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java (4)
10-11: LGTM! Imports support TASK_REF filtering logic.
58-59: LGTM! New dependency properly declared.
64-74: LGTM! Constructor injection follows existing patterns.
300-302: LGTM! Consistent pattern and typo fix.The
handleMainSetDataEventOutcomeintegration is applied consistently, and the spelling correction from "sucessfully" to "successfully" improves message quality.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java (3)
74-75: LGTM! Immutable constant for forbidden field types.Using
Set.of()ensures the forbidden types collection is immutable and provides O(1) lookup for the security check.
223-226: LGTM! Backward-compatible delegation.Delegating to the new overload with
runSafe=falsepreserves existing behavior for all current callers.
252-265: LGTM! Validation logic correctly implements security boundaries.The asymmetric handling is appropriate:
- Forbidden field types (TASK_REF, CASE_REF) are silently skipped — prevents information disclosure about what fields exist
- Non-editable fields throw
IllegalArgumentException— provides explicit feedback when attempting unauthorized editsThis matches the test expectations in
TaskControllerTest.
...engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java
Show resolved
Hide resolved
...ion-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
Show resolved
Hide resolved
- change the parameter type in TaskControllerTest
- rename parameter
Description
Security improvements
Implements NAE-2303
Dependencies
Updated jupiter dependencies to fix tests
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
Bug Fixes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.