Domain is passed as a REGEX pattern, not a literal string. by daviftorres · Pull Request #12937 · apache/cloudstack

daviftorres · 2026-03-31T15:21:30Z

Description

This change ensures consistency with how paths are parsed when updating a domain path.

The modified line was passing the domain name as a literal string, but it is actually interpreted as a regular expression internally.

I couldn’t find a way to exploit this issue, but it could still cause data corruption if a domain name accidentally contains regex metacharacters.

Note that this same technique is already used in a similar situation on line 1118.

A common example is when an organization uses its DNS name as the "domain" (tenant), like company.com.

In this case, the . (dot) is treated as a regex wildcard, meaning it can match any character...

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

This fix was not tested. I am not a Java developer, and I have no skill for such a thing.

How did you try to break this feature and the system with this change?

I could trigger, but I could not exploit this bug.

codecov · 2026-03-31T15:53:03Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.41%. Comparing base (4f11707) to head (66101d8).
⚠️ Report is 4 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (4f11707) and HEAD (66101d8). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (4f11707) HEAD (66101d8)

unittests 1 0

Additional details and impacted files

@@              Coverage Diff              @@
##               main   #12937       +/-   ##
=============================================
- Coverage     19.65%    3.41%   -16.24%     
=============================================
  Files          6368      487     -5881     
  Lines        574881    41867   -533014     
  Branches      70351     7912    -62439     
=============================================
- Hits         112970     1429   -111541     
+ Misses       449639    40238   -409401     
+ Partials      12272      200    -12072

Flag	Coverage Δ
uitests	`3.41% <ø> (ø)`
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

daviftorres · 2026-03-31T16:01:12Z

            DomainVO domainWithResourceUsedByDomainToBeMoved = _domainDao.findById(entry.getKey());

-            Pattern pattern = Pattern.compile(domainWithResourceUsedByDomainToBeMoved.getPath().replace("/", "\\/").concat(".*"));
+            Pattern pattern = Pattern.compile(domainWithResourceUsedByDomainToBeMoved.getPath().replace("/", "\\/").concat(".*")); // This only scaped one Regex metacharacter (/). The wildcard `.` is more common and dangerous in my opinion. 


@DaanHoogland ,

I did not propose a fix because I need help for that.

Hey @DaanHoogland, do not forget this other part of the code that only escapes /, then adds the God of wildcards (.*), and passes for a domain to be moved.

I guess this has the potential of corrupting all resources of the domain being moved or affecting some other unrelated domain that happens to match the Regex.

But I am not qualified to propose a code fix in this case. I would like your help to assess this bug, too.

Dear @sureshanaparti , please note that this is not completed yet. I guess...

Dear @DaanHoogland , you are correct. This was my intention to point out that the issue is also present in another line and I was looking for collaboration because I barely read some of the Java code I see.

DaanHoogland · 2026-03-31T16:10:26Z

I have no idea if this is a sensible change @daviftorres , I cannot see the issue and do not see what the change entails in behavioural change.

daviftorres · 2026-03-31T16:27:32Z

I have no idea if this is a sensible change @daviftorres , I cannot see the issue and do not see what the change entails in behavioural change.

If a domain name contains regex metacharacters, renaming it can fail because the regex gets interpreted incorrectly.
A proper solution already exists in this file (line 1118), so I reused that implementation.

By the way, I’m fine if you decide not to proceed. I just wanted to point out an issue I came across, which could potentially lead to data corruption if certain conditions are met.

DaanHoogland · 2026-04-01T13:17:22Z

I’m fine with it, @daviftorres , if for nothing else, to make th4e code consistent. I still don’t get the fix but will have to study on StringUtils I guess.

sureshanaparti · 2026-04-02T04:01:50Z

@blueorangutan package

DaanHoogland · 2026-05-27T12:51:28Z

@blueorangutan package

daviftorres · 2026-05-27T12:53:00Z

@blueorangutan package

blueorangutan · 2026-05-27T12:54:04Z

@daviftorres a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-05-27T13:11:26Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18059

blueorangutan · 2026-05-27T13:11:26Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18060

DaanHoogland · 2026-05-27T13:21:04Z

@daviftorres ,

15:03:30 [INFO] Starting audit...
15:03:38 [ERROR] /jenkins/workspace/acs-centos8-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.23.0.0-SNAPSHOT/server/src/main/java/com/cloud/user/DomainManagerImpl.java:1096: Line has trailing spaces. [RegexpSingleline]

also this line has no logical change yet, I think you need to implement some change in the regexp for this PR to make sense.

This change ensures consistency with how paths are parsed when updating a domain path. The modified line was passing the domain name as a literal string, but it is actually interpreted as a regular expression internally. I couldn’t find a way to exploit this issue, but it could still cause data corruption if a domain name accidentally contains regex metacharacters. Note that this same technique is already used in a similar situation on line 1118. A common example is when an organization uses its DNS name as the "domain" (tenant), like `company.com`. In this case, the `.` (dot) is treated as a regex wildcard, meaning it can match any character...

Updated regex pattern to escape only the '/' character while noting potential risks with the wildcard '.' character.

Replaced StringUtils from Apache Commons Lang with local one.

Copilot

Pull request overview

This PR aims to prevent domain-path corruption caused by treating domain path strings as regular expressions during path updates, ensuring path manipulation behaves consistently for domain names that may contain regex metacharacters (e.g., company.com).

Changes:

Switched child-domain path updates from replaceFirst (regex) to a literal first-occurrence replace to avoid regex interpretation.
Standardized StringUtils usage by importing com.cloud.utils.StringUtils (which extends Apache Commons StringUtils).
Added an inline note highlighting regex-escaping concerns in domain-path matching logic.

Comments suppressed due to low confidence (2)

server/src/main/java/com/cloud/user/DomainManagerImpl.java:1096

The regex used to check whether the new path is within an existing domain path only escapes "/". If a domain name/path segment contains regex metacharacters (e.g., ".", "+", "["), this pattern can match incorrect paths and allow/deny moves incorrectly. Quote the entire path when building the regex instead of selectively escaping characters (and remove the in-line comment which currently documents the issue but does not fix it).

            Pattern pattern = Pattern.compile(domainWithResourceUsedByDomainToBeMoved.getPath().replace("/", "\\/").concat(".*")); // This only scaped one Regex metacharacter (/). The wildcard `.` is more common and dangerous in my opinion.

server/src/main/java/com/cloud/user/DomainManagerImpl.java:1096

The access-validation logic relies on a regex prefix match of domain paths. Add a unit test that uses a domain path containing regex metacharacters (e.g., "/company.com/child/") to verify the matcher logic behaves correctly after quoting the path.

            Pattern pattern = Pattern.compile(domainWithResourceUsedByDomainToBeMoved.getPath().replace("/", "\\/").concat(".*")); // This only scaped one Regex metacharacter (/). The wildcard `.` is more common and dangerous in my opinion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

        // for each child, update the path
        for (DomainVO dom : domainChildren) {
-            dom.setPath(dom.getPath().replaceFirst(domain.getPath(), updatedDomainPrefix));
+            dom.setPath(StringUtils.replaceOnce(dom.getPath(), domain.getPath(), updatedDomainPrefix));


DaanHoogland reviewed Mar 31, 2026

View reviewed changes

Comment thread server/src/main/java/com/cloud/user/DomainManagerImpl.java

daviftorres commented Mar 31, 2026

View reviewed changes

apache deleted a comment from blueorangutan May 27, 2026

weizhouapache added this to the 4.22.2 milestone Jun 29, 2026

daviftorres and others added 3 commits July 27, 2026 10:48

Refine regex pattern for domain path matching

ca01db4

Updated regex pattern to escape only the '/' character while noting potential risks with the wildcard '.' character.

Remove unused StringUtils import

66101d8

Replaced StringUtils from Apache Commons Lang with local one.

daviftorres force-pushed the domain-name-regex-characters-not-scaped branch from fc28651 to 66101d8 Compare July 27, 2026 14:48

Copilot AI review requested due to automatic review settings July 27, 2026 14:48

Copilot started reviewing on behalf of daviftorres July 27, 2026 15:01 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

DaanHoogland added this to CloudStack Testing Aug 31, 2026

github-project-automation Bot moved this to Backlog in CloudStack Testing Aug 31, 2026

DaanHoogland moved this from Backlog to Ready in CloudStack Testing Aug 31, 2026

DaanHoogland assigned RosiKyu Sep 1, 2026

Uh oh!

Conversation

daviftorres commented Mar 31, 2026

Description

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?

Uh oh!

Uh oh!

codecov Bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

daviftorres Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

daviftorres Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

davift Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

daviftorres May 27, 2026

Choose a reason for hiding this comment

Uh oh!

daviftorres May 27, 2026

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Mar 31, 2026

Uh oh!

daviftorres commented Mar 31, 2026

Uh oh!

DaanHoogland commented Apr 1, 2026

Uh oh!

sureshanaparti commented Apr 2, 2026

Uh oh!

DaanHoogland commented May 27, 2026

Uh oh!

daviftorres commented May 27, 2026

Uh oh!

blueorangutan commented May 27, 2026

Uh oh!

blueorangutan commented May 27, 2026

Uh oh!

blueorangutan commented May 27, 2026

Uh oh!

DaanHoogland commented May 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov Bot commented Mar 31, 2026 •

edited

Loading