Skip to content

Fix RPC response validation with string keys - #22142

Open
Yeagerist0 wants to merge 1 commit into
rapid7:masterfrom
Yeagerist0:fix/json-rpc-string-key-version
Open

Yeagerist0 wants to merge 1 commit into
rapid7:masterfrom
Yeagerist0:fix/json-rpc-string-key-version

Conversation

@Yeagerist0

Copy link
Copy Markdown
Contributor

Description

Msf::RPC::JSON::Request supports symbolize_names: false, but its JSON-RPC version check always looks up :jsonrpc. JSON.parse returns "jsonrpc" in that mode, so every otherwise valid response is rejected as InvalidResponse.

Use the selected key format for the version check and add focused response-processing specs. This also contributes request coverage toward #21518. No API or default-behavior change is intended.

AI tools used: Codex helped identify the mismatch, write the fix and regression specs, and run verification. I reviewed the resulting diff and test output.

Setup and Reproduction

On Ruby 3.3.8, instantiate Msf::RPC::JSON::Request with method: 'core.version' and symbolize_names: false, then process {"jsonrpc":"2.0","id":1,"result":{"version":"6"}}. Before the change, the request fires an InvalidResponse errback despite the valid version. After the change, it fires a Response callback with string keys in the result.

Verification

  • Run bundle exec rspec spec/lib/msf/core/rpc/json/request_spec.rb with the repository's locked dependencies and confirm all 3 examples pass.
  • Change the version in the string-key response to 1.0 and confirm InvalidResponse is returned.
  • Process a valid response with the default symbolized keys and confirm the result still uses symbol keys.

Results and evidence

The 3 new RSpec examples passed with an isolated bootstrap using the vendored em-http-request and RSpec gems (3 examples, 0 failures). A separate direct response-processing check passed 4 valid/invalid combinations across both key formats. ruby -c passed on both changed files, and RuboCop reported no offenses in the new spec.

The full bundle exec rspec command could not run in this checkout: its current Gemfile.lock references gem versions unavailable in the local vendor bundle. The existing request file has pre-existing RuboCop offenses; the changed line adds none. No network service or credentials are needed for these specs.

Signed-off-by: Yeagerist0 <180412661+Yeagerist0@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Nov 14, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

  • Bug Fixes
    • JSON-RPC responses are now validated correctly whether their keys are symbols or strings.
    • Unsupported JSON-RPC versions continue to be rejected with an invalid response error.

The JSON-RPC response version check now uses a symbol or string key based on symbolize_names. Request specs cover valid responses in both modes and an unsupported version.

Changes: JSON-RPC Response Validation

Layer / File(s) Summary
Response version check and coverage
lib/msf/core/rpc/json/request.rb, spec/lib/msf/core/rpc/json/request_spec.rb
The version check selects its key type based on symbolize_names. Specs cover symbolized and string-key responses, and verify that an unsupported version invokes the errback with Msf::RPC::JSON::InvalidResponse.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mkienow-r7

Merge Risk: Minimal · up to cf619

This fixes valid JSON-RPC responses being rejected when symbolize_names is false. Default behavior is unchanged and the new specs cover both key formats and unsupported versions. No merge-blocking risk was found.

Security architecture risk: Minimal · up to cf619

The fix makes response validation honor the existing string-key option while retaining the JSON-RPC version requirement and other validation checks. No introduced or worsened architecture-level security risk was established.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the affected component (RPC response validation) and the specific fix (handling string keys). The change makes the version check use the key format selected by symbolize_names, so the …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Pr Description And Verification Passed The PR description meets the "PR description and verification" criteria. - What and why: It states the defect. Msf::RPC::JSON::Request uses symbolize_names: false, but the JSON-RPC version che…
Full details: Title check

Explanation

The title names the affected component (RPC response validation) and the specific fix (handling string keys). The change makes the version check use the key format selected by symbolize_names, so the title matches the main change in the request.rb file and its specs.

Full details: Pr Description And Verification

Explanation

The PR description meets the "PR description and verification" criteria. - What and why: It states the defect. Msf::RPC::JSON::Request uses symbolize_names: false, but the JSON-RPC version check reads :jsonrpc. Valid responses are therefore rejected as InvalidResponse. It states the fix: the version check uses the selected key format. - Reproduction: It gives concrete failure steps. Use Ruby 3.3.8, method: 'core.version', symbolize_names: false, and the response {"jsonrpc":"2.0","id":1,"result":{"version":"6"}}. It states the observed result before the change (InvalidResponse) and after the change (Response callback). - Verification commands and expected outcomes: It lists bundle exec rspec spec/lib/msf/core/rpc/json/request_spec.rb with 3 expected passing examples. It also lists a 1.0 version case that should return InvalidResponse, and a default-symbol-keys case that should keep symbol keys. - Actual results: It reports 3 examples, 0 failures from an isolated bootstrap. It reports 4 valid/invalid combinations passing across both key formats. It reports ruby -c passing and no RuboCop offenses in the new spec. - Limits: It states that the full bundle exec rspec run failed in this checkout because locked gem versions were unavailable. It states that the existing request file has pre-existing RuboCop offenses. It states that no network service or credentials are needed. - Unchecked boxes: The unchecked verification items are expected reviewer steps, so they do not count against the PR. The contributor testing claims come from the supplied description and were not re-run. This is consistent with the check, which asks for assessment of supplied evidence only.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between bea1fb0 and cf619fe.

Files selected for processing: 2

  • lib/msf/core/rpc/json/request.rb
  • spec/lib/msf/core/rpc/json/request_spec.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant