fix path param override by query params

[ljmsc]

References to other Issues or PRs

Fixes #3069
original fix was reverted because of #3848

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

This PR should fix the possibility to override path parameters with query parameters. It was originally fixed with #3072 but a year later reverted because of #3848. In this PR the fix is a different approach, since the JSON names are the problem, I added a normalization function to rename query param names to the corresponding field name. The validation logic will now drop values which would override path parameters.

Other comments

I once again want to point out the criticality of this issue. A lot of authorization systems validate access based on the requested resource url. With this security issue it is easy to bypass those validations to get unauthorized access to other resources.

Summary by CodeRabbit

• New Features

  • Introduced new optional parameters (ResourceId, NIdNId, and NIdVal) across various methods to enhance query capabilities.
  • Added ExamplepbNestedMessage and ExamplepbUnannotatedNestedMessage structs for improved data structuring.
  • Expanded test coverage with new test functions focusing on HTTP GET requests and response handling.
  • Updated EchoService and UnannotatedEchoService with new message types and additional route bindings.

• Enhancements

  • Added new fields (ResourceId and NId) to ExamplepbSimpleMessage and ExamplepbUnannotatedSimpleMessage structs for better resource identification and nested message support.

• Refactor

  • Implemented normalizeFieldPath and findFieldFromPath functions in query.go for optimized field path normalization and retrieval, enhancing parsing and field population logic.

[coderabbitai]

Walkthrough

The recent updates focus on enhancing the Echo Service API by introducing optional parameters to methods across several files, adding new message structures for nested messages, and implementing additional test functions. These changes aim to enrich the API's functionality, address security concerns related to parameter overwriting, and improve test coverage. The modifications span across both annotated and unannotated echo services, reflecting a comprehensive approach to evolving the API's capabilities.

Changes

File Path	Change Summary
.../echo/api_echo_service.go	Added optional parameters (ResourceId, NIdNId, NIdVal) to methods.
.../echo/model_examplepb_*.go	Introduced ExamplepbNestedMessage and added new fields to ExamplepbSimpleMessage.
.../unannotatedecho/api_unannotated_echo_service.go	Added new optional parameters to methods in UnannotatedEchoServiceEchoOpts structs.
.../unannotatedecho/model_examplepb_unannotated_*.go	New ExamplepbUnannotatedNestedMessage struct and added fields to ExamplepbUnannotatedSimpleMessage.
.../integration/integration_test.go	Added new test functions (testEchoPathParamOverwrite, testEchoNested, testEchoNestedOverride).
.../proto/examplepb/echo_service.proto	Added NestedMessage, new fields, and additional bindings.
.../proto/examplepb/unannotated_echo_service.proto	Introduced UnannotatedNestedMessage and added new fields.
runtime/query.go	Added normalizeFieldPath and findFieldFromPath functions for field path normalization.

Assessment against linked issues

Objective	Addressed	Explanation
Prevent overwriting path parameters with query parameters (#3069)	✅
Ensure the immutability of path parameters and prevent alteration by query parameters (#3069)	✅
Address security concerns related to parameter overwriting (#3069)	✅
Implement validation or enforcement mechanisms against overwriting (#3069)	❓	It's unclear without specific details on how the implementation was done.
Ensure gateway or routing rules prevent unintended overwriting of path parameters (#3069)	❓	The summary doesn't explicitly mention changes to gateway or routing rules.

Poem

In the code's vast garden, changes bloom, 🌼
Paths and queries, now in harmony, room.
A rabbit hops along, inspecting each line, 🐇
"Security and tests, all now align!"

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 5a55932 and 15e1c5b.

Files ignored due to path filters (7)

• examples/internal/clients/echo/api/swagger.yaml is excluded by: !**/*.yaml
• examples/internal/clients/unannotatedecho/api/swagger.yaml is excluded by: !**/*.yaml
• examples/internal/proto/examplepb/echo_service.pb.go is excluded by: !**/*.pb.go
• examples/internal/proto/examplepb/echo_service.pb.gw.go is excluded by: !**/*.pb.gw.go
• examples/internal/proto/examplepb/echo_service.swagger.json is excluded by: !**/*.json
• examples/internal/proto/examplepb/unannotated_echo_service.pb.go is excluded by: !**/*.pb.go
• examples/internal/proto/examplepb/unannotated_echo_service.swagger.json is excluded by: !**/*.json

Files selected for processing (10)

• examples/internal/clients/echo/api_echo_service.go (24 hunks)
• examples/internal/clients/echo/model_examplepb_nested_message.go (1 hunks)
• examples/internal/clients/echo/model_examplepb_simple_message.go (1 hunks)
• examples/internal/clients/unannotatedecho/api_unannotated_echo_service.go (9 hunks)
• examples/internal/clients/unannotatedecho/model_examplepb_unannotated_nested_message.go (1 hunks)
• examples/internal/clients/unannotatedecho/model_examplepb_unannotated_simple_message.go (1 hunks)
• examples/internal/integration/integration_test.go (2 hunks)
• examples/internal/proto/examplepb/echo_service.proto (3 hunks)
• examples/internal/proto/examplepb/unannotated_echo_service.proto (2 hunks)
• runtime/query.go (2 hunks)

Additional comments: 26

examples/internal/clients/echo/model_examplepb_nested_message.go (1)

• 12-15: The ExamplepbNestedMessage struct is well-defined with appropriate JSON tags for omitempty. This ensures that these fields will only be included in the JSON output if they are not empty, which is a good practice for optional fields.

examples/internal/clients/unannotatedecho/model_examplepb_unannotated_nested_message.go (1)

• 13-16: The ExamplepbUnannotatedNestedMessage struct is correctly defined with JSON tags indicating optional fields (omitempty). This is consistent with the approach taken in model_examplepb_nested_message.go, ensuring uniform handling of optional fields across related structs.

examples/internal/clients/echo/model_examplepb_simple_message.go (1)

• 22-23: The addition of ResourceId and NId fields to the ExamplepbSimpleMessage struct is consistent with the PR's objective to enhance security by preventing path parameter overrides. The use of pointers for complex types (*ExamplepbNestedMessage) is a good practice for indicating optional fields in Go.

examples/internal/clients/unannotatedecho/model_examplepb_unannotated_simple_message.go (1)

• 25-26: The addition of ResourceId and NId fields to the ExamplepbUnannotatedSimpleMessage struct aligns with the changes made in the annotated version, ensuring consistency across both annotated and unannotated versions. This supports the PR's goal of enhancing security through the URL routing mechanism.

examples/internal/proto/examplepb/unannotated_echo_service.proto (2)

• 25-28: The definition of UnannotatedNestedMessage with fields n_id and val is correctly implemented in Proto3 syntax. This addition is necessary for supporting the new validation logic that prevents path parameter override.
• 45-47: The addition of resource_id and n_id fields to UnannotatedSimpleMessage is consistent with the changes in the corresponding Go structs, ensuring that the protobuf definitions align with the backend models. This is crucial for the integrity of the data model and the security enhancements described in the PR.

examples/internal/proto/examplepb/echo_service.proto (3)

• 23-26: The NestedMessage message type is correctly defined with n_id and val fields, following Proto3 syntax. This addition supports the new security feature by allowing nested messages to be securely passed without risking path parameter override.
• 42-44: The addition of resource_id and n_id fields to the SimpleMessage message type is correctly implemented. This change is essential for the security feature that prevents path parameter overrides by ensuring these fields are explicitly defined in the message structure.
• 72-73: The new route bindings for resource_id and n_id in the EchoService service are correctly added, following the grpc-gateway conventions. These bindings are necessary for the new security feature to function correctly by ensuring that these parameters are handled appropriately in the URL routing.

runtime/query.go (2)

• 73-101: The normalizeFieldPath function is correctly implemented, ensuring that field paths are normalized based on the message descriptor. This is crucial for the security feature, as it ensures that query parameters are correctly mapped to their corresponding fields without allowing for path parameter overrides.
• 103-134: The findFieldFromPath function is well-implemented, providing a mechanism to retrieve a field descriptor and its value from a given field path. This function is essential for the new security feature, as it aids in accurately parsing and populating fields from a message, ensuring that query parameters do not override path parameters.

examples/internal/clients/unannotatedecho/api_unannotated_echo_service.go (3)

• 45-47: The addition of ResourceId, NIdNId, and NIdVal as optional parameters in the UnannotatedEchoServiceEchoOpts struct aligns with the PR's objectives to enhance security by preventing path parameter overrides. This ensures that these new fields are correctly handled in the API calls.
• 253-255: Similarly, the addition of ResourceId, NIdNId, and NIdVal as optional parameters in the UnannotatedEchoServiceEcho2Opts struct is consistent with the changes made in the UnannotatedEchoServiceEchoOpts struct, ensuring uniform handling of these new fields across different API calls.
• 595-597: The inclusion of ResourceId, NIdNId, and NIdVal as optional parameters in the UnannotatedEchoServiceEchoDeleteOpts struct is a necessary extension of the security feature to delete operations, ensuring that these fields can be passed securely without risking path parameter override.

examples/internal/clients/echo/api_echo_service.go (8)

• 43-45: The addition of ResourceId, NIdNId, and NIdVal as optional parameters in the method signatures is consistent with the PR's objective to enhance security by preventing query parameters from overriding path parameters. This change ensures that these new parameters are considered in the query construction, aligning with the security enhancement goals.
• 59-61: The inclusion of ResourceId, NIdNId, and NIdVal in the EchoServiceEchoOpts struct is correctly implemented. These fields are marked as optional, which is appropriate given their use case in query parameter construction. This change supports the PR's objective of enhancing URL routing security.
• 105-113: The implementation correctly adds the new optional parameters ResourceId, NIdNId, and NIdVal to the query parameters in the EchoServiceEcho method. This ensures that these parameters are included in the request, supporting the PR's security enhancements.
• 203-205: The addition of ResourceId, NIdNId, and NIdVal as optional parameters in the EchoServiceEcho2 method aligns with the PR's objectives. This consistent application of security enhancements across methods is noted and approved.
• 218-220: The inclusion of ResourceId, NIdNId, and NIdVal in the EchoServiceEcho2Opts struct is correctly implemented, supporting the PR's security objectives by allowing these parameters to be included in the query string.
• 262-270: The correct implementation of adding ResourceId, NIdNId, and NIdVal to the query parameters in the EchoServiceEcho2 method is noted. This consistency in applying security enhancements across different methods is crucial for the overall security objective of the PR.
• 360-362: The addition of ResourceId, NIdNId, and NIdVal as optional parameters in the EchoServiceEcho3 method is consistent with the PR's security enhancement goals. This change is correctly implemented.
• 374-376: The inclusion of ResourceId, NIdNId, and NIdVal in the EchoServiceEcho3Opts struct supports the PR's objective of enhancing security. These fields are correctly marked as optional and are implemented consistently across the service methods.

examples/internal/integration/integration_test.go (4)

• 49-51: The addition of testEchoPathParamOverwrite, testEchoNested, and testEchoNestedOverride functions expands the test coverage for the TestEcho function, specifically testing the prevention of path parameter override by query parameters.
• 355-381: The testEchoPathParamOverwrite function correctly tests the scenario where a query parameter (resourceId) attempts to override a path parameter. It validates that the path parameter's value remains unchanged, ensuring path parameters' immutability.
• 384-409: The testEchoNested function tests nested parameter handling, ensuring that nested query parameters (n_id.val) are correctly parsed and do not override path parameters. This test is crucial for validating the handling of complex parameter structures.
• 412-436: The testEchoNestedOverride function tests the prevention of nested path parameter override by nested query parameters (nId.nId). It ensures that nested path parameters remain immutable against query parameter manipulation, which is essential for maintaining the integrity of URL routing.

[ljmsc]

@coderabbitai review

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 15e1c5b and 7128b99.

Files selected for processing (3)

• examples/internal/proto/examplepb/echo_service.proto (3 hunks)
• examples/internal/proto/examplepb/unannotated_echo_service.proto (2 hunks)
• runtime/query.go (2 hunks)

Files skipped from review as they are similar to previous changes (2)

• examples/internal/proto/examplepb/echo_service.proto
• examples/internal/proto/examplepb/unannotated_echo_service.proto

Additional comments: 1

runtime/query.go (1)

• 55-60: The implementation of normalizeFieldPath within the Parse method effectively prevents query parameters from overriding path parameters by normalizing field paths. This change is crucial for enhancing security by ensuring the immutability of requested resource URLs. However, it's important to ensure that all possible edge cases are covered in the test scenarios, especially those involving nested fields and arrays.

Verification successful

---

The verification process has successfully identified the presence of test cases testEchoNested and testEchoNestedOverride within examples/internal/integration/integration_test.go. This confirms that the test scenarios covering edge cases for nested fields and arrays, as mentioned in the review comment, do exist in the codebase.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify that test scenarios cover edge cases for nested fields and arrays.
rg "testEchoNested" tests/
rg "testEchoNestedOverride" tests/

Length of output: 155

---

---

Script:

#!/bin/bash
# Attempting a broader search for test cases covering nested fields and arrays without assuming a specific directory name.
rg "testEchoNested" --type-add 'custom:*.go' --type custom
rg "testEchoNestedOverride" --type-add 'custom:*.go' --type custom

Length of output: 676

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 7128b99 and 07edb2b.

Files selected for processing (3)

• examples/internal/clients/echo/BUILD.bazel (1 hunks)
• examples/internal/clients/unannotatedecho/BUILD.bazel (1 hunks)
• runtime/query.go (2 hunks)

Additional comments: 5

examples/internal/clients/unannotatedecho/BUILD.bazel (1)

• 13-13: Added model_examplepb_unannotated_nested_message.go to the srcs list of the go_library rule. This change is straightforward and aligns with the PR's objective to enhance the Echo Service API and Unannotated Echo Service with new structs to support enhanced validation logic.

examples/internal/clients/echo/BUILD.bazel (1)

• 14-14: Added model_examplepb_nested_message.go to the srcs list of the go_library rule. This addition is consistent with the PR's goals to introduce new structs for supporting the updated validation logic in the Echo Service API.

runtime/query.go (3)

• 55-60: The logic within the Parse method now includes calls to normalizeFieldPath and populateFieldValueFromPath, reflecting the PR's strategy to prevent query parameters from overriding path parameters. This change is crucial for enhancing security. Ensure that the filter.HasCommonPrefix(fieldPath) check correctly filters out unwanted fields before attempting to populate field values.
• 73-103: The normalizeFieldPath function transforms field paths based on the message descriptor, ensuring that the field names used in the URL match the actual field names in the message. This function is a key part of the new validation logic. It's important to verify that this function correctly handles edge cases, such as fields not found in the message descriptor, and that it only allows singular message fields to be navigated. The logic appears sound, but testing for edge cases in field names (e.g., JSON names vs. Protobuf field names) is recommended.
• 104-104: The populateFieldValueFromPath function is responsible for setting the value of a field in a message based on the provided path and value(s). This function is central to the PR's functionality, ensuring that query parameters do not override path parameters. It's important to ensure comprehensive error handling and validation of field types, especially for repeated fields and maps. The implementation seems to align with these requirements, but additional testing for complex field types (e.g., maps, oneofs) is advised.

[johanbrandhorst]

LGTM

[johanbrandhorst]

Thank you so much for coming back and reimplementing a fix for this :). Trust me, I appreciate the security concerns. I'll prepare a new release with this fix in place.