Skip to content
This repository was archived by the owner on Apr 18, 2024. It is now read-only.

Conversation

@BradNeuberg
Copy link
Contributor

@BradNeuberg BradNeuberg commented Feb 13, 2023

PR fulfills these requirements

  • [*] Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • [*] Tests for the changes have been added/updated (for bug fixes/features)
  • [*] Docs have been added/updated (for bug fixes/features)
  • [*] Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • [*] Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • [*] Frontend

Describe the reason for change

(link to issue, supportive screenshots etc.)

What does this fix?

(if this is a bug fix)

AWS S3's CORS headers have a bug where if an image is requested in an S3 bucket initially from an HTML <img> tag the HTTP response incorrectly leaves out the HTTP Origin header. Then, later on, if a JavaScript CORS request is made to this same image, Chrome will return the cached request without the Origin header, which means the CORS request will fail. This issue is described here: https://bugs.chromium.org/p/chromium/issues/detail?id=409090

The user facing impact of this bug is that if there is a gallery of multiple images to be labelled beneath the stage, the thumbnails will be shown correctly, but once you click into them you get a CORS error shown on the stage.

The fix is as follows: If the Magic Wand feature flag is on AND if the Label Studio Image tag has the crossOrigin attribute set AND if the src image is an AWS S3 bucket, then we do a trick to break the cache for the image by adding a ?v=1 timestap on it.

What is the new behavior?

(if this is a breaking or feature change)

What is the current behavior?

(if this is a breaking or feature change)

What libraries were added/updated?

(list all with version changes)

Does this change affect performance?

(if so describe the impacts positive or negative)

Breaking the image cache can potentially affect image loading performance. We break the image cache as focused as possible (only for magic wand feature flag, only when using crossOrigin attribute, and only for AWS S3 URLs). In addition, we make sure that we are storing the timestamp as a volatile value on the Image object, so that we don't mindlessly keep breaking the cache with a new timestamp each time React might redraw the screen. We only need to break the cache on the initial image load.

Does this change affect security?

(if so describe the impacts positive or negative)

What alternative approaches were there?

(briefly list any if applicable)

Unfortunately there are no other known workarounds to this AWS S3 issue - Amazon should fix the bug on their side, but it sounds like they've dropped the ball. This does not effect other cloud bucket providers, such as GCP.

What feature flags were used to cover this change?

(briefly list any if applicable)

The Magic Wand feature flag: FF_DEV_4081 = 'fflag_feat_front_dev_4081_magic_wand_tool'

Does this PR introduce a breaking change?

(check only one)

  • [*] Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • [*] e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

(for bug fixes/features, be as precise as possible. ex. Authentication, Annotation History, Review Stream etc.)

Image segmentation labelling and Image gallery if multiple images are present to be labelled

@swarmia
Copy link

swarmia bot commented Feb 13, 2023

@BradNeuberg
Copy link
Contributor Author

I've updated to use a much simpler caching key, ?v=1, and confirmed that everything works fine.

@hogepodge
Copy link

Internal tracking added, and waiting for assignment.

@BradNeuberg
Copy link
Contributor Author

@hogepodge Any updates on this ticket? I think the work and all feedback to comments is essentially complete, I'd love to land this if possible soon to close out tracking tickets on Planet's side related to this work.

@hogepodge
Copy link

@BradNeuberg I'll check in on Monday in the next planning meeting.

@erinmikailstaples erinmikailstaples added the Community Community-created or focused label Mar 29, 2023
@hogepodge
Copy link

@BradNeuberg, it's sounding like we're going to fix this in an upcoming patch that doesn't require cache busting. I'll reach out with more info once I have it.

@bmartel
Copy link
Contributor

bmartel commented Apr 21, 2023

@BradNeuberg There is a big overhaul coming to url handling with respect to CORS and external urls/presigning, which the changes as far as the frontend repository goes will be tracked here: #1299. This should be merged very soon (looking to be in by early next week), just some housekeeping items to take care of there. The underlying issue presented in the MagicWand scenarios were able to be reproduced and fixed there.

@BradNeuberg
Copy link
Contributor Author

@bmartel It sounds like I can close this PR without merging due to the other fixes you all already did around CORS handling?

@bmartel
Copy link
Contributor

bmartel commented Jun 6, 2023

Hey @BradNeuberg that is correct. This should now be fixed by the other PRs surrounding CORS and pre-signed url changes.

@hlomzik hlomzik closed this Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking Community Community-created or focused fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants