Jump to content

Colonel_mortis

Clients
  • Posts

    1,451
  • Joined

  • Last visited

  • Days Won

    5

Reputation Activity

  1. Like
    Colonel_mortis got a reaction from DawPi in users that are mod queued can edit topics without approval   
    If a user has content moderation enabled, any new topics or posts need to be approved by a moderator, and any edits to existing non-topic-startong posts. However, editing the topic's original post does not need to be approved.
    Especially when used to combat spam, this is a pretty big loophole.
    To reproduce:
    * Enable content moderation for a user
    * Have that user post a new topic - moderator approval will be required
    * Have a moderator approve the topic
    * Have the user go back and edit that post - moderator approval will incorrectly not be required
  2. Thanks
    Colonel_mortis reacted to teraßyte in Breaking changes in minor releases   
    Yup, it's been mentioned on the forum already, and while IPS did list the change in the changelog, it wasn't clear at all what the removal would actually entail:
     
    See this topic:
     
  3. Like
    Colonel_mortis got a reaction from DawPi in PHP OOMing while computing authorsPostedIn   
    FastCGI sent in stderr: "PHP message: PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 10485760 bytes) in [...]/system/Node/Statistics.php on line 118" while reading response header from upstream, client: [], server: [], request: "GET /forum/17-[...]/?sortby=views&sortdirection=desc HTTP/2.0" That's in authorsPostedIn. It looks like there's two places where that gets called - computing the content that the current user has posted in, and computing which postedIn indicator to show.
    The query that gets run for this will load about 260k entries (there are a bunch of topics with a very large range of contributors). I'm surprised that's enough to OOM PHP with 128M memory, but it seems to be the case.
    For computing content that the current user has posted in, you can do a way more efficient query by filtering by user.
    For computing the postedIn indicator, you could just skip it entirely on installs where that feature isn't used, but also I think you could make it much more efficient by joining the members in the query to just end up with the group IDs (though that query is probably a bit non-trivial).
  4. Agree
    Colonel_mortis got a reaction from Princess Celestia in Controllers can be initialized on detatched DOM nodes if another controller uses cleanContentsOf   
    The symptom of this for regular status updates is that when you submit the status, it gets stuck on "Saving", despite having submitted successfully:

  5. Agree
    Colonel_mortis got a reaction from Princess Celestia in Controllers can be initialized on detatched DOM nodes if another controller uses cleanContentsOf   
    If you have something like
    <div data-controller="foo"> <div data-controller="bar"></div> </div> ips.controller.register("foo", { initialize: function() { ips.controller.cleanContentsOf(this.scope); } }); The controller on bar will be initialized on the dead element, and never cleaned up:
    ips.controller.register("bar", { initialize: function() { console.log(document.contains(this.scope[0])); // => false }, destroy: function() { console.log("destroyed"); // never called } }); This happens because ips.controller#_findControllers is called once to get all controllers to load, then the controllers are run one by one. If the foo controller is run first (and I'm not sure if that's guaranteed to be the case), cleanContentsOf will drop the bar element, but it won't run the destructor because the controller hasn't been initialized yet. When all of that logic has finished running, it then runs the bar controller, even though it is no longer applicable.
    Sure, you might say, but nobody would call cleanContentsOf synchronously during init, right? Yes, you do.
    Concretely, the problem this causes me is that the commentFeed of a status update exists twice, one in the document and one detatched. This means addToCommentFeed events end up getting processed by both, the detached one first, and the detached one ends up throwing an exception.
    There are a few different ways this could be fixed:
    Fix the double initialization of profiles. Seems straight forward enough, and that appears to be the only instance of this issue at the moment. Guard ips.controller#initControllerOnElem with node.contains(elem), to ensure that the node is still a child Run the destructor code from cleanContentsOf in a `setImmediate` block, to give the other controllers an opportunity to be initialized before getting cleaned up
  6. Agree
    Colonel_mortis got a reaction from Princess Celestia in Profiles reload when first opened   
    To reproduce:
    Click https://invisioncommunity.com/profile/536475-colonel_mortis/ (note that this will not occur again if you refresh the page or use the browser back/forwards buttons - you need to actually click the link to trigger it) Observe that the page shows up for a moment, then changes to a loading spinner, before finally loading again a few seconds later. All of the page data was reloaded from the server in the process. This can be fairly jarring, especially for users on slower connections.
    This is because ips.profile.main.js calls History.replaceState from setup. setup is called from initialize after the statechange event handler has already been bound. I'm not sure why you're calling History.replaceState there, but it seems like it should be safe to reorder initialize so you call setup before adding the statechange event handler.
  7. Thanks
    Colonel_mortis reacted to teraßyte in [BUG 4.7.7] New Images tab in profile always shows even if there's no content   
    As mentioned in the topic's title, in 4.4.7 with the new gallery version includes a new IMAGES profile extension when viewing a member profile. However, unlike other tabs (for example the ALBUMS TAB in the same gallery application) it doesn't check if the user has uploaded any images before deciding if it should be shown or not.
     
    This is the code inside /applications/gallery/extensions/core/Profile/galleryImages.php:
    public function showTab(): bool { return TRUE; } The ALBUMS profile extension in the same folder (gallery.php) does check if the user has any albums before showing the profile's tab.
  8. Like
    Colonel_mortis got a reaction from SeNioR- in Uncaught exception when deleting guest content from pages   
    The background task to delete guest content from pages is spewing uncaught exceptions
    SELECT * FROM `cms_custom_database_1` WHERE record_author_name='<redacted>' ORDER BY primary_id_field LIMIT 0,250 Unknown column 'record_author_name' in 'where clause' #0 /opt/forum/system/Db/Select.php(388): IPS\_Db->preparedQuery() #1 /opt/forum/system/Db/Select.php(446): IPS\Db\_Select->runQuery() #2 [internal function]: IPS\Db\_Select->rewind() #3 /opt/forum/applications/core/extensions/core/Queue/MemberContent.php(93): IteratorIterator->rewind() #4 /opt/forum/system/Task/Task.php(47): IPS\core\extensions\core\Queue\_MemberContent->run() #5 /opt/forum/applications/core/tasks/queue.php(43): IPS\_Task::runQueue() #6 /opt/forum/system/Task/Task.php(375): IPS\core\tasks\_queue->IPS\core\tasks\{closure}() #7 /opt/forum/applications/core/tasks/queue.php(38): IPS\_Task->runUntilTimeout() #8 /opt/forum/system/Task/Task.php(274): IPS\core\tasks\_queue->execute() #9 /opt/forum/system/Task/Task.php(237): IPS\_Task->run() #10 /opt/forum/applications/core/interface/task/task.php(58): IPS\_Task->runAndLog() #11 {main}  
  9. Agree
    Colonel_mortis got a reaction from teraßyte in Direct links to alerts   
    For moderator bookkeeping and discussion it would be helpful if you could direct link to a specific alert within modcp (to a page containing just that alert). Right now the only way to view an alert as a moderator is to scroll through the list of all of them, which is not useful when referencing it elsewhere.
  10. Like
    Colonel_mortis got a reaction from SeNioR- in Direct links to alerts   
    For moderator bookkeeping and discussion it would be helpful if you could direct link to a specific alert within modcp (to a page containing just that alert). Right now the only way to view an alert as a moderator is to scroll through the list of all of them, which is not useful when referencing it elsewhere.
  11. Like
    Colonel_mortis got a reaction from Daniel F in Direct links to alerts   
    For moderator bookkeeping and discussion it would be helpful if you could direct link to a specific alert within modcp (to a page containing just that alert). Right now the only way to view an alert as a moderator is to scroll through the list of all of them, which is not useful when referencing it elsewhere.
  12. Like
    Colonel_mortis got a reaction from SeNioR- in Editing posts breaks filestore-agnostic URLs   
    To reproduce:
    (Prerequisites: lazy loading is enabled (on by default), and the default disk storage location is used for attachments) Attach an image to a post Look at that post in the DB - it should look something like <img class="ipsImage ipsImage_thumbnailed" data-fileid="1" data-ratio="75.00" width="640" alt="foo.jpg" data-src="/monthly_2022_10/foo.jpg" src="/applications/core/interface/js/spacer.png" /> Edit the post (don't need to change anything)
    Look at the post again in the DB - it should now look something like <img alt="foo.jpg" class="ipsImage ipsImage_thumbnailed" data-fileid="1" data-ratio="75.00" width="640" data-src="/uploads/monthly_2022_10/foo.jpg" src="/applications/core/interface/js/spacer.png" /> Notice that the data-src has changed from /monthly_2022_10/foo.jpg to /uploads/monthly_2022_10/foo.jpg, so it is no longer agnostic of the backing file store Moving the attachments filestore will now cause these images to point to the wrong place.
    This is because in \IPS\Text\Parser::_parseImgElement, you have
    /* Lazy-loaded? */ $srcAttribute = ( $element->hasAttribute( 'src' ) AND $element->hasAttribute( 'data-src' ) ) ? 'data-src' : 'src'; // ... /* Or an attachment? */ elseif ( $attachment = static::_getAttachment( $element->getAttribute($srcAttribute ), $element->hasAttribute('data-fileid') ? $element->getAttribute('data-fileid') : NULL ) ) { $file = $this->_getFile( 'core_Attachment', $element->getAttribute( $srcAttribute ) ); $element->setAttribute( 'data-fileid', $attachment['attach_id'] ); $element->setAttribute( 'src', str_replace( array( 'http:', 'https:' ), '', str_replace( static::$fileObjectClasses['core_Attachment']->baseUrl(), '{fileStore.core_Attachment}', $element->getAttribute( $srcAttribute ) ) ) ); the last line there is setting the 'src' attribute, when in fact it should be setting $srcAttribute. The src attribute is subsequently discarded to replace with the lazy load placeholder.
    This works for new posts because $srcAttribute is 'src' there, but when editing posts (with lazy loading enabled) it is actually data-src.
  13. Agree
    Colonel_mortis got a reaction from Martin A. in Add "not banned" as a filter in the ACP members table   
    You can filter the table by banned (and validating, admin, etc), but not non-banned. This would be really useful when searching for spammers/bad users that match a certain pattern, where most of them have already been caught but you're looking for the ones that have slipped through the gaps.
  14. Agree
    Colonel_mortis got a reaction from teraßyte in Web push notifications have incorrect numbers   
    Firefox, Linux

    When this notification was sent, there was only one new unread post in the topic, and I had no prior unread notifications (as judged by in-platform notifications). I'm a bit confused about where 6 has come from - it is less than the number of other members' replies to this topic (which I have been following since creation), but more than the number of replies since I last rebooted this computer.
    I usually don't clear my system notifications (the system notification list that these push notifications are delivered to), but I had manually cleared them prior to receiving this to see if that makes any difference. It didn't (or at least it didn't solve it, perhaps it would otherwise have come up with a different number?).
    I'm pretty sure this is something to do with the self.registration.getNotifications and unseenCount stuff in serviceWorker.js, but I'm not quite sure the semantics here (I assumed that clearing the notification list, or restarting my computer, would close all the open notifications).
    The correct behaviour here, in my mind, is that the numbers here should be no greater than the in-platform notification list - if there have been 3 notifications about replies to a given topic since I last read my in-platform notifications, the number that is shown here should represent the number of notifications about replies that are unread in both the in-platform notifications and push notifications (at most 3, but could be lower if I dismissed some native notifications). There are definitely alternative approaches that would make sense too, but it's pretty clear to me that the current approach is not correct.
    Maybe this is just a Firefox+Linux issue, I haven't tried reproing on any other platforms yet.
  15. Like
    Colonel_mortis got a reaction from SeNioR- in CSRF key left in URL when converting product to subscription   
    http://localhost/ips/admin/?app=nexus&module=subscriptions&controller=subscriptions&do=convertToSubscription&id=1&csrfKey=3ed05127f5b65a77dcf8987de5da8e5c causes a whoops error because the CSRF key is left in (and is required).
  16. Agree
    Colonel_mortis got a reaction from Afrodude in Make approval queue reasons hookable   
    I have my own hooks to add additional spam filter rules, and I imagine I'm not alone. I want them to be able to take advantage of showing the reason that the post got mod queued, but there's no easy way to do that, because the post ID is only available after it's saved, and there's no suitable hook point in \IPS\Content::checkProfanityFilters between saving and constructing the approval entry.
    It would make me much happier if you replaced the switch statement on line 773 with either:
    A method call that we can hook Or fall back to setting $log->held_data = ["match" => $filtersMatched["match"]] Or just always do that, and drop the logic to choose between the cases entirely You already made Approval itself hookable by pulling availableReasons out into a method (though why it's necessary to have it at all is a bit confusing to me), so this feels like an unfortunate oversight.
    I believe the image hold log in \IPS\Content::shouldTriggerProfanityFilters on line 639-643 is also going to hit the issue with the post ID not being available yet, but I obviously don't pay enough to be able to test that. The changes suggested here should make that code easier to unify with the rest of the cases.
  17. Agree
    Colonel_mortis got a reaction from teraßyte in Make approval queue reasons hookable   
    I have my own hooks to add additional spam filter rules, and I imagine I'm not alone. I want them to be able to take advantage of showing the reason that the post got mod queued, but there's no easy way to do that, because the post ID is only available after it's saved, and there's no suitable hook point in \IPS\Content::checkProfanityFilters between saving and constructing the approval entry.
    It would make me much happier if you replaced the switch statement on line 773 with either:
    A method call that we can hook Or fall back to setting $log->held_data = ["match" => $filtersMatched["match"]] Or just always do that, and drop the logic to choose between the cases entirely You already made Approval itself hookable by pulling availableReasons out into a method (though why it's necessary to have it at all is a bit confusing to me), so this feels like an unfortunate oversight.
    I believe the image hold log in \IPS\Content::shouldTriggerProfanityFilters on line 639-643 is also going to hit the issue with the post ID not being available yet, but I obviously don't pay enough to be able to test that. The changes suggested here should make that code easier to unify with the rest of the cases.
  18. Like
    Colonel_mortis got a reaction from SeNioR- in Uncaught exception when reporting content if there are restricted moderators   
    To reproduce:
    Create a moderator with the permissions set to "Restricted", but leave everything checked (the thing that actually matters here is that the "All" checkbox is enabled in the forum selector in the forums tab) (From any user) attempt to report a forum post Observe that you get redirected to an error saying "You have already reported that.", and that the original request returned 500 Stack trace:
    TypeError: in_array(): Argument #2 ($haystack) must be of type array, int given (0) #0 /opt/forum/system/Content/Content.php(1714): in_array() #1 /opt/forum/init.php(940) : eval()'d code(62): IPS\_Content->report() #2 /opt/forum/system/Content/Controller.php(3001): IPS\moderation_hook_Content->report() #3 /opt/forum/system/Content/Controller.php(2271): IPS\Content\_Controller->_report() #4 /opt/forum/applications/forums/modules/front/forums/topic.php(1240): IPS\Content\_Controller->__call() #5 /opt/forum/system/Dispatcher/Controller.php(107): IPS\forums\modules\front\forums\_topic->__call() #6 /opt/forum/system/Content/Controller.php(50): IPS\Dispatcher\_Controller->execute() #7 /opt/forum/applications/forums/modules/front/forums/topic.php(39): IPS\Content\_Controller->execute() #8 /opt/forum/system/Dispatcher/Dispatcher.php(153): IPS\forums\modules\front\forums\_topic->execute() #9 /opt/forum/index.php(13): IPS\_Dispatcher->run() #10 {main} The buggy code is \IPS\Content::report, L1710-1719:
    if ( $canView === TRUE and $container = $item->containerWrapper() and isset( $container::$modPerm ) ) { if ( isset( $perms[ $container::$modPerm ] ) and $perms[ $container::$modPerm ] != '*' ) { if ( ! in_array( $item->mapped('container'), $perms[ $container::$modPerm ] ) ) { $canView = FALSE; } } } The tag for a moderator with no forum restrictions is -1, not '*':
    if ( isset( $perms[ $container::$modPerm ] ) and $perms[ $container::$modPerm ] != -1 )  
  19. Like
    Colonel_mortis got a reaction from SeNioR- in Events - Error: Call to a member function format() on null   
    Leads to

    It had originally been failing to load the whole page, but I think that's because I had designer mode enabled.
  20. Like
    Colonel_mortis got a reaction from Afrodude in Events - Error: Call to a member function format() on null   
    Error: Call to a member function format() on null (0) #0 /var/www/html/ips-themes/system/Patterns/ActiveRecord.php(335): IPS\calendar\_Event->get__happening() #1 /var/www/html/ips-themes/system/Theme/Theme.php(4620) : eval()'d code(36): IPS\Patterns\_ActiveRecord->__get() #2 /var/www/html/ips-themes/system/Theme/Dev/Template.php(171): IPS\Theme\theme_calendar_front_view_coverPhotoOverlay() #3 /var/www/html/ips-themes/applications/calendar/sources/Event/Event.php(2309): IPS\Theme\Dev\_Template->__call() #4 /var/www/html/ips-themes/applications/calendar/modules/front/calendar/event.php(166): IPS\calendar\_Event->coverPhoto() #5 /var/www/html/ips-themes/system/Dispatcher/Controller.php(118): IPS\calendar\modules\front\calendar\_event->manage() #6 /var/www/html/ips-themes/system/Content/Controller.php(50): IPS\Dispatcher\_Controller->execute() #7 /var/www/html/ips-themes/applications/calendar/modules/front/calendar/event.php(64): IPS\Content\_Controller->execute() #8 /var/www/html/ips-themes/system/Dispatcher/Dispatcher.php(153): IPS\calendar\modules\front\calendar\_event->execute() #9 /var/www/html/ips-themes/index.php(13): IPS\_Dispatcher->run() #10 {main} To repro:
    Create an event that repeats every 6 years, with a start date in 2019 Try to view that event This is because Event::nextOccurrence only searches within a couple of years, and returns null if it goes out of range, but get__happening doesn't handle null.
  21. Like
    Colonel_mortis got a reaction from SeNioR- in Hiding a topic doesn't clear notifications that are still queued   
    When you hide a topic, the notifications relating to that topic are deleted. However, if there are notifications still queued to be sent (eg because there were a large number of followers), those notifications still end up getting sent, so users receive a notification about a post that they can't see, and the notification doesn't ever get deleted.
  22. Like
    Colonel_mortis got a reaction from SeNioR- in Allow members to turn off quote notifications for a specific post   
    Sometimes when a topic is becoming fairly heated, members want to step back and stop replying. However, when they get quoted, they frequently can't resist coming back to continue the argument.
    It would be really helpful for those users if they could choose to stop receiving quote (and perhaps mention, although that's less prevalent) notifications for a specific post or topic, to help with their self-control.
  23. Like
    Colonel_mortis got a reaction from SeNioR- in Upgrader doesn't add all the relevant event columns to the events table   
    When databaseCheck is run as part of the 4.7.2 -> 4.7.3 upgrade, it ends up running additional queries to fix the schema, because they weren't included as queries by the upgrader:
    ALTER TABLE `calendar_events` ADD COLUMN `event_external_url` VARCHAR (255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL , ADD COLUMN `event_latitude` DECIMAL (10,8) NULL , ADD COLUMN `event_longitude` DECIMAL (11,8) NULL ; This doesn't cause issues because it was caught by the databaseCheck, but as with the previous time that something like this happened:
    this could cause problems in future.
  24. Like
    Colonel_mortis got a reaction from OptimusBain in Allow members to turn off quote notifications for a specific post   
    Sometimes when a topic is becoming fairly heated, members want to step back and stop replying. However, when they get quoted, they frequently can't resist coming back to continue the argument.
    It would be really helpful for those users if they could choose to stop receiving quote (and perhaps mention, although that's less prevalent) notifications for a specific post or topic, to help with their self-control.
  25. Like
    Colonel_mortis got a reaction from Afrodude in Hiding a topic doesn't clear notifications that are still queued   
    When you hide a topic, the notifications relating to that topic are deleted. However, if there are notifications still queued to be sent (eg because there were a large number of followers), those notifications still end up getting sent, so users receive a notification about a post that they can't see, and the notification doesn't ever get deleted.
×
×
  • Create New...