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.

Sign up for GitHub

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

add form-url-encoded body filter #366

Merged
merged 3 commits into from
Oct 15, 2018
+96 −16
Merged

add form-url-encoded body filter #366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d1ae6f
add form-url-encoded body filter
nsmolenskii Oct 12, 2018
78cd18a
fix code review issues
nsmolenskii Oct 12, 2018
d38cbe1
mode BodyFilters::replaceFormUrlEncodedProperty experimental
nsmolenskii Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,15 +167,15 @@ e.g. *password*.

Logbook supports different types of filters:

| Type | Operates on | Applies to | Default |
|---------------------|--------------------------------|------------|-----------------|
| `RawRequestFilter` | `RawHttpRequest` | request | binary/streams |
| `RawResponseFilter` | `RawHttpResponse` | response | binary/streams |
| `QueryFilter` | Query string | request | `access_token` |
| `HeaderFilter` | Header (single key-value pair) | both | `Authorization` |
| `BodyFilter` | Content-Type and body | both | n/a |
| `RequestFilter` | `HttpRequest` | request | n/a |
| `ResponseFilter` | `HttpResponse` | response | n/a |
| Type | Operates on | Applies to | Default |
|---------------------|--------------------------------|------------|---------------------------------------------------------------------------------------|
| `RawRequestFilter` | `RawHttpRequest` | request | binary/streams |
| `RawResponseFilter` | `RawHttpResponse` | response | binary/streams |
| `QueryFilter` | Query string | request | `access_token` |
| `HeaderFilter` | Header (single key-value pair) | both | `Authorization` |
| `BodyFilter` | Content-Type and body | both | json -> `access_token` and `refresh_token`, form-url -> `client_secret` and `password`|
| `RequestFilter` | `HttpRequest` | request | n/a |
| `ResponseFilter` | `HttpResponse` | response | n/a |

`QueryFilter`, `HeaderFilter` and `BodyFilter` are relatively high-level and should cover all needs in ~90% of all
cases. For more complicated setups one should fallback to the low-level variants, i.e. `RawRequestFilter` and
Expand Down
39 changes: 33 additions & 6 deletions logbook-core/src/main/java/org/zalando/logbook/BodyFilters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.HashSet;
import java.util.Set;
import java.util.function.Predicate;
import java.util.function.UnaryOperator;
import java.util.regex.Pattern;

import static java.util.stream.Collectors.joining;
Expand All @@ -29,11 +30,20 @@ public static BodyFilter defaultValue() {
public static BodyFilter accessToken() {
final Set<String> properties = new HashSet<>();
properties.add("access_token");
properties.add("refresh_token");
properties.add("open_id");
properties.add("id_token");
return replaceJsonStringProperty(properties, "XXX");
}

@API(status = EXPERIMENTAL)
public static BodyFilter oauthRequest() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that standardized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to client-credentials and password flows those parameters looks quite standardised. Also I think it makes sense to add filtering out refresh_token form accessToken() cause it in the end have much higher risks that access_token

Copy link
Contributor Author

@nsmolenskii nsmolenskii Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

according to w3.org: x-ww-form-urlencoded should follow RFC-1738, that describes URL formats.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also I think it makes sense to add filtering out refresh_token form accessToken() cause it in the end have much higher risks that access_token

Agreed!

Copy link
Collaborator

Choose a reason for hiding this comment

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

RFC-1738, that describes URL formats.

That one doesn't describe query parameters, does it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't find anything query parameter related in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree that rfc is quite unreadable. Therefore there is a part describes encoding part as well as format of HTTP URL

Unsafe:

   Characters can be unsafe for a number of reasons.  The space
   character is unsafe because significant spaces may disappear and
   insignificant spaces may be introduced when URLs are transcribed or
   typeset or subjected to the treatment of word-processing programs.
   The characters "<" and ">" are unsafe because they are used as the
   delimiters around URLs in free text; the quote mark (""") is used to
   delimit URLs in some systems.  The character "#" is unsafe and should
   always be encoded because it is used in World Wide Web and in other
   systems to delimit a URL from a fragment/anchor identifier that might
   follow it.  The character "%" is unsafe because it is used for
   encodings of other characters.  Other characters are unsafe because
   gateways and other transport agents are known to sometimes modify
   such characters. These characters are "{", "}", "|", "\", "^", "~",
   "[", "]", and "`".

   All unsafe characters must always be encoded within a URL. For
   example, the character "#" must be encoded within URLs even in
   systems that do not normally deal with fragment or anchor
   identifiers, so that if the URL is copied into another system that
   does use them, it will not be necessary to change the URL encoding.

; HTTP

httpurl        = "http://" hostport [ "/" hpath [ "?" search ]]
hpath          = hsegment *[ "/" hsegment ]
hsegment       = *[ uchar | ";" | ":" | "@" | "&" | "=" ]
search         = *[ uchar | ";" | ":" | "@" | "&" | "=" ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example RFC2396 mentioned in java.net.URL provides almost same idea

2. URI Characters and Escape Sequences

   URI consist of a restricted set of characters, primarily chosen to
   aid transcribability and usability both in computer systems and in
   non-computer communications. Characters used conventionally as
   delimiters around URI were excluded.  The restricted set of
   characters consists of digits, letters, and a few graphic symbols
   were chosen from those common to most of the character encodings and
   input facilities available to Internet users.

      uric          = reserved | unreserved | escaped

   Within a URI, characters are either used as delimiters, or to
   represent strings of data (octets) within the delimited portions.
   Octets are either represented directly by a character (using the US-
   ASCII character for that octet [ASCII]) or by an escape encoding.
   This representation is elaborated below.

3.4. Query Component

   The query component is a string of information to be interpreted by
   the resource.

      query         = *uric

   Within a query component, the characters ";", "/", "?", ":", "@",
   "&", "=", "+", ",", and "$" are reserved.

Copy link
Contributor Author

@nsmolenskii nsmolenskii Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So, how I personally understand it. x-www-form-urlencoded bodies should follow same RFCs, those describe URL format. Both (URI and URL) RFCs describes same for query|search part.

Copy link
Contributor

@maksymgendin maksymgendin Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Guys, what about refresh_token? Did you consciously decide not to obfuscate it? I just stumbled upon the fact that the refresh token appears in our logs....

Currently I would need to configure a custom BodyFilter bean:

@Bean
BodyFilter bodyFilter() {
    return Stream.concat(
                    StreamSupport.stream(ServiceLoader.load(BodyFilter.class).spliterator(), false),
                    Stream.of(BodyFilters.oauthRequest())
            )
            .reduce(
                    BodyFilters.replaceFormUrlEncodedProperty(Set.of("refresh_token"), "XXX"),
                    BodyFilter::merge
            );
}

final Set<String> properties = new HashSet<>();
properties.add("client_secret");
properties.add("password");
return replaceFormUrlEncodedProperty(properties, "XXX");
}

/**
* Creates a {@link BodyFilter} that replaces the properties in the json response with the replacement passed as argument.
* This {@link BodyFilter} works on all levels inside the json tree and it only works with string values<br><br>
Expand All @@ -52,15 +62,32 @@ public static BodyFilter accessToken() {
*/
@API(status = MAINTAINED)
public static BodyFilter replaceJsonStringProperty(final Set<String> properties, final String replacement) {
final String regex = properties.stream()
.map(Pattern::quote)
.collect(joining("|"));

final Predicate<String> json = MediaTypeQuery.compile("application/json", "application/*+json");

final String regex = properties.stream().map(Pattern::quote).collect(joining("|"));
final Pattern pattern = Pattern.compile("(\"(?:" + regex + ")\"\\s*:\\s*)\".*?\"");
final UnaryOperator<String> delegate = body -> pattern.matcher(body).replaceAll("$1\"" + replacement + "\"");

return (contentType, body) -> json.test(contentType) ? delegate.apply(body) : body;
}

/**
* Creates a {@link BodyFilter} that replaces the properties in the form url encoded body with given replacement.
*
* @param properties query names properties to replace
* @param replacement String to replace the properties values
* @return BodyFilter generated
*/
@API(status = EXPERIMENTAL)
public static BodyFilter replaceFormUrlEncodedProperty(final Set<String> properties, final String replacement) {
final Predicate<String> formUrlEncoded = MediaTypeQuery.compile("application/x-www-form-urlencoded");

final QueryFilter delegate = properties.stream()
Copy link
Collaborator

Choose a reason for hiding this comment

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

It works since the query string and the form bodies are encoded in the same way. Should we extract this parsing/transformation from query filter into a separate, shared unit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

x-www-form-urlencoded has same format as normal query. I think it is quite OK, to just delegate to query filters.

.map(name -> QueryFilters.replaceQuery(name, replacement))
.reduce(QueryFilter::merge)
.orElseGet(QueryFilter::none);

return (contentType, body) -> json.test(contentType) ?
pattern.matcher(body).replaceAll("$1\"" + replacement + "\"") : body;
return (contentType, body) -> formUrlEncoded.test(contentType) ? delegate.filter(body) : body;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have a private, shared method that takes a Predicate and a UnaryOperator and produces a BodyFilter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see some reasons to do that - ternary operator quite readable itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like it's a quite common pattern, almost suggested by the BodyFilter API since it passes the content type for exactly that reason.

}

@API(status = EXPERIMENTAL)
Expand Down
55 changes: 54 additions & 1 deletion logbook-core/src/test/java/org/zalando/logbook/BodyFiltersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
import org.junit.jupiter.api.Test;

import java.util.Collections;
import java.util.function.UnaryOperator;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;

public final class BodyFiltersTest {
final class BodyFiltersTest {

@Test
void shouldFilterAccessTokenByDefault() {
Expand All @@ -29,6 +30,24 @@ void shouldNotFilterAccessTokenInTextPlainByDefault() {
assertThat(actual, is("{\"access_token\":\"secret\"}"));
}

@Test
void shouldFilterClientSecretByOauthRequestFilter() {
final BodyFilter unit = BodyFilters.oauthRequest();

final String actual = unit.filter("application/x-www-form-urlencoded", "client_secret=secret");

assertThat(actual, is("client_secret=XXX"));
}

@Test
void shouldNotFilterClientSecretInTextPlainByOauthRequestFilter() {
final BodyFilter unit = BodyFilters.oauthRequest();

final String actual = unit.filter("text/plain", "client_secret=secret");

assertThat(actual, is("client_secret=secret"));
}

@Test
void shouldFilterNotEmptyJSONProperty() {
final BodyFilter unit = BodyFilters.replaceJsonStringProperty(Collections.singleton("foo"), "XXX");
Expand Down Expand Up @@ -87,4 +106,38 @@ void shouldReturnXmlCompactingBodyFilter() {

assertThat(bodyFilter, instanceOf(XmlCompactingBodyFilter.class));
}

@Test
void shouldFilterFormUrlEncodedBodyIfValidRequest() {
final BodyFilter unit = BodyFilters.replaceFormUrlEncodedProperty(Collections.singleton("q"), "XXX");

final UnaryOperator<String> filter = value -> unit.filter("application/x-www-form-urlencoded", value);

assertThat(filter.apply("q=boots&sort=price&direction=asc"), is("q=XXX&sort=price&direction=asc"));
assertThat(filter.apply("sort=price&direction=asc&q=boots"), is("sort=price&direction=asc&q=XXX"));
assertThat(filter.apply("sort=price&q=boots&direction=asc"), is("sort=price&q=XXX&direction=asc"));
assertThat(filter.apply("sort=price&direction=asc"), is("sort=price&direction=asc"));
assertThat(filter.apply("q=boots"), is("q=XXX"));
assertThat(filter.apply(""), is(""));
}

@Test
void shouldNotFilterFormUrlEncodedBodyIfNotValidContentType() {
final BodyFilter unit = BodyFilters.replaceFormUrlEncodedProperty(Collections.singleton("q"), "XXX");

assertThat(unit.filter("application/json", "{\"q\":\"boots\"}"), is("{\"q\":\"boots\"}"));
assertThat(unit.filter("application/xml", "<q>boots</q>"), is("<q>boots</q>"));
assertThat(unit.filter("invalid", "{\"q\":\"boots\"}"), is("{\"q\":\"boots\"}"));
assertThat(unit.filter(null, "{\"q\":\"boots\"}"), is("{\"q\":\"boots\"}"));
}

@Test
void shouldNotFilterFormUrlEncodedBodyIfNotValidContent() {
final BodyFilter unit = BodyFilters.replaceFormUrlEncodedProperty(Collections.singleton("q"), "XXX");

final UnaryOperator<String> filter = value -> unit.filter("application/x-www-form-urlencoded", value);

assertThat(filter.apply("{\"q\":\"boots\"}"), is("{\"q\":\"boots\"}"));
assertThat(filter.apply("<q>boots</q>"), is("<q>boots</q>"));
}
}