FIX: Secure upload post processing race condition #23968
Merged
Conversation
This file contains 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
martin-brennan
commented
Oct 18, 2023
| @@ -1084,8 +1080,8 @@ def link_post_uploads(fragments: nil) | |||
| end | |||
|
|
|||
| def update_uploads_secure_status(source:) | |||
| if Discourse.store.external? && SiteSetting.secure_uploads? | |||
There was a problem hiding this comment.
Doesn't make a difference if secure_uploads is not enabled -- since if that happens a post's uploads could change from secure -> not secure
martin-brennan
force-pushed
the
issue/post-process-secure-upload-status-race-condition
branch
3 times, most recently
from
74303bd
to
ffb22b6
Compare
martin-brennan
commented
Oct 18, 2023
| @@ -629,7 +631,14 @@ def stub_deliver_response(message) | |||
| expect(message.to_s.scan(/cid:[\w\-@.]+/).uniq.length).to eq(2) | |||
| end | |||
|
|
|||
| it "does not attach images that are not marked as secure" do | |||
| it "does not attach images that are not marked as secure, in the case of a non-secure upload copied to a PM" do | |||
There was a problem hiding this comment.
CookedPostProcessor always updates upload security now, so the original way this was tested didn't work. Only in the case of an upload copied from a public post into a PM/secure category would we end up with a mix of secure/non-secure images in a post.
This commit fixes a couple of issues. A little background -- when uploads are created in the composer for posts, regardless of whether the upload will eventually be marked secure or not, if secure_uploads is enabled we always mark the upload secure at first. This is so the upload is by default protected, regardless of post type (regular or PM) or category. This was causing issues in some rare occasions though because of the order of operations of our post creation and processing pipeline. When creating a post, we enqueue a sidekiq job to post-process the post which does various things including converting images to lightboxes. We were also enqueuing a job to update the secure status for all uploads in that post. Sometimes the secure status job would run before the post process job, marking uploads as _not secure_ in the background and changing their ACL before the post processor ran, which meant the users would see a broken image in their posts. This commit fixes that issue by always running the upload security changes inline _within_ the cooked_post_processor job. The other issue was that the lightbox wrapper link for images in the post would end up with a URL like this: ``` href="/secure-uploads/original/2X/4/4e1f00a40b6c952198bbdacae383ba77932fc542.jpeg" ``` Since we weren't actually using the `upload.url` to pass to `UrlHelper.cook_url` here, we weren't converting this href to the CDN URL if the post was not in a secure context (the UrlHelper does not know how to convert a secure-uploads URL to a CDN one). Now we always end up with the correct lightbox href. This was less of an issue than the other one, since the secure-uploads URL works even when the upload has become non-secure, but it was a good inconsistency to fix anyway.
martin-brennan
force-pushed
the
issue/post-process-secure-upload-status-race-condition
branch
from
ffb22b6
to
fafdd1a
Compare
|
Frontend test failures unrelated |
ZogStriP
reviewed
Oct 18, 2023
ZogStriP
approved these changes
Oct 18, 2023
martin-brennan
deleted the
issue/post-process-secure-upload-status-race-condition
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit fixes a couple of issues.
A little background -- when uploads are created in the composer
for posts, regardless of whether the upload will eventually be
marked secure or not, if secure_uploads is enabled we always mark
the upload secure at first. This is so the upload is by default
protected, regardless of post type (regular or PM) or category.
This was causing issues in some rare occasions though because
of the order of operations of our post creation and processing
pipeline. When creating a post, we enqueue a sidekiq job to
post-process the post which does various things including
converting images to lightboxes. We were also enqueuing a job
to update the secure status for all uploads in that post.
Sometimes the secure status job would run before the post process
job, marking uploads as not secure in the background and changing
their ACL before the post processor ran, which meant the users
would see a broken image in their posts. This commit fixes that issue
by always running the upload security changes inline within the
cooked_post_processor job.
The other issue was that the lightbox wrapper link for images in
the post would end up with a URL like this:
Since we weren't actually using the
upload.urlto pass toUrlHelper.cook_urlhere, we weren't converting this href to the CDNURL if the post was not in a secure context (the UrlHelper does not
know how to convert a secure-uploads URL to a CDN one). Now we
always end up with the correct lightbox href. This was less of an issue
than the other one, since the secure-uploads URL works even when the
upload has become non-secure, but it was a good inconsistency to fix
anyway.