Skip to content

Create rule in SSL task#11249

Open
hmacr wants to merge 3 commits into1.8.xfrom
ser-1092-v2
Open

Create rule in SSL task#11249
hmacr wants to merge 3 commits into1.8.xfrom
ser-1092-v2

Conversation

@hmacr
Copy link
Contributor

@hmacr hmacr commented Feb 4, 2026

Closes SER-1092

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

This pull request modifies SSL certificate handling and updates the deployment environment configuration. The SSL task action method is expanded with dependency injection for Document $console and Database $dbForPlatform parameters. Domain validation logic is introduced to reject unknown or test domains early. Rule retrieval and creation logic now considers the _APP_RULES_FORMAT environment variable, using MD5-hashed domains as rule keys when applicable. New rule documents are created with domain, type, status, project identifiers, owner (detected from environment variables for Appwrite-owned domains), search, and region fields. Existing rules are updated with status changes. Comprehensive logging is added throughout rule creation, updates, and certificate scheduling. Environment configuration transitions from development/test setup to production with DNS server and domain changes. Docker-compose infrastructure is simplified by removing CoreDNS service and related network IPAM subnet configuration.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~28 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Create rule in SSL task' directly matches the main change: creating SSL rules in the SSL.php task with new domain validation and rule creation logic.
Description check ✅ Passed The description 'Closes SER-1092' is a valid reference to the related issue/ticket and is relevant to the changeset's purpose.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ser-1092-v2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
libcrypto3 3.5.4-r0 CVE-2025-15467 CRITICAL
libcrypto3 3.5.4-r0 CVE-2025-69419 HIGH
libcrypto3 3.5.4-r0 CVE-2025-69421 HIGH
libexpat 2.7.3-r0 CVE-2026-24515 CRITICAL
libpng 1.6.51-r0 CVE-2025-66293 HIGH
libpng 1.6.51-r0 CVE-2026-22695 HIGH
libpng 1.6.51-r0 CVE-2026-22801 HIGH
libpng-dev 1.6.51-r0 CVE-2025-66293 HIGH
libpng-dev 1.6.51-r0 CVE-2026-22695 HIGH
libpng-dev 1.6.51-r0 CVE-2026-22801 HIGH
libssl3 3.5.4-r0 CVE-2025-15467 CRITICAL
libssl3 3.5.4-r0 CVE-2025-69419 HIGH
libssl3 3.5.4-r0 CVE-2025-69421 HIGH
libxml2 2.13.8-r0 CVE-2025-49794 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49796 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49795 HIGH
libxml2 2.13.8-r0 CVE-2025-6021 HIGH
openssl 3.5.4-r0 CVE-2025-15467 CRITICAL
openssl 3.5.4-r0 CVE-2025-69419 HIGH
openssl 3.5.4-r0 CVE-2025-69421 HIGH
openssl-dev 3.5.4-r0 CVE-2025-15467 CRITICAL
openssl-dev 3.5.4-r0 CVE-2025-69419 HIGH
openssl-dev 3.5.4-r0 CVE-2025-69421 HIGH
py3-urllib3 1.26.20-r0 CVE-2026-21441 HIGH
py3-urllib3-pyc 1.26.20-r0 CVE-2026-21441 HIGH
github.com/containerd/containerd/v2 v2.0.2 CVE-2024-25621 HIGH
golang.org/x/crypto v0.31.0 CVE-2025-22869 HIGH
golang.org/x/oauth2 v0.24.0 CVE-2025-22868 HIGH
stdlib 1.22.10 CVE-2025-47907 HIGH
stdlib 1.22.10 CVE-2025-58183 HIGH
stdlib 1.22.10 CVE-2025-61726 HIGH
stdlib 1.22.10 CVE-2025-61728 HIGH
stdlib 1.22.10 CVE-2025-61729 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Tasks/SSL.php`:
- Around line 76-81: The current ownership check in the foreach loop uses
str_ends_with($domain->get(), $appwriteDomain) which yields false positives
(e.g. "evilappwrite.io"); change the condition in the loop to mark owner =
'Appwrite' only when $domain->get() equals $appwriteDomain exactly or when the
matched suffix is preceded by a dot (i.e. the char before the suffix is '.'),
keeping the rest of the foreach and the $owner assignment intact; update the
condition around $appwriteDomain and $domain->get() accordingly so subdomain
matches are valid but arbitrary suffix matches are not.
- Around line 98-103: The queued domain Document in SSL.php is missing the
domainType attribute; update the $queueForCertificates->setDomain(...) call to
include 'domainType' => $rule->getAttribute('type') alongside 'domain' (so the
new Document contains both keys), leaving the subsequent
->setSkipRenewCheck($skipCheck)->trigger() chain unchanged; locate the code
around the setDomain invocation on $queueForCertificates and add the domainType
entry sourced from $rule->getAttribute('type').

@github-actions
Copy link

github-actions bot commented Feb 4, 2026

✨ Benchmark results

  • Requests per second: 2,411
  • Requests with 200 status code: 434,118
  • P99 latency: 0.064912898

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 2,411 1,136
200 434,118 204,480
P99 0.064912898 0.180213275

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.env (1)

1-30: ⚠️ Potential issue | 🟠 Major

Production env with localhost domains will likely break routing and console access.

With _APP_ENV=production, keeping _APP_CONSOLE_DOMAIN, _APP_DOMAIN_FUNCTIONS, and _APP_DOMAIN_SITES as localhost is inconsistent and typically unusable in a real deployment. Please align these hostnames with the new _APP_DOMAIN (or document why localhost is intended in prod) to avoid broken console access, callbacks, and SSL rule handling.

🤖 Fix all issues with AI agents
In @.env:
- Line 26: Move the _APP_DOMAIN entry so dotenv-linter ordering is correct:
locate the environment keys named _APP_DOMAIN and _APP_EDITION and reorder them
so _APP_DOMAIN appears immediately before _APP_EDITION (i.e., place the line
starting with _APP_DOMAIN above the line starting with _APP_EDITION), then run
dotenv-linter to verify the warning is resolved.

.env Outdated
_APP_OPENSSL_KEY_V1=your-secret-key
_APP_DNS=172.16.238.100 # CoreDNS
_APP_DOMAIN=appwrite.test
_APP_DOMAIN=do-droplet.hmacr.space
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix dotenv-linter key ordering for _APP_DOMAIN.

dotenv-linter flags _APP_DOMAIN as out of order; it should appear before _APP_EDITION.

♻️ Proposed reorder
-_APP_ENV=production
-_APP_EDITION=self-hosted
+_APP_ENV=production
+_APP_DOMAIN=do-droplet.hmacr.space
+_APP_EDITION=self-hosted
@@
-_APP_DOMAIN=do-droplet.hmacr.space
🧰 Tools
🪛 dotenv-linter (4.0.0)

[warning] 26-26: [UnorderedKey] The _APP_DOMAIN key should go before the _APP_EDITION key

(UnorderedKey)

🤖 Prompt for AI Agents
In @.env at line 26, Move the _APP_DOMAIN entry so dotenv-linter ordering is
correct: locate the environment keys named _APP_DOMAIN and _APP_EDITION and
reorder them so _APP_DOMAIN appears immediately before _APP_EDITION (i.e., place
the line starting with _APP_DOMAIN above the line starting with _APP_EDITION),
then run dotenv-linter to verify the warning is resolved.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @.env:
- Line 1: Remove the committed production flag from the shared .env by deleting
or changing the _APP_ENV=production entry and instead add guidance to use
environment-specific files or deployment secrets; create or update an ignored
.env.production for actual production values (and ensure it is gitignored) and
provide a safe example value in a committed .env.example (e.g.,
_APP_ENV=development) so developers have defaults without shipping production
settings.
- Around line 25-26: Remove deployment-specific values from the tracked .env by
replacing the hardcoded _APP_DNS and _APP_DOMAIN with non-sensitive placeholders
or local defaults: update the _APP_DOMAIN entry (currently _APP_DOMAIN) to a
generic/example domain (e.g., example.local or appwrite.test) and change
_APP_DNS (currently _APP_DNS) to a local/default DNS (e.g., 127.0.0.1) or a
clearly marked placeholder; ensure any real deployment values are moved to env
files excluded from version control or injected by deployment tooling.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Tasks/SSL.php`:
- Around line 44-45: The $skipCheck normalization currently coerces boolean true
into a string and flips it; change the logic in the SSL task where $skipCheck is
normalized so that if $skipCheck is already a boolean it is left unchanged,
otherwise normalize string values case-insensitively (accepting 'true','1','yes'
as true) and set $skipCheck accordingly; update the code that assigns $skipCheck
(the variable named $skipCheck in this file) to perform an is_bool($skipCheck)
check first and only apply string-to-boolean parsing when it is not boolean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant