fix(forum): close the authorisation holes, and give the plugin its first tests (v2.3.x) - #5862
Merged
Merged
Conversation
…ould pick havePluginTables() matched CREATE TABLE with a pattern that required the statement to end immediately after the engine name. Most of the bundled plugins that use it declare table options after that point, so the pattern did not match them. It did not merely skip them. The body group is lazy, so when a statement failed at its tail the body ran on until it reached one that did end at its engine. forum collapsed from four tables into a single match named `forum` whose body spanned all four, and hero, linkwords and pm matched nothing at all, which made the helper throw the "no CREATE TABLE statements" error for plugins whose SQL is perfectly well formed. The engine was hardcoded to MyISAM on the way back out, and neither taking that literally nor reading it from the file is right. db_verify treats a declared engine as a request rather than an instruction and satisfies it from storageEnginePreferenceMap, so the bundled MyISAM schemas are installed as InnoDB on any server that has InnoDB, and hero's InnoDB could legitimately become XtraDB. A test given MyISAM was therefore working against a table with no transactions and different FULLTEXT behaviour from the one the plugin manager builds. Let the tail run to the semicolon, then resolve the declared engine the same way the handler does. The helper keeps its own copy of the alias table because the acceptance suite runs outside the application and cannot boot db_verify. A test compares the two by reflection, so the copy cannot drift silently. The parse and the engine resolution move into static methods so they can be tested without a module container, and the tests read the shipped SQL rather than a fixture, so they fail if the helper and e107 ever disagree.
The forum plugin has never had a test. Standing one up is not a matter of
creating its tables, and the shape of the data decides whether the tests that
follow mean anything.
Every front-end page redirects on the plug_installed preference, so the plugin
has to be installed rather than merely present; postDelete() also touches
user_extended columns that come from the plugin's extendedFields rather than
from forum_sql.php. The fixture therefore installs it properly, once per suite.
The trap worth naming: _getForumPermList() only grants permission on a forum
whose forum_parent is not 0 AND whose parent row passes the same class test.
A forum seeded at the top level grants nobody anything, so a suite built on one
would see every "X cannot do Y" assertion pass without exercising a line of the
code it names. forum.forum_moderators is also a tinyint holding a single
userclass id, and a stored 0 is userclass 0 (Everyone) rather than "admins",
which forumGetMods() only infers from e_UC_ADMIN or an empty string.
So the Cest here asserts the fixture from both sides before anything relies on
it: a public forum is readable, a restricted one is not, a seeded member can
sign in and reach a members-only page a guest is bounced from, the two forums
carry different moderator classes, and neither member is an admin. That last
one matters because getperms('0') short-circuits every MODERATOR test in the
plugin, so an admin would pass the very checks the later tests exist to probe.
Anything needing the application itself goes through a probe file dropped into
the docroot for the run, the same shape 0020, 0022 and 0023 use. Rows go in
through haveInDatabase so the Db module takes them out again after each test.
csrf_enforce is pinned per test rather than inherited. 0023 removes the
preference in its teardown and runs immediately before this file, and an unset
value resolves to a mode that reads no token and expects a Sec-Fetch-Site
header PhpBrowser never sends.
A moderator of any one forum could delete, lock, unlock, stick and unstick threads and posts in every other forum on the site, by three independent routes. Each was reproduced against a running install before this change. forumGetMods() memoises one moderator list on the forum object regardless of which userclass it is asked about, and the three getModeratorUserIdsBy*() helpers all went through it. Both forum_viewtopic.php and forum_viewforum.php prime that memo from the id in the URL while working out MODERATOR, so by the time any write was authorised the answer had already been fixed to the forum being viewed, whatever thread or post the request actually named. That memo cannot simply be keyed by userclass: templates read it to render a forum's moderator list and to badge post authors, so changing what it holds changes what they display. Authorisation now uses a separate lookup that is keyed by class and never touches it. ajaxModerate() then took whichever of thread or post arrived last, derived the permission from that, and acted on the thread anyway. Its own comment warned about precisely this and the code did the opposite. Each action is now authorised against the object it changes, and an action that names no id is refused rather than falling through to a handler with an unset variable. forum_thread_moderate() took its target straight out of the POST field name and checked nothing whatsoever, relying on a MODERATOR constant its caller had computed for a different forum. It now authorises every id it is handed. moveThread() checked MODERATOR alone, which answers for neither end of a move: the destination was never authorised at all, so a moderator could push a thread into a forum they have no rights over. Both ends are checked now. The tests exercise all three routes, and each carries a positive control that the same action still succeeds where it was granted, because a fix that refused everything would otherwise turn the file green. Reverting this change makes six of them fail. One gap worth stating: the no-id case asserts only that nothing is destroyed. Its real symptom was a PHP warning printed ahead of the JSON, which the test environment does not render, so that part is not pinned by a test.
The self-service delete authorised on `USERID == $row['post_user']` and nothing more. USERID is 0 for a caller with no account, and an anonymous post stores post_user 0, so every unauthenticated visitor owned every anonymous post on the site. Neither that handler nor postDelete() looks at which forum the post belongs to, so the reach was site-wide, including forums the caller could not read. A request with no cookie also carries no ambient authority, so the CSRF rule lets it through by design and nothing else stood in the way. Reproduced against a running install: one POST, no account, no session, no token, and the row was gone. The same pseudo-identity sat in isAuthor(), which guards editing. There a guest could rewrite another guest's post, and that is reproduced by a test here too. Three more rules the endpoint's name, its docblock and the control that offers it all promise, and which the server never checked: the post must be the last in its thread, must not be the one that opened it, and the thread must still be open. forum_post.php's own checkPerms() has always honoured the last of those. Restoring them narrows what a member may do, but to what e107 already told them they could do; the shortcode has only ever offered the control under exactly these conditions. Deleting a thread's opening post through this route left the thread row behind with nothing to open it, which is the case worth calling out separately. Reverting this change fails five of the tests added with it. The two that still pass are deliberate guards rather than regression proofs: refusing somebody else's post already worked when both ids were non-zero, and the member's own last post must still delete, which is the control that stops the rest of the file going green by breaking the feature.
…directory
post_attachments is written straight out of $_POST['post_attachments_json']
with no validation whatsoever, and postDeleteAttachments() concatenated each
stored entry onto the poster's attachment directory and unlinked the result.
A member could submit an ordinary reply carrying a relative path, delete their
own post, and have e107 remove any file the web user could reach.
Demonstrated end to end against a running install through the real forms: a
reply through the reply form, a delete through the delete control, and a file
at the docroot root was gone. e107_config.php is the obvious target, and a
site missing it believes it is uninstalled.
The entry is now refused unless it is a bare filename, and the same check runs
on the way in so a value that could never describe a real upload is not stored
at all.
The identical loop was also broken for legitimate attachments. Uploads store
array('file' => ..., 'name' => ..., 'size' => ...) per entry and only
sendFile() ever carried the shim for that shape, so here the array was
concatenated onto the path: every real attachment was "deleted" as a file
named Array, the actual upload was orphaned, and post_attachments was then
cleared, leaving nothing to find it by. Both list keys were also read without
checking they were set.
One precondition is worth recording because it cost a false negative first
time round. The traversal only resolves when the poster's own attachment
directory exists, since the operating system walks user_000002/../.. one
component at a time. It exists as soon as that member has ever attached a
file, so the fixture creates it the way an upload does. Skipping that made the
unlink fail for an unrelated reason and the defect read as absent.
Fixture threads and posts are now backdated an hour. e107's flood check
compares the newest thread_datestamp against now, so a fixture stamped "just
now" made every reply a test tried to post look like flooding and it was
refused with nothing written, which turned an assertion about what got stored
into an assertion about an empty table.
Reverting this change fails all four tests added with it.
ajaxQuickReply() took two ids out of the request and never related them. checkPerm() was asked about $_POST['post'], a forum, and $_POST['thread'] was then written into post_thread, so naming any forum you may post in bought a reply in any thread on the site, including one in a forum you are redirected away from. thread_active went unread as well, while the ordinary posting form has refused a closed thread since forever. The thread now decides the forum: it is read back from forum_thread, that forum answers the permission question, and it is what the row is filed under. A closed thread takes replies only from a moderator of it, which is the licence checkPerms() grants in forum_post.php. ajaxTrack() asked nothing beyond "are you signed in". trackEmail() posts the full body of every later reply out to whoever sits in forum_track with no per-recipient check, so a subscription to a thread you cannot open is a standing feed of its contents. Subscribing is a way of reading, and now answers to the same view permission the forum's own pages do, on both routes that dispatch it. The cross-forum reply was demonstrated against a running install before the tests were written. Reverting this commit fails five of the nine tests in 0028_ForumPostingCest; the other four are controls, so a fix that simply refused everything would not pass either.
…ssions forum.php?new is a public route and the query behind it was wrong three ways. USERLV is defined only in the authenticated branch of class2.php and was dereferenced bare, so on PHP 8 the page was a fatal for every visitor without an account, and for every crawler. It now falls back to zero, which is what login_menu does, and the read-state lookup no longer assumes a user row is there to read. There was no forum_class predicate, alone among the listings this plugin ships: e_search, e_rss and e_list have all filtered on it for years. The query now offers only forums the caller may open. What reaches the page today is not a disclosure, because the page cannot print it: forum.php hands the thread rows to the template built for forum rows, so a v2 theme renders the section empty. That is a fourth defect and it is not fixed here, but the query should not be the thing standing between a member and a closed forum once it is. The already-read filter compared thread ids against thread_forum_id, so reading one thread hid every thread in whichever forum happened to carry that id. forumGetUnreadForums() has always compared the right column. Because the page prints no thread names, the two filters are asserted against the query rather than the rendered output; the fatal is asserted against the page, which is where it lands. Reverting this commit fails four of the five tests in 0029_ForumNewListingCest, one of them by the guest fatal itself.
Three ways the forum's AJAX replies were untrue. A duplicate reply reported success. postAdd() answers a duplicate with -1 and that went straight into post_id, so the reply slid into the page looking accepted and was gone on the next refresh. forum_post.php has reported it properly for years. The check ignores the thread on purpose, so the way to reach it is the same text in two threads of one forum, which is what the test does; the double-click route is confounded by forum.js binding its click handler more than once. An empty reply reported nothing. The response array was only built on the way through the success path, so the page received neither status nor msg and said nothing at all. It is now always an array, always with a status, and the failure paths each name their own reason. Moderation swallowed every other AJAX request on the page. The forum called ajaxModerate() whenever the viewer was a moderator, whatever the action, and ajaxModerate() always ends by printing JSON and exiting. A moderator's poll vote, rating or plugin widget on a forum page was answered with a forum error while the same click by an ordinary member went through, which reads as a permissions fault and is not one. Both dispatch sites now ask first, through one predicate rather than a list repeated per page.
forum_lastpost_info means "when the last post happened, and in which thread": postAdd() writes post_datestamp into it. The recalculation read thread_datestamp instead, which is when the thread was started, and ordered by it, so a forum's last post became its newest thread, dated to that thread's birth and credited to whoever had posted in it most recently. postDelete() and threadDelete() both trigger the recalculation, so removing one spam post could point a busy forum at a thread nobody had touched in years. thread_total_replies excludes the opening post everywhere it is read; postAdd() increments it once per reply and every reader adds one back. threadUpdateCounts() stored the raw row count, so splitting a topic left both halves one reply heavy, which the reader turns into a page that is not there. "Mark all forums read" carries no forum id, and the branch meaning "all of them" tested against 0 rather than emptiness, so the request took the per-forum path with a list containing null, matched nothing and redirected having done nothing. Every other link carries an id, which is why only the board-wide one was affected. Reverting this commit fails all five tests in 0030_ForumCountsCest.
The rows on forum.php?new are threads. Under a v2 theme forum.php handed
them to $FORUM_TEMPLATE['main']['forum'], the template written for forum
rows, whose shortcodes ask for {FORUMNAME}, {REPLIESX} and {THREADSX}.
None of those resolves against a thread, so the section rendered as an
empty table and the page was decoration. Nobody had noticed because the
same page fatally errored for guests and answered members with an empty
result, both fixed alongside this.
The legacy template has carried the right shortcodes since v1
({NEWIMAGE}, {NEWSPOSTNAME}, {STARTERTITLE}, all implemented in
forum_shortcodes.php); the v2 setup simply never gained a section of its
own, and the comments beside those three assignments show the keys that
were meant to exist. This adds them, in the v2 style, and reads them with
merging on and keyed to the section, so a theme whose template predates
this falls back to the plugin's and one that overrides part of it keeps
the rest.
No breadcrumb in the new section: it renders before forum.php builds
$breadarray, and the forum listing directly underneath carries one.
Cancel meant delete. Core binds a confirm() to a[data-confirm] and returns the answer, which gets jQuery as far as preventDefault() plus stopPropagation(); stopPropagation() only stops the event reaching ancestors, so another handler on the same element runs whatever the visitor answered. forum.js binds one, and binds it first, because attachBehaviors() is registered ahead of that ready block. A moderator who clicked Cancel on "delete this thread?" deleted the thread. forum.js now asks the question itself and stops the event dead on both answers, so nothing acts on a refusal and nobody is asked twice; core's handler gains stopImmediatePropagation() as well, which closes the same hole for every other a[data-confirm] in e107. One click sent more than one request. The elements were filtered with jQuery's one(), a one-time event binder, where jQuery-Once's once() was meant: given a name and no handler, one() returns the set untouched, so nothing was marked and nothing filtered. attachBehaviors() runs again after every successful track and quick reply, so handlers piled up, and forum_track has no unique key to stop the duplicate rows or the duplicate notification mail. Any action on a page carrying TinyMCE for something else threw before the request was made: the quick-reply text was read by calling getContent() on whatever tinymce.get() returned, including on moderator links on pages with no quick-reply box at all. tinymce.get() answers null for a field it is not attached to. And a request that failed said nothing anywhere, which looks exactly like a click that never fired. There is an error callback now. All four are proven on master by ForumActionsCest, which this line has no webdriver suite to run: v2.3.x ships acceptance, functional and unit only. The source fix is identical; the coverage is not, and the pull request says so rather than implying parity.
Reporting a post inserts a row and fires an event that mails the moderators, and it had no throttle of any kind. Anyone who may post could submit that form as fast as they could script it, with the site's own mail server doing the sending, and on a forum open to guests they did not even need an account. Posting has answered to the site's flood setting for as long as the plugin has existed; reporting answers to the same one now, scoped to the reporter so one person cannot silence everybody else's reports, and skipped for admins exactly as posting skips it. Guests share a bucket, which is the conservative direction for an unauthenticated way to send mail. The same handler read USERNAME bare, twice, and class2.php defines it only for a signed-in visitor, so on a site that allows anonymous posting a guest reporting a post got a PHP 8 fatal rather than a report. That turned out to be live: reverting this commit, the guest test fails on the response code. Reverting fails two of the three tests in 0031_ForumReportCest; the third is the control that an ordinary report still lands.
Everything in the twelve commits above was written against master, which has a query builder and an engine-preference map in db_verify. This line has neither, so the parts that touched them are rewritten rather than carried over. The queries added by the authorisation and bookkeeping fixes now use $sql->select(), ->gen() and ->count() the way the rest of this file does, with every interpolated value cast. Behaviour is unchanged: the same rows are read and the same decisions taken. havePluginTables() creates each table with the engine its schema declares, because that is what db_verify::getFixQuery() does here: it puts the declared engine straight into the CREATE TABLE with no substitution, so a MyISAM schema really does install as MyISAM on this line. Master resolves the same schemas to InnoDB through a preference map and its copy of the helper mirrors that, with a unit test pinning the two together. There is no map here to pin to, so that test is replaced by ones asserting what this line actually does. The fixture also empties the forum tables once per run, before the first test seeds anything. Rows the probe inserts and rows the application creates are tracked by neither the Db module nor the tests, which clean up after themselves only when they pass, so a run that failed part way through left rows behind that the next run's assertions counted. One real failure was turning into a second, unrelated-looking one on the next run.
Deltik
approved these changes
Aug 1, 2026
Member
|
@rica-carv, I'm having @e107help go through your old open pull requests about the |
Contributor
Author
|
@rica-carv Boas! Done, and the short answer to "Should i discard them?" is no. Not one of the ten. I have read all of them against master at Ready as far as I can tell
Fixed, and worth explaining
Need a decision from you
On the ones marked as needing your call: if the appetite has gone after this long a wait, that is entirely fair. Say so on the thread and we will carry them with credit to you. Approving and merging is @Deltik's, not mine, so nothing here is a merge decision. But none of these deserved to sit for two years, and none of them should be discarded. |
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
3 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.
The forum sweep from #5861, on the stable line. Sixteen defects, six of them authorisation failures, one reachable with no account at all. The plugin had no automated test of any kind, which is why these have sat there for years.
Every fix arrives with a test that fails without it. Reverting
e107_plugins/forumon this branch fails 33 acceptance tests and 3 errors.Authorisation
forum_class.php:455,forum_post.php:1608USERID == post_userwith noUSERcheck.USERIDis 0 for a visitor with no account, and an anonymous post storespost_user = 0, so where anonymous posting is enabled a passing stranger could delete or edit anonymous posts. Needing no cookie, such a request is not challenged for a token either. Needing no configuration at all: any member could delete any post they had ever written, anywhere, including one that opened a thread, leaving the thread with no first post. Neither path enforced "last post", "not the opening post" orthread_active, all three of which the docblock and the UI claim.forum_class.php:1384,forum_post.php:1743post_attachmentsis written straight out of$_POST['post_attachments_json']with no validation of names or paths, and the delete concatenated each stored entry onto the poster's attachment directory and unlinked the result, with no basename and no containment check. Post with a relative path, delete your own post, and e107 removes any file the web user can reach.forum_class.php:1663,:558,forum_mod.php:14,forum_post.php:1331forumGetMods()memoised one list on the object whatever userclass it was asked about, and the page primes that memo before any write is authorised.ajaxModerate()derived permission from whichever of thread/post arrived last and then acted on the thread regardless.forum_thread_moderate()took its target out of the POST field name and checked nothing.moveThread()was gated only on aMODERATORcomputed for whichever forum the mover named. Authorisation now happens at the sink, against the object each action changes.forum_class.php:295checkPerm()about$_POST['post'], a forum, then wrote$_POST['thread']intopost_thread, with nothing tying the two together. Naming any forum you may post in bought a reply in any thread on the site, including one in a forum you are redirected away from.thread_activewent unread as well, so this route took replies into closed threads that the ordinary form refuses.forum_class.php:396trackEmail()then posts the full body of every later reply to everyone inforum_trackwith no per-recipient check, so a subscription to a thread you cannot open was a standing feed of its contents.Public routes
forum.php?newwas a PHP 8 fatal for every visitor without an account and every crawler, becausethreadGetNew()dereferencedUSERLVbare and class2.php defines it only for a signed-in visitor. The same query carried noforum_classpredicate, alone among the listings this plugin ships, and compared thread ids againstthread_forum_id, so reading one thread hid every thread in whichever forum carried that id. The page then rendered those thread rows through the template built for forum rows, so under a v2 theme the whole section came out as an empty table.Reporting a post inserted a row and fired an event that mails the moderators, with no throttle of any kind, on a form anyone who may post could submit as fast as they could script it. It also read
USERNAMEbare, twice, so on a forum open to guests a report was a fatal rather than a report. It answers to the site's own flood setting now, scoped per reporter and skipped for admins, exactly as posting does.Client side
Four defects in
js/forum.js:confirm()toa[data-confirm]and returns the answer, which gets jQuery as far aspreventDefault()plusstopPropagation(). That only stops the event reaching ancestors; another handler on the same element runs whatever the visitor answered, and forum.js binds one, first. Clicking Cancel on "delete this thread?" deleted the thread.one(), a one-time event binder, where jQuery-Once'sonce()was meant. Given a name and no handler,one()returns the set untouched, so nothing was marked and nothing filtered, andattachBehaviors()runs again after every successful track and quick reply.forum_trackhas no unique key to stop the duplicate rows or the duplicate notification mail.getContent()on whatevertinymce.get()returned, for every action including moderator links on pages with no quick-reply box.tinymce.get()answersnullfor a field it is not attached to.Reviewers, note one change outside the plugin.
e107_web/js/core/all.jquery.jsgainsstopImmediatePropagation()on thea[data-confirm]and.e-confirmhandlers. That closes the same hole for every confirmation dialog in e107, not just the forum's, and it is the only file here that is not forum code.Correctness
forum_lastpost_infomeans "when the last post happened, and in which thread", and the recalculation readthread_datestamp, which is when the thread was started, and ordered by it. Deleting one spam post could point a busy forum at a thread nobody had touched in years.threadUpdateCounts()stored the raw row count in a column that excludes the opening post everywhere it is read, so splitting a topic left both halves one reply heavy. "Mark all forums read" carries no forum id and the branch meaning "all of them" tested against0rather than emptiness, so it matched nothing and redirected having done nothing.These three are on the stable line rather than master only, because each is visible to an ordinary visitor: a forum's "last post" points at the wrong thread after any deletion, a split topic reports a page that is not there, and "mark all forums read" silently does nothing.
What differs from #5861
Same defects, same fixes, same tests. This line's database and test APIs are different enough to be worth calling out:
$sql->select(),->gen()and->count()in the style of the code around them, with every interpolated value cast. Same rows read, same decisions taken.db_verifyhere has no engine-preference map:getFixQuery()puts the engine straight out of a plugin's SQL into theCREATE TABLEit builds, so a MyISAM schema really does install as MyISAM. The acceptance helper mirrors that. On master the same schemas resolve to InnoDB and the helper mirrors that, with a unit test pinning the two copies together; there is no map here to pin to, so those assertions are replaced by ones describing what this line actually does.webdriversuite (it shipsacceptance,functionalandunit), so the four client-side defects ship here with no browser coverage. The source fix is identical to master's and is proven there byForumActionsCest. Better to say that plainly than to imply parity.Testing
Run on PHP 8.2, the newest this branch's CI covers. Every Cest carries positive controls, so a fix that simply refused everything would not pass either. The moderation, self-service, attachment and quick-reply exploits were each demonstrated against a running install before the tests that assert them were written.
Getting the forum into the suite also turned up a broken shared helper:
havePluginTables()matched only) ENGINE=<word>;, so for a plugin whose tables carryAUTO_INCREMENTafter the engine it silently created one table out of four and reported success. That is fixed with its own unit tests.