Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cors): allow OPTIONS request to KDF server #2191

Merged
merged 9 commits into from
Aug 24, 2024
Merged

Conversation

shamardy
Copy link
Collaborator

@shamardy shamardy commented Aug 14, 2024

@KomodoPlatform/qa To Test:
Already tested by @CharlVS

No docs needed for this PR.

laruh
laruh previously approved these changes Aug 14, 2024
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

if req.method == Method::OPTIONS {
return Response::builder()
.status(StatusCode::OK)
.header(ACCESS_CONTROL_ALLOW_ORIGIN, rpc_cors.clone())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the clone here we don't need i guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed here 3dc9715

Comment on lines 270 to 279
let rpc_cors = match ctx.conf["rpccors"].as_str() {
Some(s) => try_sf!(HeaderValue::from_str(s)),
None => HeaderValue::from_static("http://localhost:3000"),
None => {
let rpc_cors = if ctx.is_https() {
format!("https://localhost:{}", local_port)
} else {
format!("http://localhost:{}", local_port)
};
try_sf!(HeaderValue::from_str(&rpc_cors))
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we want to abandon rpccors now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now that this port should be the frontend port used in development. I can use the client port here instead, but this opens it up for any port the request comes from, e.g., a script running locally on any port in the browser can access the RPC service. I will revert to port 3000 as it's commonly used for frontend development.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here 6f144e9

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is looks good to me, only small note

mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mariocynicys
mariocynicys previously approved these changes Aug 15, 2024
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

I am wondering tho how did cors work before since the specs mention that the browser sends an OPTIONS request first and we didn't cover that.

mm2src/mm2_main/src/rpc.rs Show resolved Hide resolved
@CharlVS
Copy link
Member

CharlVS commented Aug 15, 2024

I am wondering tho how did cors work before since the specs mention that the browser sends an OPTIONS request first and we didn't cover that.

@mariocynicys I'm assuming this wasn't picked up until now because there hasn't been an attempt to implement remote RPC access from the browser until now and since running WASM KDF in the browser doesn't use HTTP.

This is being worked on now as part of a Flutter SDK package for KDF repository here. The demo site is available here: https://komodo-playground.web.app/

Copy link
Member

@CharlVS CharlVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shamardy I'm having the same CORS errors for 500 responses because the Access-Control-Allow-Origin header isn't included in the response. Because of this, the browser does not allow access to the actual response and throws a generic XML exception.

Here's an example of the request and response using curl. Note the absence of the cors headers in the response.

curl -v --url "127.0.0.1:7783" --data '{
  "method": "&version"
}'
*   Trying 127.0.0.1:7783...
* Connected to 127.0.0.1 (127.0.0.1) port 7783
> POST / HTTP/1.1
> Host: 127.0.0.1:7783
> User-Agent: curl/8.6.0
> Accept: */*
> Content-Length: 26
> Content-Type: application/x-www-form-urlencoded
> 
< HTTP/1.1 500 Internal Server Error
< content-type: application/json
< content-length: 25
< date: Tue, 20 Aug 2024 13:57:08 GMT
< 
* Connection #0 to host 127.0.0.1 left intact
{"error":"Invalid input"}

Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.. just couple notes.

mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
@borngraced
Copy link
Member

borngraced commented Aug 20, 2024

@shamardy I'm having the same CORS errors for 500 responses because the Access-Control-Allow-Origin header isn't included in the response. Because of this, the browser does not allow access to the actual response and throws a generic XML exception.

Here's an example of the request and response using curl. Note the absence of the cors headers in the response.

curl -v --url "127.0.0.1:7783" --data '{
  "method": "&version"
}'
*   Trying 127.0.0.1:7783...
* Connected to 127.0.0.1 (127.0.0.1) port 7783
> POST / HTTP/1.1
> Host: 127.0.0.1:7783
> User-Agent: curl/8.6.0
> Accept: */*
> Content-Length: 26
> Content-Type: application/x-www-form-urlencoded
> 
< HTTP/1.1 500 Internal Server Error
< content-type: application/json
< content-length: 25
< date: Tue, 20 Aug 2024 13:57:08 GMT
< 
* Connection #0 to host 127.0.0.1 left intact
{"error":"Invalid input"}

it seems like the build that you used to run this test doesn't reflect with the changes in the PR yet..

@shamardy
Copy link
Collaborator Author

I'm having the same CORS errors for 500 responses because the Access-Control-Allow-Origin header isn't included in the response. Because of this, the browser does not allow access to the actual response and throws a generic XML exception.

@CharlVS are you using latest build/commit from this PR? If you are then the error has to be related to something in your mm2 config as far as I can see. Only errors I don't return cors for is the ones before cors is set.

    let ctx = try_sf!(MmArc::from_ffi_handle(ctx_h));
    // https://github.com/artemii235/SuperNET/issues/219
    let rpc_cors = match ctx.conf["rpccors"].as_str() {
        Some(s) => try_sf!(HeaderValue::from_str(s)),
        None => {
            if ctx.is_https() {
                HeaderValue::from_static("https://localhost:3000")
            } else {
                HeaderValue::from_static("http://localhost:3000")
            }
        },
    };

So either rpccors setting has a problem or mm2/kdf was stopped/ctx dropped which shouldn't happen. How is rpccors set? this try_sf!(HeaderValue::from_str(s)) doesn't return a cors header if rpccors is not a valid header value when starting mm2.

@CharlVS
Copy link
Member

CharlVS commented Aug 21, 2024

Thanks, @shamardy. Yeah, it looks like I was not running the latest version, even though I double-checked that I had the latest zip downloaded. I suspect I may have unzipped the previous build after I downloaded the new zip. I'm testing it out now.

I will test again now and let you know.

Copy link
Member

@CharlVS CharlVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as expected. Thanks for swiftly resolving this :)

@shamardy shamardy requested a review from borngraced August 23, 2024 16:18
borngraced
borngraced previously approved these changes Aug 23, 2024
@shamardy shamardy force-pushed the fix-allow-options-req branch from a102bbb to 9b94bf0 Compare August 23, 2024 22:45
@shamardy shamardy merged commit 932669a into dev Aug 24, 2024
26 of 28 checks passed
@shamardy shamardy deleted the fix-allow-options-req branch August 24, 2024 00:41
dimxy added a commit to dimxy/komodo-defi-framework that referenced this pull request Sep 13, 2024
* dev:
  chore(RPCs): rename `get_peers_info` RPC to `get_directly_connected_peers` (KomodoPlatform#2195)
  chore(WASM-builds): remove `wasm-opt` overriding (KomodoPlatform#2200)
  fix(coins): add p2p feature to mm2_net dependency (KomodoPlatform#2210)
  chore(test): turn on debug assertion (KomodoPlatform#2204)
  feat(sia): extract sia lib to external repo (KomodoPlatform#2167)
  feat(eth-swap): eth tpu v2 methods, eth docker test enhancements (KomodoPlatform#2169)
  fix(cors): allow OPTIONS request to KDF server (KomodoPlatform#2191)
  docs(README): update commit badges to use dev branch (KomodoPlatform#2193)
  use default value for `komodo_proxy` (KomodoPlatform#2192)
  feat(cosmos): komodo-defi-proxy support (KomodoPlatform#2173)
dimxy added a commit that referenced this pull request Sep 13, 2024
* dev:
  chore(RPCs): rename `get_peers_info` RPC to `get_directly_connected_peers` (#2195)
  chore(WASM-builds): remove `wasm-opt` overriding (#2200)
  fix(coins): add p2p feature to mm2_net dependency (#2210)
  chore(test): turn on debug assertion (#2204)
  feat(sia): extract sia lib to external repo (#2167)
  feat(eth-swap): eth tpu v2 methods, eth docker test enhancements (#2169)
  fix(cors): allow OPTIONS request to KDF server (#2191)
  docs(README): update commit badges to use dev branch (#2193)
  use default value for `komodo_proxy` (#2192)
  feat(cosmos): komodo-defi-proxy support (#2173)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants