http: Add ability to merge slashes by euroelessar · Pull Request #7621 · envoyproxy/envoy

euroelessar · 2019-07-17T22:03:02Z

Description: Add an optional ability to merge slashes in the request's path
Risk Level: LOW (opt-in)
Testing: unit tests
Docs Changes: inline protobuf documentation
Release Notes: updated
Fixes #7611

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

zuercher · 2019-07-18T00:48:31Z

/retest

repokitteh-read-only · 2019-07-18T00:48:34Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #7621 (comment) was created by @zuercher.

see: more, trace.

zuercher · 2019-07-18T00:49:03Z

/assign @jmarantz could you have a look?

repokitteh-read-only · 2019-07-18T00:49:07Z

🙀 Error while processing event:

evaluation error
error: function _rk_error error: path contains forbidden characters

🐱

Caused by: a #7621 (comment) was created by @zuercher.

see: more, trace.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

venilnoronha

Thanks for working on this.

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

venilnoronha · 2019-07-18T16:14:36Z

source/common/http/conn_manager_utility.cc

+    is_valid_path = PathUtil::canonicalPath(*request_headers.Path());
  }
-  return true;
+  if (config.shouldMergeSlashes()) {


Why's the path validity checked on the original value i.e. prior to merging slashes? I'd add a comment explaining that.

why bother to merge slashes if is_valid_path is false?

source/common/http/path_utility.cc

source/common/http/path_utility.h

test/common/http/path_utility_test.cc

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2019-07-23T20:45:47Z

@venilnoronha Please have another look, and let me know if there is any outstanding feedback.

venilnoronha

LGTM. Thanks!

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2019-07-24T23:07:33Z

Can this change be merged, please?

mattklein123 · 2019-07-24T23:19:34Z

@euroelessar a maintainer has to review it. Hopefully @jmarantz can take a look soon.

jmarantz · 2019-07-24T23:42:28Z

source/common/http/conn_manager_utility.cc

+    is_valid_path = PathUtil::canonicalPath(*request_headers.Path());
  }
-  return true;
+  if (config.shouldMergeSlashes()) {


why bother to merge slashes if is_valid_path is false?

jmarantz · 2019-07-24T23:42:56Z

source/common/http/path_utility.cc

  return true;
 }

+/* static */


I don't think we use this style of commenting that a method is static in Envoy.

jmarantz · 2019-07-24T23:54:21Z

source/common/http/path_utility.cc

+  for (size_t i = 1; i < original_path.size(); ++i) {
+    if (original_path[i] == '/' && original_path[i - 1] == '/') {
+      has_adjacent_slashes = true;
+      break;


capture start-index here to avoid re-searching the same text.

Better yet, use more absl strings magic, to simplify the code

absl::string_view::size_type query_start = orignal_path.find('?'); absl::string_view query; if (query_start != absl::string_view::npos) { query = original_path.substr(query_start); original_path = original_path.substr(0, query_start); } if (original_path.find("//") != absl::string_view::npos) { path_header.value(absl::StrCat(absl::StrReplace(original_path, {{"//", "/"}}), query)); }

WDYT? I'm not 100% sure about all the corner cases like an input of "///" or "////" and what they should do.

Unfortunately absl::StrReplace-based approach does not handle triple (or more) slashes, but I've simplified the rest of the code based on your recommendation.

jmarantz · 2019-07-24T23:56:13Z

test/common/http/path_utility_test.cc

+  const std::vector<std::pair<std::string, std::string>> non_normal_pairs{
+      {"", ""},                        // empty
+      {"/a", "/a"},                    // no-op
+      {"//a/b/c", "/a/b/c"},           // double / start


include some triple and quadruple slashes in your test suite.

jmarantz · 2019-07-24T23:59:54Z

test/common/http/path_utility_test.cc

+    auto& path_header = pathHeaderEntry(path_pair.first);
+    PathUtil::mergeSlashes(path_header);
+    EXPECT_EQ(path_header.value().getStringView(), path_pair.second)
+        << "original path: " << path_pair.first;


WDYT of turning the body of the loop into a lambda or test-helper method std::string normalizePath(absl::string_view path) and then writing:

EXPECT_EQ("/a/b/c", normalizePath("//a/b/c"));

what you have is idiomatic for golang but most C++ tests I see look more like my suggestion. I'm OK with what you have; just an idea.

jmarantz · 2019-07-25T00:00:46Z

test/common/http/path_utility_test.cc

+    EXPECT_EQ(path_header.value().getStringView(), path_pair.second)
+        << "original path: " << path_pair.first;
+  }
+}


include a test method that also ensures that no slash-merging takes place when the option is off.

It's covered by ConnectionManagerUtilityTest::MergeSlashesDefaultOff test

I see, ok, that makes sense. I think handling of 3 or more slashes should be covered here, though, as this is the functional unit-test.

Test for handling of 3 or more slashes was already added in the latest revision

jmarantz · 2019-07-25T00:09:18Z

Thanks for doing this, and thanks @venilnoronha for the first review. Just a few nits/cleanups and we should be good to go.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

jmarantz · 2019-07-25T08:06:49Z

source/common/http/path_utility.cc

-  }
-  if (!has_adjacent_slashes) {
+  // Only operate on path component in URL.
+  const size_t query_start = original_path.find('?');


s/size_t/absl::string_view::size_type

jmarantz · 2019-07-25T08:06:49Z

source/common/http/path_utility.cc

+  // Only operate on path component in URL.
+  const size_t query_start = original_path.find('?');
+  const auto path = original_path.substr(0, query_start);
+  const auto query = absl::ClippedSubstr(original_path, query_start);


Handling the npos case for query_start does not appear to be a documented use of substr etc.

Point 24.4.2. of C++17 standard specifies that std::base_string_view::substr lets rlen be the smaller of n and size() - pos, which covers npos.
absl::string_view is API-compatible with std::string_view so I assume it inherits the spec behavior even if it's not called out in the documentation explicitly.

jmarantz · 2019-07-25T09:22:37Z

source/common/http/path_utility.cc

-  if (!has_adjacent_slashes) {
+  // Only operate on path component in URL.
+  const size_t query_start = original_path.find('?');
+  const auto path = original_path.substr(0, query_start);


use explicit types in this context as type is not obvious from context unless you know string_view well.

https://google.github.io/styleguide/cppguide.html#auto

jmarantz · 2019-07-25T09:25:36Z

source/common/http/path_utility.cc

  simplified_path.reserve(original_path.size());
-  for (size_t i = 0; i < original_path.size(); ++i) {
-    if (i > 0 && original_path[i] == '/' && original_path[i - 1] == '/') {
+  for (size_t i = 0; i < path.size(); ++i) {


I'd still prefer using the absl functions for this to avoid having to look carefully at string-hacking code for common patterns. But absl::Substitute is actually preferred for strings known at compile-time. Just call it in a loop till there are no more occurrences of "//" (change the 'if (path.find" to a while-loop, reversing the polarity of the condition.

I thought about this further as my 'while' suggestion would be n^2 on a string with 100 consecutive slashes.

Let's do the initial scan for "//" via if, but then we can just use split/join to remove duplicate slashes. WDYT?

return absl::StrCat(absl::StrJoin(absl::StrSplit(original_path, "/"), absl::SkipEmpty), "/"), query);

In this case we would also need to either special-case / prefix or assume that all paths are always absolute.

Yeah also trailing slash, so cases to handle are:

/a//b /a//b/ a//b a//b/

It's still probably simpler than the character-at-a-time scan but maybe there's a better absl util to use?

probably good to add those cases anyway to the test-suite regardless of the algo we use

updated, as well as added extra tests

jmarantz · 2019-07-25T09:26:33Z

test/common/http/path_utility_test.cc

-        << "original path: " << path_pair.first;
-  }
+    auto sanitized_path_value = path_header.value().getStringView();
+    return std::string(sanitized_path_value.begin(), sanitized_path_value.end());


return std::string(path_header.value().getStringView());

jmarantz · 2019-07-25T09:35:38Z

test/common/http/path_utility_test.cc

+    EXPECT_EQ(path_header.value().getStringView(), path_pair.second)
+        << "original path: " << path_pair.first;
+  }
+}


I see, ok, that makes sense. I think handling of 3 or more slashes should be covered here, though, as this is the functional unit-test.

jmarantz · 2019-07-25T09:37:00Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // Determines if adjacent slashes in the path are merged into one before any processing of
+  // requests by HTTP filters or routing. This affects the upstream *:path* header as well. Without
+  // setting this option incoming requests with path `//dir/file` will not match against route with
+  // `prefix` match set to `/dir`. Defaults to `false`.


can you point to the HTTP spec indicating that multiple slashes should be merged? I suspect such a spec for canonicalized URL paths exists -- same place where it discusses handling of "..".

This should referenced in the comments or maybe even in this doc.

HTTP spec does not specify that slashes should be merged, moreover it explicitly states that behavior around slashes is resource-specific (and this is one of the reasons to make it optional, and not enable by default).
However it's useful in real world applications to provide a safe-guard against user errors, when we know for sure that the intended behavior is similar to filesystem one.

Indeed it doesn't. It does discuss treatment of ".." which Envoy does, but this is AFAICT not a specified behavior. https://tools.ietf.org/html/rfc3986#section-6

WDYT of making this a filter rather than putting it in the core, since it's really an optional extension? I think it's possible for a filter to adjust the path. I don't feel too strongly either way, but with @mattklein123 we've been talking about building a bare-bones Envoy with only the core functionality and if this were a filter in test/extensions/filters/http/... it would be easier to compile it out. Not that it's that expensive, but it might make sense to keep the core in-spec and as small as possible.

I don't have a strong preference here and can follow a maintainers' decision.
One thing to consider is that until #6602 is resolved invalidating routing decision is somehow expensive. (and we have to redo routing on a path change)

IMO it's fine to have this type of thing be built in as an option. @alyssawilk @PiotrSikora any objection?

My only concern is the slippery slope of too many knobs.
I'm going to claim this passes the two company test, as we elide at least initial multiple slashes ourselves and my guess is we can get away with eliding all but we should either make sure things pass the multi-user test or consider making them pluggable.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

jmarantz

basically lgtm modulo the question about whether this should be a filter or core functionality, which I think can be part of the @envoyproxy/senior-maintainers review.

Thanks for all the updates :)

jmarantz · 2019-07-26T09:50:21Z

docs/root/intro/version_history.rst

 * config: async data access for local and remote data source.
 * config: changed the default value of :ref:`initial_fetch_timeout <envoy_api_field_core.ConfigSource.initial_fetch_timeout>` from 0s to 15s. This is a change in behaviour in the sense that Envoy will move to the next initialization phase, even if the first config is not delivered in 15s. Refer to :ref:`initialization process <arch_overview_initialization>` for more details.
 * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`.
+* http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path.


it's probably worth noting here that this canonicalization is not mentioned in any HTTP spec, but is offered as an opt-in convenience to upstreams.

mattklein123

LGTM on the general approach with 1 comment. Please also merge master to pick up the new CI config. Thank you!

/wait

mattklein123 · 2019-07-26T16:07:17Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // requests by HTTP filters or routing. This affects the upstream *:path* header as well. Without
+  // setting this option incoming requests with path `//dir/file` will not match against route with
+  // `prefix` match set to `/dir`. Defaults to `false`.
+  google.protobuf.BoolValue merge_slashes = 33;


This can be a simple bool type as it defaults to false.

mattklein123 · 2019-07-26T16:07:33Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // Determines if adjacent slashes in the path are merged into one before any processing of
+  // requests by HTTP filters or routing. This affects the upstream *:path* header as well. Without
+  // setting this option incoming requests with path `//dir/file` will not match against route with
+  // `prefix` match set to `/dir`. Defaults to `false`.


IMO it's fine to have this type of thing be built in as an option. @alyssawilk @PiotrSikora any objection?

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

* use bool Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

mattklein123

Thanks this LGTM but will wait until Monday when @alyssawilk is back in the office to see if she or @PiotrSikora have any comments. Nice work!

PiotrSikora

LGTM, thanks!

alyssawilk

Looks great! A few extra thoughts from my pass

alyssawilk · 2019-07-29T13:45:43Z

source/common/http/path_utility.cc

  return true;
 }

+void PathUtil::mergeSlashes(HeaderEntry& path_header) {


I'm a bit twitchy about full qualified URLs here. Granted, Envoy currently handles fully qualified urls in the H1 codec by splitting them up, so if the incoming request is GET http://foo.com//bar we'll have :authority foo.com and :path /bar so this is fine today (we won't end up with http:/foo.com/bar).

It might be worth an assert that the path is relative, just in case someone does some lua or use defined headers trying to take advantage of Path == url in firstline and having their absolute URL messed up.

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

alyssawilk · 2019-07-29T13:49:22Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // Determines if adjacent slashes in the path are merged into one before any processing of
+  // requests by HTTP filters or routing. This affects the upstream *:path* header as well. Without
+  // setting this option incoming requests with path `//dir/file` will not match against route with
+  // `prefix` match set to `/dir`. Defaults to `false`.


My only concern is the slippery slope of too many knobs.
I'm going to claim this passes the two company test, as we elide at least initial multiple slashes ourselves and my guess is we can get away with eliding all but we should either make sure things pass the multi-user test or consider making them pluggable.

alyssawilk · 2019-07-29T13:51:33Z

source/common/http/path_utility.cc

+  }
+  const absl::string_view prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view();
+  const absl::string_view suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view();
+  path_header.value(absl::StrCat(


I assume this is O(n) if we get something whacky like a path which is 16k worth of / yeah?

I don't see anything explicit in the absl documentation, but it's O(n) based on the code.

test/common/http/path_utility_test.cc

* update example in docs Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only · 2019-07-30T02:56:38Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7621 was synchronize by euroelessar.

see: more, trace.

mattklein123

Thanks!

mattklein123 · 2019-07-30T17:08:50Z

/azp run envoy-macos

azure-pipelines · 2019-07-30T17:08:59Z

Azure Pipelines successfully started running 1 pipeline(s).

Ruslan Nigmatullin added 2 commits July 17, 2019 15:01

http: Add ability to merge slashes

8af987f

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

fix spelling

5d4a821

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

zuercher assigned jmarantz Jul 18, 2019

off by one

43bd6be

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

venilnoronha reviewed Jul 18, 2019

View reviewed changes

Ruslan Nigmatullin added 3 commits July 18, 2019 17:19

address cr for comments

eb0c731

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

spelling

d0430b2

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into merge-slashes

a917d92

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

venilnoronha previously approved these changes Jul 23, 2019

View reviewed changes

Merge branch 'master' of github.com:envoyproxy/envoy into merge-slashes

7a42b3c

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar dismissed venilnoronha’s stale review via 7a42b3c July 24, 2019 23:06

jmarantz reviewed Jul 25, 2019

View reviewed changes

cr

ed6b4f3

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

jmarantz reviewed Jul 25, 2019

View reviewed changes

Ruslan Nigmatullin added 2 commits July 25, 2019 12:32

cr 2

bb05f02

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

use char

09018a7

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

jmarantz previously approved these changes Jul 25, 2019

View reviewed changes

jmarantz reviewed Jul 26, 2019

View reviewed changes

mattklein123 self-assigned this Jul 26, 2019

mattklein123 requested changes Jul 26, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 26, 2019

Ruslan Nigmatullin added 2 commits July 26, 2019 09:46

Merge branch 'master' of github.com:envoyproxy/envoy into merge-slashes

57fa3b4

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

* extend comment

958a55a

* use bool Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar dismissed jmarantz’s stale review via 958a55a July 26, 2019 18:51

repokitteh-read-only bot removed the waiting label Jul 26, 2019

fix compilation for usages

164cd4b

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

mattklein123 previously approved these changes Jul 26, 2019

View reviewed changes

PiotrSikora previously approved these changes Jul 28, 2019

View reviewed changes

alyssawilk reviewed Jul 29, 2019

View reviewed changes

mattklein123 added the waiting label Jul 29, 2019

* add test for double slash at end

16c08c4

* update example in docs Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar dismissed stale reviews from PiotrSikora and mattklein123 via 16c08c4 July 30, 2019 02:56

repokitteh-read-only bot removed the waiting label Jul 30, 2019

mattklein123 approved these changes Jul 30, 2019

View reviewed changes

mattklein123 merged commit deccb6e into envoyproxy:master Jul 30, 2019

Conversation

euroelessar commented Jul 17, 2019

Uh oh!

zuercher commented Jul 18, 2019

Uh oh!

repokitteh-read-only bot commented Jul 18, 2019

Uh oh!

zuercher commented Jul 18, 2019

Uh oh!

repokitteh-read-only bot commented Jul 18, 2019

Uh oh!

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

euroelessar commented Jul 23, 2019

Uh oh!

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

euroelessar commented Jul 24, 2019

Uh oh!

mattklein123 commented Jul 24, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jul 25, 2019

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!

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!