DNS Provider URL Validation - #13821
Conversation
| */ | ||
| private void validateDnsServerUrl(String url) { | ||
| if (StringUtils.isBlank(url)) { | ||
| return; |
There was a problem hiding this comment.
URL is a required API parameter, maybe throw an exception here ?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13821 +/- ##
=========================================
Coverage 19.64% 19.64%
- Complexity 19786 19789 +3
=========================================
Files 6368 6368
Lines 574889 574897 +8
Branches 70353 70353
=========================================
+ Hits 112957 112967 +10
+ Misses 449660 449658 -2
Partials 12272 12272
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens DNS provider configuration by adding pre-connection URL validation in DnsProviderManagerImpl so user-supplied DNS provider endpoints are checked (and normalized via trimming) before being persisted or handed to provider clients.
Changes:
- Add
validateDnsServerUrl()and invoke it inaddDnsServerand whenupdateDnsServerchanges the URL. - Normalize URLs by trimming before duplicate checks and persistence.
- Update and extend unit tests to cover trimming behavior and rejection of invalid URLs (e.g., loopback, missing scheme).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java | Adds URL trimming + validation before duplicate checks/persistence and before provider validation. |
| server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java | Adjusts existing tests and adds new cases for trimming and invalid URL rejection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void validateDnsServerUrl(String url) { | ||
| if (StringUtils.isBlank(url)) { | ||
| return; | ||
| } | ||
| UriUtils.validateUrl(url); | ||
| } |
There was a problem hiding this comment.
I think this comment makes sense (cc @sudo87 ) however, implementing this like co-pilot suggests makes no sense. ftp: or mailto: or many other scheme prefixes would have to be checked. Would we accept any others than http-like ones? (ref https://en.wikipedia.org/wiki/List_of_URI_schemes) I wouldn’t mind dns: (not listed on the page I shared)
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.51% |
| Branch coverage | 18.67% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
clgtm, not sure if copilot comment regarding "file" protocol is valid |
a555469 to
6981138
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:171
- The JavaDoc claims
UriUtils.validateUrl(String)enforces an http/https scheme, butUriUtils.validateUrlalso allowsfile. Either the JavaDoc should be corrected, orvalidateDnsServerUrlshould explicitly enforce http/https (and document that additional restriction).
* Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
* the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
* (including the requirement that the URL declares an {@code http}/{@code https} scheme).
* Expects {@code url} to already be trimmed.
*/
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:176
UriUtils.validateUrlpermits thefilescheme (seeUriUtils.validateUrl), so the new DNS server URL validation currently still allowsfile:URLs. For a provider endpoint this is unexpected and can open up local-file / non-HTTP URL handling paths in downstream clients; it also contradicts the intent to require http/https URLs.
private void validateDnsServerUrl(String url) {
if (StringUtils.isBlank(url)) {
throw new IllegalArgumentException("URL cannot be blank.");
}
UriUtils.validateUrl(url);
| @Test(expected = IllegalArgumentException.class) | ||
| public void testAddDnsServerRejectsUrlWithoutScheme() { | ||
| org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( | ||
| org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); | ||
| when(cmd.getUrl()).thenReturn("192.0.2.1:8081"); | ||
| manager.addDnsServer(cmd); | ||
| } | ||
|
|
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.51% |
| Branch coverage | 18.67% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
Description
DnsProviderManagerImpl.addDnsServer/updateDnsServer accepted a user-controlled URL and passed it straight to PowerDnsProvider/PowerDnsClient with zero host validation — no egress check at all, unlike the template/webhook paths.
Added validateDnsServerUrl(), called at the top of addDnsServer and whenever updateDnsServer changes the URL.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?