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

CORS configuration invalid #332

Open
tuler opened this issue Sep 20, 2020 · 10 comments
Open

CORS configuration invalid #332

tuler opened this issue Sep 20, 2020 · 10 comments
Labels
TeamGroot Identifies a given issue is assigned to the Groot Team

Comments

@tuler
Copy link

tuler commented Sep 20, 2020

I'm trying to setup CORS so I can access the signer in development from an application running at localhost:8000.
I tried "*", "all" and "http://localhost:8000", but they all seem to add an extra * to the header.

Access to fetch at 'http://localhost:8545/' from origin 'http://localhost:8000' has been blocked by CORS policy: The 'Access-Control-Allow-Origin' header contains multiple values '*, *', but only one is allowed. Have the server send the header with a valid value, or, if an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.
@tuler
Copy link
Author

tuler commented Sep 20, 2020

The regex produced by this piece of code for * is not right. The regex for * should be .*..

See this question

@rain-on rain-on closed this as completed Sep 21, 2020
@rain-on rain-on reopened this Sep 21, 2020
@rain-on
Copy link
Contributor

rain-on commented Sep 21, 2020

Apologies, my fingers got ahead of me - taking a look now..

@tuler
Copy link
Author

tuler commented Sep 21, 2020

I'm investigating as well. It doesn't look the problem is the regex.
I checked vertx code and * is a special case.

I am using an ethers JsonRpcProvider to access a local EthSigner.
And I call methods like provider.getSigner().getAddress() or provider.getNetwork().

I'm trying to debug EthSigner on the other side.
eth_accounts returns ok.
eth_chainId does not.
net_version does not.

I guess it's because those are passthrough methods, and the downstream is the one that should be responding CORS headers. Is this the case?

@rain-on
Copy link
Contributor

rain-on commented Sep 21, 2020

Hi @tuler,
EthSigner is split into 3 types of requests:

  • Handled in EthSigner (eg ethAccounts, eth_sign) (these appear to work correctly on master)
  • eth_sendTransaction - translated in Ethsigner, then forwarded to a web3provider (untested)
  • Everything else is "passed through" (testing now)

Are you running with Besu, or a different web3provider?

@tuler
Copy link
Author

tuler commented Sep 21, 2020

I'm not running Besu.
I'm running a local test builderevm node.

@rain-on
Copy link
Contributor

rain-on commented Sep 21, 2020

Update: When pairing EthSigner with Besu, we see eth_chainId working correctly (i.e. the returned access-control-allow-origin header has only a single "*")

I'm wondering what builderevm node does with CORS - Ethsigner deliberately removes the "ORIGIN" header from the proxied request, so the web3provider shouldn't know about CORS ...

Can I beg for a quick debug? If you can, it'd be interesting to put a debug point in ForwardedMessageResponder::handleResponse, just to confirm the headers in the response created by builderevm.

@tuler
Copy link
Author

tuler commented Sep 21, 2020

I did what you asked a couple minutes ago. I was writing this message.

buidlerevm eth_chainId call is returning these headers:

Access-Control-Allow-Headers: *
Access-Control-Allow-Origin: *
Content-Length: 43
Connection: keep-alive
Date: Mon, 21 Sep 2020 05:49:23 GMT
Keep-Alive: timeout=5

Then this line add those headers, causing the duplicate issue.

@tuler tuler mentioned this issue Sep 21, 2020
@rain-on
Copy link
Contributor

rain-on commented Sep 21, 2020

@tuler thanks for sending that through - so, it looks like builderevm is responding with CORS headers, even without having received a CORS header in the request.

Wondering if Ethsigner should strip the "access-control-allow-origins" headers from proxied responses - we had figured that stripping "ORIGIN" from the incoming request was enough, but it may be worth going a step further.

@tuler
Copy link
Author

tuler commented Sep 21, 2020

Yes, I think those response headers should be handled some other way.
I commented out the headers copy, just as a test, and everything worked.

@rain-on
Copy link
Contributor

rain-on commented Sep 21, 2020

Generally we need to copy back the headers (they're just as important as the body) - but we could deliberately discard the "access-control-allow-origin" when we copy the headers across - we'll get up a PR.

@jframe jframe added the TeamGroot Identifies a given issue is assigned to the Groot Team label Jan 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
TeamGroot Identifies a given issue is assigned to the Groot Team
Projects
None yet
Development

No branches or pull requests

3 participants