fix: check for the identifier alias for the storage backend

[jvillafanez]

Description

Prevent local storage to be used as external if it isn't explicitly allowed.

Related Issue

• Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

• test environment:
• test case 1:
• test case 2:
• ...

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

Notes:

This is likely the minimum change possible at the moment. There are no plans to add or modify the backend's aliases
or identifiers at the moment, but it could become unmanageable quickly.
We should consider to move and improve the check at some point.

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

• can you add unit tests?
• please add changelog items

[DeepDiver1975]

• I miss tests where local storage mounting is allowed.files_external_allow_create_new_local => true
• did you double check if the http status code change 422 -> 403 has any impact on the frontend. technically this is a breaking change to the api

[jvillafanez]

did you double check if the http status code change 422 -> 403 has any impact on the frontend. technically this is a breaking change to the api

I don't think it's possible to trigger the behavior from the web UI. If the backend isn't visible, it won't show up in the frontend and it won't be selectable.
I've also included the DAV storage to test and it has the same behavior: the DAV storage doesn't show up as selectable in the user's external storages. From the admin settings, the webdav checkbox isn't selected, and even though you can mark the checkbox and save it, it won't persist.

The change in the HTTP code should match the previous behavior: it returns a 403 if the user tries to create or update a backend that he isn't allowed to use. It also makes more sense to return a 403 error in those circumstances.

I miss tests where local storage mounting is allowed.files_external_allow_create_new_local => true

They should be covered. For the global / user storage controllers, the tests have the flag enabled by default, so all those tests run with that setting. The only exception are the "new" testCreateLocal and testCreateLocalClassname, which explicitly set the flag as false.

[DeepDiver1975]

They should be covered. For the global / user storage controllers, the tests have the flag enabled by default, so all those tests run with that setting. The only exception are the "new" testCreateLocal and testCreateLocalClassname, which explicitly set the flag as false.

looking at the changeset - I see files_external_allow_create_new_local only being removed from the code - is this setting actually still be checked anywhere?

[DeepDiver1975]

They should be covered.

I prefer 'are' over 'should' - assertion over assumption

[jvillafanez]

looking at the changeset - I see files_external_allow_create_new_local only being removed from the code - is this setting actually still be checked anywhere?

It's used in the settings page. The flag is in an awkward position... it doesn't allow the creation of the local storage from the frontend, but the admin can still create the local storage via API. Regular users won't be able to setup local storages anyway.

I'll try to add an isAllowedAdminBackend (similar to the isAllowedUserBackend) and check for the flag there. If not allowed, the backend visibility will be set to none at registration time (similar to what is done for regular users). I think we can go with that solution if it works.

[DeepDiver1975]

. it doesn't allow the creation of the local storage from the frontend, but the admin can still create the local storage via API.

just to be sure: the backend has to verify everything - ignore frontend beahavior.
finally: adding tests accordingly helps to verify this and to ensure that changes in the future will not fuck it up again ;-)

[jvillafanez]

There might be slightly changes in the behavior with the PR because the checks have been moved to visibility. If the backend isn't visible for someone, this means that the mount point (if any) will be hidden and won't appear (it won't be accessible), and creating or updating new mounts with the backend won't be possible. To remark, if the backend becomes visible at a later date, previously existing hidden mounts will reappear.

Specifically, for the local storage, it will be blacklisted for regular users and it will never have the visibility for regular users. Admins will require the files_external_allow_create_new_local config setting to be "true" to have the visibility.

[DeepDiver1975]

Code Review

Overview

The PR fixes a security bypass: the original code checked $backend === 'local' (string comparison) to block local storage mounting when files_external_allow_create_new_local is false, but an attacker could bypass this by supplying the fully qualified class name \OC\Files\Storage\Local instead. The fix introduces a new StoragesBackendChecker class and moves the restriction check to use $backend->getStorageClass(), which catches both forms.

---

What's Good

• Core bug fix is correct. Using getStorageClass() instead of comparing string identifiers closes the bypass. Both the controller tests (testCreateLocalClassname) and the service test (testAdminMountingBackends) verify the class-name path.
• Clean extraction into StoragesBackendChecker. The new class properly separates concerns and makes the logic testable in isolation.
• isAllowedAdminBackend() is now enforced at registration time (registerBackend() calls removeVisibility(VISIBILITY_ADMIN)), consistent with how user backend visibility works.
• DI resolution works without explicit registerService() — the container resolves StoragesBackendChecker::class by FQCN automatically.

---

Issues

1. Missing test cases for the allowed path (also raised by @DeepDiver1975)

There are no tests where files_external_allow_create_new_local = true and a local mount succeeds. Both testCreateLocal and testCreateLocalClassname only test the blocking case. A "local is allowed" test is necessary to verify the happy path and protect against regressions.

2. The new controller tests may not exercise the right code path

The old controller check ran at request time. The new check runs at registration time (registerBackend()). In the controller tests, by the time $this->controller->create() is called, backends were registered at test setup — before setValue('files_external_allow_create_new_local', false) runs. The 403 the tests observe comes from StoragesController::validate() seeing a non-visible backend, but whether that visibility was removed due to this PR's logic or something else in the test harness (e.g. 'local' not mapping to a real registered backend) is unclear. The tests should verify that visibility was specifically stripped because the config flag is false, not just that the endpoint returns 403.

3. Breaking HTTP status code change: 422 → 403

StoragesController.php:183 and :194 change STATUS_UNPROCESSABLE_ENTITY to STATUS_FORBIDDEN for the visibility check. These carry different semantics: 422 = "valid request, unprocessable content", 403 = "forbidden". While 403 is arguably more correct, this is a breaking change to the API that could affect clients and the JS frontend that processes these error codes. Has the frontend been confirmed to handle both codes equivalently?

4. Type annotation inconsistency in StoragesBackendChecker

/** @var IConfig */
private IConfig $config;           // typed property (PHP 7.4+)
/** @var bool */
private $allowUserMounting = null; // @var says bool, initialized to null
/** @var array */
private $userMountingBackends = null; // @var says array, initialized to null

Typed property syntax is mixed with old docblock-only style. The @var bool annotation on a null-initialized property is misleading and would fail strict static analysis. These should be ?bool/?array or use typed properties consistently.

5. isAllowedAdminBackend() config read is not cached

isUserMountingAllowed() and allowedBackendsForUsers() both cache their config reads. isAllowedAdminBackend() calls getSystemValue() on every invocation with no caching. Consistent with the existing pattern, this result should be cached too.

6. Hardcoded blacklist in isAllowedUserBackend is undocumented

$blacklistedBackendsForUsers = ['\OC\Files\Storage\Local'];

This in-method local variable carries no documentation. A short comment explaining why local storage is always blacklisted for users regardless of user_mounting_backends would prevent future confusion.

7. Changelog grammar (also noted by @phil-davis)

if is was explicitly disallowed → if it was explicitly disallowed

---

Summary

The security fix itself is sound and well-targeted. Main blockers before merge: (a) missing "allowed" happy-path test cases, (b) confirming the new controller tests actually exercise the intended code path, and (c) verifying the 422→403 change doesn't break frontend error handling. The type annotation issues, missing cache, and undocumented blacklist are minor but worth a cleanup pass.

[jvillafanez]

1. Missing test cases for the allowed path

That's covered. The code checks for the visibility of the backend, and we have tests for both cases.

2. The new controller tests may not exercise the right code path

The controller will check for the backend visibility to allow or disallow the creation of the storage. The configuration check is deeper in the stack and it shouldn't matter for the unit tests because the controller doesn't depend directly on the StoragesBackendChecker (which is the one checking the configuration)

3. Breaking HTTP status code change: 422 → 403

I'd rather keep the current 403 error because that's what happened before when the user tried to mount a local storage. We return the same error code, just from a different place.
In addition, it makes sense to return a 403 if you try to setup a mount using a backend not visible to you.

---

The rest of the topics are solved.

Something to check is

core/apps/files_external/lib/Controller/UserStoragesController.php

Line 246 in 45ffc86

private function isUserMountingAllowed(): bool {

The isMountingUserAllowed is also checked during the validation for the StoragesBackendChecker, and it will cause the affected mounts to have the visibility reduced. We can remove that call and remove the config dependency from the controller. The behaviour will remain the same returning a 403 error code (if we keep the current error code in the validation)

[jvillafanez]

The isMountingUserAllowed is also checked during the validation for the StoragesBackendChecker, and it will cause the affected mounts to have the visibility reduced. We can remove that call and remove the config dependency from the controller. The behaviour will remain the same returning a 403 error code (if we keep the current error code in the validation)

This is done in the last commit

[DeepDiver1975]

Code Review

Overview

This PR fixes a security bypass where the local filesystem could be mounted as external storage by passing the internal PHP class name (\OC\Files\Storage\Local) instead of the string identifier local, circumventing an ad-hoc string comparison check. The fix moves enforcement to backend registration time using a new StoragesBackendChecker service class.

---

Code Quality

Positive:

• Clean extraction of StoragesBackendChecker into a dedicated, testable class
• Lazy caching of config values is well-structured
• Good test coverage: StoragesBackendCheckerTest covers all three public methods with data providers
• The %i → %d format string fix is a legitimate correctness fix
• Changelog entry is present and clear

Minor style issues:

StoragesBackendChecker.php — Redundant @var docblocks on typed properties:

/** @var IConfig */
private IConfig $config;

Same applies to $canCreateNewLocalStorage, $allowUserMounting, $userMountingBackends. With typed properties the docblock is redundant.

StoragesBackendChecker.php:488 — "blacklisted" is deprecated terminology:

$blacklistedBackendsForUsers = ['\OC\Files\Storage\Local'];

Prefer $deniedBackendsForUsers or $blockedBackendsForUsers.

---

Potential Issues

1. Fragile test reasoning in GlobalStoragesControllerTest::testCreateLocal

// there is already a teardown in the parent class setting this value to false
\OC::$server->getSystemConfig()->setValue('files_external_allow_create_new_local', false);
$result = $this->controller->create(...);
$this->assertEquals(Http::STATUS_FORBIDDEN, $result->getStatus());

The 403 now comes from validate() detecting that VISIBILITY_ADMIN was stripped at backend registration time — not from anything happening during this create() call. The test happens to pass because the config was already false at registration (setUp) time, but the intent is non-obvious and the test would silently stop testing the right thing if setup order changed.

2. Inconsistent backend formats between testCreateLocal and testCreateLocalNotAllowed in UserStoragesControllerTest

• testCreateLocal: $backend = "identifier:{$localBackendName}" → expects HTTP 201
• testCreateLocalNotAllowed: $backend = $localBackendName (no identifier: prefix) → expects HTTP 403

The different outcome based on the identifier: prefix for the same underlying backend is non-obvious. A comment explaining that the identifier: prefix is what triggers backend resolution (and thus visibility checking) would help future readers understand why these two cases differ.

3. Undocumented behavioral contract after removing runtime guards

The PR removes all isUserMountingAllowed() guards from UserStoragesController::index(), create(), update(), and destroy(). Enforcement now relies entirely on VISIBILITY_PERSONAL having been stripped at registration time.

This is correct since StoragesBackendChecker lazily reads config per-instance (and PHP creates a new instance per request), so dynamic config changes work. But this is an implicit contract — a comment noting that enforcement moved to StoragesBackendService::registerBackend() would make the intent clear for anyone who later wonders why the controller doesn't guard these endpoints.

---

Security

The core fix is correct. Moving the check from $backend === 'local' (bypassable with the class name) to getStorageClass() === '\OC\Files\Storage\Local' (resolved from the registered backend object) properly closes the bypass. Enforcement at registration time via removeVisibility is a robust approach.

---

Summary

Area	Status
Core bug fix	Correct approach
Test coverage	Good
Behavioral change (removed runtime guards)	Works correctly, worth a comment
Test clarity (fragile reasoning, prefix inconsistency)	Minor, worth addressing
Code style	Minor (redundant @var, terminology)
%i → %d fix	Correct but unrelated — could be a separate commit

No blockers. The main suggestions are adding clarifying comments around the registration-time enforcement contract and the identifier: prefix behavior in tests.