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

Event Timeline

lionello created this revision.Feb 19 2018, 02:40
schancel added a comment.EditedFeb 20 2018, 10:13

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

wildnewlineappeared

280 ↗(On Diff #2923)

wildnewlineappeared

schancel added a comment.Mar 6 2018, 05:17

Paging @deadalnix

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

schancel added a comment.Mar 9 2018, 04:33

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. :)

lionello updated this revision to Diff 3533.Apr 17 2018, 11:58

Removed the stray newlines.

jasonbcox accepted this revision.Apr 17 2018, 15:53

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

jasonbcox commandeered this revision.Apr 21 2018, 23:35
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
jasonbcox added inline comments.Apr 22 2018, 00:28
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 updated this revision to Diff 3568.Apr 22 2018, 00:36
jasonbcox edited the test plan for this revision. (Show Details)

Rebased on master; tests pass

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 22 2018, 00:36
jasonbcox updated this revision to Diff 3569.Apr 22 2018, 00:41

Added to release notes

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