yyqdbngt opened a new pull request, #4681:
URL: https://github.com/apache/rocketmq-dashboard/pull/4681
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. -->
### Brief Description
`POST /api/settings/datasources/test` probes a caller-supplied URL
server-side, and it judged that
host with a **copy** of the SSRF policy
(`SettingsService.areAllowedDataSourceAddresses`,
`settings/SettingsService.java:421`) instead of the shared guard that
`POST /api/settings/datasources/create|update` uses
(`SettingsService.validateDataSourceUrl` →
`common/util/UrlHostGuard.check`). The copy had drifted from the shared
guard: it checked
any-local, link-local, loopback and two known metadata endpoints, but not
multicast
(`224.0.0.0/4`) or IPv6 unique-local (`fc00::/7`) addresses. Both of those
are refused by
`UrlHostGuard`, so the test endpoint resolved and connected to hosts that
saving the same data
source rejects with 400 — "Test connection" and "Save" could not agree, and
a metadata-range
IPv6 target outside the single hard-coded `fd00:ec2::254` entry (e.g.
`fd12:…`) was reachable
through the test endpoint.
The address categories now come from the shared guard:
`UrlHostGuard.areAllowed` is widened from
package-private to public (it is the guard's single source of truth for
resolved addresses), and
`areAllowedDataSourceAddresses` keeps only the metadata endpoints Java's
address categories do not
cover before delegating the rest. The `localhost` name check and the "fail
closed on an
unresolvable host" behaviour are unchanged.
No issue exists for this; it comes from an audit of the settings module.
### How Did You Test This Change?
New tests in `settings/SettingsServiceTest` —
`connectionShouldRejectMulticastAddressTest`,
`connectionShouldRejectUniqueLocalIpv6AddressTest` and
`dataSourceAddressPolicyShouldRejectMulticastAndUniqueLocalAddressesTest` —
plus the existing
`connectionShouldRejectLocalhostHostnameTest`,
`connectionShouldRejectLinkLocalMetadataAddressTest`,
`connectionShouldRejectAwsImdsIpv6AddressTest`,
`connectionShouldRejectAlibabaCloudMetadataAddressTest`
and `dataSourceAddressPolicyShould*` tests, which pin the behaviour that
must not regress.
Before the fix (red) — note that the two URL-level tests fail because the
request is actually
dispatched to the address, which is the vulnerability:
```
$ cd server && mvn -B -ntp test
-Dtest='SettingsServiceTest#dataSourceAddressPolicyShouldRejectMulticastAndUniqueLocalAddressesTest+connectionShouldRejectMulticastAddressTest+connectionShouldRejectUniqueLocalIpv6AddressTest'
[ERROR] Tests run: 3, Failures: 3, Errors: 0, Skipped: 0
[ERROR]
SettingsServiceTest.dataSourceAddressPolicyShouldRejectMulticastAndUniqueLocalAddressesTest:699
Expecting value to be false but was true
[ERROR] SettingsServiceTest.connectionShouldRejectMulticastAddressTest:706
No further requests expected: HTTP GET
http://239.192.1.1:9090/api/v1/query?query=up
[ERROR]
SettingsServiceTest.connectionShouldRejectUniqueLocalIpv6AddressTest:717
No further requests expected: HTTP GET
http://[fd12:3456:789a::1]:9090/api/v1/query?query=up
[INFO] BUILD FAILURE
```
After the fix (green) — the whole class, and then every settings/common
class that touches the
guard:
```
$ cd server && mvn -B -ntp test -Dtest=SettingsServiceTest
[INFO] You have 0 Checkstyle violations.
[INFO] Tests run: 46, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
$ cd server && mvn -B -ntp test
-Dtest=UrlHostGuardTest,UrlHostGuardMulticastTest,SettingsServiceTest,SettingsServiceCachingTest,SettingsControllerTest,DataSourceDTOTest,DataSourceTestDTOTest,DataSourceClientHttpRequestFactoryTest,MybatisPlusSettingsRepositoryTest
[INFO] Tests run: 91, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
```
`mvn test` runs checkstyle in the `validate` phase: `You have 0 Checkstyle
violations.`
Note on the full suite: on a clean `rocketmq-studio` checkout `mvn -B -ntp
test` already reports
`Tests run: 3051, Failures: 6, Errors: 25, Skipped: 4`; the eleven
pre-existing red classes are the
MySQL 8 backed Spring integration tests
(`AuthServiceBootstrapIntegrationTest`,
`AuthServiceConcurrencyIntegrationTest`,
`AuthServiceSessionOverviewIntegrationTest`,
`HealthProbeIntegrationTest`, `QueryHistoryServiceIntegrationTest`,
`NativeAlertEvaluationTransactionTest`,
`NotificationOutboxMapperIntegrationTest`,
`RmqAlertStateMapperIntegrationTest`, `StudioApplicationTest`) and the
external-CLI ones
(`CliAgentProviderTest`, `ClaudeCodeAgentProviderTest`). None of them are
touched by this change.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`fix:`)
- [x] Tests added or updated for non-trivial changes, test methods named
`...Test`
- [x] New UI text has both Chinese and English entries under `web/src/i18n/`
(no UI text in this change)
- [x] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks)
- [x] New source files carry the ASF license header (no new source files)
- [x] Documentation touched where behaviour changed (the guard javadoc now
states the categories it owns; no user-facing API change)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
