Under scenarios of extremely high load where large numbers of `Reviewable*` items are being created, it has been observed that multiple instances of the `NotifyReviewable` job may run simultaneously.
These jobs will work satisfactorily if the concurrency is limited to 1, and the different types of jobs (items reviewable by admins, vs moderators, vs particular groups, etc.) are run eventually.
This change introduces a new option to `DistributedMutex` which allows the `max_get_lock_attempts` to be specified. If the number is exceeded an error will be raised, which will cause Sidekiq to requeue the job. Sidekiq has existing logic to back-off on retry times for jobs that have failed multiple times.
The check used to be necessary because we validated the referrer too and
this bypass was a workaround a bug that is present in some browsers that
do not send the correct referrer.
When creating a group membership request, there is no character
limit on the 'reason' field. This can be potentially be used by
an attacker to create enormous amount of data in the database.
Co-authored-by: Ted Johansson <ted@discourse.org>
Currently we don’t have an association between reviewables and posts.
This sometimes leads to inconsistencies in the DB as a post can have
been deleted but an associated reviewable is still present.
This patch addresses this issue simply by adding a new association to
the `Post` model and by using the `dependent: :destroy` option.
This is just cleaning up a TODO I had to add more specs
to this controller -- there are more thorough tests on the
actual HashtagService class and the type-specific hashtag
classes.
When a user checks "Open all external links in a new tab" preference
he expects not to be overruled by unrelated text selections.
Yet if text is selected during a link click the link is followed on
the same tab. This change corrects that.
There was an issue where if hashtag-cooked HTML was sent
to the ExcerptParser without the keep_svg option, we would
end up with empty </use> and </svg> tags on the parts of the
excerpt where the hashtag was, in this case when a post
push notification was sent.
Fixed this, and also added a way to only display a plaintext
version of the hashtag for cases like this via PrettyText#excerpt.
We've had the UploadReference table for some time now in core,
but it was added after ChatUpload was and chat was just never
moved over to this new system.
This commit changes all chat code dealing with uploads to create/
update/delete/query UploadReference records instead of ChatUpload
records for consistency. At a later date we will drop the ChatUpload
table, but for now keeping it for data backup.
The migration + post migration are the same, we need both in case
any chat uploads are added/removed during deploy.
This commit allows us to set the channel slug when creating new chat
channels. As well as this, it introduces a new `SlugsController` which can
generate a slug using `Slug.for` and a name string for input. We call this
after the user finishes typing the channel name (debounced) and fill in
the autogenerated slug in the background, and update the slug input
placeholder.
This autogenerated slug is used by default, but if the user writes anything
else in the input it will be used instead.
Meta topic: https://meta.discourse.org/t/android-keyboard-overlaps-text-when-flagging-with-something-else/249687?u=osama
On Android, it's currently not possible to scroll modals that take input from the user (such as the flagging modal) when the keyboard is open which means that the keyboard can cover up part of the modal with no way for the user to see the covered part without closing the keyboard. This commit adds some CSS to make these modals scrollable when the keyboard is open.
In the group member bulk edit menu we are displaying staff-only options
to non-staff. The requests are blocked by the back-end, so there is no
harm other than to the user experience.
Notably the individual user edit menu is correctly filtering out
unavailable options. This change brings the bulk edit menu in line with
that.
When EmbeddableHost is configured for a specific category and that category is deleted, then EmbeddableHost should be deleted as well.
In addition, migration was added to fix existing data.
Currently, `Tag#topic_count` is a count of all regular topics regardless of whether the topic is in a read restricted category or not. As a result, any users can technically poll a sensitive tag to determine if a new topic is created in a category which the user has not excess to. We classify this as a minor leak in sensitive information.
The following changes are introduced in this commit:
1. Introduce `Tag#public_topic_count` which only count topics which have been tagged with a given tag in public categories.
2. Rename `Tag#topic_count` to `Tag#staff_topic_count` which counts the same way as `Tag#topic_count`. In other words, it counts all topics tagged with a given tag regardless of the category the topic is in. The rename is also done so that we indicate that this column contains sensitive information.
3. Change all previous spots which relied on `Topic#topic_count` to rely on `Tag.topic_column_count(guardian)` which will return the right "topic count" column to use based on the current scope.
4. Introduce `SiteSetting.include_secure_categories_in_tag_counts` site setting to allow site administrators to always display the tag topics count using `Tag#staff_topic_count` instead.
Prior to this change, we were parsing `Post#cooked` every time we
serialize a post to extract the usernames of mentioned users in the
post. However, the only reason we have to do this is to support
displaying a user's status beside each mention in a post on the client side when
the `enable_user_status` site setting is enabled. When
`enable_user_status` is disabled, we should avoid having to parse
`Post#cooked` since there is no point in doing so.
Our JS files reference sourcemaps relative to their current path. On sites with non-S3 CDN setups, we use a special path for brotli assets (39a524aa). This caused the sourcemap requests to 404.
This commit fixes the issue by allowing the `.map` files to be accessed under `/brotli_asset/*`.
This was previously disabled because of incompatibility with the ember-cli proxy. This commit fixes that incompatibility, and restores the development behaviour to match production.
There were three issues at play:
1. Our bootstrap-js addon handles the forwarding of most requests in the ember-cli proxy. This is not built to handle streaming responses. Solution: skip our custom request processing for `/message-bus/*` and use ember-cli's default `http-proxy`.
2. The request/response size-limiting middleware (`rawMiddleware`) would apply even to unhandled paths, causing request and response bodies to be buffered. Solution: skip it for any paths which are not handled by our custom addon.
3. Expressjs servers will buffer/compress responses. Solution: add `Cache-Control: no-transform` to message-bus responses. For now I've done this in development only, but it may be useful to add it to message-bus's default headers in future
When the `tags_listed_by_group` site setting is enabled, we were seeing
the N+1 queries problem when multiple `TagGroup` records are listed.
This commit fixes that by ensuring that we are not filtering through the
`tags` association after the association has been eager loaded.
So it can easily be overwritten in a plugin for example.
### Added more tests to provide better coverage
We previously only had `u.silenced_till IS NULL` but I made it consistent with pretty much every other places where we check for "active" users.
These two new lines do change the query a tiny bit though.
**Before**
- You could not get the badge if you were currently silenced (no matter what period is being checked)
- You could get the badge if you were suspended 😬
**After**
- You can't get the badge if you were silenced during the past year
- You can't get the badge if you were suspended during the past year
### Improved the performance of the query by using `NOT EXISTS` instead of `LEFT JOIN / COUNT() = 0`
There is no difference in behaviour between
```sql
LEFT JOIN user_badges AS ub ON ub.user_id = u.id AND ...
[...]
HAVING COUNT(ub.*) = 0
```
and
```sql
NOT EXISTS (SELECT 1 FROM user_badges AS ub WHERE ub.user_id = u.id AND ...)
```
The only difference is performance-wise. The `NOT EXISTS` is 10-30% faster on very large databases (aka. posts and users in X millions). I checked on 3 of the largest datasets I could find.
The `enable_new_notifications_menu` site setting allows sites that have
`navigation_menu` set to `legacy` to use the redesigned notifications
menu before switching to the new sidebar navigation menu.
Data Explorer queries have a `user_id` assigned to each query created. DE Reports can be bookmarked for later reference.
When creating the bookmark notification there was the possibility of a notification error being thrown (that made the notification menu inaccessible) due to a DE Query not having a owner (associated user_id). This can happen in a couple ways:
- having a query created by a user that was then later deleted leaving the query without ownership
- having a TA create a query for a customer using a temporary account, that would then later be deleted leaving the query without ownership
Since there is a case that `bookmark.user` is not valid the PR makes the `bookmark.user.username` optional for a bookmark notification. As [tested](https://github.com/discourse/discourse/pull/19851/files#diff-5b5154de37f96988d551feff6f1dfe5ba804fbcbc1c33b5478dde02a447a634f) in the case a username is not present, we will still render the `content` of the notification minus the username. This creates a safe fallback when looking up non-valid users.
This is a very subtle one. Setting the redirect URL is done by passing
a hash through a Discourse event. This is broken on Ruby 2 since the
support for keyword arguments in events was added.
In Ruby 2 the last argument is cast to keyword arguments if it is a
hash. The key point here is that creates a new copy of the hash, so
what the plugin is modifying is not the hash that was passed.
In "GlobalSetting.redirect_avatar_requests" mode, when the application gets
an avatar request it returns a "redirect" to the S3 CDN.
This shields the application from caching avatars and downloading from S3.
However clients will make 2 requests per avatar. (one to get redirect,
second to get avatar)
A one hour cache on a redirect means there may be an increase in CDN
traffic, given more clients will ask for the redirect every hour.
This may also lead to an increase in origin requests to the application.
To mitigate lets cache the CDN URL for 1 day.
The downside is that any changes to S3 CDN need extra care to allow for
the extra 1 day delay. (leave data around for 1 extra day)
0403cda1d1 introduced a regression where
topics in non read-restricted categories have its TopicTrackingState
MessageBus messages published with the `group_ids: [nil]` option. This
essentially means that no one would be able to view the message.
* Firefox now finally returns PerformanceMeasure from performance.measure
* Some TODOs were really more NOTE or FIXME material or no longer relevant
* retain_hours is not needed in ExternalUploadsManager, it doesn't seem like anywhere in the UI sends this as a param for uploads
* https://github.com/discourse/discourse/pull/18413 was merged so we can remove JS test workaround for settings
Currently when generating a onebox for Discourse topics, some important
context is missing such as categories and tags.
This patch addresses this issue by introducing a new onebox engine
dedicated to display this information when available. Indeed to get this
new information, categories and tags are exposed in the topic metadata
as opengraph tags.
When a topic belongs to category that is read restricted but permission
has not been granted to any groups, publishing ceratin topic tracking state
updates for the topic will result in the `MessageBus::InvalidMessageTarget` error being raised
because we're passing `nil` to `group_ids` which is not support by
MessageBus.
This commit ensures that for said category above, we will publish the
updates to the admin groups.
```
class Jobs::DummyDelayedJob < Jobs::Base
def execute(args = {})
end
end
RSpec.describe "Jobs.run_immediately!" do
before { Jobs.run_immediately! }
it "explodes" do
current_user = Fabricate(:user)
Jobs.enqueue_in(1.seconds, :dummy_delayed_job)
sign_in(current_user)
end
end
```
The test above will fail with the following error if `ActiveRecord::Base.connection_handler.clear_active_connections!` is called before the configured Capybara server checks out a connection from the connection pool.
```
ActiveRecord::ActiveRecordError:
Cannot expire connection, it is owned by a different thread: #<Thread:0x00007f437391df58@puma srv tp 001 /home/tgxworld/.asdf/installs/ruby/3.1.3/lib/ruby/gems/3.1.0/gems/puma-6.0.2/lib/puma/thread_pool.rb:106 sleep_forever>. Current thread: #<Thread:0x00007f437d6cfc60 run>.
```
We're not exactly sure if this is an ActiveRecord bug or not but we've
invested too much time into investigating this problem. Fundamentally,
we also no longer understand why `ActiveRecord::Base.connection_handler.clear_active_connections!` is being called in an ensure block
within `Jobs::Base#perform` which was added in
ceddb6e0da 10 years ago. This
commit moves the logic for running jobs immediately out of the
`Jobs::Base#perform` method into another `Jobs::Base#perform_immediately` method such that
`ActiveRecord::Base.connection_handler.clear_active_connections!` is not
called. This change will only impact the test environment.
These accidental inclusions are mostly no-ops (because the method name is also included as an explicit symbol). The mistakes were made more obvious because syntax_tree adjusted the indentation of these methods
There are various performance issues with the Canvas in iOS Safari
that are causing crashes when processing images with spikes of over 100%
CPU usage. The cause of this is unknown, but profiling points to
CanvasRenderingContext2D.getImageData() and
CanvasRenderingContext2D.drawImage().
Until Safari makes some progress with OffscreenCanvas or other
alternatives we cannot support this workflow. We will revisit in 6
months.
This is gated behind the hidden `composer_ios_media_optimisation_image_enabled`
site setting for people who really still want to try using this.
This change adds `target` to the set of attributes allowed by the
HTML sanitizer which is applied to the description of a user_field.
The rationale for this change:
* If one puts a link (<a>...</a>) in the description of a user_field
that is present and/or required at sign-up, the expectation is that
a prospective new user will click on that link during sign-up.
* Without an appropriate `target` attribute on the link, the new page
will be loaded in the same window/tab as the sign-up form, but this
will obliterate any fields that the user had already filled-out on
the form. (E.g., hitting the back-button will return to an
empty form.)
* Such UX behavior is incredibly aggravating to new users.
This change allows an admin to add a `target` attribute to links, to
instruct the browser to open them in a different window/tab, leaving
a sign-up form intact.
This commit introduces the experimental `registerUserCategorySectionLinkCountable`
and `refreshUserSidebarCategoriesSectionCounts` plugin APIs that allows
a plugin to register custom countables to category section links on top
of the defaults of unread and new.
Links to category settings were created using the category name. If the name was a single word, the link would be valid (regardless of capitalization).
For example, if the category was named `Awesome`
`/c/Awesome/edit/settings`
is a valid URL as that is a case-insensitive match for the category slug of `awesome`.
However, if the category had a space in it, the URL would be
`/c/Awesome%20Name/edit/settings`
which does not match the slug of `awesome-name`.
This change uses the category slug, rather than the name, which is the expected behaviour (see `Category.find_by_slug_path`).
