Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude %2F(/) from decoding of percents in a request path. #3855

Merged
merged 3 commits into from Oct 5, 2021

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Oct 5, 2021

Motivation:

Currently, %2F and %2f are converted into / while decoding
request paths. It might route a path to an unexpected service if users
really want to send %2F as data itself.
Furthermore, other specifications such as gRPC transcoding do not
expect to decode %2F to /.
https://github.com/googleapis/googleapis/blob/02710fa0ea5312d79d7fb986c9c9823fb41049a9/google/api/http.proto#L257-L258
It should be better to exclude %2F for less confusion and better
interop with other echo systems.

Modifications:

  • Exclude %2F and %2f from decoding percent-encoded characters in paths.

Result:

%2F and %2f are no longer converted to / when decoding a request path.

Motivation:

Currently, `%2F` and `%2f` are converted into `/` while decoding
request paths. It might route a path to an unexpected service if users
really want to send `%2F` as data itself.
Furthermore, other specifications such as gRPC transcoding do not
expect to decode `%2F` as `/`.
https://github.com/googleapis/googleapis/blob/02710fa0ea5312d79d7fb986c9c9823fb41049a9/google/api/http.proto#L257-L258
It should be better to exclude `%2F` for less confusion and better
interop with other echo systems.

Modifications:

- Exclude `%2F` and `%2f` from decoding percent-encoded characters in paths.

Result:

`%2F` and `%2f` are no longer converted to `/` when decoding a request path.
@ikhoon ikhoon added the defect label Oct 5, 2021
@ikhoon ikhoon added this to the 1.12.0 milestone Oct 5, 2021
wasSlash = decoded == '/';
if (decoded == '/') {
// Do not decode '%2F' and '%2f' in the path to '/' for compatibility with
// other echo systems, e.g. HTTP/JSON to gRPC transcoding.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// other echo systems, e.g. HTTP/JSON to gRPC transcoding.
// other implementations in the ecosystem, e.g. HTTP/JSON to gRPC transcoding.

// Do not decode '%2F' and '%2f' in the path to '/' for compatibility with
// other echo systems, e.g. HTTP/JSON to gRPC transcoding.
// https://github.com/googleapis/googleapis/blob/02710fa0ea5312d79d7fb986c9c9823fb41049a9/google/api/http.proto#L257-L258
final byte marker = RAW_CHAR_TO_MARKER['/'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding the comment like we did for the other branch below:

// Insert a special mark so we can distinguish a raw character ('/') and
// percent-encoded character ('%2F') in a path string.
// We will encode this mark back into a percent-encoded character later.

@trustin
Copy link
Collaborator

trustin commented Oct 5, 2021

@ikhoon Could you check the test failures?

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #3855 (b37ea06) into master (8c45211) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3855      +/-   ##
============================================
+ Coverage     73.20%   73.31%   +0.10%     
- Complexity    15041    15285     +244     
============================================
  Files          1323     1332       +9     
  Lines         57902    58511     +609     
  Branches       7342     7410      +68     
============================================
+ Hits          42387    42895     +508     
- Misses        11775    11846      +71     
- Partials       3740     3770      +30     
Impacted Files Coverage Δ
...linecorp/armeria/internal/common/PathAndQuery.java 86.71% <100.00%> (+0.35%) ⬆️
.../common/AbstractHttp2ConnectionHandlerBuilder.java 34.78% <0.00%> (-4.35%) ⬇️
...nternal/common/AbstractHttp2ConnectionHandler.java 82.00% <0.00%> (-4.00%) ⬇️
...ecorp/armeria/internal/common/HttpHeadersUtil.java 83.87% <0.00%> (-2.80%) ⬇️
.../com/linecorp/armeria/server/file/FileService.java 82.43% <0.00%> (-2.60%) ⬇️
...ia/common/stream/ConcatPublisherStreamMessage.java 78.29% <0.00%> (-2.33%) ⬇️
...ia/internal/common/stream/ByteBufDecoderInput.java 84.89% <0.00%> (-2.16%) ⬇️
...p/armeria/common/stream/AbstractStreamMessage.java 79.20% <0.00%> (-1.99%) ⬇️
.../com/linecorp/armeria/server/RoutingPredicate.java 70.96% <0.00%> (-1.62%) ⬇️
...ecorp/armeria/server/grpc/UnframedGrpcService.java 76.63% <0.00%> (-1.25%) ⬇️
... and 76 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c45211...b37ea06. Read the comment docs.

@trustin trustin merged commit 4a4a575 into line:master Oct 5, 2021
12 checks passed
@ikhoon ikhoon deleted the exclude-%2f-decoding branch November 3, 2021 02:26
trustin added a commit that referenced this pull request Dec 2, 2021
Motivation:

- We changed how `PathAndQuery` handles `%2F` (/) in 1.12.0 via #3855.
  This change introduces an unexpected hole in its double-dot detection
  logic.
- Since we decided not to decode `%2F`, we should not decode it
  whereever possible.

Modifications:

- Hardened the double-dot detection logic in `PathAndQuery`.
- `Bytes.data` now always store the bytes in their decoded form. We keep
  whether the byte has to be encoded in a separate `BitSet`.
- Split `ArmeriaHttpUtil.decodePath()` into `decodePath()` and
  `decodePathParam()`.
  - We don't decode `%2F` in `decodePath()` but we do in
    `decodePathParam()`.
  - `RoutingResultBuilder.rawParam()` now uses `decodePathParam()`
    because `decodePath()` doesn't decode `%2F` anymore.

Result:

- A path that contains double dots with `%2F`, such as
  `/files/..%2Fsecrets.txt`, are now rejected correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants