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.65% 19.65%
Complexity 19792 19792
=========================================
Files 6368 6368
Lines 574881 574897 +16
Branches 70351 70353 +2
=========================================
+ Hits 112970 112976 +6
- Misses 449639 449652 +13
+ Partials 12272 12269 -3
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); | ||
| } |
|
🔴 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 |
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?