fix: sanitize branch names for valid domain generation #11245
Conversation
📝 WalkthroughWalkthroughReplaces manual VCS branch domain construction with a Transformation-based flow using a new BranchDomain adapter. Controllers and compute/functions modules now create a Transformation, pass inputs (branch, resourceId, projectId, sitesDomain), execute transform, and use the transformation output as the branch preview domain and ruleId source. Adds BranchDomain adapter (sanitization, prefix length, hash suffix) and unit tests validating domain normalization, consistency, and sensitivity to resource/project changes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/controllers/api/vcs.php`:
- Line 7: The PSR-12 ordered_imports error is caused by the Appwrite\Vcs\Domain
import being out of alphabetical order; update the use statements in api/vcs.php
so all Appwrite\SDK\* imports come first and then place use Appwrite\Vcs\Domain
after them, ensuring imports within the Appwrite namespace are sorted
alphabetically (and run the linter to confirm); reference the use statement for
Domain to locate and reorder it.
In `@src/Appwrite/Platform/Modules/Compute/Base.php`:
- Line 8: The PSR-12 ordered_imports lint is failing because the `use
Appwrite\Vcs\Domain;` import in src/Appwrite/Platform/Modules/Compute/Base.php
is out of alphabetical order; move the `use Appwrite\Vcs\Domain;` statement so
it appears after other `Appwrite\Platform\*` imports and before any `Utopia\*`
imports (i.e., reorder the `use` block around the `Base` class imports), then
re-run the linter to confirm the `ordered_imports` violation is resolved.
In `@src/Appwrite/Vcs/Domain.php`:
- Around line 41-52: In generateBranchPrefix, handle the case where
sanitizeBranchName(substr(...)) returns an empty string or yields a leading
hyphen: after computing $branchPrefix = self::sanitizeBranchName(...), if
$branchPrefix is empty then derive a safe base by stripping invalid chars from
the original substring (or fallback to a short fixed token like 'br') before
appending the hash; when adding the hash suffix (using self::HASH_SUFFIX_LENGTH)
only prepend the '-' separator if the base is non-empty, and finally trim any
leading '-' from the combined result and ensure it is not empty before
returning; reference generateBranchPrefix, sanitizeBranchName,
BRANCH_PREFIX_MAX_LENGTH, and HASH_SUFFIX_LENGTH when making the change.
🧹 Nitpick comments (1)
tests/unit/Vcs/DomainTest.php (1)
108-127: Consider adding a test for all-invalid prefix edge case.The test suite is comprehensive. Consider adding a test case for when the first 16 characters are all invalid (e.g.,
"////////////////something"). This would verify the behavior mentioned in my comment onDomain.php.Suggested test case
public function testGenerateBranchPrefixAllInvalidFirstChars(): void { // First 16 chars are all slashes, remainder has valid chars $prefix = Domain::generateBranchPrefix('////////////////validpart'); // Verify the prefix doesn't start with a hyphen (invalid DNS label) $this->assertDoesNotMatchRegularExpression('/^-/', $prefix); }
Branch names containing invalid domain characters (like '/') were being used directly when creating VCS preview domains, resulting in invalid domains like 'branch-abc/test.appwrite.network'. This adds a Domain helper class that sanitizes branch names by replacing invalid characters with hyphens before generating domains. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Appwrite/Vcs/Domain.php`:
- Around line 18-31: The sanitizeBranchName function currently allows
underscores; update it to forbid underscores by changing the regex in
sanitizeBranchName from '/[^a-zA-Z0-9_-]+/' to '/[^a-zA-Z0-9-]+/' so only
letters, digits, and hyphens are permitted, and update the docblock text to
reflect that underscores are not allowed (mentioning RFC 1035/1123 LDH rules and
CA/Browser requirements if desired).
|
|
||
| // VCS branch preview | ||
| if (!empty($providerBranch)) { | ||
| $branchPrefix = substr($providerBranch, 0, 16); |
There was a problem hiding this comment.
Note for future: We will need to apply same fix in Cloud vcs.php
Replace regex with explicit character validation using Utopia Text constants for better readability and maintainability. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Modules/Functions/Workers/Builds.php`:
- Around line 12-16: Reorder the import statements to satisfy PSR-12
ordered_imports: sort the Appwrite imports alphabetically by their fully
qualified names (e.g., Appwrite\Transformation\Transformation,
Appwrite\Transformation\Adapter\BranchDomain,
Appwrite\Utopia\Response\Model\Deployment, Appwrite\Vcs\Comment) and place the
global Exception import after a single blank line following the Appwrite group;
update the import block containing Transformation, BranchDomain, Deployment,
Comment, and Exception accordingly.
In `@src/Appwrite/Transformation/Adapter/BranchDomain.php`:
- Around line 78-103: sanitizeBranchName currently preserves uppercase because
allowedChars includes upper and lower; normalize the input to lowercase before
building allowedCharsFlip and iterating. In sanitizeBranchName(string $branch):
string, convert the branch string to lowercase (e.g. $branch =
strtolower($branch) or mb_strtolower for multibyte safety) immediately at the
start so the subsequent loop, allowedChars/allowedCharsFlip checks,
dash-collapsing logic and final trim(...) produce a lowercase sanitized branch
(e.g. 'feature/SER-1234' -> 'branch-feature-ser-1234-') and avoid case-variant
duplicates.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1056-1074:⚠️ Potential issue | 🟠 MajorBug:
deploymentResourceIdshould reference the site resource, not the deployment.Lines 1066-1067 incorrectly use
$deployment->getId()and$deployment->getSequence()fordeploymentResourceIdanddeploymentResourceInternalId. Based on the pattern inBase.php(lines 258-259, 284-285, 324-326) and the semantic meaning of "deploymentResource", these fields should reference the site being deployed, not the deployment itself.🐛 Proposed fix
'deploymentId' => $deployment->getId(), 'deploymentInternalId' => $deployment->getSequence(), 'deploymentResourceType' => 'site', - 'deploymentResourceId' => $deployment->getId(), - 'deploymentResourceInternalId' => $deployment->getSequence(), + 'deploymentResourceId' => $resource->getId(), + 'deploymentResourceInternalId' => $resource->getSequence(), 'deploymentVcsProviderBranch' => $branchName,
🧹 Nitpick comments (2)
src/Appwrite/Transformation/Adapter/BranchDomain.php (2)
20-26: Consider removing unused$traitsparameter or marking it intentionally unused.The static analysis tool correctly identifies that the
$traitsparameter inisValid()is never used since the method always returnstrue. If this is intentional for interface compliance with the parentAdapterclass, consider adding a@SuppressWarnings(PHPMD.UnusedFormalParameter)annotation or prefixing with underscore to indicate intentional non-use.♻️ Suggested fix
/** - * `@param` array<mixed> $traits + * `@param` array<mixed> $traits Unused, required by Adapter interface + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function isValid(array $traits): bool { return true; }
37-47: Consider validating required inputs to avoid malformed domains.When
branch,resourceId,projectId, orsitesDomainare empty, the generated domain could be malformed (e.g.,branch---abc1234.if sitesDomain is also empty). Consider adding validation or throwing an exception for missing required inputs.🛡️ Suggested validation
public function transform(): void { $branch = $this->input['branch'] ?? ''; $resourceId = $this->input['resourceId'] ?? ''; $projectId = $this->input['projectId'] ?? ''; $sitesDomain = $this->input['sitesDomain'] ?? ''; + if (empty($branch) || empty($resourceId) || empty($projectId) || empty($sitesDomain)) { + throw new \InvalidArgumentException('Missing required input: branch, resourceId, projectId, and sitesDomain are required'); + } + $branchPrefix = $this->generateBranchPrefix($branch);
| 'sitesDomain' => $sitesDomain, | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); |
There was a problem hiding this comment.
| ->transform(); | |
| $domain = "branch-{$branchPrefix}-{$resourceProjectHash}.{$sitesDomain}"; | |
| $transformation | |
| ->setInput($domain) | |
| ->setTraits([ maxLenght: 10, suffixLenght: 7 ]) | |
| ->transform(); |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where branch names containing invalid domain characters (like '/') were being used directly when creating VCS preview domains, resulting in invalid domains. The fix introduces a new BranchDomain transformation adapter class that sanitizes branch names by replacing invalid characters with hyphens before generating domains.
Changes:
- Created a new
BranchDomaintransformation adapter class that sanitizes branch names and generates valid domain names - Refactored three locations where branch domain generation was implemented inline to use the new adapter
- Added comprehensive unit tests covering various real-world branch naming patterns
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Appwrite/Transformation/Adapter/BranchDomain.php | New adapter class that sanitizes branch names and generates valid domains with proper character handling |
| tests/unit/Transformation/TransformationTest.php | Added comprehensive tests for the BranchDomain adapter covering various branch naming patterns |
| src/Appwrite/Platform/Modules/Functions/Workers/Builds.php | Refactored to use the new BranchDomain adapter instead of inline domain generation |
| src/Appwrite/Platform/Modules/Compute/Base.php | Refactored to use the new BranchDomain adapter instead of inline domain generation |
| app/controllers/api/vcs.php | Refactored to use the new BranchDomain adapter instead of inline domain generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isset($allowedCharsFlip[$char])) { | ||
| $sanitized .= $char; | ||
| } else { | ||
| // Prevents two -- or more in row |
There was a problem hiding this comment.
The comment has a typo: "two -- or more in row" should be "two -- or more in a row".
| // Prevents two -- or more in row | |
| // Prevents two -- or more in a row |
| public function transform(): void | ||
| { | ||
| $branch = $this->input['branch'] ?? ''; | ||
| $resourceId = $this->input['resourceId'] ?? ''; | ||
| $projectId = $this->input['projectId'] ?? ''; | ||
| $sitesDomain = $this->input['sitesDomain'] ?? ''; | ||
|
|
||
| $branchPrefix = $this->generateBranchPrefix($branch); | ||
| $resourceProjectHash = substr(hash('sha256', $resourceId . $projectId), 0, self::HASH_SUFFIX_LENGTH); | ||
| $this->output = strtolower("branch-{$branchPrefix}-{$resourceProjectHash}.{$sitesDomain}"); | ||
| } |
There was a problem hiding this comment.
The transform method doesn't validate that required input values are non-empty. If 'branch', 'resourceId', 'projectId', or 'sitesDomain' are empty strings, this will generate malformed domains. Consider adding validation to ensure these required fields are provided and non-empty, or at least handle the empty branch case to prevent invalid domain generation.
| private function generateBranchPrefix(string $branch): string | ||
| { | ||
| $branchPrefix = substr($branch, 0, self::BRANCH_PREFIX_MAX_LENGTH); | ||
| $branchPrefix = $this->sanitizeBranchName($branchPrefix); | ||
|
|
||
| if (strlen($branch) > self::BRANCH_PREFIX_MAX_LENGTH) { | ||
| $remainingChars = substr($branch, self::BRANCH_PREFIX_MAX_LENGTH); | ||
| $branchPrefix .= '-' . substr(hash('sha256', $remainingChars), 0, self::HASH_SUFFIX_LENGTH); | ||
| } | ||
|
|
||
| return $branchPrefix; |
There was a problem hiding this comment.
If the sanitized branch prefix is empty (which can happen when the first 16 characters of a branch name consist entirely of special characters), the generated domain will have consecutive hyphens (e.g., "branch--{hash}.{domain}"). While technically valid, this is not ideal. Consider either falling back to a default prefix (e.g., "branch") or ensuring at least one character remains after sanitization by checking if the result is empty and handling it appropriately.
| public function testBranchDomain(): void | ||
| { | ||
| $transformer = new Transformation([new BranchDomain()]); | ||
|
|
||
| // Branch name with slash | ||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'feature/test', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain = $transformer->getOutput(); | ||
| $this->assertStringNotContainsString('/', $domain); | ||
| $this->assertStringStartsWith('branch-feature-test-', $domain); | ||
| $this->assertStringEndsWith('.appwrite.network', $domain); | ||
|
|
||
| // Branch domain consistency | ||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'feature/test', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain2 = $transformer->getOutput(); | ||
| $this->assertEquals($domain, $domain2); | ||
|
|
||
| // Different resources should produce different domains | ||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'feature/test', | ||
| 'resourceId' => 'site789', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain2 = $transformer->getOutput(); | ||
| $this->assertNotEquals($domain, $domain2); | ||
|
|
||
| // Different projects should produce different domains | ||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'feature/test', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj789', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain2 = $transformer->getOutput(); | ||
| $this->assertNotEquals($domain, $domain2); | ||
|
|
||
| // Some real-world branch names | ||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'feature/SER-1234', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain = $transformer->getOutput(); | ||
| $this->assertStringStartsWith('branch-feature-ser-1234-', $domain); | ||
| $this->assertStringEndsWith('.appwrite.network', $domain); | ||
|
|
||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'bugfix/fix-login', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain = $transformer->getOutput(); | ||
| $this->assertStringStartsWith('branch-bugfix-fix-login-', $domain); | ||
| $this->assertStringEndsWith('.appwrite.network', $domain); | ||
|
|
||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'hotfix/v1.2.3', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain = $transformer->getOutput(); | ||
| $this->assertStringStartsWith('branch-hotfix-v1-2-3-', $domain); | ||
| $this->assertStringEndsWith('.appwrite.network', $domain); | ||
|
|
||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'release/2024.01', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain = $transformer->getOutput(); | ||
| $this->assertStringStartsWith('branch-release-2024-01-', $domain); | ||
| $this->assertStringEndsWith('.appwrite.network', $domain); | ||
|
|
||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'user/john/experiment', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain = $transformer->getOutput(); | ||
| $this->assertStringStartsWith('branch-user-john-experi-', $domain); | ||
| $this->assertStringEndsWith('.appwrite.network', $domain); | ||
|
|
||
| $transformer | ||
| ->setInput([ | ||
| 'branch' => 'dependabot/npm_and_yarn/lodash-4.17.21', | ||
| 'resourceId' => 'site123', | ||
| 'projectId' => 'proj456', | ||
| 'sitesDomain' => 'appwrite.network' | ||
| ]) | ||
| ->setTraits([]) | ||
| ->transform(); | ||
| $domain = $transformer->getOutput(); | ||
| $this->assertStringStartsWith('branch-dependabot-npm-a-', $domain); | ||
| $this->assertStringEndsWith('.appwrite.network', $domain); | ||
| } |
There was a problem hiding this comment.
The test suite should include edge cases such as: empty branch names, branch names consisting entirely of special characters (e.g., "///"), branch names starting with special characters, very long branch names exceeding typical limits, and branch names with consecutive special characters. These cases would help ensure the sanitization logic handles all scenarios correctly.
|
|
||
| $branchPrefix = $this->generateBranchPrefix($branch); | ||
| $resourceProjectHash = substr(hash('sha256', $resourceId . $projectId), 0, self::HASH_SUFFIX_LENGTH); | ||
| $this->output = strtolower("branch-{$branchPrefix}-{$resourceProjectHash}.{$sitesDomain}"); |
There was a problem hiding this comment.
The new implementation converts the entire domain to lowercase, while the old implementation didn't explicitly do this. While this is good practice for domain names and DNS is case-insensitive, for existing deployments with branch names containing uppercase letters (without special characters), this will generate a different rule ID and create a new rule instead of updating the existing one. Consider documenting this behavior change in release notes or migration guide.
Branch names containing invalid domain characters (like '/') were being used directly when creating VCS preview domains, resulting in invalid domains like 'branch-abc/test.appwrite.network'. This adds a Domain helper class that sanitizes branch names by replacing invalid characters with hyphens before generating domains.
Closes SER-1102