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

Parity rejects MetaMask's `x-metamask-origin` header, and consequently all MetaMask/Firefox requests, during CORS preflight #6616

Closed
interfect opened this issue Oct 2, 2017 · 25 comments

Comments

@interfect
Copy link

@interfect interfect commented Oct 2, 2017

I'm running:

  • Parity version: 1.6.10
  • Operating system: Linux
  • And installed: via parity/parity:stable Docker image

When using MetaMask 3.10.8 on Firefox 55 on Windows 7, pointed at my Parity node running on another Ubuntu machine, I observe an HTTP exchange that goeth thusly:

OPTIONS / HTTP/1.1
Host: 10.1.0.8:8545
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Access-Control-Request-Method: POST
Access-Control-Request-Headers: content-type,x-metamask-origin
Origin: moz-extension://c2480724-8a32-4afc-a7aa-c1d18a8bf229
Connection: keep-alive

HTTP/1.1 200 OK
Content-Type: application/json
Allow: OPTIONS, POST
Access-Control-Allow-Headers: origin, content-type, accept
Access-Control-Allow-Origin: moz-extension://c2480724-8a32-4afc-a7aa-c1d18a8bf229
Date: Mon, 02 Oct 2017 00:58:04 GMT
Transfer-Encoding: chunked

This is a CORS preflight for an actual request, but the actual request is never sent.

MetaMask wants to send an x-metamask-origin header with its request, giving the actual page (as opposed to moz-extension://c2480724-8a32-4afc-a7aa-c1d18a8bf229) that it is proxying requests for. Presumably this is so that you could potentially configure your node to handle requests only for certain duly authorized actual page origins, even when using MetaMask.

Parity doesn't seem to know about this header, and in particular is not returning it in the Access-Control-Allow-Headers. According to the CORS spec, that means that the browser must not permit a request to be made with that header on it. MetaMask (on Firefox, at least) insists on trying to send the header, so Firefox blocks its request altogether. The net result of all this is that MetaMask on Firefox does not work with a Parity node as the backend.

For maximum compatibility, Parity should echo back all the headers from Access-Control-Request-Headers in Access-Control-Allow-Headers. There could be a blacklist of headers that web pages should be prohibited from sending, but I don't think Parity trusts the RPC headers at all anyway, so I can't think of a reason not to just let pages send whatever they want.

This is related to MetaMask/metamask-extension#1779. It can be fixed in MetaMask with MetaMask/metamask-extension#2250 to have MetaMask not send the header, or MetaMask/metamask-extension#2138 to just ignore/skip the CORS preflight (as long as the Parity instance you are using is running on one of the domains mentioned in the MetaMask manifest). However, Parity should also fix it; you never know what x- headers a client is going to want to send.

@kumavis

This comment has been minimized.

Copy link

@kumavis kumavis commented Oct 2, 2017

I think its safe to say this is a MetaMask issue - will address

@kumavis

This comment has been minimized.

Copy link

@kumavis kumavis commented Oct 2, 2017

But I agree with this sentiment

you never know what x- headers a client is going to want to send.

@tomusdrw

This comment has been minimized.

Copy link
Contributor

@tomusdrw tomusdrw commented Oct 17, 2017

Note: This actually needs to be implemented in https://github.com/paritytech/jsonrpc (http server).
I'd like to see something like:

enum AllowedCorsHeaders {
  /// Accept all incoming headers
  Any,
  /// Accept only headers listed here.
  Only(Vec<HeaderName>),
}

ServerBuilder::new().allowed_cors_headers(AllowedCorsHeaders::Any)
@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented May 31, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 0.18 ETH (100.82 USD @ $560.13/ETH) attached to it.

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented May 31, 2018

Issue Status: 1. Open 2. Cancelled


Work has been started.

These users each claimed they can complete the work by 8 months, 1 week ago.
Please review their action plans below:

1) cooganb has started work.

Hey there! Happy to look at the code and see what sort of CORS stuff I can help with.

Learn more on the Gitcoin Issue Details page.

@cooganb

This comment has been minimized.

Copy link

@cooganb cooganb commented May 31, 2018

@tomusdrw @5chdn @folsen Seems like the issue is in paritytech/jsonrpc, not paritytech/parity itself. Is that correct?

@tomusdrw

This comment has been minimized.

Copy link
Contributor

@tomusdrw tomusdrw commented May 31, 2018

@cooganb correct, see #6616 (comment)

@folsen

This comment has been minimized.

Copy link
Member

@folsen folsen commented Jun 1, 2018

@cooganb if you fix it over there and reference this issue so it's closed by that PR then hopefully gitcoin still works and gives you your reward

@cooganb

This comment has been minimized.

Copy link

@cooganb cooganb commented Jun 1, 2018

@vs77bb Hey there! There's a question here about Gitcoin's reward system, wondering if you could help illuminate. Essentially, the work required is a dependency of paritytech/parity but located in paritytech/jsonrpc -- i.e. the PR will be on the jsonrpc repo, not here.

Is there a way to register this through Gitcoin?

@tomusdrw

This comment has been minimized.

Copy link
Contributor

@tomusdrw tomusdrw commented Jun 1, 2018

@cooganb I believe the bounty is also for integrating that fix with parity, so you can first open jsonrpc PR and then another PR integrating that work with Parity

@vs77bb

This comment has been minimized.

Copy link

@vs77bb vs77bb commented Jun 2, 2018

@cooganb @folsen Yes, the Gitcoin reward will still work! In fact, now knowing the scope is two PR's, we're happy to also tip a bit extra to get both accomplished 👍

@cooganb

This comment has been minimized.

Copy link

@cooganb cooganb commented Jun 2, 2018

@vs77bb Thank you!

@folsen @tomusdrw I've filed an initial PR in jsonrpc, please take a look and we can discuss the details there.

Also, is there a commit message schema used by Parity? When working on Geth, commit messages were required to start with the package they were changing. I'm not as familiar with Rust, but wanted to know if anything similar existed.

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Jun 7, 2018

@cooganb Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Jun 10, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@cooganb due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@cooganb

This comment has been minimized.

Copy link

@cooganb cooganb commented Jun 10, 2018

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Jun 16, 2018

@cooganb Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@cooganb

This comment has been minimized.

Copy link

@cooganb cooganb commented Jun 17, 2018

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Jun 22, 2018

@cooganb Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Jun 25, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@cooganb due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@5chdn 5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018
@Tbaut

This comment has been minimized.

Copy link
Collaborator

@Tbaut Tbaut commented Aug 3, 2018

@gitcoinbot up

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Aug 22, 2018

Issue Status: 1. Open 2. Cancelled


The funding of 0.18 ETH (47.84 USD @ $265.77/ETH) attached to this issue has been cancelled by the bounty submitter

@5chdn 5chdn added this to the 2.3 milestone Sep 27, 2018
@5chdn 5chdn modified the milestones: 2.3, 2.4 Oct 29, 2018
@cmichi

This comment has been minimized.

Copy link
Member

@cmichi cmichi commented Jan 9, 2019

Customizing the Access-Control-Allow-Headers has been implemented in paritytech/jsonrpc#305. This means it is now possible to configure e.g. the x-metamask-origin header field to be allowed:

.cors_allow_headers(
  AccessControlAllowHeaders::Only(vec!["x-metamask-origin".to_owned()])
)

(see also https://github.com/paritytech/jsonrpc/blob/master/http/examples/http_meta.rs#L27-L31)

A default set of header fields is always allowed: https://github.com/paritytech/jsonrpc/blob/master/server-utils/src/cors.rs#L282-L299

So from my point of view this issue could be closed. What do you think?

@joshua-mir

This comment has been minimized.

Copy link
Contributor

@joshua-mir joshua-mir commented Jan 9, 2019

@cmichi you're right, this has been resolved for a long time now 😅

@joshua-mir joshua-mir closed this Jan 9, 2019
Core automation moved this from Easy Picks (Good for external contribution) to Done Jan 9, 2019
@tomusdrw

This comment has been minimized.

Copy link
Contributor

@tomusdrw tomusdrw commented Jan 9, 2019

@cmichi shouldn't we still configure Parity Ethereum to accept x-metamask-origin though?

@joshua-mir joshua-mir reopened this Jan 9, 2019
Core automation moved this from Done to Needs Assignment Jan 9, 2019
@5chdn 5chdn modified the milestones: 2.4, 2.5 Jan 10, 2019
@cmichi cmichi self-assigned this Jan 18, 2019
cmichi added a commit that referenced this issue Jan 21, 2019
More details in #6616.
ascjones added a commit that referenced this issue Jan 21, 2019
* Echo CORS request headers by default

More details in #6616.

* fixup: Single line
@cmichi

This comment has been minimized.

Copy link
Member

@cmichi cmichi commented Jan 21, 2019

#10221 was merged this morning, so from my point of view the issue is resolved now.

@joshua-mir joshua-mir closed this Jan 21, 2019
Core automation moved this from Needs Assignment to Done Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.