cors: add a flag for cors

[tac0turtle]

• added a flag to the rest server that if you are fine with accepting all cors you can.

@ethanfrey

Signed-off-by: Marko Baricevic [email protected]

Closes: #XXX

Description

---

For contributor use:

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

---

For admin use:

• Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
• Reviewers assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[okwme]

First off VERY glad to see CORS coming back as an option. This greatly improves the developer experience of working with browser clients that interact with the REST endpoint. This has traditionally been a hot topic because of the security considerations of allowing users to expose a REST endpoint unfettered. While I think it should be up to the developer using the SDK to make those decisions and precautions themselves, I wonder whether this feature should be considered as intended for production or testing use.

If just for testing then I think making it accept all CORS as it currently does makes sense, but maybe we should rename it something like unsafeCORS or testCORS to emphasize that.

If we are happy with the idea that this might be a production setting then maybe we should offer the flag to equal a string of comma separated values that can be parsed as follows:

CORS(AllowedOrigins([]string{}))

(usage seen here)

This would allow a user to pass "*" to accept CORS from all origins or a comma separated list of approved origins.

[alessio]

This should be added in client/lcd.RegisterRestServerFlags()

[alessio]

I'm blocking this because I believe this requires discussion before we press ahead.
I acknowledge that this would make developers experience easier. I agree that especially while testing an app locally it's convenient to disable security features. Though IMHO it's our responsibility to make it as difficult as possible for developers to make dumb mistakes. I have few ideas on how to implement this without compromising on security (e.g. --cors flag may take effect on condition that a certain compilation flag is passed at build time + confirmation prompt when used). Let's discuss options, shall we? :)

[alessio]

Good progress here 👍

[codecov]

Codecov Report

Merging #5671 into master will decrease coverage by 0.79%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #5671     +/-   ##
=========================================
- Coverage   37.76%   36.96%   -0.8%     
=========================================
  Files         328      328             
  Lines       30267    30915    +648     
=========================================
  Hits        11429    11429             
- Misses      17645    18293    +648     
  Partials     1193     1193

Impacted Files	Coverage Δ
x/params/types/proposal/types.pb.go	1% <0%> (-0.1%)	⬇️
x/slashing/types/types.pb.go	0.8% <0%> (-0.03%)	⬇️
x/staking/types/types.pb.go	10.73% <0%> (-0.29%)	⬇️
x/bank/types/types.pb.go	0.69% <0%> (-0.09%)	⬇️
x/distribution/types/types.pb.go	0.63% <0%> (-0.09%)	⬇️
x/auth/types/types.pb.go	16.01% <0%> (-0.56%)	⬇️
types/types.pb.go	5.73% <0%> (-0.3%)	⬇️
x/supply/types/types.pb.go	0.99% <0%> (-0.06%)	⬇️

[alexanderbez]

ACK

[alexanderbez]

nit:

Suggested change

	var h http.Handler = rs.Mux
	h := rs.Mux

All changes requested have been addressed.

[ethanfrey]

That was fast. Nice one, @marbar3778