Feat: Add executionId and client IP to function headers #9147
Conversation
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
|
Unsure how to proceed with the following lines: appwrite/app/controllers/general.php Lines 264 to 272 in 0ab86f4 appwrite/app/controllers/api/functions.php Lines 2019 to 2027 in 0ab86f4 Should they be added to v2 runtimes as well? |
V2 is backwards compatibility for older functions syntax. It's for when functions used to get No need to implement any of the new features into |
| "APPWRITE_FUNCTION_JWT": req.variables["APPWRITE_FUNCTION_JWT"], | ||
| "APPWRITE_FUNCTION_PROJECT_ID": req.variables["APPWRITE_FUNCTION_PROJECT_ID"], | ||
| "APPWRITE_FUNCTION_EXECUTION_ID": req.variables["APPWRITE_FUNCTION_EXECUTION_ID"], | ||
| "APPWRITE_FUNCTION_CLIENT_IP": req.variables["APPWRITE_FUNCTION_CLIENT_IP"], |
There was a problem hiding this comment.
Amazing job updating tests too 🔥
Let's also update test themselves in FunctionsCustomClientTest.php and FunctionsCustomServerTest.php. You can look for tests for APPWRITE_FUNCTION_USER_ID, and in those places, add test for APPWRITE_FUNCTION_EXECUTION_ID and APPWRITE_FUNCTION_CLIENT_IP.
There was a problem hiding this comment.
I added tests, eventho rather basic ones. If you need them to be more extensive just give me a heads up.
|
Can we consider adding too? |
|
Cannot wait for this to be implemented |
|
Any movement on this? I need a way to get the execution id from within my function call. |
1948f27 to
0243517
Compare
|
Warning Rate limit exceeded@vermakhushboo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates multiple areas: introduces execution ID propagation and headers in general.php; adds request format V2.0 injection and warning aggregation; updates constants to 1.8.0 and allowlists x-appwrite-client-ip; adds TablesDB service, roles/scopes for tables/rows, and event/error mappings for tables/rows/columns; adds a databases.type attribute; adds invalidateSessions option and APIs with session invalidation in account/users; extends migrations with internalFile and platform DB; adjusts storage permission handling; updates workflows, Docker images, SDK versions, and README tags; refactors resource resolution and shared API scope logic; removes a worker resource; bumps dependencies. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
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/Http/Executions/Create.php (1)
248-255: Ensure the request header allowlist includes execution and client IP headersThe constant FUNCTION_ALLOWLIST_HEADERS_REQUEST (defined in app/init/constants.php at line 149) currently only whitelists the following headers:
const FUNCTION_ALLOWLIST_HEADERS_REQUEST = [ 'content-type', 'agent', 'content-length', 'host' ];You need to add both
x-appwrite-execution-idandx-appwrite-client-ipso that these headers aren’t dropped when persisting the execution’s requestHeaders. For example:app/init/constants.php @@ -147,5 +147,8 @@ // Function headers -const FUNCTION_ALLOWLIST_HEADERS_REQUEST = ['content-type', 'agent', 'content-length', 'host']; +const FUNCTION_ALLOWLIST_HEADERS_REQUEST = [ + 'content-type', + 'agent', + 'content-length', + 'host', + 'x-appwrite-execution-id', + 'x-appwrite-client-ip', +]; const FUNCTION_ALLOWLIST_HEADERS_RESPONSE = ['content-type', 'content-length'];– Without these additions, observability tools cannot read the execution ID or client IP from the stored execution document.
♻️ Duplicate comments (2)
app/controllers/general.php (1)
359-365: Naming settled; “client” namespace aligns with earlier discussion.
- Using x-appwrite-client-ip is consistent with the prior decision to use “client” for internet clients and to avoid implying authenticated users. No further action needed.
src/Appwrite/Platform/Workers/Functions.php (1)
402-412: Don’t clobber upstream client IP; optionally adopt header executionId when param is missing
- If controllers already populated
x-appwrite-client-ip, this unconditionally overwrites it with an empty string, losing information. Set it only when it’s absent.- For event/schedule paths where
$executionIdmay be null/empty, consider adopting an upstreamx-appwrite-execution-idheader if present to preserve single-source execution ID across the pipeline.- Header names LGTM and align with prior discussion.
Apply:
- $headers['x-appwrite-execution-id'] = $executionId ?? ''; + // Prefer provided executionId; if missing, adopt one from headers (e.g., controllers) + if (empty($executionId) && !empty($headers['x-appwrite-execution-id'])) { + $executionId = $headers['x-appwrite-execution-id']; + } + $headers['x-appwrite-execution-id'] = $executionId ?? ''; $headers['x-appwrite-key'] = API_KEY_DYNAMIC . '_' . $apiKey; $headers['x-appwrite-trigger'] = $trigger; $headers['x-appwrite-event'] = $event ?? ''; $headers['x-appwrite-user-id'] = $user->getId() ?? ''; $headers['x-appwrite-user-jwt'] = $jwt ?? ''; $headers['x-appwrite-country-code'] = ''; $headers['x-appwrite-continent-code'] = ''; $headers['x-appwrite-continent-eu'] = 'false'; - $headers['x-appwrite-client-ip'] = ''; + // Do not overwrite IP if controller already set it; default to empty string otherwise + $headers['x-appwrite-client-ip'] = $headers['x-appwrite-client-ip'] ?? ($headers['x-forwarded-for'] ?? '');
🧹 Nitpick comments (3)
app/controllers/general.php (1)
377-388: Prefer robust client-IP resolution for geolocation.
- You currently geolocate by x-real-ip only. In environments where that header isn’t present, you’ll miss geodata while still sending x-appwrite-client-ip from Request::getIP().
- Optional: fall back to $request->getIP() (or first IP from X-Forwarded-For) when x-real-ip is empty, so geodata remains consistent with the client IP you expose to functions.
Example (outside the changed range, for illustration only):
$ip = $headers['x-real-ip'] ?? ''; if (empty($ip)) { $xff = $headers['x-forwarded-for'] ?? ''; $ip = $xff ? \trim(\explode(',', $xff)[0]) : $request->getIP(); }src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
224-226: Add test coverage for header override protection.
- We already test non-override for several headers, but not for x-appwrite-execution-id. Consider extending tests to assert that user-supplied x-appwrite-execution-id is ignored/overridden.
Proposed addition (tests/e2e/Services/Functions/FunctionsCustomClientTest.php, method testNonOverrideOfHeaders):
$execution = $this->client->call(Client::METHOD_POST, '/functions/' . $functionId . '/executions', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'x-appwrite-event' => "OVERRIDDEN", 'x-appwrite-trigger' => "OVERRIDDEN", 'x-appwrite-user-id' => "OVERRIDDEN", 'x-appwrite-user-jwt' => "OVERRIDDEN", + 'x-appwrite-execution-id' => "OVERRIDDEN", + 'x-appwrite-client-ip' => "255.255.255.255", ]); $output = json_decode($execution['body']['responseBody'], true); $this->assertNotEquals('OVERRIDDEN', $output['APPWRITE_FUNCTION_JWT']); $this->assertNotEquals('OVERRIDDEN', $output['APPWRITE_FUNCTION_EVENT']); $this->assertNotEquals('OVERRIDDEN', $output['APPWRITE_FUNCTION_TRIGGER']); $this->assertNotEquals('OVERRIDDEN', $output['APPWRITE_FUNCTION_USER_ID']); + $this->assertNotEquals('OVERRIDDEN', $output['APPWRITE_FUNCTION_EXECUTION_ID']); + $this->assertNotEquals('255.255.255.255', $output['APPWRITE_FUNCTION_CLIENT_IP']);tests/e2e/Services/Functions/FunctionsCustomClientTest.php (1)
162-166: Great assertions for execution ID and client IP.
- Verifying APPWRITE_FUNCTION_EXECUTION_ID matches the execution document ID and that APPWRITE_FUNCTION_CLIENT_IP is non-empty gives strong end-to-end coverage.
Optional: Also assert the ID format (e.g., not empty and matches your ID charset/length) to catch regressions in ID generation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/controllers/general.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Workers/Functions.php(3 hunks)tests/e2e/Services/Functions/FunctionsCustomClientTest.php(1 hunks)tests/e2e/Services/Functions/FunctionsCustomServerTest.php(1 hunks)tests/resources/functions/basic/index.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
src/Appwrite/Event/Audit.php (1)
getIP(113-116)
app/controllers/general.php (1)
src/Appwrite/Event/Audit.php (1)
getIP(113-116)
tests/e2e/Services/Functions/FunctionsCustomClientTest.php (1)
src/Appwrite/Utopia/Response.php (1)
output(658-740)
🔇 Additional comments (5)
app/controllers/general.php (1)
359-361: Single-source executionId propagation looks correct.
- Generating the executionId once and reusing it for both the header and the execution document mitigates correlation bugs and simplifies tracing. Good call.
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
224-226: Good: single executionId + client IP propagated into function context.
- Creating executionId once and overriding any inbound x-appwrite-execution-id prevents spoofing. Setting x-appwrite-client-ip from Request::getIP() is appropriate.
Also applies to: 233-234
tests/resources/functions/basic/index.js (1)
44-46: Tests helper now surfaces execution and client IP — LGTM.
- Pulling x-appwrite-execution-id and x-appwrite-client-ip from headers and echoing them in the JSON response is aligned with the new behavior.
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (1)
1337-1340: Server-side E2E parity — looks good.
- Mirroring the client test here reduces the chance of environment-specific regressions. Nice.
src/Appwrite/Platform/Workers/Functions.php (1)
416-418: LGTM: single-source execution ID when creating a new executionGenerating a new
$executionIdonly when the document is missing and immediately reflecting it into the header keeps the ID consistent between the header and the persisted Execution.
app/controllers/general.php
Outdated
| $headers['x-appwrite-country-code'] = ''; | ||
| $headers['x-appwrite-continent-code'] = ''; | ||
| $headers['x-appwrite-continent-eu'] = 'false'; | ||
| $headers['x-appwrite-client-ip'] = $request->getIP(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure header allowlist includes new headers.
- You set x-appwrite-client-ip here. Confirm FUNCTION_ALLOWLIST_HEADERS_REQUEST contains both x-appwrite-execution-id and x-appwrite-client-ip; otherwise they’ll be filtered out of requestHeaders (even though the executor still gets $headers), and any tooling that relies on persisted headers may miss them.
Run this to verify:
🏁 Script executed:
#!/bin/bash
# Verify these headers are allowlisted (request side) and not treated as user-overrideable.
rg -nP '(?i)FUNCTION_ALLOWLIST_HEADERS_REQUEST|FUNCTION_ALLOWLIST_HEADERS_RESPONSE' -A3 -B3
rg -nP '(?i)x-appwrite-execution-id|x-appwrite-client-ip'
# Also confirm validator accepts/ignores them as expected.
rg -nP 'class\s+Headers\b|allow(ed)?Headers' --type=php -C3Length of output: 7644
Add new Appwrite headers to the request allowlist
The new headers x-appwrite-execution-id and x-appwrite-client-ip are being set in app/controllers/general.php (and in several Functions runners) but are not present in the request‐side allowlist. As a result, they will be stripped out by the header filter and never make it into requestHeaders.
Please update your constants as follows:
• File: app/init/constants.php
- const FUNCTION_ALLOWLIST_HEADERS_REQUEST = ['content-type', 'agent', 'content-length', 'host'];
+ const FUNCTION_ALLOWLIST_HEADERS_REQUEST = [
+ 'content-type',
+ 'agent',
+ 'content-length',
+ 'host',
+ 'x-appwrite-execution-id',
+ 'x-appwrite-client-ip',
+ ];Once added, the filter in app/controllers/general.php (around line 392) and in the Functions runners will correctly propagate both headers into requestHeaders so that any tooling or downstream logic relying on them continues to work.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $headers['x-appwrite-client-ip'] = $request->getIP(); | |
| ++ b/app/init/constants.php | |
| @@ -1,5 +1,8 @@ | |
| const FUNCTION_ALLOWLIST_HEADERS_REQUEST = [ | |
| 'content-type', | |
| 'agent', | |
| 'content-length', | |
| 'host', | |
| 'x-appwrite-execution-id', | |
| 'x-appwrite-client-ip', | |
| ]; |
🤖 Prompt for AI Agents
In app/controllers/general.php around line 365 and the header filter near line
392, the new headers x-appwrite-execution-id and x-appwrite-client-ip are being
set but are not in the request-side allowlist, so they get stripped before
reaching requestHeaders; to fix this, add "x-appwrite-execution-id" and
"x-appwrite-client-ip" to the allowed headers constant array in
app/init/constants.php, then verify the header-filter logic in
app/controllers/general.php and the Functions runner files references that
constant so the filter will propagate these two headers into requestHeaders.
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php
Outdated
Show resolved
Hide resolved
af3d585 to
2876fad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Appwrite/Platform/Workers/Functions.php (2)
401-410: Don’t overwrite existing headers; set defaults only when missing (case-insensitive)If upstream already set x-appwrite-execution-id or x-appwrite-client-ip, current logic can clobber values (notably replacing a valid execution id with an empty string). Respect existing headers; only set empty defaults when missing. This aligns with maintainer guidance.
- $headers['x-appwrite-execution-id'] = $executionId ?? ''; + // Normalize for case-insensitive existence checks + $headerKeysLower = array_change_key_case($headers, CASE_LOWER); + if (!empty($executionId)) { + $headers['x-appwrite-execution-id'] = $executionId; + } elseif (!array_key_exists('x-appwrite-execution-id', $headerKeysLower)) { + // Ensure header key is always present for typed SDKs + $headers['x-appwrite-execution-id'] = ''; + } + if (!array_key_exists('x-appwrite-client-ip', $headerKeysLower)) { + // Preserve incoming value if present; otherwise default to empty string + $headers['x-appwrite-client-ip'] = ''; + }
267-269: Initialize $headers; avoid undefined-variable notice and set client IP default onceAssigning to $headers[...] before initialization can emit notices. Also, set x-appwrite-client-ip explicitly to an empty string here for consistency with strictly typed SDKs (as discussed), since no incoming headers exist in fail().
- $executionId = ID::unique(); - $headers['x-appwrite-execution-id'] = $executionId ?? ''; + $executionId = ID::unique(); + $headers = []; + $headers['x-appwrite-execution-id'] = $executionId; + // Emit header even when unknown to allow safe casting downstream + $headers['x-appwrite-client-ip'] = '';
🧹 Nitpick comments (1)
tests/e2e/Services/FunctionsSchedule/FunctionsScheduleTest.php (1)
65-67: Good coverage; consider asserting header presence (optional)The new checks validate that scheduled executions have an ID and no client IP. To further lock behavior once worker always emits the header (even when empty), optionally assert the key exists as well:
- $this->assertEmpty($headers['x-appwrite-client-ip'] ?? ''); + $this->assertArrayHasKey('x-appwrite-client-ip', $headers); + $this->assertEmpty($headers['x-appwrite-client-ip'] ?? '');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/init/constants.php(1 hunks)src/Appwrite/Platform/Workers/Functions.php(3 hunks)tests/e2e/Services/Functions/FunctionsCustomServerTest.php(2 hunks)tests/e2e/Services/FunctionsSchedule/FunctionsScheduleTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Services/Functions/FunctionsCustomServerTest.php
🔇 Additional comments (1)
src/Appwrite/Platform/Workers/Functions.php (1)
414-416: LGTM: single generation path ensures consistencyGenerating the executionId once when creating a new execution and reflecting it in headers avoids divergence between stored $id and the header value. This matches the PR’s objective.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
233-235: Use of $request->getIP() is correct; normalize value to a stringSwitching from raw headers to
$request->getIP()is the right move. To avoidnullsneaking into headers (and to keep types consistent), normalize to an empty string.Apply:
- $ip = $request->getIP(); - $headers['x-appwrite-client-ip'] = $ip; + $ip = (string)($request->getIP() ?? ''); + $headers['x-appwrite-client-ip'] = $ip;Two quick follow-ups:
- Ensure no other code paths still use
x-real-ipdirectly.- Confirm that non-client-triggered runs (scheduled, event-triggered) don’t propagate a client IP.
You can check with:
#!/bin/bash # Find residual direct uses of x-real-ip rg -nP -C3 'x-real-ip' --hidden # Where is x-appwrite-client-ip set or modified? rg -nP -C3 'x-appwrite-client-ip' src --hiddenIf scheduled flows still carry a client IP from the scheduling request, consider scrubbing before enqueueing:
@@ } else { $data = [ - 'headers' => $headers, + // Do not propagate client IP to scheduled executions + 'headers' => (function($h) { unset($h['x-appwrite-client-ip']); return $h; })($headers), 'path' => $path, 'method' => $method, 'body' => $body, 'userId' => $user->getId() ];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/controllers/general.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/general.php
⏰ 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/Platform/Modules/Functions/Http/Executions/Create.php (1)
255-255: Single source of truth for executionId usage — LGTMAssuming this hunk reflects removal of a later re-generation, keeping
$executionIdas the only ID used across headers and the execution document looks good.
commit: |
What does this PR do?
This PR adds the executionId and the client IP to the function headers so that they are accessible within the function execution.
Fixes #9316
Test Plan
Manual testing (see screenshots)
Related PRs and Issues
Checklist