### Background
Several call sites use `FileStore#download` (through `Discourse.store.download`). In some cases the author seems aware that the method can raise an error if the download fails, and in some cases not. Because of this we're seeing some of these exceptions bubble all the way up and getting logged in production. Although they are not really actionable at that point. Rather each call site needs to be considered to figure out how to handle them.
### What is this change?
This change accomplishes primarily two things.
Firstly it separates the method into a safe version which will handle errors by returning `nil`, and an unsafe version which will re-package upstream errors in a new `FileStore::DownloadError` class.
Secondly it updates the call sites which have been doing error handling downstream to use the new safe version.
For backwards compatibility, there's an interim situation and a desired end state.
**Interim:**
```
FileStore#download → Old unsafe version. Will raise any error and show a deprecation warning.
FileStore#download! → New unsafe version. Will raise FileStore::DownloadError.
FileStore#download_safe → New safe version. Will return nil.
```
**Desired end-state:**
```
FileStore#download → New safe version. Will return nil.
FileStore#download! → New unsafe version. Will raise FileStore::DownloadError.
```
### What's next?
We need to do a quick audit of the call sites that are using the old unsafe version without any error handling, as well as check for call sites in plugins other repos. Follow-up PRs incoming.
We don't need these, they are causing a lot of
log noise on our servers, they have been removed
from the main branch from some time and it is
doubtful that anyone else needs to be told these
warnings on stable.
This was inadvertently removed in 4c46c7e. In very specific scenarios,
this could be used execute arbitrary JavaScript.
Only affects instances where SVGs are allowed as uploads and CDN is not
configured.
This is a backport of 84e13e9.
We caught it in logs, race condition led to this error:
ActiveRecord::RecordNotUnique
(PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "user_statuses_pkey"
DETAIL: Key (user_id)=(15) already exists.)
The reason the problem happened was that we were checking if a user has status and if not inserting status:
if user_status
...
else
self.user_status = UserStatus.create!(status)
end
The problem is that it's possible that another request will insert status just after we check if status exists and just before our request call `UserStatus.create!(status)`. Using `upsert` fixes the problem because under the hood `upsert` generates the only SQL request that uses "INSERT ... ON CONFLICT DO UPDATE". So we do everything in one SQL query, and that query takes care of resolving possible conflicts.
Currently `Topic#pm_topic_count` is a count of all personal messages tagged for a given tag. As a result, any user with access to PM tags can poll a sensitive tag to determine if a new personal message has been created using that tag even if the user does not have access to the personal message. We classify this as a minor leak in sensitive information.
With this commit, `Topic#pm_topic_count` is hidden from users by default unless the `display_personal_messages_tag_counts` site setting is enabled.
The presence service would retry `/presence/update` requests every second (or immediately in tests) in case where server returns 429 (rate limit) errors. That could lead to infinite spamming (until user refreshed tab/tabs)
Co-authored-by: David Taylor <david@taylorhq.com>
Our fork was needed for OpenSSL 3 and Ruby 2.X compatibility.
The OpenSSL 3 part was merged into the gem for version 3.
Discourse dropped support for Ruby 2.X.
That means we don't need our fork anymore.
Co-authored-by: Rafael dos Santos Silva <xfalcox@gmail.com>
The `git` version in our discourse_test docker image was recently updated to include a permissions check before running any git commands. For this to pass, the owner of the discourse directory needs to match the user running any git commands.
Under GitHub actions, by default the working directory is created with uid=1000 as the owner. We run all our tests as `root`, so this mismatch causes git to raise the permissions error. We can't switch to run the entire workflow as the `discourse (uid=1000)` user because our discourse_test image is not configured to allow `discourse` access to postgres/redis directories. For now, this commit updates the working directory's owner to match the user running the workflow.
Under some situations, we would inadvertently return a public (unauthenticated) result to an authenticated API request. This commit adds the `Api-Key` header to our anonymous cache bypass logic.
