feat: improved reference client ip through _APP_TRUSTED_HEADERS#10941
feat: improved reference client ip through _APP_TRUSTED_HEADERS#10941levivannoort merged 8 commits into1.8.xfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds a new trusted-headers configuration option: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-07-08T01:20:14.364ZApplied to files:
🪛 dotenv-linter (4.0.0).env[warning] 128-128: [EndingBlankLine] No blank line at the end of the file (EndingBlankLine) [warning] 128-128: [UnorderedKey] The _APP_TRUSTED_HEADERS key should go before the _APP_USAGE_AGGREGATION_INTERVAL key (UnorderedKey) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
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 |
levivannoort
changed the title
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docker-compose.yml (1)
223-223: Wiring of_APP_TRUSTED_HEADERSinto the API container looks goodExposing
_APP_TRUSTED_HEADERSon the mainappwriteservice matches whereAppwrite\Utopia\Requestis typically used. If other HTTP-facing services (for exampleappwrite-realtime) also rely on the same Request wrapper and should honor the same trusted headers, consider adding this env there as well for consistency.tests/e2e/Services/Account/AccountBase.php (1)
330-365: Good end‑to‑end coverage of the trusted IP fallback, with a couple of optional refinementsThis test does a good job validating that
x-forwarded-foris honored as the client IP when talking directly to the API container.Two optional tweaks you might consider:
- Also assert the IP on the
/accountcreation response if that endpoint surfaces aclientIp/ipfield, to cover both write and auth flows.- Use one of the RFC 5737 documentation ranges (for example
203.0.113.x) instead of191.0.113.195to make it clear the address is non‑production.Both are non‑blocking; the current test meaningfully exercises the new behavior.
app/config/variables.php (1)
361-368: Config entry for_APP_TRUSTED_HEADERSis clear and aligned with implementationThe new variable definition matches the runtime behavior (default
'x-forwarded-for', comma‑separated list, left‑to‑right evaluation) and gives operators enough context to configure multiple headers.If you end up distinguishing between “unset” (use default) and “empty string” (disable trusted headers), it might be worth adding a brief note to the description later; otherwise this looks good as‑is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.env(1 hunks)app/config/variables.php(1 hunks)docker-compose.yml(1 hunks)src/Appwrite/Utopia/Request.php(2 hunks)tests/e2e/Services/Account/AccountBase.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-08T01:20:14.364Z
Learnt from: stnguyen90
Repo: appwrite/appwrite PR: 10119
File: app/controllers/api/account.php:1226-1232
Timestamp: 2025-07-08T01:20:14.364Z
Learning: In Appwrite, `_APP_DOMAIN` is a required environment variable that must always be set for the system to function properly.
Applied to files:
.env
🧬 Code graph analysis (1)
tests/e2e/Services/Account/AccountBase.php (1)
tests/e2e/Client.php (2)
setEndpoint(140-145)Client(8-342)
🪛 dotenv-linter (4.0.0)
.env
[warning] 128-128: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 128-128: [UnorderedKey] The _APP_TRUSTED_HEADERS key should go before the _APP_USAGE_AGGREGATION_INTERVAL key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Utopia/Request.php (1)
12-27: No changes needed. The current implementation is correct and robust.The parent class
Utopia\Swoole\Request::__construct()does not initialize or overwrite thetrustedIpHeadersproperty—it only sets$this->swoole. The call order in your code (setting trusted headers beforeparent::__construct()) is safe and works as intended.Additionally, the
setTrustedIpHeaders()method already normalizes headers to lowercase, trims whitespace, and filters empty values viaarray_map('strtolower', ...),array_map('trim', ...), andarray_filter(). The comma-separated env var parsing is handled correctly without needing extra trimming in the constructor.
2 participants
What does this PR do?
As not to default on the x-forwarded-for header, this pull-request introduces the option to set trusted headers. Which are checked from left to right on request presence wise, where the left most address will be used for as the client reference. With a default set to x-forwarded-for.
To-do:
Test Plan
Add a test that should validate from not diverting from the original behaviour, while allowing the overwrite of the to be trusted headers through the variable.
Related PRs and Issues
Checklist