Page MenuHomePhabricator

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

Authored by jasonbcox on Feb 19 2018, 02:40.

Details

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
SeverityLocationCodeMessage
Auto-Fixsrc/httprpc.cpp:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 2366
Build 2866: Bitcoin ABC Buildbot (legacy)
Build 2865: arc lint + arc unit

Event Timeline

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.

jasonbcox requested changes to this revision.Feb 20 2018, 19:17

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
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?

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

nit

wildnewlineappeared

280 ↗(On Diff #2923)

wildnewlineappeared

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
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

jasonbcox edited reviewers, added: lionello; removed: jasonbcox.

Bitbox folks are really interested in this change, so I'm commandeering it so we can get it onto master.

This revision now requires review to proceed.Apr 21 2018, 23:35
src/httprpc.cpp
63 ↗(On Diff #3533)

Doing this requires some cascading changes in this file. I'm going to take care of this in T329.

jasonbcox edited the test plan for this revision. (Show Details)

Rebased on master; tests pass

This revision is now accepted and ready to land.Apr 22 2018, 00:42
This revision was automatically updated to reflect the committed changes.