Allow wildcard for allow control allowed origins

[thegoldenmule]

Description

The new --access-control-allow-origins flag did not function correctly with wildcards. This means that tech that didn't explicitly specify an Origin header on requests to the JSON RPC API would be blocked. This includes standard tech like MetaMask.

Changes include

• [ x ] Bugfix (non-breaking change that solves an issue)

Testing

• [ x ] I have tested this code manually

Manual tests

I was able to do various actions via MetaMask and explore the blockchain via typical eth_ functions.

Fixes EDGE-436

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[thegoldenmule]

I have read the CLA Document and I hereby sign the CLA

[Kourin1996]

I appreciate your PR. I've left a small comment. Would be great if you could check it. Thank you!

[thegoldenmule]

@Kourin1996 Since config.AccessControlAllowOrigin is an array, I still need to loop to see if * is among the values provided. There is also some ambiguity here in that it's not clear what should happen if a user passes a wildcard AND a specific domain. I considered three general cases:

--access-control-allow-origins "*"

Here the header will be set directly to *.

--access-control-allow-origins "*,https://mydomain.com"

Here the header will be set to * regardless.

--access-control-allow-origins "https://mydomain.com,*"

Here the header will be set to https://mydomain.com if the origin is mydomain.com, otherwise *.

[Kourin1996]

@thegoldenmule Thank you for the comments. That's true. In current implementation, multiple domains that contains wildcard can be given. So my comment at that line was not correct actually. We need to talk about whether we can give domains with wildcard.
cc: @zivkovicmilos @lazartravica @ptitluca

[thegoldenmule]

A simple alternative, btw, would be to skip the array and loop entirely and simply set the string directly to whatever was specified in the configuration.

[zivkovicmilos]

@Kourin1996 @ptitluca @lazartravica @thegoldenmule

What do you think about having * be the default value for this specific CORS configuration (Allow-Origins)?

It would mitigate this additional code complication, as you'd assume if anything specific was provided for allowed origins, you'd need to check against it, otherwise it's all allowed because the wildcard (*) would be the default.

[Kourin1996]

@zivkovicmilos Not sure setting wildcard as default is good. Wildcard will allow all web browsers to access to JSON-RPC. (Of course, you can block by different layer)

[thegoldenmule]

@zivkovicmilos @Kourin1996 IMO * by default makes the most sense. I can't tell you what the goals of your project are-- but as I see it, an opensource blockchain server that by default doesn't work with standard web3 clients (like Metamask) doesn't make a ton of sense.

[thegoldenmule]

@STZeed I'm not sure I'm following but if any offense was taken, it was certainly not intended. My point is only that this project provides an open API that, by default, does not work in the larger open ecosystem. It's not CORS that is providing the security guarantees behind dapps, though I understand you might as well use every tool your disposal.

As far as ideas, I have already mentioned one that may have been passed over. Why not simply set the header explicitly to the value provided by --access-control-allow-origins rather than any sort of dynamic logic? This option would result in the simplest implementation, the easiest documentation to understand, and cover a large number of use cases.

[Kourin1996]

@thegoldenmule @STZeed Thank you for having opinions and apologize for late reply. Basically I'm not clearly disagree with that. Perhaps there will be few cases of that we want to disable CORS. It will work with allowing CORS to any domains for now.

[thegoldenmule]

@Kourin1996 Please let me know how you'd like me to proceed.

[Kourin1996]

I think you will need to do the following things:

1. Set '*' as the default CORS domain
2. In case of '', set '' in "Access-Control-Allow-Origin HTTP/S header

@thegoldenmule PR doesn't have 1 now. Thank you

[Kourin1996]

A simplest way is to add default value here

polygon-edge/command/helper/helper.go

Line 479 in 8a033aa

flags.Var(&accessControlAllowOrigins, "access-control-allow-origins", "")

[thegoldenmule]

@Kourin1996 That file seems to have gone through a large refactor recently. I changed it in command/server/config.go, as it looked like the right place.

[ZeljkoBenovic]

LGTM 🥇
Tested everything. It works as intended.

Just one observation:
If there is a need to allow multiple domains with --access-control-allow-origins flag, proper usage is setting multiple flags in same command instead of setting multiple domains separated by comma.
This will work:

polygon-edge server  --access-control-allow-origins https://example.com --access-control-allow-origins https://edge-docs.polygon.technology

This will not work:

polygon-edge server  --access-control-allow-origins "https://example.com,https://edge-docs.polygon.technology"

We will publish the docs with proper usage for this flag soon.
Just fix these linter errors, and we're good to go.

Great job 💯

[zivkovicmilos]

Looks great 💯

Thank you for the contribution 🙏

Please check out Zeljko's comment and resolve the linting errors, after that we're good to go 🚀

[thegoldenmule]

@zivkovicmilos @ZeljkoBenovic Thanks so much. I think I've fixed the linter issues. I have not used golangci-lint before, but make lint seems to complete successfully locally for me.

[zivkovicmilos]

Thank you for resolving the linting errors, and for the contribution 🙏

Merging this now 🚀