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

Safari doesn't like multiple CSP headers #2251

Closed
wants to merge 4 commits into from

Conversation

devlz303
Copy link

@devlz303 devlz303 commented Jul 6, 2023

Recursive Ordinals aren't working on Safari because Safari only uses the first CSP header from the response.
By moving all values to the insert en removing the append Recursive Ordinals will also work on Safari.

You can use this recursive ordinal to test
https://ordinals.com/inscription/370706072e6a03c3eb1349bf4531fe20e60d28063fac1f1c3a0774b094efb931i0

@bodily11
Copy link

bodily11 commented Jul 6, 2023

I don't think this works. I just tested it on my local version of ordinals . com and I am still getting CSP errors.

I posted what I thought was a fix a few days ago, but it ended up not working either. I'm still working on trying to get it working on Safari. Safari CSP implementation is a bit tricky to figure out.

@devlz303
Copy link
Author

devlz303 commented Jul 6, 2023

Hey that's weird. I'm sure I had it working but when I run it again now it's giving errors again. Let's see if I missed something

@devlz303
Copy link
Author

devlz303 commented Jul 6, 2023

Okay, so it was the /inscription/ page which was loading the /preview/ into an iframe which still wasn't working. I've added 'allow-same-origin' to the sandbox attribute and now it's also working on the /inscription/ page with preview

image

@bodily11
Copy link

bodily11 commented Jul 7, 2023

Just letting @veryordinally and @raph know that I have verified that this PR works. The test inscriptions originally mentioned in #2236 all work with this latest PR. I tested on Safari and I tested on iPhone.

@bodily11
Copy link

bodily11 commented Jul 7, 2023

Another consideration for this PR regarding using sandbox + allow-scripts + allow-same-origin.

"When the embedded document has the same origin as the embedding page, it is strongly discouraged to use both allow-scripts and allow-same-origin, as that lets the embedded document remove the sandbox attribute — making it no more secure than not using the sandbox attribute at all." - from https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe)

@devlz303
Copy link
Author

devlz303 commented Jul 7, 2023

Another consideration for this PR regarding using sandbox + allow-scripts + allow-same-origin.

"When the embedded document has the same origin as the embedding page, it is strongly discouraged to use both allow-scripts and allow-same-origin, as that lets the embedded document remove the sandbox attribute — making it no more secure than not using the sandbox attribute at all." - from https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe)

Okay, when I remove allow-scripts and only set allow-same-origin it won't work because then script execution is not allowed.
So this means using both allow-scripts and allow-same-origin attribute possibly isn't the solution and it has to be done differently.

But it does show why Safari is refusing to execute the scripts because it treats the urls in the iframe as coming from a different origin.

@devlz303
Copy link
Author

devlz303 commented Jul 7, 2023

I have read a bit more about sandbox + allow-same-origin flag and I think it's probably not the way you want to go because it is not secure enough. I also saw some posts from other people related to Safari ignoring the allow-scripts flag on sandboxed iframes and some of them are a few years old. So even if it is a Safari bug, I don't think it will be fixed soon. :-(

@devlz303
Copy link
Author

devlz303 commented Jul 7, 2023

I've reverted last change and removed 'allow-same-origin' because it is a security risk and allows the code from within the iframe to access cookies, localStorage, etc.
So the /inscription/[inscriptionid] page is broken again for Safari. But I think this is due to a bug in Safari ignoring or at least not correctly using the allow-scripts value for sandbox.

@devlz303
Copy link
Author

devlz303 commented Jul 7, 2023

@bodily11
I've been digging a bit deeper and suddenly I noticed none of the ordinals are showing up in Safari when they are loaded in an iframe running ord on a local server. So I think the fix will work when deployed on a real server. It's just not working with Safari and running on localhost.

So only the fix moving all CSP values into a single header should do the job

@bodily11
Copy link

bodily11 commented Jul 7, 2023

Hmmm not sure this is right. I pulled your CSP change into my instance of Ord and deployed it to a production server, but it still wasn't working. It needed the iframe fix before it started working for me, and this was on the latest version of Ord deployed to a production server.

@devlz303
Copy link
Author

devlz303 commented Jul 7, 2023

Bummer, maybe there are still some missing values in the CSP header, like :/inscription/ and/or :/preview/ because the page being loaded is /inscription/... and this page creates an iframe which loads /preview/...
Although it works, I don't think adding the allow-same-origin is secure.

@devlz303
Copy link
Author

devlz303 commented Jul 7, 2023

By the way Xverse wallet now supports recursive inscriptions. And when I look into the source on the inscription page I see they also use allow-same-origin and allow-scripts.

image

@bodily11
Copy link

bodily11 commented Jul 7, 2023

Yeah, if you allow scripts and allow same origin you are essentially removing sandbox (because it can be removed via script). So in that case, as long as you are disabling access to those APIs yourself on your frontend (maybe Xverse is doing this) then you should be ok. But if you aren't disabling access to those APIs, then you are essentially allowing Ordinals to access local storage and cookies.

@devlz303
Copy link
Author

devlz303 commented Jul 7, 2023

Maybe it should be configurable with an argument ord server --allow-same-origin so you have to explicitly set it.
But I'm new to Rust so I'm not sure how to do this in code :-)

@lifofifoX
Copy link
Collaborator

Why does the ord server serve paths that aren't a part of the public APIs? It seems best to use a separate host name for pages that aren't exposed via the API. With that, this CSP for whitelisting paths will no longer be needed:

HeaderValue::from_static("default-src *:*/content/ *:*/blockheight *:*/blockhash *:*/blockhash/ *:*/blocktime 'unsafe-eval' 'unsafe-inline' data: blob:"),

@raphjaph
Copy link
Collaborator

I just tested this and still not working on Safari. I'm using a M1 MacBook Pro running Safari Version 16.5.1 (18615.2.9.11.7). The consolidation of the CSP headers into one does work (on Chrome and Firefox) so definitely an improvement!

A note on contributing. I wrote a small guide to how our tests work here. When you make a change run the ci script and modify the test or write a new test. Thanks!

@raphjaph
Copy link
Collaborator

Why does the ord server serve paths that aren't a part of the public APIs? It seems best to use a separate host name for pages that aren't exposed via the API. With that, this CSP for whitelisting paths will no longer be needed:

I'm not quite sure what you mean by this. All of those endpoints are served by the server and are public.

@devlz303
Copy link
Author

I just tested this and still not working on Safari. I'm using a M1 MacBook Pro running Safari Version 16.5.1 (18615.2.9.11.7). The consolidation of the CSP headers into one does work (on Chrome and Firefox) so definitely an improvement!

A note on contributing. I wrote a small guide to how our tests work here. When you make a change run the ci script and modify the test or write a new test. Thanks!

The reason it's not working is because I removed the last change which added 'allow-same-origin' to the sandbox attribute on the iframe, because it's considered insecure. I thought it would also work without that change because on my side all the other pages with iframes gave the same error but they don't give an error in production.

So unfortunately the only solution I know can think of is to add the "allow-same-origin" again. I think it's a bug in how Safari handles iframes with sandbox 'allow-scripts', but that's not something we can change. :-(

Thanks for the guide on how to test work, I didn't know this.

@lifofifoX
Copy link
Collaborator

@raphjaph I believe the whitelisting of paths via CSP header is needed because ordinals.com serve paths that shouldn't be available to inscriptions, such as https://ordinals.com/sat/a, https://ordinals.com/block/00000000000000000000ad2683dabaee3b02e12f6a14af9a8cd4ab9cd719d0c3 etc.

If inscriptions are served from its own subdomain, where only paths exposed are the ones available through the official API (i.e /content/, /blockheight etc.), there's no need to whitelist any specific paths via a CSP header. Because that subdomain should only serve /content/ and other paths that should be available to all inscriptions.

@bodily11
Copy link

I'll reply back in another day or two, but the crux of the issue here is the following:

  1. Consolidate CSP headers into one rather than two (this is for Safari)
  2. Next, you have to allow cross origin and allow scripts to the /inscription iframe to get it working in Safari, but in doing so, you allow the inscription itself to completely remove all sandbox restrictions. This means inscriptions can access local storage, cookies, indexedDb, etc. You could try blocking those, but the inscription could still access top (the outer pages context). This solution was originally attempted (and it worked on Safari), but it removes sandboxing of inscriptions which is undesirable for security.
  3. The next solution we are going to try is to serve up the inscriptions from a separate sub-domain as the parent. This way you have separation of domain between child/parent, you can protect parent cookies, local storage, indexedDb, etc, but the child domain loading the inscriptions can still load content from the parent. I'll let you know how this test works, but I think this might be the way to get recursive inscriptions loading securely in Safari/iPhone. The annoying part is you have to use a separate subdomain, but I think it will be worth it.

@casey
Copy link
Collaborator

casey commented Jul 10, 2023

Merging CSP headers as is done in this PR does not work. When a single CSP header is used, requests to any of the sources are allowed, so, for example, https://foo.com/content/bar will work, even though it's an external resource, because it matches *:*/content/, and so matching self is not required.

@casey casey closed this Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants