fix: cherry pick form https://github.com/apache/commons-beanutils/com…#1
fix: cherry pick form https://github.com/apache/commons-beanutils/com…#1kycheng merged 4 commits intoalauda-1.9from
Conversation
WalkthroughThis change updates the project version and Java compatibility in the build configuration, adds a new release entry documenting a security fix, introduces a new introspector to suppress the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as EnumDeclaringClassTest
participant BeanUtils as BeanUtilsBean / PropertyUtilsBean
participant Introspector as SuppressPropertiesBeanIntrospector
participant Enum as TestEnum
Test->>BeanUtils: Request property "testEnum.declaringClass"
BeanUtils->>Introspector: Check if "declaringClass" is suppressed
alt Suppression enabled (default)
Introspector-->>BeanUtils: "declaringClass" suppressed
BeanUtils-->>Test: Throw NoSuchMethodException
else Suppression disabled (opt-out)
Introspector-->>BeanUtils: "declaringClass" allowed
BeanUtils->>Enum: Access declaringClass
Enum-->>BeanUtils: Return Class object
BeanUtils-->>Test: Return Class object
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/changes/changes.xml (1)
31-34: Release entry missing tracking IDConsider attaching the internal JIRA ticket (if any) or the CVE number directly in an
issueattribute so downstream tooling can hyperlink it, e.g.:- <action type="add" dev="ggregory" due-to="Gary Gregory">Add org.apache.commons.beanutils.SuppressPropertiesBeanIntrospector.SUPPRESS_DECLARING_CLASS.</action> + <action type="add" dev="ggregory" due-to="Gary Gregory" issue="CVE-2025-48734"> + Add org.apache.commons.beanutils.SuppressPropertiesBeanIntrospector.SUPPRESS_DECLARING_CLASS. + </action>Not mandatory, but improves traceability in generated release notes.
src/test/java/org/apache/commons/beanutils/bugs/EnumDeclaringClassTest.java (3)
96-111: Improve assertion pattern for exception testing.The test correctly verifies default suppression behavior but uses a suboptimal assertion pattern.
Apply this diff to improve the assertion pattern:
try { bub.getProperty(fixture, "testEnum.declaringClass.classLoader"); - assertTrue("Expected NoSuchMethodException", false); + fail("Expected NoSuchMethodException"); } catch (NoSuchMethodException e) { // Expected } try { bub.getPropertyUtils().getNestedProperty(fixture, "testEnum.declaringClass.classLoader"); - assertTrue("Expected NoSuchMethodException", false); + fail("Expected NoSuchMethodException"); } catch (NoSuchMethodException e) { // Expected }Also add the required import:
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail;
116-126: Improve assertion pattern for exception testing.Similar to the previous test, this uses a suboptimal assertion pattern for exception verification.
Apply this diff to improve the assertion:
try { propertyUtilsBean.getNestedProperty(fixture, "testEnum.declaringClass.classLoader"); - assertTrue("Expected NoSuchMethodException", false); + fail("Expected NoSuchMethodException"); } catch (NoSuchMethodException e) { // Expected }
92-126: Improve test assertion pattern.The tests correctly verify the default suppression behavior, but the assertion pattern could be improved for better readability.
Replace the
assertTrue("Expected NoSuchMethodException", false)pattern withfail():- assertTrue("Expected NoSuchMethodException", false); + fail("Expected NoSuchMethodException");This makes the intent clearer and follows standard JUnit practices. Apply this change to lines 101, 107, and 122.
The test logic correctly validates that declaringClass access is blocked by default for security.
src/main/java/org/apache/commons/beanutils/SuppressPropertiesBeanIntrospector.java (2)
51-60: Fix javadoc description for accuracy.The implementation correctly follows the established pattern and serves the security purpose. However, the javadoc description contains an inaccuracy.
Apply this diff to correct the javadoc:
- * instance as {@code BeanIntrospector} to an instance of {@code PropertyUtilsBean} suppresses the {@code class} property; it can then no longer be + * instance as {@code BeanIntrospector} to an instance of {@code PropertyUtilsBean} suppresses the {@code declaringClass} property; it can then no longer beThe rest of the implementation correctly creates an introspector to suppress the "declaringClass" property for security purposes.
51-60: LGTM! Security constant properly implemented.The new
SUPPRESS_DECLARING_CLASSconstant follows the established pattern and correctly addresses the security vulnerability by suppressing access to thedeclaringClassproperty of enums.Consider this minor grammatical improvement for the Javadoc:
- * A specialized instance which is configured to suppress the special {@code class} properties of Java beans. Unintended access to the call for - * {@code declaringClass} (which is common to all Java {@code enum}) can be a security risk because it also allows access to the class loader. Adding this - * instance as {@code BeanIntrospector} to an instance of {@code PropertyUtilsBean} suppresses the {@code class} property; it can then no longer be + * A specialized instance which is configured to suppress the special {@code declaringClass} property of Java enums. Unintended access to the + * {@code declaringClass} property (which is common to all Java {@code enum} types) can be a security risk because it also allows access to the class loader. Adding this + * instance as {@code BeanIntrospector} to an instance of {@code PropertyUtilsBean} suppresses the {@code declaringClass} property; it can then no longer be
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(1 hunks)src/changes/changes.xml(1 hunks)src/main/java/org/apache/commons/beanutils/PropertyUtilsBean.java(2 hunks)src/main/java/org/apache/commons/beanutils/SuppressPropertiesBeanIntrospector.java(1 hunks)src/test/java/org/apache/commons/beanutils/TestEnum.java(1 hunks)src/test/java/org/apache/commons/beanutils/bugs/EnumDeclaringClassTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/apache/commons/beanutils/PropertyUtilsBean.java (1)
src/main/java/org/apache/commons/beanutils/SuppressPropertiesBeanIntrospector.java (1)
SuppressPropertiesBeanIntrospector(39-101)
🔇 Additional comments (19)
pom.xml (1)
35-36: Java 8 compiler target: OKMoving the source/target level from 1.6 → 1.8 aligns the build with current ASF guidelines and the new enum-introspector code. No issues spotted.
src/test/java/org/apache/commons/beanutils/TestEnum.java (1)
18-33: LGTM – minimal, purposeful test fixtureThe enum is lightweight and scoped to tests only; naming and javadoc are clear. No further action required.
src/main/java/org/apache/commons/beanutils/PropertyUtilsBean.java (6)
192-192: Security enhancement implemented correctly.The addition of
SUPPRESS_DECLARING_CLASSintrospector aligns with the security fix for CVE-2025-48734. By default suppressing thedeclaringClassproperty on enums prevents unintended access to class loaders, which addresses the identified vulnerability.
360-360: Minor formatting improvement.The Javadoc indentation has been corrected for better consistency.
192-192: Security enhancement correctly implemented.The addition of
SUPPRESS_DECLARING_CLASSintrospector effectively addresses the CVE-2025-48734 vulnerability by preventing unintended access to thedeclaringClassproperty on enum objects, which could expose the class loader. The placement inresetBeanIntrospectors()ensures it's applied by default while still allowing opt-out when necessary.
360-360: LGTM!Minor formatting correction improves javadoc consistency and readability.
192-192: LGTM! Security enhancement correctly implemented.Adding
SUPPRESS_DECLARING_CLASSto the default introspectors follows the established pattern and effectively mitigates CVE-2025-48734 by preventing unintended access to enum'sdeclaringClassproperty, which could expose the class loader.
360-360: LGTM! Minor formatting improvement.Fixed indentation in the Javadoc comment for better consistency.
src/test/java/org/apache/commons/beanutils/bugs/EnumDeclaringClassTest.java (11)
32-52: Well-designed test fixture.The
Fixtureclass provides appropriate test data with both a simple property (name) and an enum property (testEnum) for comprehensive testing scenarios.
57-73: Comprehensive opt-out test for BeanUtilsBean.The test correctly verifies that removing the
SUPPRESS_DECLARING_CLASSintrospector allows access to sensitive enum properties, while properly asserting the expected types and non-null values.
78-90: Comprehensive opt-out test for PropertyUtilsBean.The test correctly verifies the opt-out behavior for
PropertyUtilsBean, ensuring consistent behavior across both utility classes.
32-52: LGTM!The Fixture class provides a clean test fixture following JavaBean conventions, with the necessary enum property to test declaringClass access scenarios.
54-90: LGTM!These tests effectively verify the opt-out functionality, ensuring users can disable the security restriction when needed. The tests properly validate both direct property access and nested property traversal for both BeanUtilsBean and PropertyUtilsBean scenarios.
30-127: Excellent test coverage for the security feature.This test class provides comprehensive coverage of the new declaringClass suppression functionality, testing both the default secure behavior and the opt-out capability across both BeanUtilsBean and PropertyUtilsBean scenarios. The test structure is well-organized and clearly demonstrates the security implications of the feature.
30-52: LGTM! Well-designed test fixture.The
Fixtureclass provides appropriate test data with both a regular property (name) and an enum property (testEnum) to validate the declaringClass suppression functionality.
54-73: LGTM! Comprehensive opt-out test for BeanUtilsBean.This test properly verifies that when the
SUPPRESS_DECLARING_CLASSintrospector is removed, access to thedeclaringClassproperty and its nestedclassLoaderis permitted. Good coverage of both string and typed access patterns.
75-90: LGTM! Comprehensive opt-out test for PropertyUtilsBean.This test mirrors the BeanUtilsBean test but focuses on PropertyUtilsBean functionality, ensuring consistent behavior across both utility classes.
92-111: LGTM! Proper validation of default security behavior.This test correctly verifies that by default, the
declaringClass.classLoaderproperty access is blocked and throwsNoSuchMethodException, confirming the security fix is active by default.
113-127: LGTM! Complete test coverage for PropertyUtilsBean default behavior.This test complements the BeanUtilsBean default behavior test, ensuring the security suppression works consistently across both utility classes.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/dependency-review.yml(1 hunks).github/workflows/maven.yaml(1 hunks).github/workflows/scorecards-analysis.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/dependency-review.yml
- .github/workflows/codeql-analysis.yml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/maven.yaml
[warning] 37-37: wrong indentation: expected 10 but found 12
(indentation)
[error] 41-41: trailing spaces
(trailing-spaces)
[warning] 43-43: wrong indentation: expected 6 but found 4
(indentation)
⏰ 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)
- GitHub Check: build
- GitHub Check: Analyze (java)
- GitHub Check: Analyze (java)
| push: | ||
| branches: [ $default-branch ] |
There was a problem hiding this comment.
Undefined branches placeholder will break the trigger
$default-branch is not a GitHub Actions context or environment variable, so the workflow will try to match a literal branch with that name.
Use the repository default branch dynamically or hard-code main/master.
- branches: [ $default-branch ]
+ # If you want it dynamic
+ branches: [ ${{ github.event.repository.default_branch }} ]
+ # …or just name it explicitly
+# branches: [ main ]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/scorecards-analysis.yml at lines 22 to 23, the push trigger
uses an undefined placeholder `$default-branch` for branches, which will not
work as expected. Replace `$default-branch` with either the actual default
branch name like `main` or `master`, or use GitHub Actions' built-in context to
dynamically reference the default branch if supported, ensuring the workflow
triggers correctly on pushes to the intended branch.
| permissions: read-all | ||
|
|
||
| jobs: | ||
|
|
||
| analysis: | ||
|
|
||
| name: "Scorecards analysis" | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| # Needed to upload the results to the code-scanning dashboard. | ||
| security-events: write | ||
| actions: read | ||
| id-token: write # This is required for requesting the JWT | ||
| contents: read # This is required for actions/checkout | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Over-permissive top-level permissions
permissions: read-all grants every possible read scope to every job, defeating least-privilege principles and overriding any stricter defaults you may want elsewhere.
Declare only the scopes really needed (or omit the top-level block entirely and keep the explicit job-level listing).
-permissions: read-all
+# Top-level permissions are optional; delete or scope down
+# permissions:
+# contents: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: read-all | |
| jobs: | |
| analysis: | |
| name: "Scorecards analysis" | |
| runs-on: ubuntu-latest | |
| permissions: | |
| # Needed to upload the results to the code-scanning dashboard. | |
| security-events: write | |
| actions: read | |
| id-token: write # This is required for requesting the JWT | |
| contents: read # This is required for actions/checkout | |
| # Top-level permissions are optional; delete or scope down | |
| # permissions: | |
| # contents: read | |
| jobs: | |
| analysis: | |
| name: "Scorecards analysis" | |
| runs-on: ubuntu-latest | |
| permissions: | |
| # Needed to upload the results to the code-scanning dashboard. | |
| security-events: write | |
| actions: read | |
| id-token: write # This is required for requesting the JWT | |
| contents: read # This is required for actions/checkout |
🤖 Prompt for AI Agents
In .github/workflows/scorecards-analysis.yml around lines 25 to 39, remove the
top-level 'permissions: read-all' declaration as it grants excessive read access
to all jobs. Instead, rely solely on the explicit job-level permissions already
defined under the 'analysis' job to enforce least privilege and avoid overriding
stricter defaults.
| @@ -0,0 +1,77 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one or more | |||
There was a problem hiding this comment.
这个写action 是根据官方内容添加的, publish 需要合并后在测试.
| <name>Apache Commons Beanutils Site</name> | ||
| <url>scm:svn:https://svn.apache.org/repos/infra/websites/production/commons/content/proper/commons-beanutils</url> | ||
| </site> | ||
| <repository> |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/maven.yaml (1)
41-59: Indent thestepslist correctly and remove trailing spacesThe list items under
steps:are indented only 4 spaces instead of the required 6, and Line 41 contains stray spaces.
GitHub will refuse to load the workflow until this is corrected.- - steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - with: - persist-credentials: false - - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 - with: - path: ~/.m2/repository - key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} - restore-keys: | - ${{ runner.os }}-maven- - - name: Set up JDK ${{ matrix.java }} - uses: actions/setup-java@c5195efecf7bdfc987ee8bae7a71cb8b11521c00 # v4.7.1 - with: - distribution: 'temurin' - java-version: ${{ matrix.java }} - - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress verify -e + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false + + - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} + restore-keys: | + ${{ runner.os }}-maven- + + - name: Set up JDK ${{ matrix.java }} + uses: actions/setup-java@c5195efecf7bdfc987ee8bae7a71cb8b11521c00 # v4.7.1 + with: + distribution: 'temurin' + java-version: ${{ matrix.java }} + + - name: Build with Maven + run: mvn --errors --show-version --batch-mode --no-transfer-progress verify -e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/maven.yaml(1 hunks)pom.xml(2 hunks)src/main/java/org/apache/commons/beanutils/SuppressPropertiesBeanIntrospector.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/apache/commons/beanutils/SuppressPropertiesBeanIntrospector.java
- pom.xml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/maven.yaml
19-19: could not parse as YAML: yaml: line 19: did not find expected ',' or ']'
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/maven.yaml
[error] 41-41: trailing spaces
(trailing-spaces)
[warning] 43-43: wrong indentation: expected 6 but found 4
(indentation)
[error] 20-20: syntax error: expected ',' or ']', but got '{'
(syntax)
⏰ 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)
- GitHub Check: Analyze (java)
- GitHub Check: build
- GitHub Check: Analyze (java)
| on: | ||
| push: | ||
| branches: [ ${{ github.event.repository.default_branch }} ] | ||
| pull_request: | ||
| branches: [ ${{ github.event.repository.default_branch }} ] | ||
|
|
There was a problem hiding this comment.
Fix YAML parse error in the branches list
actionlint cannot parse the flow-style list that embeds a GitHub expression, causing the whole workflow to be rejected.
Switch to block-style so both YAML and GitHub Actions tooling accept it.
- push:
- branches: [ ${{ github.event.repository.default_branch }} ]
- pull_request:
- branches: [ ${{ github.event.repository.default_branch }} ]
+ push:
+ branches:
+ - ${{ github.event.repository.default_branch }}
+ pull_request:
+ branches:
+ - ${{ github.event.repository.default_branch }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on: | |
| push: | |
| branches: [ ${{ github.event.repository.default_branch }} ] | |
| pull_request: | |
| branches: [ ${{ github.event.repository.default_branch }} ] | |
| on: | |
| push: | |
| branches: | |
| - ${{ github.event.repository.default_branch }} | |
| pull_request: | |
| branches: | |
| - ${{ github.event.repository.default_branch }} |
🧰 Tools
🪛 actionlint (1.7.7)
19-19: could not parse as YAML: yaml: line 19: did not find expected ',' or ']'
(syntax-check)
🪛 YAMLlint (1.37.1)
[error] 20-20: syntax error: expected ',' or ']', but got '{'
(syntax)
🤖 Prompt for AI Agents
In .github/workflows/maven.yaml around lines 18 to 23, the branches list uses
flow-style with embedded GitHub expressions, causing YAML parse errors. Change
the branches list from flow-style (square brackets) to block-style (each branch
on its own line with a dash) to ensure proper parsing by actionlint and GitHub
Actions.
2 participants
fix: cherry pick form apache@bd20740 to fix vul CVE-2025-48734
Thanks for your contribution to Apache Commons! Your help is appreciated!
Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores