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: unexpected http method #1651

Merged
merged 10 commits into from
Jul 20, 2020
Merged

Conversation

ripzery
Copy link
Contributor

@ripzery ripzery commented Jul 20, 2020

Closes #1646

Overview

Adds one more clause to MethodParamsFilter to capture other HTTP methods.

Changes

  • Adds def call(conn, _), do: conn to OMG.WatcherRPC.Web.Plugs.MethodParamFilter
  • Adds def call(conn, _), do: conn to OMG.ChildChainRPC.Web.Plugs.MethodParamFilter

Testing

Runs unit tests or curl as follows:

Watcher

curl -X PUT http://localhost:7434/status.get -H "Content-Length: 0" | jq .

# PUT should not be allowed
{
  "data": {
    "code": "operation:not_found",
    "description": "Operation cannot be found. Check request URL.",
    "object": "error"
  },
  "service_name": "watcher",
  "success": false,
  "version": "1.0.3+e48599b"
}

curl -X POST http://localhost:7434/status.get -H "Content-Length: 0" | jq .

# POST should work as normal
{
  "data": {
    "byzantine_events": [],
    "contract_addr": {
      "erc20_vault": "0x135505d9f4ea773dd977de3b2b108f2dae67b63a",
      "eth_vault": "0x4e3aeff70f022a6d4cc5947423887e7152826cf7",
      "payment_exit_game": "0x89afce326e7da55647d22e24336c6a2816c99f6b",
      "plasma_framework": "0xc673e4ffcb8464faff908a6804fe0e635af0ea2f"
    },
    "eth_syncing": false,
    "in_flight_exits": [],
    "last_mined_child_block_number": 1000,
    "last_mined_child_block_timestamp": 1595075721,
    "last_seen_eth_block_number": 12365,
    "last_seen_eth_block_timestamp": 1595225750,
    "last_validated_child_block_number": 0,
    "last_validated_child_block_timestamp": 0,
    "services_synced_heights": [
      {
        "height": 672,
        "service": "block_getter"
      },
      {
        "height": 12346,
        "service": "challenges_responds_processor"
      },
      {
        "height": 12346,
        "service": "competitor_processor"
      },
      {
        "height": 12348,
        "service": "depositor"
      },
      {
        "height": 12346,
        "service": "exit_challenger"
      },
      {
        "height": 672,
        "service": "exit_finalizer"
      },
      {
        "height": 12346,
        "service": "exit_processor"
      },
      {
        "height": 672,
        "service": "ife_exit_finalizer"
      },
      {
        "height": 12346,
        "service": "in_flight_exit_processor"
      },
      {
        "height": 12346,
        "service": "piggyback_challenges_processor"
      },
      {
        "height": 12346,
        "service": "piggyback_processor"
      },
      {
        "height": 12364,
        "service": "root_chain_height"
      }
    ]
  },
  "service_name": "watcher",
  "success": true,
  "version": "1.0.3+e48599b"
}

Childchain

curl -X PUT http://localhost:9656/alarm.get -H "Content-Length: 0" | jq .

# PUT should not be allowed

{
  "data": {
    "code": "operation:not_found",
    "description": "Operation cannot be found. Check request URL.",
    "object": "error"
  },
  "service_name": "child_chain",
  "success": false,
  "version": "1.0.3+e48599b"
}

curl -X GET http://localhost:9656/alarm.get -H "Content-Length: 0" | jq .

# GET should work as normal

{
  "data": [
    {
      "main_supervisor_halted": {
        "node": "child_chain@127.0.0.1",
        "reporter": "Elixir.OMG.ChildChain.Monitor"
      }
    }
  ],
  "service_name": "child_chain",
  "success": true,
  "version": "1.0.3+e48599b"
}

@ripzery ripzery self-assigned this Jul 20, 2020
Copy link
Contributor

@unnawut unnawut left a comment

Choose a reason for hiding this comment

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

Lgtm to just early reject headers we don't intend to support. Although can you double check that this works client apps e.g. blockexplorer and webwallet? I think CORS require OPTIONS requests.

If we require OPTIONS also check that MethodParamFilter plug can handle it.

@mederic-p
Copy link
Contributor

Looking good, maybe just rename the plug so it's consistent with MethodParamFilter, you could name it MethodFilter.

@ripzery ripzery changed the title fix: unexpected http method [in progress] fix: unexpected http method Jul 20, 2020
@ripzery ripzery marked this pull request as draft July 20, 2020 05:54
@unnawut
Copy link
Contributor

unnawut commented Jul 20, 2020

Changed my mind as @mederic-p mentioned separately that our routes should already take care of the unsupported methods. So we only need to update the existing MethodParamFilter plug to handle unknown methods.

@ripzery ripzery changed the title [in progress] fix: unexpected http method fix: unexpected http method Jul 20, 2020
@ripzery ripzery marked this pull request as ready for review July 20, 2020 06:21
Copy link
Contributor

@unnawut unnawut left a comment

Choose a reason for hiding this comment

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

amazingly amazing

@ripzery ripzery merged commit 3ca4cf8 into master Jul 20, 2020
@ripzery ripzery deleted the ripzery/handle-unexpected-http-method branch July 20, 2020 08:28
@unnawut unnawut added the bug Something isn't working label Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle unexpected HTTP method gracefully
3 participants