Allow to disable import from server from server side #3694
Conversation
📝 WalkthroughWalkthroughAdds an environment flag and feature config to disable server imports, migrates several owner checks to policy-based Gate checks, updates AlbumPolicy to require the configured owner and feature flag, and removes direct Auth/Configs usage in affected requests and resources. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![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 (2)
.env.example (1)
242-242: Add trailing newline and keep a short hint about security posture.
- Fix dotenv-linter warning by ending the file with a newline.
- Optional: add a comment line above to state “set to true to hard-disable server imports.”
-# DISABLE_IMPORT_FROM_SERVER=false +# DISABLE_IMPORT_FROM_SERVER=false +config/features.php (1)
118-131: Fail closed by default (secure-by-default).Current default allows imports when the env var is unset. Consider defaulting to disabled to reduce attack surface; call this out as a potential BC change in the changelog.
- 'disable-import-from-server' => (bool) env('DISABLE_IMPORT_FROM_SERVER', false), + // Default to true (disabled) if env is not set; improves security by default. + 'disable-import-from-server' => (bool) env('DISABLE_IMPORT_FROM_SERVER', true),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)app/Http/Resources/Rights/AlbumRightsResource.php(1 hunks)config/features.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: New PHP files must include the project license header and have a single blank line after the opening <?php tag
Use snake_case for PHP variable names
Apply PSR-4 coding standard to PHP code
Always call in_array with true as the third parameter for strict checking (in_array($needle, $haystack, true))
Only use boolean expressions in if statements; avoid integers or strings as conditions
Use strict comparison (===, !==) instead of loose comparison (==, !=)
Avoid duplicating code in both if and else branches
Do not use empty(); prefer explicit checks (e.g., === null, === '', count(...) === 0)
Files:
app/Http/Resources/Rights/AlbumRightsResource.phpconfig/features.php
app/Http/Resources/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use Laravel JsonResource for API resources; replace with Spatie Data objects
Files:
app/Http/Resources/Rights/AlbumRightsResource.php
🧠 Learnings (1)
📚 Learning: 2025-08-14T11:05:31.237Z
Learnt from: ildyria
PR: LycheeOrg/Lychee#3616
File: app/Http/Requests/Tags/ListTagRequest.php:21-24
Timestamp: 2025-08-14T11:05:31.237Z
Learning: Laravel's Gate::check() system is flexible with policy method signatures. When calling Gate::check(PolicyConstant, [Model::class]) with a policy method that only accepts (User $user), Laravel does not throw "too many arguments" errors - it handles argument mismatches gracefully by only passing the arguments the method can accept. This pattern is used extensively in the Lychee codebase.
Applied to files:
app/Http/Resources/Rights/AlbumRightsResource.php
🧬 Code graph analysis (1)
app/Http/Resources/Rights/AlbumRightsResource.php (2)
app/Services/Auth/SessionOrTokenGuard.php (1)
id(227-230)app/Models/Configs.php (2)
Configs(65-347)getValueAsInt(248-251)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 242-242: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
- GitHub Check: 2️⃣ PHP tests / 8.4 - postgresql -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - sqlite -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - mariadb -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit,Feature_v2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Http/Resources/Rights/RootAlbumRightsResource.php (1)
32-32: Consistency nit: align Gate call argument shape with neighborsFor symmetry with adjacent checks (which pass [AbstractAlbum::class, null]), consider passing a second null. No functional change; just consistency.
- $this->can_import_from_server = Gate::check(AlbumPolicy::CAN_IMPORT_FROM_SERVER, [AbstractAlbum::class]); + $this->can_import_from_server = Gate::check(AlbumPolicy::CAN_IMPORT_FROM_SERVER, [AbstractAlbum::class, null]);app/Http/Resources/Rights/AlbumRightsResource.php (1)
49-49: Consistency nit: unify Gate call argument listMatch the pattern used by other abilities in this constructor for readability.
- $this->can_import_from_server = Gate::check(AlbumPolicy::CAN_IMPORT_FROM_SERVER, [AbstractAlbum::class]); + $this->can_import_from_server = Gate::check(AlbumPolicy::CAN_IMPORT_FROM_SERVER, [AbstractAlbum::class, null]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Http/Requests/Admin/ImportFromServerBrowseRequest.php(1 hunks)app/Http/Requests/Admin/ImportFromServerOptionsRequest.php(1 hunks)app/Http/Requests/Admin/ImportFromServerRequest.php(2 hunks)app/Http/Resources/Rights/AlbumRightsResource.php(1 hunks)app/Http/Resources/Rights/RootAlbumRightsResource.php(1 hunks)app/Policies/AlbumPolicy.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: New PHP files must include the project license header and have a single blank line after the opening <?php tag
Use snake_case for PHP variable names
Apply PSR-4 coding standard to PHP code
Always call in_array with true as the third parameter for strict checking (in_array($needle, $haystack, true))
Only use boolean expressions in if statements; avoid integers or strings as conditions
Use strict comparison (===, !==) instead of loose comparison (==, !=)
Avoid duplicating code in both if and else branches
Do not use empty(); prefer explicit checks (e.g., === null, === '', count(...) === 0)
Files:
app/Http/Resources/Rights/RootAlbumRightsResource.phpapp/Http/Requests/Admin/ImportFromServerBrowseRequest.phpapp/Http/Requests/Admin/ImportFromServerRequest.phpapp/Policies/AlbumPolicy.phpapp/Http/Requests/Admin/ImportFromServerOptionsRequest.phpapp/Http/Resources/Rights/AlbumRightsResource.php
app/Http/Resources/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use Laravel JsonResource for API resources; replace with Spatie Data objects
Files:
app/Http/Resources/Rights/RootAlbumRightsResource.phpapp/Http/Resources/Rights/AlbumRightsResource.php
app/Http/Requests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/Http/Requests/**/*.php: In Request classes, if a user is provided via query, assign it to $this->user2
In Request classes, $this->user is reserved for the authenticated requester
Files:
app/Http/Requests/Admin/ImportFromServerBrowseRequest.phpapp/Http/Requests/Admin/ImportFromServerRequest.phpapp/Http/Requests/Admin/ImportFromServerOptionsRequest.php
🧠 Learnings (4)
📚 Learning: 2025-08-14T11:05:31.237Z
Learnt from: ildyria
PR: LycheeOrg/Lychee#3616
File: app/Http/Requests/Tags/ListTagRequest.php:21-24
Timestamp: 2025-08-14T11:05:31.237Z
Learning: Laravel's Gate::check() system is flexible with policy method signatures. When calling Gate::check(PolicyConstant, [Model::class]) with a policy method that only accepts (User $user), Laravel does not throw "too many arguments" errors - it handles argument mismatches gracefully by only passing the arguments the method can accept. This pattern is used extensively in the Lychee codebase.
Applied to files:
app/Http/Resources/Rights/RootAlbumRightsResource.phpapp/Http/Resources/Rights/AlbumRightsResource.php
📚 Learning: 2025-08-27T08:53:20.883Z
Learnt from: ildyria
PR: LycheeOrg/Lychee#3654
File: routes/api_v2.php:100-100
Timestamp: 2025-08-27T08:53:20.883Z
Learning: In Lychee, Import endpoints use form request authorization (e.g., ImportFromServerBrowseRequest::authorize()) rather than route-level middleware for authentication. The authorize() method checks if Auth::user()->id equals the owner_id config value.
Applied to files:
app/Http/Requests/Admin/ImportFromServerBrowseRequest.phpapp/Http/Requests/Admin/ImportFromServerRequest.phpapp/Policies/AlbumPolicy.phpapp/Http/Requests/Admin/ImportFromServerOptionsRequest.php
📚 Learning: 2025-08-27T08:53:20.883Z
Learnt from: ildyria
PR: LycheeOrg/Lychee#3654
File: routes/api_v2.php:100-100
Timestamp: 2025-08-27T08:53:20.883Z
Learning: In Lychee, Import endpoints use form request authorization (e.g., ImportFromServerBrowseRequest::authorize()) rather than route-level middleware for authentication. The authorize() method checks if Auth::user()->id equals the owner_id config value, and this happens before controller execution to prevent unauthorized filesystem access.
Applied to files:
app/Http/Requests/Admin/ImportFromServerBrowseRequest.phpapp/Http/Requests/Admin/ImportFromServerRequest.phpapp/Policies/AlbumPolicy.phpapp/Http/Requests/Admin/ImportFromServerOptionsRequest.php
📚 Learning: 2025-08-12T11:31:09.853Z
Learnt from: ildyria
PR: LycheeOrg/Lychee#3605
File: docs/Lifecycle-of-a-request-photo-upload.md:144-151
Timestamp: 2025-08-12T11:31:09.853Z
Learning: In Lychee, the BaseApiRequest class has been modified to change Laravel's default execution order - processValidatedValues() runs BEFORE authorize(), not after. This means that in Lychee request classes, $this->album and other processed properties are available during authorization checks, unlike standard Laravel behavior.
Applied to files:
app/Http/Requests/Admin/ImportFromServerBrowseRequest.phpapp/Http/Requests/Admin/ImportFromServerOptionsRequest.php
🧬 Code graph analysis (6)
app/Http/Resources/Rights/RootAlbumRightsResource.php (1)
app/Policies/AlbumPolicy.php (1)
AlbumPolicy(26-603)
app/Http/Requests/Admin/ImportFromServerBrowseRequest.php (4)
app/Http/Requests/BaseApiRequest.php (1)
BaseApiRequest(28-172)app/Policies/AlbumPolicy.php (1)
AlbumPolicy(26-603)app/Http/Requests/Admin/ImportFromServerOptionsRequest.php (1)
authorize(18-21)app/Http/Requests/Admin/ImportFromServerRequest.php (1)
authorize(38-41)
app/Http/Requests/Admin/ImportFromServerRequest.php (1)
app/Policies/AlbumPolicy.php (1)
AlbumPolicy(26-603)
app/Policies/AlbumPolicy.php (1)
app/Models/Configs.php (2)
Configs(65-347)getValueAsInt(248-251)
app/Http/Requests/Admin/ImportFromServerOptionsRequest.php (3)
app/Policies/AlbumPolicy.php (1)
AlbumPolicy(26-603)app/Http/Requests/Admin/ImportFromServerBrowseRequest.php (1)
authorize(20-23)app/Http/Requests/Admin/ImportFromServerRequest.php (1)
authorize(38-41)
app/Http/Resources/Rights/AlbumRightsResource.php (1)
app/Policies/AlbumPolicy.php (1)
AlbumPolicy(26-603)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 2️⃣ PHP tests / 8.4 - sqlite -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - postgresql -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - mariadb -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit,Feature_v2
🔇 Additional comments (4)
app/Http/Requests/Admin/ImportFromServerOptionsRequest.php (1)
18-21: Policy-based authorization is correct and enforces the feature flag server-sideUsing Gate::check(AlbumPolicy::CAN_IMPORT_FROM_SERVER, [AbstractAlbum::class]) centralizes owner + flag checks in the policy. Good move.
app/Http/Requests/Admin/ImportFromServerBrowseRequest.php (1)
20-23: LGTM: browse request now defers to the policyAuthorization via Gate aligns with the other import requests and ensures the flag is enforced before any controller logic.
app/Policies/AlbumPolicy.php (1)
518-528: Owner + feature-flag gate with secure default — looks goodNon-null User type-hint, strict comparisons, and default-disabled config guard are appropriate.
app/Http/Requests/Admin/ImportFromServerRequest.php (1)
38-41: Authorize via policy — verified and approvedImport requests and rights resources consistently use Gate::check(AlbumPolicy::CAN_IMPORT_FROM_SERVER); no leftover direct 'disable-import-from-server' checks found; feature flag exists in config/features.php.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
For improved security.
Summary by CodeRabbit
New Features
Chores