Re-enable the unit tests skipped in the security fix - #59
Draft
kubrickfr wants to merge 1 commit into
Draft
Conversation
Commit 5bca458 disabled 19 tests in EditorControllerTest with bare markTestSkipped() calls because they broke when the security fix (58b45b8) scoped id resolution to the caller's user folder: the tests stubbed IRootFolder::getById(), which the fixed code no longer calls, and PHPUnit's auto-mocked user folder returned [] for every lookup, so each test died with a 404 before reaching what it asserted. Stub getUserFolder()->getById() instead - in givenUserFile() for most of the tests, and inline for the tests that build their own folder mocks - and drop the now-dead root getById() stubs. All 19 tests pass again unchanged. Also: - testLoadUnknownFileIdIsNotFound previously stayed green only by accident (its root stub was ignored and the auto-mock returned []). It now stubs the user folder explicitly. - testLoadReturnsFilePayloadForLoggedInUser and testCreateReturnsFileInfoArray now assert that IRootFolder::getById() is never called, locking in the fix for GHSA-c9x8-9w2j-4m94. - Revert the lib/ half of 5bca458: it had replaced the explanatory comment on the user-folder scoping with an unreachable null guard (IRootFolder::getUserFolder() is non-nullable in OCP stable33). getFileById() is byte-identical to 58b45b8 again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Commit 5bca458 ("Skip unit tests which won't work as expected with mock objects") disabled 19 tests in
EditorControllerTestwith baremarkTestSkipped()calls. The tests themselves were still valid — they broke for a purely mechanical reason: the security fix (58b45b8) changed id resolution fromIRootFolder::getById()togetUserFolder($uid)->getById(), while the test fixtures stubbed only the root folder. PHPUnit's auto-mocked user folder then returned[]for every lookup, so each test hit a 404 before reaching what it actually asserted.Among the skipped tests were the permission checks (
testLoadUnreadableFileIsForbidden,testSaveWithoutWritePermissionIsForbidden,testSavePreviewForbiddenWithoutWritePermission,testCreateWithoutPermissionReturnsError) — the closest thing the unit suite has to regression coverage for the security properties the 4.3.6 release fixed.What this PR does
getUserFolder()->getById()instead — in the sharedgivenUserFile()helper for most tests, inline for the tests that build their own folder mocks — and removes the now-dead rootgetById()stubs. All 19 tests pass again unchanged in what they assert.testLoadUnknownFileIdIsNotFoundwas green only by accident (its root stub was ignored; the auto-mock happened to also produce a 404). It now stubs the user folder explicitly.$this->root->expects($this->never())->method('getById')to the load and create happy-path tests, locking in the user-folder scoping that fixed GHSA-c9x8-9w2j-4m94 so a future refactor can't quietly reintroduce instance-wide id resolution.lib/half of 5bca458: it had replaced the explanatory comment on the user-folder scoping with a null guard ongetUserFolder(), which is non-nullable in OCP stable33 (the guard was unreachable, and the auto-mock never returns null either — it wasn't what broke the tests).getFileById()is byte-identical to 58b45b8 again, comment included.Result:
composer run test:unit— 106 tests, 278 assertions, 0 skipped.Note: #58 (folder-share create fix) touches neighbouring test code; whichever merges second may need a trivial rebase.
🤖 Generated with Claude Code