fix(log): authorize the change history against its objects (#150) - #160
Merged
Conversation
recall-changes and the system context's "recent changes" section are both read tier, and the read tier's capability is `read` — which every logged-in user holds, a Subscriber included. Each row's `summary` carries the title of the thing that changed, so an account that could never open a draft was being handed its title anyway. Same class as #148; different mechanism, because the log is its own CPT and never passes through collection(). The filter goes in Saddle_Log::recent_executed(), not in the ability: the same rows are assembled into the context every connected session receives, and fixing only the ability would leave that path disclosing the same titles. Two edges, both deliberate and both pinned: - A row naming no post — a settings change, a plugin activation — has no object to authorize against and is an admin-tier action in the first place. It stays. Term ids are numeric and get judged as post ids; the worst that costs is hiding a public taxonomy row from a low-privilege connection, never disclosing anything. - A row whose post no longer exists is the record of a deletion, which is what this log is for. Dropping it would erase deletion history from the owner's own record, so it survives for an account that can delete content and is withheld from one that cannot. Every named post is primed in one query before the per-row check, so this is not a query in a loop. Saddle_Context_Test now runs as an administrator. It never set a current user, and the system context is only ever assembled for a resolved one — without a user, recent_executed() correctly withholds every row naming a post, which is right for user 0 and wrong for what those tests measure. Closes #150
4 tasks
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
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.
Closes #150
What
saddle/recall-changesand the "recent changes" section of the system context bothreturn
Saddle_Log::recent_executed()unfiltered. Each row'ssummarycarries thetitle of the thing that changed, so a connection made with a low-permission account
could enumerate the titles of drafts and private posts it can never open. This filters
each row against the object it names.
Why
Found while sweeping the read surface for #148 and deliberately left out of that PR,
which was a review blocker that needed to stay reviewable. It is the same class of gap
the WordPress.org reviewer raised — the read tier's capability is
read, which everylogged-in user holds — and #148's fix does not reach it, because the log is its own CPT
and never passes through
collection().How
Saddle_Log::recent_executed(), not in the ability. Both callersshare it, and the context path is the one that reaches every connected session at
whatever tier — fixing only the ability would have left it disclosing the same titles.
entry_is_visible()judges a row the waySaddle_Abilities::collection()judges alisting:
read_postagainst the target.Two edges, both deliberate and both pinned by tests:
have no object to authorize against, and the action itself is admin tier. Term ids are
numeric and are judged as post ids; the worst that costs is hiding a public taxonomy
row from a low-privilege connection, never disclosing anything.
That row is the record of a deletion, which is what this log exists for; dropping it
would erase deletion history from the owner's own record. A trashed post is still a
post, so it follows
read_postnormally — which fortrashfalls through to the editcapabilities.
Every post the log names is primed in one query before the per-row check, so this is not
a query in a loop.
Saddle_Context_Testnow runs as an administrator. It never set a current user, and thesystem context is only ever assembled for a resolved one; without a user
recent_executed()correctly withholds every row naming a post, which is right for user0 and wrong for what those tests measure.
Testing
composer test— 653 tests, 2229 assertions, 0 failures (was 645). The eightnew cases are in
tests/read-authorization-test.php, written red first: the threedisclosure cases failed naming the exact symptom before the fix landed.
composer lint— 0 errors (3 pre-existing warnings, none in the changed files).rebuilt.
WP_DEBUGon (the suite runs with it enabled).Screenshots
None — no admin UI change. The admin Activity screen reads
Saddle_Log::query(), whichis
manage_options-gated and untouched.