Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add crossorigin attribute to webmanifest link #1376

Merged
merged 2 commits into from
Apr 7, 2021

Conversation

acortelyou
Copy link
Contributor

Description

Solves potential 401 error with ​webmanifest link.

Fixes broken adaptive icons (#1274) in some scenarios.

See: https://medium.com/@aurelien.delogu/401-error-on-a-webmanifest-file-cb9e3678b9f3

To-Dos

  • Successful build yarn build
  • Translation keys yarn i18n:extract

@acortelyou acortelyou requested a review from sct as a code owner April 6, 2021 22:08
@TheCatLady
Copy link
Collaborator

Hm, do we need to send user credentials for requests for the manifest file? We don't restrict access to that resource by default. Is there any impact to adding this attribute that we need to be aware of, and, if so, should this be a configurable option?

Could you share an example of the scenarios where this is currently broken? The linked Medium blog post didn't really go into much detail, haha.

@acortelyou
Copy link
Contributor Author

A common pattern for securing this sort of service is to place it behind a reverse proxy such as nginx with SSL termination and a auth_request directive.

A concrete example would be the proxy auth feature of tools such as Organizr.

In the above scenario the crossorigin attribute is required for the webmanifest to load successfully.

To my knowledge, there are no side-effects when this attribute is included in standard standalone usage.

@sct
Copy link
Owner

sct commented Apr 7, 2021

A common pattern for securing this sort of service is to place it behind a reverse proxy such as nginx with SSL termination and a auth_request directive.

A concrete example would be the proxy auth feature of tools such as Organizr.

In the above scenario the crossorigin attribute is required for the webmanifest to load successfully.

To my knowledge, there are no side-effects when this attribute is included in standard standalone usage.

Wouldn't you be adding Organizr to your home screen in that case though?

@acortelyou
Copy link
Contributor Author

acortelyou commented Apr 7, 2021

Wouldn't you be adding Organizr to your home screen in that case though?

I don't think I understand what you meant by this.

Using Organizr for auth on the backend doesn't require that you use Organizr's UI/iframe to access the services nor for they or it to be aware of each other.

When Overseerr is accessed via a reverse proxy (very common) that is secured using client authorization (less common but frequently used in conjunction with oauth/plex/radarr/sonarr), external resource <link> tags need the crossorigin="use-credentials" attribute for the browser to include the required auth cookie with the request in the same way that it already does for every other non-manifest resource on the page.

I'm not sure what the rationale is for the browser treating the manifest different than, say, a stylesheet resource, but it is documented here https://web.dev/add-manifest/:

Gotchas! The request for the manifest is made without credentials (even if it's on the same domain), thus if the manifest requires credentials, you must include crossorigin="use-credentials" in the manifest tag.

More context about this non-standard manifest behavior being a weird quirk of the spec here: w3c/manifest#535 (comment)

Radarr and Sonarr have already adopted this attribute to work around the behavior as well:
Sonarr/Sonarr#4305
Radarr/Radarr#5863

@sct
Copy link
Owner

sct commented Apr 7, 2021

Oh, I see. I was confused about the use case but I wasn't thinking about the fact Overseerr may be behind organizr auth. Makes sense. Thanks for the explanation.

@sct sct enabled auto-merge (squash) April 7, 2021 23:26
@sct sct disabled auto-merge April 7, 2021 23:26
@sct sct enabled auto-merge (squash) April 7, 2021 23:26
@sct sct merged commit 82ca2f5 into sct:develop Apr 7, 2021
@sct
Copy link
Owner

sct commented Apr 7, 2021

@all-contributors please add @acortelyou for code

@allcontributors
Copy link
Contributor

@sct

I've put up a pull request to add @acortelyou! 🎉

@sct
Copy link
Owner

sct commented Apr 7, 2021

Thanks @acortelyou!

@github-actions
Copy link

🎉 This PR is included in version 1.23.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants