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

CORS error connecting to api.nft.storage through the browser #175

Closed
dysbulic opened this issue Jun 4, 2021 · 14 comments
Closed

CORS error connecting to api.nft.storage through the browser #175

dysbulic opened this issue Jun 4, 2021 · 14 comments
Assignees
Labels
P1 High: Likely tackled by core team if no one steps up topic/docs Documentation

Comments

@dysbulic
Copy link

dysbulic commented Jun 4, 2021

When I attempt to upload a file using the web UI, I am getting a CORS error and the file isn't added:

Access to fetch at 'https://api.nft.storage/upload' from origin 'https://nft.storage' has been
blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the
requested resource. If an opaque response serves your needs, set the request's mode
to 'no-cors' to fetch the resource with CORS disabled.

POST https://api.nft.storage/upload net::ERR_FAILED
@dysbulic
Copy link
Author

dysbulic commented Jun 4, 2021

The error is misleading. Apparently, I was getting it b/c I was trying to upload a SVG. It works with a PNG.

@hugomrdias
Copy link
Collaborator

hugomrdias commented Jun 4, 2021

@dysbulic any chance we can get access to that svg to debug this ?

@dysbulic
Copy link
Author

dysbulic commented Jun 4, 2021

any chance we can get access to that svg to debug this?

Sure. I was disappointed when it didn't work. I tried a couple different ones, and thought it was the script tag, but this one also fails.

@hugomrdias
Copy link
Collaborator

hugomrdias commented Jun 4, 2021

Cool thank you im able to reproduce, i will look into it.

@hugomrdias hugomrdias self-assigned this Jun 4, 2021
@hugomrdias hugomrdias added the kind/bug A bug in existing code (including security flaws) label Jun 4, 2021
@dysbulic
Copy link
Author

dysbulic commented Jun 4, 2021

I didn't realize there was a script tag in the image I referenced. It didn't do anything, but it was there.

Once I removed it, the file uploaded.

There are valid reasons for disallowing images with scripts though honestly the img tag doesn't execute them and that's what people will be using to display NFTs.

@hugomrdias
Copy link
Collaborator

hugomrdias commented Jun 4, 2021

Well its actually a security issue, that svg has a inline script tag to inject the animation library which in this case is probably
harmless but for bad actors could be bad.
To be able to upload this you need to use the others options that we have that use FormData and not Blob.
For the js library this would be:

  • client.store
  • client.storeDirectory

For the HTTP API this would be (https://staging.nft.storage/api-docs/):

  • /upload with the body being a multipart/form-data
  • /store which is new and not documented yet

@hugomrdias
Copy link
Collaborator

hugomrdias commented Jun 4, 2021

what we could improve is to return a better error in this situation, i will look into that

@hugomrdias hugomrdias removed the kind/bug A bug in existing code (including security flaws) label Jun 4, 2021
@dchoi27 dchoi27 added P3 Low: Not priority right now topic/docs Documentation labels Aug 17, 2021
@yusefnapora yusefnapora self-assigned this Sep 17, 2021
@mikeal
Copy link
Member

mikeal commented Sep 17, 2021

Can we construct/document an HTML form such that it posts to the form-data API. If the file is taken directly from the file input field it can’t be tampered with by client script and CORS shouldn’t be an issue.

@yusefnapora
Copy link
Collaborator

yusefnapora commented Sep 17, 2021

@mikeal can we attach an auth header to the form's POST request without javascript though?

@yusefnapora
Copy link
Collaborator

yusefnapora commented Sep 17, 2021

About my last comment... maybe we could generate an id token on the client and put it in a hidden form field. Then it can be POST'ed directly, so our script never needs to see the File data. The backend would need to be updated to check for the DID token field though.

@dchoi27 dchoi27 added P1 High: Likely tackled by core team if no one steps up and removed P3 Low: Not priority right now labels Sep 17, 2021
@yusefnapora
Copy link
Collaborator

yusefnapora commented Sep 23, 2021

Just updating to mention that the forever fix for this seems to be to always send CARs to the API, which is being tracked in #220.

However, @dysbulic, I noticed in the client code that the storeCar method already accepts a Blob argument and will "promote" it into a CAR file, which should avoid the issue.

So if you're currently using storeBlob, you should be able to just switch to storeCar and just pass the same blob in; storeCar will convert it for you.

If you're not using storeBlob, you could make a CAR yourself and store it with storeCar. There's some docs on working with CARs here: https://docs.web3.storage/how-tos/work-with-car-files/

@yusefnapora
Copy link
Collaborator

yusefnapora commented Sep 23, 2021

@dysbulic please disregard my comment above about passing in your blob unchanged to storeCar! A closer reading shows that it only works if you're passing in a Blob of CAR-formatted data. Sorry about the misdirect :)

@Gozala
Copy link
Collaborator

Gozala commented Sep 23, 2021

@dysbulic I was trying to write up an example to demonstrate how this limitation could be overcome using client.storeDirectory API, however in the process I've discovered that it works now here is the link to an
https://observablehq.com/d/06fdb0bbee195e58 demonstrating it.

Have you by a chance updated a file under the URL you've shared ? If so could you please share the file that causes this ?

@Gozala
Copy link
Collaborator

Gozala commented Sep 24, 2021

Turns out svg linked does not contain script tag that which is probably why example above worked. That said we have changed cloudfare config to no longer block requests that contain script's in them so this issue is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High: Likely tackled by core team if no one steps up topic/docs Documentation
Projects
None yet
Development

No branches or pull requests

6 participants