Fix user IDOR, account takeover, and audit-log stored XSS#56
Merged
Conversation
Authorise every user-mutation endpoint through UserPolicy via Gate::authorize, fixing IDOR in soft-delete (F001), skills (F004), language (F006) and notification preferences (F007), and migrating the existing edit-info/password/photo checks onto the same policy. Rewrite edit() to drop the Host bypass that allowed account takeover (F002): only an administrator or the user themselves may edit, and group membership is synced only by an administrator for a non-administrator target, so a self-editing user can no longer rewrite their own groups.
Render audited values through Blade's {{ }} auto-escaping instead of a
raw PHP echo of json_encode, so a stored script payload in an audited
field no longer executes in an admin's browser (F005).
Add cross-user 403 and self/admin-success cases for soft-delete, edit, skills, language and preferences, plus group-membership regression tests and an audit-log XSS-escaping test. Drop the redundant RefreshDatabase trait (it nests a second transaction inside TestCase's and breaks the savepoint on legacy-table writes) and enable exception handling on the 403-asserting cases.
Collaborator
Author
|
QA 👍 confirmed the changes in the test cluster instance. |
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
2 participants
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.
Description
Closes six security findings in the user-account endpoints and the audit log, disclosed against this fork and already remediated upstream in TheRestartProject#881.
Changes
UserPolicy): addupdateanddeleteabilities ("self or Administrator") as the single canonical ownership rule.UserController): authorize soft-delete (F001), skills (F004), language (F006), and notification preferences (F007) through the policy, and migrate the existing edit-info/password/photo checks onto it.edit()to drop the Host bypass — only an administrator or the user themselves may edit. Group membership is synced only by an administrator for a non-administrator target, so a self-editing user can no longer rewrite their own group memberships (and an absentgroupsfield no longer wipes existing ones).{{ }}auto-escaping instead of a rawechoofjson_encode.Testing
PrivilegeEscalationTest: 24/24 passing — cross-user attempts return 403 for delete/edit/tags/language/preferences; a Host cannot edit or reset another user's password; self and admin still succeed; self-promotion to Host via a category-1 skill is preserved; plus regression tests asserting a non-admin cannot self-assign groups and that anedit()POST without agroupsfield preserves memberships.GroupEditTest: new test asserts an injected<script>in an audited field renders HTML-escaped, not raw.ProfileTest: updated so a Host now gets 403 and an admin reaches the password-mismatch path.ProfileTest/GroupEditTestcases fail identically on the base branch due to local environment issues (logged-out auth expectation, geocoding) unrelated to this change; expected to pass in CI.