Skip to content

DNS Provider URL Validation #13821

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.

Sign up for GitHub

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

Draft
DaanHoogland wants to merge 3 commits into
base: main
Choose a base branch
from
+93 −12
Draft

DNS Provider URL Validation #13821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9714982
validate DNS server URLs in provider framework
Aug 7, 2026
1fa1638
fixes
Aug 7, 2026
6981138
Apply suggestion from @DaanHoogland
DaanHoogland Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
import com.cloud.user.dao.AccountDao;
import com.cloud.utils.Pair;
import com.cloud.utils.StringUtils;
import com.cloud.utils.UriUtils;
import com.cloud.utils.component.ManagerBase;
import com.cloud.utils.component.PluggableService;
import com.cloud.utils.db.Filter;
Expand Down Expand Up @@ -162,14 +163,29 @@
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}

/**
* 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.

@winterhazel winterhazel Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we trim the URL inside the method instead of expecting it to be trimmed?

*/
private void validateDnsServerUrl(String url) {
if (StringUtils.isBlank(url)) {

Check failure on line 173 in server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use static access with "org.apache.commons.lang3.StringUtils" for "isBlank". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ_b59lPGDdNQ584wbmr&open=AZ_b59lPGDdNQ584wbmr&pullRequest=13821
throw new IllegalArgumentException("URL cannot be blank.");

@winterhazel winterhazel Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new IllegalArgumentException("URL cannot be blank.");
throw new InvalidParameterValueException("URL cannot be blank.");

A InvalidParameterValueException makes more sense. Also, can we catch the exception of UriUtils.validateUrl and throw a InvalidParameterValueException too?

}
UriUtils.validateUrl(url);

@winterhazel winterhazel Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UriUtils.validateUrl does not reject addresses belonging private networks (e.g 192.168.1.1 is still accepted). If this URL specifies a DNS server which the MS will send commands to, then it should be rejected in my opinion.

cc @sudo87 @weizhouapache

}

@Override
@ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server")
public DnsServer addDnsServer(AddDnsServerCmd cmd) {
String url = StringUtils.trim(cmd.getUrl());

Check failure on line 182 in server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use static access with "org.apache.commons.lang3.StringUtils" for "trim". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ_b59lPGDdNQ584wbms&open=AZ_b59lPGDdNQ584wbms&pullRequest=13821
validateDnsServerUrl(url);
Account caller = CallContext.current().getCallingAccount();
DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId());
DnsServer existing = dnsServerDao.findByUrlAndAccount(url, caller.getId());
if (existing != null) {
throw new InvalidParameterValueException(
"This Account already has a DNS server integration for URL: " + cmd.getUrl());
"This Account already has a DNS server integration for URL: " + url);
}

boolean isDnsPublic = cmd.isPublic();
Expand All @@ -185,7 +201,7 @@
}

DnsProviderType type = cmd.getProvider();
DnsServerVO server = new DnsServerVO(cmd.getName(), cmd.getUrl(), cmd.getPort(), type,
DnsServerVO server = new DnsServerVO(cmd.getName(), url, cmd.getPort(), type,
cmd.getDnsUserName(), cmd.getDnsApiKey(), isDnsPublic, publicDomainSuffix, cmd.getNameServers(),
caller.getAccountId(), caller.getDomainId());

Expand Down Expand Up @@ -251,12 +267,14 @@
}

if (cmd.getUrl() != null) {
if (!cmd.getUrl().equals(originalUrl)) {
DnsServer duplicate = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), dnsServer.getAccountId());
String url = StringUtils.trim(cmd.getUrl());

Check failure on line 270 in server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use static access with "org.apache.commons.lang3.StringUtils" for "trim". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ_b59lPGDdNQ584wbmt&open=AZ_b59lPGDdNQ584wbmt&pullRequest=13821
if (!url.equals(originalUrl)) {
validateDnsServerUrl(url);
DnsServer duplicate = dnsServerDao.findByUrlAndAccount(url, dnsServer.getAccountId());
if (duplicate != null && duplicate.getId() != dnsServer.getId()) {
throw new InvalidParameterValueException("Another DNS server with this URL already exists.");
}
dnsServer.setUrl(cmd.getUrl());
dnsServer.setUrl(url);
validationRequired = true;
}
}
Expand Down
75 changes: 69 additions & 6 deletions server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081");
when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS);
when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null);
when(dnsProviderMock.validateAndResolveServer(any())).thenReturn("resolved-id");
Expand Down Expand Up @@ -781,18 +781,51 @@
public void testAddDnsServerAlreadyExists() {
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081");
when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(serverVO);
manager.addDnsServer(cmd);
}

@Test
public void testAddDnsServerTrimsUrlBeforeDuplicateCheckAndPersistence() throws Exception {
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
when(cmd.getUrl()).thenReturn(" http://192.0.2.1:8081 ");
when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS);
when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null);
when(dnsProviderMock.validateAndResolveServer(any())).thenReturn("resolved-id");
when(dnsServerDao.persist(any())).thenReturn(serverVO);

manager.addDnsServer(cmd);

verify(dnsServerDao).findByUrlAndAccount(eq("http://192.0.2.1:8081"), anyLong());
verify(dnsServerDao).persist(Mockito.argThat(s -> "http://192.0.2.1:8081".equals(((DnsServerVO) s).getUrl())));
}

@Test(expected = IllegalArgumentException.class)
public void testAddDnsServerRejectsLoopbackUrl() {
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
when(cmd.getUrl()).thenReturn("http://127.0.0.1:8081");
manager.addDnsServer(cmd);
}

@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);
}

Comment on lines +814 to +821
@Test
public void testAddDnsServerNormalUser() throws Exception {
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(false);
when(accountMgr.isDomainAdmin(callerMock.getId())).thenReturn(false);
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081");
when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS);
when(cmd.getNameServers()).thenReturn(Collections.emptyList());
when(cmd.isPublic()).thenReturn(true);
Expand All @@ -811,7 +844,7 @@
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081");
when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS);
when(cmd.getNameServers()).thenReturn(Collections.emptyList());
when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null);
Expand All @@ -824,7 +857,7 @@
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class);
when(cmd.getId()).thenReturn(SERVER_ID);
when(cmd.getUrl()).thenReturn("http://duplicate:8081");
when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081");
DnsServerVO existingServer = mock(DnsServerVO.class);
when(existingServer.getId()).thenReturn(SERVER_ID + 1); // Different ID implies duplicate

Expand All @@ -835,12 +868,42 @@
manager.updateDnsServer(cmd);
}

@Test(expected = IllegalArgumentException.class)
public void testUpdateDnsServerRejectsLoopbackUrl() {
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class);
when(cmd.getId()).thenReturn(SERVER_ID);
when(cmd.getUrl()).thenReturn("http://127.0.0.1:8081");
when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
Mockito.doReturn("http://original:8081").when(serverVO).getUrl();

Check warning on line 878 in server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "doReturn". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ_b59q_GDdNQ584wbmu&open=AZ_b59q_GDdNQ584wbmu&pullRequest=13821

manager.updateDnsServer(cmd);
}

@Test
public void testUpdateDnsServerTreatsWhitespaceOnlyUrlChangeAsUnchanged() throws Exception {
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class);
Integer unchangedPort = serverVO.getPort();
when(cmd.getId()).thenReturn(SERVER_ID);
when(cmd.getUrl()).thenReturn(" http://192.0.2.1:8081 ");
when(cmd.getPort()).thenReturn(unchangedPort);
when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
Mockito.doReturn("http://192.0.2.1:8081").when(serverVO).getUrl();

Check warning on line 892 in server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "doReturn". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ_b59q_GDdNQ584wbmv&open=AZ_b59q_GDdNQ584wbmv&pullRequest=13821
when(dnsServerDao.update(anyLong(), any())).thenReturn(true);

DnsServer result = manager.updateDnsServer(cmd);
assertNotNull(result);
verify(dnsProviderMock, never()).validate(any());
verify(serverVO, never()).setUrl(anyString());
}

@Test
public void testUpdateDnsServerUrlValid() throws Exception {
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class);
when(cmd.getId()).thenReturn(SERVER_ID);
when(cmd.getUrl()).thenReturn("http://new-url:8081");
when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081");
when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);

Mockito.doReturn("http://original:8081").when(serverVO).getUrl();
Expand Down
Loading