Support full decoding of reserved expansions by euroelessar · Pull Request #45 · grpc-ecosystem/grpc-httpjson-transcoding

euroelessar · 2020-11-12T21:27:29Z

Implement underlying logic for google.api.Http.fully_decode_reserved_expansion.
Add a configurable option to always decode all path and query bindings.

Fixes #44

qiwzhang · 2020-11-12T21:47:04Z

src/include/grpc_transcoding/path_matcher.h

      // For multipart matches only unescape non-reserved characters.
-      binding.value += UrlUnescapeString(parts[i], !is_multipart);
+      binding.value += UrlUnescapeString(
+          parts[i], fully_decode_reserved_expansion || !is_multipart);


Hmm, we should not decode %2F even with fully_decode_reserved_expansion_

Hm, not decoding everything beats the purpose of this change..

Depends on how we set this flag fully_decode_reserved_expansion_? If it is from option.http, I think we should follow it.
At least its comment clearly say so.

We can add a flag in envoy grpc_transcode filter_config, as always_decode_reserved_. it may serve your purpose. I will go for it.

Since you are here, maybe we can support both:

one from proto file option.http, and 2) one from grpc_trancode filter_config.

since they are mutually exclusive, we can do: if 2) exists, don't use 1), otherwise check 1).

Sounds good, let me rename the function/field with corresponding docs.

Works for me

Added support for both options, covered logic of all their combinations by tests

euroelessar · 2020-11-13T01:29:01Z

test/path_matcher_test.cc

  return os;
 }

+}  // namespace


Note: this is unrelated change, but it fixes pretty debug printing of Binding type in gtest (operator<< must be in the same namespace as the type it covers).

qiwzhang · 2020-11-13T01:41:01Z

src/include/grpc_transcoding/path_matcher.h

+  //
+  // The default behavior is to not decode RFC 6570 reserved characters in multi
+  // segment matches.
+  void SetFullyDecodeReservedExpansion(bool value) {


I like the names to be more specific for readability. how about UrlUnescapeSpec?

These two bools are mutual exclusive, Can we combine them into one flag with an enum value?

Thanks, updated

qiwzhang · 2020-11-13T01:52:52Z

test/path_matcher_test.cc

+TEST_F(PathMatcherTest,
+       OnlyUnreservedCharsAreUnescapedForMultiSegmentMatchAlwaysDecode) {
+  SetFullyDecodeReservedExpansion(true);
+  // All %XX are reserved characters, they should be decoded.


comment is wrong, slash is not be decoded

Thanks, updated

qiwzhang · 2020-11-13T02:11:52Z

src/include/grpc_transcoding/path_matcher.h

-                            always_decode_);
+    ExtractBindingsFromPath(
+        method_data->variables, parts, variable_bindings,
+        unescape_spec_ == UrlUnescapeSpec::kAllCharacters ||


We should not use two arguments to control one aspect. We should just one one argument. Same as UrlUnescape function.

Passed UrlUnescapeSpec all the way through.

qiwzhang · 2020-11-13T02:13:12Z

Cool, just one minor request.

qiwzhang · 2020-11-13T02:30:01Z

src/include/grpc_transcoding/path_matcher.h

-                    bool unescape_reserved_chars, bool unescape_slash_char,
-                    char* out) {
+                    UrlUnescapeSpec unescape_spec, char* out) {
+  const bool unescape_slash_char =


the code readability is bad, you are converting an enum type into two bool, inside the loop, can we just use switch() on the spec. and handle it accordingly. the code will be much easier to read

great point, addressed

qiwzhang

Very good. Thanks

qiwzhang · 2020-11-13T03:26:46Z

@euroelessar Thanks for the PR. You need to update envoy dependent to get this change and hook it up. Thanks

…#14009) Functionality is implemented in grpc-ecosystem/grpc-httpjson-transcoding#45, add configuration options and wire them into filter. The reasoning is provided in grpc-ecosystem/grpc-httpjson-transcoding#44. Risk Level: Low Testing: added unit test Docs Changes: added proto docs Release Notes: updated Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

… (#14009) Functionality is implemented in grpc-ecosystem/grpc-httpjson-transcoding#45, add configuration options and wire them into filter. The reasoning is provided in grpc-ecosystem/grpc-httpjson-transcoding#44. Risk Level: Low Testing: added unit test Docs Changes: added proto docs Release Notes: updated Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com> Mirrored from https://github.com/envoyproxy/envoy @ 39e43e1abb45c96993f2df3323df12bc94937910

…envoyproxy#14009) Functionality is implemented in grpc-ecosystem/grpc-httpjson-transcoding#45, add configuration options and wire them into filter. The reasoning is provided in grpc-ecosystem/grpc-httpjson-transcoding#44. Risk Level: Low Testing: added unit test Docs Changes: added proto docs Release Notes: updated Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

…envoyproxy#14009) Functionality is implemented in grpc-ecosystem/grpc-httpjson-transcoding#45, add configuration options and wire them into filter. The reasoning is provided in grpc-ecosystem/grpc-httpjson-transcoding#44. Risk Level: Low Testing: added unit test Docs Changes: added proto docs Release Notes: updated Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com> Signed-off-by: Qin Qin <qqin@google.com>

… (#14009) Functionality is implemented in grpc-ecosystem/grpc-httpjson-transcoding#45, add configuration options and wire them into filter. The reasoning is provided in grpc-ecosystem/grpc-httpjson-transcoding#44. Risk Level: Low Testing: added unit test Docs Changes: added proto docs Release Notes: updated Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Support full decoding of reserved expansions

9b117c4

qiwzhang reviewed Nov 12, 2020

View reviewed changes

qiwzhang mentioned this pull request Nov 12, 2020

UrlUnescapeString irreversabily transforms data #44

Closed

Ruslan Nigmatullin added 2 commits November 12, 2020 17:26

Add SetAlwaysDecode, cover both cases by tests

22987af

rm test

cfada35

euroelessar commented Nov 13, 2020

View reviewed changes

qiwzhang reviewed Nov 13, 2020

View reviewed changes

Ruslan Nigmatullin added 2 commits November 12, 2020 17:59

CR

87fd3fb

move field init to field definition

06868a8

grpc-ecosystem deleted a comment from euroelessar Nov 13, 2020

qiwzhang reviewed Nov 13, 2020

View reviewed changes

pass UrlUnescapeSpec all the way through

aa0b113

qiwzhang reviewed Nov 13, 2020

View reviewed changes

use switch

8d0ff5f

qiwzhang approved these changes Nov 13, 2020

View reviewed changes

qiwzhang merged commit 4d095f0 into grpc-ecosystem:master Nov 13, 2020

euroelessar mentioned this pull request Nov 13, 2020

grpc-json-transcoder: Add support for configuring unescaping behavior envoyproxy/envoy#14009

Merged

Conversation

euroelessar commented Nov 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Nov 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang left a comment

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Nov 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qiwzhang Nov 13, 2020 •

edited

Loading