fix: add missing encryption property to file metadata (#11648)#11783
fix: add missing encryption property to file metadata (#11648)#11783aakarsh12x wants to merge 6 commits intoappwrite:1.9.xfrom
Conversation
Greptile SummaryThis PR fixes a gap in the storage file upload flow where the Key changes:
Confidence Score: 5/5Safe to merge — the fix correctly aligns the stored metadata flag with the actual encryption logic and all previously raised concerns have been addressed or were already discussed in prior review threads. The No files require special attention beyond the concerns already captured in prior review threads (storage device metadata side-effect, migration encrypted-source semantics).
|
| Filename | Overview |
|---|---|
| src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Create.php | Core fix: adds $metadata['encryption'] flag before upload using the same condition as the actual encryption step; accurately reflects the file's encryption state in the DB document across all four write paths. |
| src/Appwrite/Platform/Modules/Functions/Workers/Screenshots.php | Adds 'encryption' => false to screenshot file metadata — correct since screenshot files are written directly with no OpenSSL fields. |
| src/Appwrite/Platform/Workers/Migrations.php | Adds 'encryption' => false to export-file metadata; accurate because the document is created with openSSLVersion/Cipher/Tag/IV => null, confirming no encryption is applied. |
| src/Appwrite/Platform/Modules/Proxy/Action.php | Independent fix: adds empty($functionsDomain) guard when iterating domain entries from _APP_DOMAIN_FUNCTIONS, preventing a crash on trailing-comma env values. |
| tests/unit/Platform/Modules/Proxy/ActionTest.php | New unit test covering the empty-entry guard in validateDomainRestrictions; test passes a trailing-comma env value and asserts no exception is thrown. |
Reviews (5): Last reviewed commit: "Merge branch 'appwrite:1.9.x' into fix/1..." | Re-trigger Greptile
| } | ||
| } | ||
|
|
||
| $metadata['encryption'] = $bucket->getAttribute('encryption', true) && $fileSize <= APP_STORAGE_READ_BUFFER; |
There was a problem hiding this comment.
$metadata is passed directly into $deviceForFiles->upload() on the very next line. This means 'encryption' is now included in the metadata forwarded to the storage device (S3, GCS, local, etc.). Depending on the device adapter, this extra key may be written as an object-metadata header on every chunk. While the value is correct for the DB document (and all four write paths below correctly use $metadata), the side-effect on storage device metadata was not present before this change.
If the device adapters ignore unknown metadata keys this is safe, but if they forward them verbatim to the object store (as is common for S3-compatible adapters) you may end up with unwanted object metadata on stored files. Worth verifying in the device implementations before shipping.
| 'openSSLTag' => null, | ||
| 'openSSLIV' => null, | ||
| 'search' => \implode(' ', [$fileId, $filename]), | ||
| 'metadata' => ['content_type' => $mime] | ||
| 'metadata' => [ | ||
| 'content_type' => $mime, | ||
| 'encryption' => false, | ||
| ] | ||
| ])); |
There was a problem hiding this comment.
encryption => false may misrepresent migrated file state
The migration worker writes file documents for files imported from another Appwrite instance. If the source project had bucket encryption enabled, those files are already encrypted at rest. Persisting 'encryption' => false in the metadata of a file whose bytes are actually encrypted would cause consumers that rely on this flag to incorrectly believe the file is plain text.
It would be safer to derive the flag from the destination bucket's configuration (mirroring the logic in Create.php), or — if the migration always re-uploads raw bytes — to add a comment explaining why false is always correct here.
|
|
||
| $this->addToAssertionCount(1); |
There was a problem hiding this comment.
expectNotToPerformAssertions() over addToAssertionCount(1)
$this->addToAssertionCount(1) is a low-level bookkeeping call that was not designed to be used directly in test bodies — it just increments the counter without actually asserting anything. The idiomatic PHPUnit way to express "this block must not throw" is:
| $this->addToAssertionCount(1); | |
| $this->expectNotToPerformAssertions(); | |
| $action->validate('test.functions.example.com', ['hostnames' => []]); |
Alternatively, $this->assertTrue(true) is widely understood by teams that don't yet target PHPUnit 10+.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
This adds the missing encryption property to persisted file metadata so the file document metadata stays aligned with whether the stored file content is encrypted.
References #11648.
What Was Missing
File uploads decide whether content is encrypted based on bucket configuration and file size, but that result was not being written back into the file's metadata payload.
That meant encrypted files could have metadata without an explicit encryption flag even though the file had been processed with encryption internally.
Root Cause
The upload flow stored content_type in metadata, but never persisted the resolved encryption state after the encryption decision was made.
Fix Applied
Consistency
After this change:
I was not able to run the PHP test suite in this shell because php is not installed in the current environment.