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.