You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/08/18 01:20:07 UTC
Review Request 68420: Added regression test for encoded queries over
the `/files` API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
This test currently fails with:
```
src\tests\files_tests.cpp(346): error: Value of: (response)->status
Actual: "404 Not Found"
Expected: OK().status
Which is: "200 OK"
Body: ""
src\tests\files_tests.cpp(347): error: Value of: (response)->body
Actual: ""
Expected: stringify(expected)
Which is: "{\"data\":\"body\"}"
```
This is due to libprocess decoding the query in both `http::get()` and
in the `DataDecoder`. However, libprocess only encodes once. When the
user has a query with a literal ASCII escape sequence, it is expected
that it should only need to be encoded once. That is `foo%3Abar`
should be represented in the query as `foo%253Abar`. Because
libprocess decodes twice, but currently only encodes once, this query
erroneously becomes `foo:bar`. The added test demonstrates how this
breaks something such as the `files` server.
Diffs
-----
src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
Diff: https://reviews.apache.org/r/68420/diff/1/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 68420: Added /files API test for reserved query
characters.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/
-----------------------------------------------------------
(Updated Aug. 28, 2018, 10:40 a.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Updated with better explanation and a new test name and commit message.
Summary (updated)
-----------------
Added /files API test for reserved query characters.
Bugs: MESOS-9168
https://issues.apache.org/jira/browse/MESOS-9168
Repository: mesos
Description (updated)
-------
Due to a bug in the HTTP client in libprocess (MESOS-9168), the
use of reserved query characters in paths did not work when using
the HTTP client in libprocess. This adds a test to ensure that
the /files API correctly handles reserved query characters in
file paths.
Diffs (updated)
-----
src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
Diff: https://reviews.apache.org/r/68420/diff/2/
Changes: https://reviews.apache.org/r/68420/diff/1-2/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 68420: Added /files API test for reserved query
characters.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote:
> > src/tests/files_tests.cpp
> > Lines 331-332 (patched)
> > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line331>
> >
> > This is a little odd to read without the context, this tests reserved characters by using `%`, but we could use anything other reserved character to acheive the same test, e.g. `+` seems the simplest:
> >
> > ```
> > // We use a reserved character for query parameters `+` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo+bar";
> > ```
> >
> > If you'd like to stick with using `%`, probably we should just say:
> >
> > ```
> > // We use a reserved character for query parameters `%` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo%3Abar";
> > ```
> >
> > And not do line 334 as it's irrelevant to this test
>
> Andrew Schwartzmeyer wrote:
> You couldn't use `+` because it wouldn't have the same problem: the second decode would not turn `+` into something else (this is why we haven't run into this problem before with other characters, even a literal `:`). The problem is that `%3A` _can_ be decoded, and so decoding more times than encoding causes it to erroneously become `:`. The number of encodes and decodes must be balanced.
>
> Also, line 334 is relevant to this test: it's demonstrating that the query can be erroneously decoded too many times.
I explained why thits is explicitly an entire percent-encoding, and am considering this fixed. Thanks!
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/#review207830
-----------------------------------------------------------
On Aug. 28, 2018, 10:40 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68420/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2018, 10:40 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9168
> https://issues.apache.org/jira/browse/MESOS-9168
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Due to a bug in the HTTP client in libprocess (MESOS-9168), the
> use of reserved query characters in paths did not work when using
> the HTTP client in libprocess. This adds a test to ensure that
> the /files API correctly handles reserved query characters in
> file paths.
>
>
> Diffs
> -----
>
> src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
>
>
> Diff: https://reviews.apache.org/r/68420/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 68420: Added /files API test for reserved query
characters.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote:
> > src/tests/files_tests.cpp
> > Lines 331-332 (patched)
> > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line331>
> >
> > This is a little odd to read without the context, this tests reserved characters by using `%`, but we could use anything other reserved character to acheive the same test, e.g. `+` seems the simplest:
> >
> > ```
> > // We use a reserved character for query parameters `+` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo+bar";
> > ```
> >
> > If you'd like to stick with using `%`, probably we should just say:
> >
> > ```
> > // We use a reserved character for query parameters `%` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo%3Abar";
> > ```
> >
> > And not do line 334 as it's irrelevant to this test
>
> Andrew Schwartzmeyer wrote:
> You couldn't use `+` because it wouldn't have the same problem: the second decode would not turn `+` into something else (this is why we haven't run into this problem before with other characters, even a literal `:`). The problem is that `%3A` _can_ be decoded, and so decoding more times than encoding causes it to erroneously become `:`. The number of encodes and decodes must be balanced.
>
> Also, line 334 is relevant to this test: it's demonstrating that the query can be erroneously decoded too many times.
>
> Andrew Schwartzmeyer wrote:
> I explained why thits is explicitly an entire percent-encoding, and am considering this fixed. Thanks!
Okay I am going to assume this was "fix it, then ship it" as you marked the other two as ship it, and I'd like to get at least the tests and libprocess fix in.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/#review207830
-----------------------------------------------------------
On Aug. 28, 2018, 10:40 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68420/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2018, 10:40 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9168
> https://issues.apache.org/jira/browse/MESOS-9168
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Due to a bug in the HTTP client in libprocess (MESOS-9168), the
> use of reserved query characters in paths did not work when using
> the HTTP client in libprocess. This adds a test to ensure that
> the /files API correctly handles reserved query characters in
> file paths.
>
>
> Diffs
> -----
>
> src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
>
>
> Diff: https://reviews.apache.org/r/68420/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 68420: Added regression test for encoded queries
over the `/files` API.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote:
> > src/tests/files_tests.cpp
> > Lines 326 (patched)
> > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line326>
> >
> > How about: `ReservedQueryCharacter`
It's not quite the same. This isn't testing any arbitray reserved character existing in the query; it's explicitly testing a percent-encoded sequence in the query. How about: `QueryWithEncodedSequence`?
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote:
> > src/tests/files_tests.cpp
> > Lines 331-332 (patched)
> > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line331>
> >
> > This is a little odd to read without the context, this tests reserved characters by using `%`, but we could use anything other reserved character to acheive the same test, e.g. `+` seems the simplest:
> >
> > ```
> > // We use a reserved character for query parameters `+` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo+bar";
> > ```
> >
> > If you'd like to stick with using `%`, probably we should just say:
> >
> > ```
> > // We use a reserved character for query parameters `%` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo%3Abar";
> > ```
> >
> > And not do line 334 as it's irrelevant to this test
You couldn't use `+` because it wouldn't have the same problem: the second decode would not turn `+` into something else (this is why we haven't run into this problem before with other characters, even a literal `:`). The problem is that `%3A` _can_ be decoded, and so decoding more times than encoding causes it to erroneously become `:`. The number of encodes and decodes must be balanced.
Also, line 334 is relevant to this test: it's demonstrating that the query can be erroneously decoded too many times.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/#review207830
-----------------------------------------------------------
On Aug. 17, 2018, 6:20 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68420/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 6:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9168
> https://issues.apache.org/jira/browse/MESOS-9168
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test currently fails with:
>
> ```
> src\tests\files_tests.cpp(346): error: Value of: (response)->status
> Actual: "404 Not Found"
> Expected: OK().status
> Which is: "200 OK"
> Body: ""
> src\tests\files_tests.cpp(347): error: Value of: (response)->body
> Actual: ""
> Expected: stringify(expected)
> Which is: "{\"data\":\"body\"}"
> ```
>
> This is due to libprocess decoding the query in both `http::get()` and
> in the `DataDecoder`. However, libprocess only encodes once. When the
> user has a query with a literal ASCII escape sequence, it is expected
> that it should only need to be encoded once. That is `foo%3Abar`
> should be represented in the query as `foo%253Abar`. Because
> libprocess decodes twice, but currently only encodes once, this query
> erroneously becomes `foo:bar`. The added test demonstrates how this
> breaks something such as the `files` server.
>
>
> Diffs
> -----
>
> src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
>
>
> Diff: https://reviews.apache.org/r/68420/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 68420: Added regression test for encoded queries
over the `/files` API.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/#review207830
-----------------------------------------------------------
How about the following for commit summary and description:
```
Added /files API test for reserved query characters.
Due to a bug in the http client in libprocess (MESOS-9168), the
use of reserved query characters in paths did not work when using
the http client in libprocess. This adds a test to ensure that
the /files API correctly handles reserved query characters in
file paths.
```
src/tests/files_tests.cpp
Lines 323-325 (patched)
<https://reviews.apache.org/r/68420/#comment291357>
How about:
```
// Tests paths with reserved characters that must be percent
// encoded in the http request query.
```
src/tests/files_tests.cpp
Lines 326 (patched)
<https://reviews.apache.org/r/68420/#comment291359>
How about: `ReservedQueryCharacter`
src/tests/files_tests.cpp
Lines 331-332 (patched)
<https://reviews.apache.org/r/68420/#comment291358>
This is a little odd to read without the context, this tests reserved characters by using `%`, but we could use anything other reserved character to acheive the same test, e.g. `+` seems the simplest:
```
// We use a reserved character for query parameters `+` to
// ensure it is encoded and decoded correctly.
const string filename = "foo+bar";
```
If you'd like to stick with using `%`, probably we should just say:
```
// We use a reserved character for query parameters `%` to
// ensure it is encoded and decoded correctly.
const string filename = "foo%3Abar";
```
And not do line 334 as it's irrelevant to this test
- Benjamin Mahler
On Aug. 18, 2018, 1:20 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68420/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2018, 1:20 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9168
> https://issues.apache.org/jira/browse/MESOS-9168
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test currently fails with:
>
> ```
> src\tests\files_tests.cpp(346): error: Value of: (response)->status
> Actual: "404 Not Found"
> Expected: OK().status
> Which is: "200 OK"
> Body: ""
> src\tests\files_tests.cpp(347): error: Value of: (response)->body
> Actual: ""
> Expected: stringify(expected)
> Which is: "{\"data\":\"body\"}"
> ```
>
> This is due to libprocess decoding the query in both `http::get()` and
> in the `DataDecoder`. However, libprocess only encodes once. When the
> user has a query with a literal ASCII escape sequence, it is expected
> that it should only need to be encoded once. That is `foo%3Abar`
> should be represented in the query as `foo%253Abar`. Because
> libprocess decodes twice, but currently only encodes once, this query
> erroneously becomes `foo:bar`. The added test demonstrates how this
> breaks something such as the `files` server.
>
>
> Diffs
> -----
>
> src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
>
>
> Diff: https://reviews.apache.org/r/68420/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>