User tests: Successful: Unsuccessful:
Pull Request resolves #
The local filesystem adapter of com_media validates every path it acts on through getLocalPath(), which contains the Path::check() call that keeps the request inside the adapter root. getResource() is the one exception: it concatenates the raw path onto the root and opens it directly:
return fopen($this->rootPath . '/' . $path, 'r');Today both call sites in ApiModel only pass paths that were already normalised by getFile() or getFiles(), so nothing is reachable through this gap at the moment. The asymmetry is a footgun for future callers: any code path that starts invoking getResource() with a request supplied path would silently skip the containment check that all other read and write operations enforce.
This PR routes getResource() through getLocalPath() as well, so a traversal style path now fails with an InvalidPathException before anything is opened. A missing file now also raises a FileNotFoundException, consistent with getFile(), instead of returning false from fopen() together with a PHP warning.
Unit tests cover the readable resource case, the traversal rejection and the missing file case.
Run the adapter unit tests:
libraries/vendor/bin/phpunit tests/Unit/Plugin/Filesystem/Local/Adapter/LocalAdapterTest.php
Optionally, with the API enabled, request GET /api/index.php/v1/media/files/local/{path}?content=true for an existing file and confirm the base64 content is still returned as before.
getResource() opens the raw concatenation of the adapter root and the given path. No validation runs inside the method itself, and a missing file yields a PHP warning plus a false return value.
getResource() resolves the path through getLocalPath() and throws an InvalidPathException for any path escaping the adapter root, and a FileNotFoundException for missing files. Valid files are returned exactly as before.
| Status | New | ⇒ | Pending |
| Category | ⇒ | Front End Plugins Unit Tests |
| Labels |
Added:
Feature
Unit/System Tests
PR-5.4-dev
|
||
Test instructions are not sufficient, that the unit tests pass is not enough. You need to create a before and after situation to show the problem and how the change fix the problem.
As Joomla 5.4 will now only receive security updates then this will need to be rebased to 6.2
As Joomla 5.4 will now only receive security updates then this will need to be rebased to 6.2
@brianteeman Except if we consider it a security hardening. We haven't decided about that yet.
I wouldn't describe this as fixing a Joomla security vulnerability. I'd describe it as closing an inconsistent path-validation gap in the local filesystem adapter.
@laoneo Hi, wanted to follow up on this PR. Let me know if there's anything else I can do to help move it forward. Thanks!