-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix safeDelete/safeUpdate to validate criteria contain actual conditions #947
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: master
Are you sure you want to change the base?
Conversation
- Modified OGNL.exampleHasAtLeastOneCriteriaCheck() to check criteria.isValid() - Added unit tests for the OGNL validation method - Fixes issue where invalid criteria (null values or empty) would bypass safe checks Co-authored-by: abel533 <1787798+abel533@users.noreply.github.com>
|
@copilot 处理 review 中的信息。 |
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.
Pull request overview
This PR fixes a security vulnerability where safeDelete and safeUpdate protections could be bypassed. Previously, these features only checked if criteria existed, not if they contained actual query conditions. This allowed operations with null values or empty criteria to execute dangerous full-table deletes/updates.
Changes:
- Modified
OGNL.exampleHasAtLeastOneCriteriaCheck()to iterate through all criteria and validate each one usingcriteria.isValid() - Added unit tests to verify empty criteria, null values, and multiple invalid criteria are properly rejected
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/main/java/tk/mybatis/mapper/util/OGNL.java | Enhanced validation logic to check isValid() for each criteria, handling both direct Example instances and reflection-based access |
| core/src/test/java/tk/mybatis/mapper/test/util/TestOGNLCriteriaCheck.java | Added comprehensive test suite covering null examples, empty criteria, invalid criteria with null values, and multiple invalid criteria scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public class TestOGNLCriteriaCheck { | ||
|
|
||
| // Simple test entity class with table annotation | ||
| @Table(name = "test_entity") | ||
| static class TestEntity { | ||
| private Integer id; | ||
| private String name; | ||
|
|
||
| public Integer getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public void setId(Integer id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public void setName(String name) { | ||
| this.name = name; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Test with null example - should throw MapperException | ||
| */ | ||
| @Test(expected = MapperException.class) | ||
| public void testNullExample() { | ||
| OGNL.exampleHasAtLeastOneCriteriaCheck(null); | ||
| } | ||
|
|
||
| /** | ||
| * Test with empty criteria list - should throw MapperException | ||
| */ | ||
| @Test(expected = MapperException.class) | ||
| public void testEmptyCriteriaList() { | ||
| Example example = new Example(TestEntity.class); | ||
| OGNL.exampleHasAtLeastOneCriteriaCheck(example); | ||
| } | ||
|
|
||
| /** | ||
| * Test with criteria that has no conditions (invalid) - should throw MapperException | ||
| */ | ||
| @Test(expected = MapperException.class) | ||
| public void testInvalidCriteria() { | ||
| Example example = new Example(TestEntity.class); | ||
| example.createCriteria(); // Create criteria but don't add any conditions | ||
| OGNL.exampleHasAtLeastOneCriteriaCheck(example); | ||
| } | ||
|
|
||
| /** | ||
| * Test with criteria that has null value (invalid) - should throw MapperException | ||
| */ | ||
| @Test(expected = MapperException.class) | ||
| public void testCriteriaWithNullValue() { | ||
| Example example = new Example(TestEntity.class); | ||
| // When notNull=false (default), null values are ignored and no criteria is added | ||
| example.createCriteria().andEqualTo("name", null); | ||
| OGNL.exampleHasAtLeastOneCriteriaCheck(example); | ||
| } | ||
|
|
||
| /** | ||
| * Test with multiple criteria where all are invalid - should throw MapperException | ||
| */ | ||
| @Test(expected = MapperException.class) | ||
| public void testMultipleInvalidCriteria() { | ||
| Example example = new Example(TestEntity.class); | ||
| example.createCriteria().andEqualTo("name", null); | ||
| example.or().andEqualTo("name", null); | ||
| OGNL.exampleHasAtLeastOneCriteriaCheck(example); | ||
| } | ||
| } |
Copilot
AI
Jan 20, 2026
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.
The test suite is missing positive test cases that verify the method returns true when valid criteria exist. Consider adding tests for:
- A single criteria with a valid condition (e.g., andEqualTo with a non-null value)
- Multiple criteria where at least one is valid (mixed valid/invalid scenario)
These positive tests are important to ensure the fix doesn't break legitimate use cases and properly validates that valid criteria still work as expected.
- Added comprehensive JavaDoc for exampleHasAtLeastOneCriteriaCheck method - Added @SuppressWarnings("unchecked") to suppress unchecked cast warnings - Clarified method purpose for safeDelete/safeUpdate validation Co-authored-by: abel533 <1787798+abel533@users.noreply.github.com>
Fix safeDelete/safeUpdate validation for invalid criteria
Issue Summary:
When
safeDeleteorsafeUpdateis set to true, operations likedeleteByExampleandupdateByExampleshould prevent deletion/update of all table rows. However, when criteria have null values (e.g.,andEqualTo("userName", null)), the current validation incorrectly allows the operation to proceed, even though the criteria are invalid.Root Cause:
The
exampleHasAtLeastOneCriteriaCheckmethod inOGNL.javaonly checks if thecriteriaListsize is greater than 0, but doesn't validate whether the criteria contain any valid conditions. TheCriteria.isValid()method exists and returns true only whencriteria.size() > 0.Implementation Plan:
OGNL.exampleHasAtLeastOneCriteriaCheck()to checkcriteria.isValid()for each criteriaSecurity Summary:
✅ No security vulnerabilities detected by CodeQL scanner.
✅ The fix actually improves security by properly enforcing safeDelete/safeUpdate constraints.
✅ Prevents accidental full-table deletions/updates when criteria are invalid.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.