Added veryfication if path contains query params and add them to path header by lukidzi · Pull Request #6466 · envoyproxy/envoy

lukidzi · 2019-04-02T18:42:02Z

Currently when you send request with query parameters they are striped.
Added veryfication if path contains query params and if so append path with query parameters and then set header :path.

I think there is problem when you have in your configuration:

http_protocol_options:
   allow_absolute_url: true

Risk Level: low
Testing: added new test cases
Docs Changes: n/a
Release Notes: n/a
Part of #6459

Signed-off-by: Lukasz Dziedziak lukasz.dziedziak@allegro.pl

…end path headers if it has them Risk Level: low Testing: added new test cases Docs Changes: n/a Release Notes: n/a Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

alyssawilk

Great catch, and thanks for the quick and well tested fix as well!

alyssawilk · 2019-04-03T16:54:58Z

source/common/http/utility.cc

@@ -57,8 +57,15 @@ bool Utility::Url::initialize(absl::string_view absolute_url) {
  // RFC allows the absolute-uri to not end in /, but the absolute path form
  // must start with
  if ((u.field_set & (1 << UF_PATH)) == (1 << UF_PATH) && u.field_data[UF_PATH].len > 0) {


It might be a bit easier to see the differences here if we split out length settings, e.g.

uint64_t path_len = u.field_data[UF_PATH].len;
if ([query param exists) {
path_len += 1 + u.field_data[UF_PATH].len
}
path_ = path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off, path_len);

yes it looks much better.

alyssawilk · 2019-04-03T16:56:46Z

source/common/http/utility.cc

+      path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off,
+                                u.field_data[UF_PATH].len + 1 + u.field_data[UF_QUERY].len);
+    } else {
+      path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off,


What do you think of renaming path_ to path_and_query_params_ or some variant, to make it clear it includes both? I don't think it's used too many places and I could see the clarity being useful going forward

I've changed to one you suggested

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

alyssawilk

Looks good - just one remaining question!

alyssawilk · 2019-04-04T12:58:33Z

source/common/http/utility.cc

-    path_ = absl::string_view(absolute_url.data() + u.field_data[UF_PATH].off,
-                              u.field_data[UF_PATH].len);
+    uint64_t path_len = u.field_data[UF_PATH].len;
+    if ((u.field_set & (1 << UF_QUERY)) == (1 << UF_QUERY) && u.field_data[UF_QUERY].len > 0) {


My one remaining question - is it possible for UF_QUERY to be set and u.field_data[UF_QUERY].len == 0?

If path length is 0 we substitute a hard-coded "/". If the +1 for length is due to the "?" being considered the delimiter I'd like to make sure that http://foo.com? would get proxied without losing the ?. Can you add a test for that corner case?

Ok I investigated it a bit.
Http url parser works different for path / and ?<-query start :
e.g: http://foo.com/? it sets bit to 1 for UF_PATH but won't set UF_QUERY.
When there is query start sign(?) it will go to the next iteration and won't set UF_QUERY bit to 1. So it is impossible to get this information from parser. Same for http://foo.com?. When it gets to (?) sign and won't see another character after (?) it won't set UF_QUERY bit to 1.
I added more test cases and fixed issue when url looks like: http://foo.com?query=param

for url: http://foo.com/ -> UF_PATH bit is set and len = 1

Oh nice, thanks for digging into this, and adding the extra corner case testing!

So while this is clearly an improvement over what we have, I'm still pretty uncomfortable with /? being normalized to / unless it's illegal by the spec.

I'd be fine

if this is correct as defined by some corner of the URL spec (@PiotrSikora do you know this one?)

landing this as is today to get most query params in this Envoy build, and leaving the bug open to sort out to retain the ? next week

setting the host to "everything after the beginning of the path delimiter", though we're getting close enough to merge freeze that might lose us this landing in this release.

I'd lean towards 2. @mattklein123 thoughts/preferences?

(2) SGTM, agreed.

Cool. Merged because I'd prefer the clearly-better-behavior to make next week's release.

Lukasz, thanks so much for tackling this. Bonus points if you'd be willing to poke through the spec or sync with @PiotrSikora on expected behavior for /? and tackle that in a follow-up :-)

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

* master: (137 commits) test: router upstream log to v2 config stubs (envoyproxy#6499) remove idle timeout validation (envoyproxy#6500) build: Change namespace of chromium_url. (envoyproxy#6506) coverage: exclude chromium_url (envoyproxy#6498) fix(tracing): allow 256 chars in path tag (envoyproxy#6492) Common: Introduce StopAllIteration filter status for decoding and encoding filters (envoyproxy#5954) build: update PGV url (envoyproxy#6495) subset lb: avoid partitioning host lists on worker threads (envoyproxy#6302) ci: Make envoy_select_quiche no-op. (envoyproxy#6393) watcher: notify when watched files are modified (envoyproxy#6215) stat: Add counterFromStatName(), gaugeFromStatName(), and histogramFromStatName() (envoyproxy#6475) bump to 1.11.0-dev (envoyproxy#6490) release: bump to 1.10.0 (envoyproxy#6489) hcm: path normalization. (#1) build: import manually minified Chrome URL lib. (envoyproxy#3) codec: reject embedded NUL in headers. (envoyproxy#2) Added veryfication if path contains query params and add them to path header (envoyproxy#6466) redis: basic integration test for redis_proxy (envoyproxy#6450) stats: report sample count as an integer to prevent loss of precision (envoyproxy#6274) Added VHDS protobuf message and updated RouteConfig to include it. (envoyproxy#6418) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Description: Added veryfication if path contains query params and app…

5dc3388

…end path headers if it has them Risk Level: low Testing: added new test cases Docs Changes: n/a Release Notes: n/a Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

lukidzi changed the title ~~Description: Added veryfication if path contains query params and app…~~ Added veryfication if path contains query params and add them to path header Apr 2, 2019

mattklein123 assigned alyssawilk Apr 2, 2019

alyssawilk reviewed Apr 3, 2019

View reviewed changes

Renamed path_ to path_and_query_params_. Refactored added method.

3a0c97a

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

alyssawilk reviewed Apr 4, 2019

View reviewed changes

added more test cases

059b14b

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

alyssawilk approved these changes Apr 4, 2019

View reviewed changes

alyssawilk merged commit 1cf59d4 into envoyproxy:master Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added veryfication if path contains query params and add them to path header#6466

Added veryfication if path contains query params and add them to path header#6466
alyssawilk merged 3 commits intoenvoyproxy:masterfrom
lukidzi:missing_query_params_absolute

lukidzi commented Apr 2, 2019 •

edited by alyssawilk

Loading

Uh oh!

alyssawilk left a comment

Uh oh!

alyssawilk Apr 3, 2019

Uh oh!

lukidzi Apr 3, 2019

Uh oh!

alyssawilk Apr 3, 2019

Uh oh!

lukidzi Apr 3, 2019

Uh oh!

alyssawilk left a comment

Uh oh!

alyssawilk Apr 4, 2019

Uh oh!

lukidzi Apr 4, 2019 •

edited

Loading

Uh oh!

alyssawilk Apr 4, 2019

Uh oh!

mattklein123 Apr 4, 2019

Uh oh!

alyssawilk Apr 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lukidzi commented Apr 2, 2019 • edited by alyssawilk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk left a comment

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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukidzi Apr 4, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukidzi commented Apr 2, 2019 •

edited by alyssawilk

Loading

lukidzi Apr 4, 2019 •

edited

Loading