Sys 320 options preflight #294
Conversation
eugenekatsov
requested review from
dwasyluk,
jnessbcf,
Keyare,
radubcf and
sidhujag
as code owners
|
Also pls add the rpccors domain config functionality. We want to disable by default and enable only on BMD. |
@sidhujag Can you re-approve on the basis of our conversation in stride pls? As discussed we only want OPTIONS exposed right now and not CORS. |
| // any of the values in list of origins do not set any additional headers | ||
| // and terminate this set of steps. | ||
| // Note: Always matching is acceptable since the list of origins can be | ||
| // unbounded. |
There was a problem hiding this comment.
@eugenekatsov Can you add an explicit note here with the
// syscoin
piece that notes explicitly why this is disabled? I know why and you know why, but the next maintainer may not.
There was a problem hiding this comment.
Also document what behavior occurs when rpcallowip is not set to localhost, and the locally running SPA attempts to connect if you can.
src/httprpc.cpp
Outdated
| static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &) | ||
| { | ||
| // First, check and/or set CORS headers | ||
| if (checkCORS(req)) { |
There was a problem hiding this comment.
I would rename this method for now to indicate what it is we're doing here (because we don't actually check for CORS according to this method and it might be misleading to a reader).
src/httprpc.cpp
Outdated
| @@ -228,6 +357,8 @@ static bool InitRPCAuthentication() | |||
| LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcuser for rpcauth auth generation.\n"); | |||
| strRPCUserColonPass = GetArg("-rpcuser", "") + ":" + GetArg("-rpcpassword", ""); | |||
| } | |||
|
|
|||
| strRPCCORSDomain = GetArg("-rpccorsdomain", ""); | |||
There was a problem hiding this comment.
This line should go away - it would be misleading to both end user and developer in that we don't currently support this flag.
Tagging @sidhujag for visibility.
src/init.cpp
Outdated
| @@ -661,6 +661,7 @@ std::string HelpMessage(HelpMessageMode mode) | |||
| strUsage += HelpMessageOpt("-rpcbind=<addr>", _("Bind to given address to listen for JSON-RPC connections. Use [host]:port notation for IPv6. This option can be specified multiple times (default: bind to all interfaces)")); | |||
| strUsage += HelpMessageOpt("-rpccookiefile=<loc>", _("Location of the auth cookie (default: data dir)")); | |||
| strUsage += HelpMessageOpt("-rpcuser=<user>", _("Username for JSON-RPC connections")); | |||
| strUsage += HelpMessageOpt("-rpccorsdomain=<value>", _("Domain from which to accept cross origin requests (browser enforced)")); | |||
There was a problem hiding this comment.
This should also be removed, as this is misleading to a user who will think that this is a supported option.
Taggin @sidhujag for visibility
|
Hi Everyone, I have updated the PR with all of Justice' recommendations. |
| // any of the values in list of origins do not set any additional headers | ||
| // and terminate this set of steps. | ||
| // Note: Always matching is acceptable since the list of origins can be | ||
| // unbounded. |
There was a problem hiding this comment.
Also document what behavior occurs when rpcallowip is not set to localhost, and the locally running SPA attempts to connect if you can.
This is a redo of #283 Changing the base fork from 3.2.0 to 3.1.5
From bitcoin#12040
Specific to the use case of rpc to localhost
The only code left out of the original bitcoin pr is setting and checking rpccors domain from config
The test is specific to syscoin rpc-tests