Details

Reviewers

schancel
deadalnix
matiu
lionello

Group Reviewers

Restricted Project

Commits

rSTAGING08c165190a80: fix: add support for CORS headers and pre-flight request
rABC08c165190a80: fix: add support for CORS headers and pre-flight request

Summary

This PR implements basic Cross-Origin Resource Sharing (CORS) support to the RPC server, as per the spec at https://www.w3.org/TR/cors/#resource-requests . The spec has been quoted verbatim in the source code for easier validation and maintenance of the code.

added support for OPTIONS HTTP method
interpret CORS request headers for pre-flight requests
set CORS response headers
-rpccorsdomain=value command line option for whitelisted domains
four test cases: standard CORS request, and pre-flight request, with/without -rpccorsdomain

In practice this PR allows the REST interface to be used directly from a browser.

All the existing restrictions to the REST interface still apply: IP subnet, port, username, password.

Test Plan

Standard CORS request (Origin set) to whitelisted domain (should succeed)
CORS pre-flight request (OPTIONS) to whitelisted domain (should succeed)
Standard CORS request (Origin set) to non-whitelisted domain (should fail)
CORS pre-flight request (OPTIONS) to non-whitelisted domain (should fail)

Diff Detail

Repository

rABC Bitcoin ABC

Branch

arcpatch-D1112

Lint

Lint Passed

Severity	Location	Code	Message
Auto-Fix	src/httprpc.cpp:1	CFMT	Code style violation

Unit

No Test Coverage

Build Status

Buildable 2365
Build 2864: Bitcoin ABC Buildbot (legacy)
Build 2863: arc lint + arc unit

Event Timeline

lionello created this revision.Feb 19 2018, 02:40

schancel added reviewers: deadalnix, jasonbcox, matiu.Feb 19 2018, 04:45

Hi lionello, I know I reviewed this before on github. I want you to know I'm not ignoring it -- I'm waiting for some other people to take a look at it since it is a relatively large change w/ added functionality.

I did an initial sweep of this review and it looks ok. However, I'm concerned about opening the attack surface on ABC by implementing this. I'll take a closer look when I have some extra time.

src/httpserver.cpp
420 ↗	(On Diff #2923)	This comment appears to be inaccurate.

This revision now requires changes to proceed.Feb 20 2018, 19:17

lionello added inline comments.Feb 20 2018, 23:53

src/httpserver.cpp
420 ↗	(On Diff #2923)	The comment refers to the fact that bitcoin RPC only supports POST and OPTIONS, but that we want to return HTTP status 405 for the others (to avoid breaking changes.) By default evhttp handles GET, POST, HEAD, PUT, and DELETE. Anything else (OPTIONS) is dropped by evhttp before we get a chance to reply. On line 287 we handle anything that's not POST and return a custom error result.

lgtm

src/init.cpp
853 ↗	(On Diff #2923)	Can this also be `*`, and if not, does it need to be?

schancel added inline comments.Feb 23 2018, 02:41

src/httprpc.cpp
156 ↗	(On Diff #2923)	nit
280 ↗	(On Diff #2923)

Paging @deadalnix

Any problem with this patch? It looks good to me.

I'm sorry for the delay in getting this in. We will likely get this into the 0.17.1 release.

I took a good look at this diff and it looks fine to me. I'll hold off on accepting it since we don't have a release branch (yet). Chances are, you'll be able to land this change once the hard fork release goes out. :)

Removed the stray newlines.

LGTM. Thanks for being patient.

This revision is now accepted and ready to land.Apr 17 2018, 15:53

schancel added inline comments.Apr 21 2018, 23:23

src/httprpc.cpp
63 ↗	(On Diff #3533)	This commit looks good. If you have time, we've been moving things like this unto the configuration object. it would be good to have this there, then to pass it into `checkCORS` as a parameter