fix: include json name to query param filter #3072
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
I think we'd need to add some tests to make sure we don't regress on this - do you think you could add a test to the integration tests that would have failed without this change? Also you'll need to regenerated the files after this update, see CONTRIBUTING.md in the root of the repository for a step by step guide.
|
Please rebase on |
ljmsc
force-pushed
the
ljmsc/path_param_json_name_filter
branch
from
85dbfbd to
ce75200
Compare
|
Thanks for your contribution! |
) [](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/grpc-ecosystem/grpc-gateway/v2](https://togithub.com/grpc-ecosystem/grpc-gateway) | require | minor | `v2.14.0` -> `v2.15.0` | --- ### Release Notes <details> <summary>grpc-ecosystem/grpc-gateway</summary> ### [`v2.15.0`](https://togithub.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.15.0) [Compare Source](https://togithub.com/grpc-ecosystem/grpc-gateway/compare/v2.14.0...v2.15.0) #### What's Changed - utilities: Add preallocate for encoded by [@​sashamelentyev](https://togithub.com/sashamelentyev) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3024](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3024) - chore(logspam): allow_repeated_fields_in_body is deprecated by [@​alexeagle](https://togithub.com/alexeagle) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3040](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3040) - Various fixes by [@​johanbrandhorst](https://togithub.com/johanbrandhorst) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3083](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3083) - fix(docs): add protoc-gen-openapiv2 to tutorial by [@​Fahmadi](https://togithub.com/Fahmadi) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3075](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3075) - fix: add check for repeated google.protobuf.Any type by [@​veith](https://togithub.com/veith) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3080](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3080) - Fix field order in yaml generated OAS file by [@​same-id](https://togithub.com/same-id) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3084](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3084) - fix: include json name to query param filter by [@​ljmsc](https://togithub.com/ljmsc) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3072](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3072) - Make read_only work with references using AllOf by [@​same-id](https://togithub.com/same-id) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3082](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3082) #### New Contributors - [@​alexeagle](https://togithub.com/alexeagle) made their first contribution in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3040](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3040) - [@​Fahmadi](https://togithub.com/Fahmadi) made their first contribution in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3075](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3075) - [@​ljmsc](https://togithub.com/ljmsc) made their first contribution in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3072](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3072) **Full Changelog**: grpc-ecosystem/grpc-gateway@v2.14.0...v2.15.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on monday" in timezone Australia/Sydney, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/google/osv.dev). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzMuMyJ9-->
](https://renovatebot.com){kind=link}
|
Hi, unfortunately I've had to revert this feature for the next release because it introduced a regression (not your fault, we just didn't see it when working on it). If you'd like to reintroduce this, please open a new PR and we can make sure we cover the case that was affected. Thanks! CC @ljmsc |
|
hey @johanbrandhorst thanks for letting me know about this change. I'm actually a bit confused about this since we talked about the breaking change in the original issue I created #3069 and you agreed that it is more important to fix a potential security issue. The revert of this change will introduce the security issue back into the system. :( |
|
Yes, it does reintroduce the security issue, but as you can see in #3848 this fix actually made it impossible to set any values on the referenced message through the query. Ideally we'd be able to reintroduce this fix in a way that still allows other fields on the same message to be set via the query. Does that make sense? |
|
It makes sense to fix the bug, yeah. But for me it doesn't make sense to reintroduce a security issue to the library because of a bug which wasn't noticed over a year. IMHO a security issue is much worse than a bug which seems to be only a corner case. |
References to other Issues or PRs
Fixes #3069
Have you read the Contributing Guidelines?
Yes
Brief description of what is fixed or changed
The fix will include the JSON name of the proto field into the query param filter to prevent the overwrite of the path parameters with query parameters