Commit Graph

27 Commits

Author SHA1 Message Date
Osama Sayegh
eaa3f813c1
FIX: Don't secure the about banner image ()
Uploads that are linked to site settings shouldn't be flagged as secure in login-required sites that enable secure uploads. However, in order for site setting uploads to not be marked secured, the frontend uploader has to include 2 params in the upload request: `for_site_setting: true` and `type: "site_setting"`.

Since these 2 params are semantically identical, we want the `type: "site_setting"` param alone to make the upload correctly treated as a site setting upload. To achieve that, we need to include the `site_setting` type in the public types list because the `for_site_setting` param has the same effect — it marks the upload as a public type.

b138eaf9e5/lib/upload_security.rb (L128-L131)
2024-11-25 11:12:00 +03:00
Martin Brennan
0d0dbd391a
DEV: Rename with_secure_uploads? to should_secure_uploads? on Post ()
This method name is a bit confusing; with_secure_uploads implies
it may return a block or something with the uploads of the post,
and has_secure_uploads implies that it's checking whether the post
is linked to any secure uploads.

should_secure_uploads? communicates the true intent of this method --
which is to say whether uploads attached to this post should be
secure or not.
2024-04-09 13:23:11 +10:00
Martin Brennan
c532f6eb3d
FEATURE: Secure uploads in PMs only ()
This adds a new secure_uploads_pm_only site setting. When secure_uploads
is true with this setting, only uploads created in PMs will be marked
secure; no uploads in secure categories will be marked as secure, and
the login_required site setting has no bearing on upload security
either.

This is meant to be a stopgap solution to prevent secure uploads
in a single place (private messages) for sensitive admin data exports.
Ideally we would want a more comprehensive way of saying that certain
upload types get secured which is a hybrid/mixed mode secure uploads,
but for now this will do the trick.
2023-09-06 09:39:09 +10:00
Martin Brennan
5ea89d1fcb
FIX: UploadReference order by tiebreaker for UploadSecurity ()
Follow up to 4d2a95ffe6. Sometimes
due to the original UploadReference migration or other issues,
multiple UploadReference records can have the exact same
created_at date and time. To tiebreak and correct the SQL order
when this happens, we can add a secondary `id ASC` ordering
when we check for the first upload reference.
2023-03-09 11:52:26 +10:00
Martin Brennan
110c96e6d7
FIX: Do not count deleted post for upload ref security ()
When checking whether an existing upload should be secure
based on upload references, do not count deleted posts, since
there is still a reference attached to them. This can lead to
issues where e.g. an upload is used for a post then later on
a custom emoji.
2023-01-24 10:01:48 +10:00
Martin Brennan
4d2a95ffe6
FIX: Query UploadReference in UploadSecurity for existing uploads ()
This fixes a longstanding issue for sites with the
secure_uploads setting enabled. What would happen is a scenario
like this, since we did not check all places an upload could be
linked to whenever we used UploadSecurity to check whether an
upload should be secure:

* Upload is created and used for site setting, set to secure: false
  since site setting uploads should not be secure. Let's say favicon
* Favicon for the site is used inside a post in a private category,
  e.g. via a Onebox
* We changed the secure status for the upload to true, since it's been
  used in a private category and we don't check if it's originator
  was a public place
* The site favicon breaks :'(

This was a source of constant consternation. Now, when an upload is _not_
being created, and we are checking if an existing upload should be
secure, we now check to see what the first record in the UploadReference
table is for that upload. If it's something public like a site setting,
then we will never change the upload to `secure`.
2023-01-20 10:24:52 +10:00
David Taylor
6417173082
DEV: Apply syntax_tree formatting to lib/* 2023-01-09 12:10:19 +00:00
Jan Cernik
08476f17ff
FEATURE: Add dark mode option for category logos ()
Adds a new upload field for a second dark mode category logo. 
This alternative will be used when the browser is in dark mode (similar to the global site setting for a dark logo).
2022-10-07 11:00:44 -04:00
Martin Brennan
8ebd5edd1e
DEV: Rename secure_media to secure_uploads ()
This commit renames all secure_media related settings to secure_uploads_* along with the associated functionality.

This is being done because "media" does not really cover it, we aren't just doing this for images and videos etc. but for all uploads in the site.

Additionally, in future we want to secure more types of uploads, and enable a kind of "mixed mode" where some uploads are secure and some are not, so keeping media in the name is just confusing.

This also keeps compatibility with the `secure-media-uploads` path, and changes new
secure URLs to be `secure-uploads`.

Deprecated settings:

* secure_media -> secure_uploads
* secure_media_allow_embed_images_in_emails -> secure_uploads_allow_embed_images_in_emails
* secure_media_max_email_embed_image_size_kb -> secure_uploads_max_email_embed_image_size_kb
2022-09-29 09:24:33 +10:00
Martin Brennan
faf5b4d3e9
PERF: Speed up secure media and ACL sync rake tasks ()
Incorporates learnings from /t/64227:

* Changes the code to set access control posts in the rake
  task to be an efficient UPDATE SQL query.
  The original version was timing out with 312017 post uploads,
  the new query took ~3s to run.
* Changes the code to mark uploads as secure/not secure in
  the rake task to be an efficient UPDATE SQL query rather than
  using UploadSecurity. This took a very long time previously,
  and now takes only a few seconds.
* Spread out ACL syncing for uploads into jobs with batches of
  100 uploads at a time, so they can be parallelized instead
  of having to wait ~1.25 seconds for each ACL to be changed
  in S3 serially.

One issue that still remains is post rebaking. Doing this serially
is painfully slow. We have a way to do this in sidekiq via PeriodicalUpdates
but this is limited by max_old_rebakes_per_15_minutes. It would
be better to fan this rebaking out into jobs like we did for the
ACL sync, but that should be done in another PR.
2022-05-23 13:14:11 +10:00
Martin Brennan
501de809da
FIX: Do not mark badge image uploads as secure ()
* FIX: Do not mark badge image uploads as secure

We do not need badge_image upload types to be marked as secure.
Post migration is the same as
https://github.com/discourse/discourse/pull/12081.

See
https://meta.discourse.org/t/secure-media-uploads/140017/122?u=martin
2021-05-28 12:35:52 +10:00
Martin Brennan
9f0f801ae3
FIX: Do not mark group_flair images as secure on upload ()
See https://meta.discourse.org/t/secure-media-uploads-breaks-group-flair-image/173671/4

Group flair image uploads definitely do not need to be secure.
2021-02-16 12:34:03 +10:00
Martin Brennan
f49e3e5731
DEV: Add security_last_changed_at and security_last_changed_reason to uploads ()
This PR adds security_last_changed_at and security_last_changed_reason to uploads. This has been done to make it easier to track down why an upload's secure column has changed and when. This necessitated a refactor of the UploadSecurity class to provide reasons why the upload security would have changed.

As well as this, a source is now provided from the location which called for the upload's security status to be updated as they are several (e.g. post creator, topic security updater, rake tasks, manual change).
2021-01-29 09:03:44 +10:00
Penar Musaraj
c664ba8fa8
DEV: Fix flaky UploadSecurity spec 2020-09-18 14:36:41 -04:00
Martin Brennan
14b324e5ed
FIX: Provide better API for registering custom upload public types ()
With secure media and the UploadSecurity class, we need a nice way for plugins to register custom upload types that should be considered public and never secure.
2020-09-18 11:54:33 +10:00
Martin Brennan
e8a842ab8c
FIX: Stop category logo + background being marked secure ()
Meta topic: https://meta.discourse.org/t/secure-media-uploads-breaks-category-logos/161693

Category backgrounds and logos are public uploads and should not be marked as secure.

I also discovered that a lot of the UploadSecurity specs for public types were returning false positives; this has been fixed.
2020-08-24 17:12:28 +10:00
Martin Brennan
36841210fb Move UploadSecurity public types into PUBLIC_TYPES constant
* this is so plugins can extend the public types if required
2020-06-05 10:56:57 +10:00
Martin Brennan
097851c135
FIX: Change secure media to encompass attachments as well ()
If the “secure media” site setting is enabled then ALL files uploaded to Discourse (images, video, audio, pdf, txt, zip etc. etc.) will follow the secure media rules. The “prevent anons from downloading files” setting will no longer have any bearing on upload security. Basically, the feature will more appropriately be called “secure uploads” instead of “secure media”.

This is being done because there are communities out there that would like all attachments and media to be secure based on category rules but still allow anonymous users to download attachments in public places, which is not possible in the current arrangement.
2020-03-26 07:16:02 +10:00
Martin Brennan
0388653a4d
DEV: Upload and secure media retroactive rake task improvements ()
* Add uploads:sync_s3_acls rake task to ensure the ACLs in S3 are the correct (public-read or private) setting based on upload security

* Improved uploads:disable_secure_media to be more efficient and provide better messages to the user.

* Rename uploads:ensure_correct_acl task to uploads:secure_upload_analyse_and_update as it does more than check the ACL

* Many improvements to uploads:secure_upload_analyse_and_update

* Make sure that upload.access_control_post is unscoped so deleted posts are still fetched, because they still affect the security of the upload.

* Add escape hatch for capture_stdout in the form of RAILS_ENABLE_TEST_STDOUT. If provided the capture_stdout code will be ignored, so you can see the output if you need.
2020-03-03 10:03:58 +11:00
Martin Brennan
04df3bd46d
FIX: Only mark attachments as secure media if SiteSetting.secure_media? ()
* Attachments (non media files) were being marked as secure if just
SiteSetting.prevent_anons_from_downloading_files was enabled. this
was not correct as nothing should be marked as actually "secure" in
the DB without that site setting enabled
* Also add a proper standalone spec file for the upload security class
2020-02-21 09:35:16 +10:00
Martin Brennan
5dc6100acc Work around deleted upload.access_control_post scoping issue
* TODO to come back and deal with this in a better way, it
  just needs to be done for the rake task to set secure media
  to work correctly
2020-02-20 10:57:40 +10:00
Martin Brennan
e8efdd60d4
FIX: Tweak upload security emoji check ()
Further on from my earlier PR  also reject upload as secure if its origin URL contains images/emoji. We still check Emoji.all first to try and be canonical.

This may be a little heavy handed (e.g. if an external URL followed this same path it would be a false positive), but there are a lot of emoji aliases where the actual Emoji url is something, but you can have another image that should not be secure that that thing is an alias for. For example slight_smile.png does not show up in Emoji.all BUT slightly_smiling_face does, and it aliases slight_smile e.g. /images/emoji/twitter/slight_smile.png?v=9 and /images/emoji/twitter/slightly_smiling_face.png?v=9 are equivalent.
2020-02-17 15:11:15 +10:00
Martin Brennan
dac923379a
FIX: Never mark uploads based on regular emoji secure ()
Sometimes PullHotlinkedImages pulls down a site emoji and creates a new upload record for it. In the cases where these happen the upload is not created via the normal path that custom emoji follows, so we need to check in UploadSecurity whether the origin of the upload is based on a regular site emoji. If it is we never want to mark it as secure (we don't want emoji not accessible from other posts because of secure media).

This only became apparent because the uploads:ensure_correct_acl rake task uses UploadSecurity to check whether an upload should be secure, which would have marked a whole bunch of regular-old-emojis as secure.
2020-02-17 12:30:47 +10:00
Martin Brennan
56b16bc68e
FIX: Never allow custom emoji to be marked secure ()
* Because custom emoji count as post "uploads" we were
marking them as secure when updating the secure status for post uploads.
* We were also giving them an access control post id, which meant
broken image previews from 403 errors in the admin custom emoji list.
* We now check if an upload is used as a custom emoji and do not
assign the access control post + never mark as secure.
2020-02-14 11:17:09 +10:00
Martin Brennan
1b3b0708c0
FEATURE: Update upload security status on post move, topic conversion, category change ()
Add TopicUploadSecurityManager to handle post moves. When a post moves around or a topic changes between categories and public/private message status the uploads connected to posts in the topic need to have their secure status updated, depending on the security context the topic now lives in.
2020-01-23 12:01:10 +10:00
Martin Brennan
2583aedd42
FIX: Change additional public uploads to not be secure ()
Custom emoji, profile background, and card background were being set to secure, which we do not want as they are always in a public context and result in a 403 error from the ACL if linked directly.
2020-01-17 13:16:27 +10:00
Martin Brennan
7c32411881
FEATURE: Secure media allowing duplicated uploads with category-level privacy and post-based access rules ()
### General Changes and Duplication

* We now consider a post `with_secure_media?` if it is in a read-restricted category.
* When uploading we now set an upload's secure status straight away.
* When uploading if `SiteSetting.secure_media` is enabled, we do not check to see if the upload already exists using the `sha1` digest of the upload. The `sha1` column of the upload is filled with a `SecureRandom.hex(20)` value which is the same length as `Upload::SHA1_LENGTH`. The `original_sha1` column is filled with the _real_ sha1 digest of the file. 
* Whether an upload `should_be_secure?` is now determined by whether the `access_control_post` is `with_secure_media?` (if there is no access control post then we leave the secure status as is).
* When serializing the upload, we now cook the URL if the upload is secure. This is so it shows up correctly in the composer preview, because we set secure status on upload.

### Viewing Secure Media

* The secure-media-upload URL will take the post that the upload is attached to into account via `Guardian.can_see?` for access permissions
* If there is no `access_control_post` then we just deliver the media. This should be a rare occurrance and shouldn't cause issues as the `access_control_post` is set when `link_post_uploads` is called via `CookedPostProcessor`

### Removed

We no longer do any of these because we do not reuse uploads by sha1 if secure media is enabled.

* We no longer have a way to prevent cross-posting of a secure upload from a private context to a public context.
* We no longer have to set `secure: false` for uploads when uploading for a theme component.
2020-01-16 13:50:27 +10:00