Fix a bug where the default `HeadersSanitizer` does not work in logging decorators #5519

ikhoon · 2024-03-19T13:26:31Z

Motivation:

HeadersSanitizer was added in #5188. If no HeadersSanitizer is set, the default sanitizer masks Authorization, Cookie Set-Cookie, and Proxy-Authorization with ****.

In addition to HeadersSantizer, other new features were added to LoggingDecoratorBuilder and the code became messy to maintain backward compatibility. LoggingDecoratorBuilder, LogWriterBuilder, and LogFormatterBuilder each have their own properties, so making the overall operation consistent was not easy.

As a result, while LoggingClient.newDecorator() sanitizes sensitive headers, LoggingClient.builder().requestLogLevel(...).newDecorator()` did not work. Neither code set a sanitizer, so a default sanitizer should have been set, but it didn't.

Modifications:

Refactor LoggingDecoratorBuilder to delegate all builder properties to either LogWriterBuilder or LogFormatterBuilder.
Add missing @Deprecated annotation to LoggingClientBuilder and LoggingRpcClientBuilder and LoggingServiceBuilder.
- @Deprecated in the super method isn't inspected by IntelliJ Mark @Deprecated annotation in overridden methods #5395
Breaking) Change the signature of LogWriterBuilder.responseCauseFilter() to have RequestContext as the first parameter.
- It may be a bug or a mistake when adding the API. Our APIs usually provide RequestContext for predicates or handlers.

Result:

LoggingClient and LoggingService now correctly mask sensitive headers by default.

Motivation: `HeadersSanitizer` was added in line#5188. If no `HeadersSanitizer` is set, the default santizer masks `Authorization`, `Cookie` `Set-Cookie` and `Proxy-Authorization` with `****`. In addition to `HeadersSantizer`, other new features are added to `LoggingDecoratorBuilder` and the code became messy to maintain backward compatibility. `LoggingDecoratorBuilder`, `LogWriterBuilder`, and `LogFormatterBuilder` each have their own properties, so it was not easy to make the overall operation consistent. As a result, while `LoggingClient.newDecorator()` santizes sensitive headers, LoggingClient.builder().requestLogLevel(...).newDecorator()` did not work. Neither code set a sanitizer, so a default sanitizer should have been set, but it didn't. Modifications: - Refactor `LoggingDecoratorBuilder` to delegate all builder properties to either `LogWriterBuilder` or `LogFormatterBuilder`. - Add missing `@Deprecated` annotation to `LoggingClientBuilder` and `LoggingRpcClientBuilder` and `LoggingServiceBuilder`. - `@Deprecated` in the super method aren't inspected by IntelliJ line#5395 - Breaking) Change the signature of `LogWriterBuilder.responseCauseFilter()` to have `RequestContext` as the first parameter. - It may be a bug or a mistake when adding the API. Our APIs usually provide `RequestContext` for predicates or handlers. Result: `LoggingClient` and `LoggingService` now correctly mask sensitive headers by default.

github-actions · 2024-03-19T13:34:37Z

🔍 Build Scan® (commit: `cd0203c`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/b32s6tra2345o
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/2gr5anzan6bks
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/xm5knfm2v6gio
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/cjgjcvac4c5eq
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/5bgqzx4imvsvo
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/ih6pz2y3pzpoe
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/hg2yyh6dfzngu
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/bdftik67xa2oy

codecov · 2024-03-19T14:08:32Z

Codecov Report

Attention: Patch coverage is 82.05128% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 74.07%. Comparing base (c960d23) to head (cd0203c).

Files	Patch %	Lines
...rmeria/common/logging/LoggingDecoratorBuilder.java	80.19%	15 Missing and 5 partials ⚠️
...rmeria/client/logging/LoggingRpcClientBuilder.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5519       +/-   ##
===========================================
+ Coverage        0   74.07%   +74.07%     
- Complexity      0    20807    +20807     
===========================================
  Files           0     1801     +1801     
  Lines           0    76558    +76558     
  Branches        0     9749     +9749     
===========================================
+ Hits            0    56708    +56708     
- Misses          0    15233    +15233     
- Partials        0     4617     +4617

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ikhoon · 2024-03-20T03:33:45Z

core/src/main/java/com/linecorp/armeria/common/logging/LoggingDecoratorBuilder.java

-    }
-
-    @Override
-    public String toString() {


Removed this since we reached a consensus that toString() is not a mandatory implementation for builder classes.

jrhee17

Left some nits, but the fix looks solid 👍 Thanks @ikhoon ! 🙇 👍 🙇

jrhee17 · 2024-03-20T12:20:48Z

core/src/main/java/com/linecorp/armeria/common/logging/LogWriterBuilder.java

@@ -176,7 +186,8 @@ private ResponseLogLevelMapper responseLogLevelMapper() {
     * You can prevent logging the response cause by returning {@code true}
     * in the {@link Predicate}. By default, the response cause will always be logged.
     */
-    public LogWriterBuilder responseCauseFilter(Predicate<? super Throwable> responseCauseFilter) {
+    public LogWriterBuilder responseCauseFilter(


Optional) I think we can avoid breaking changes by maintaining both methods ((ctx, t) -> bool, (t) -> (bool)).

Having said this, I've searched for usages and I think there aren't any usages so far

I thought changing was okay since the API is unstable and was added recently.

jrhee17 · 2024-03-20T12:22:29Z

core/src/main/java/com/linecorp/armeria/common/logging/LogWriterBuilder.java

@@ -42,12 +45,14 @@ public final class LogWriterBuilder {
    static final ResponseLogLevelMapper DEFAULT_RESPONSE_LOG_LEVEL_MAPPER =
            ResponseLogLevelMapper.of(LogLevel.DEBUG, LogLevel.WARN);

-    private Logger logger = DefaultLogWriter.defaultLogger;
+    @Nullable
+    private Logger logger;


I understood this change is just a change of code style and there is no difference in functionality

I made it nullable to know if a custom logger is set when building LogWriter.
https://github.com/line/armeria/pull/5519/files#diff-f09a8bbc22f127a6a25e54937af747ca128e8cbb3df28cc19aa326db0e1cedf6R609

jrhee17 · 2024-03-20T12:31:39Z

core/src/main/java/com/linecorp/armeria/common/logging/LoggingDecoratorBuilder.java

@@ -235,8 +214,9 @@ public LoggingDecoratorBuilder responseLogLevel(Class<? extends Throwable> clazz
     */
    @Deprecated
    public LoggingDecoratorBuilder successfulResponseLogLevel(LogLevel successfulResponseLogLevel) {
-        requireNonNull(successfulResponseLogLevel, "successfulResponseLogLevel");


Can you check the missing rnn in this class overall?

It was delegated to logWriterBuilder. Do you think we also need to add null-checking here?

I see, I guess I missed it. Thanks for the explanation 👍

jrhee17 · 2024-03-20T12:40:35Z

core/src/main/java/com/linecorp/armeria/common/logging/LoggingDecoratorBuilder.java

    protected final BiFunction<? super RequestContext, ? super Throwable, ? extends @Nullable Object>
    responseCauseSanitizer() {
-        return responseCauseSanitizer;
+        return null;


Question) Why do we handle this method differently from the other methods?

Suggested change

return null;

if (logFormatterBuilder != null) {

return logFormatterBuilder.responseContentSanitizer();

}

return null;

I figured out that responseCauseSanitizer was not used to sanitize an exception.
responseCauseFilter was actually used and responseCauseSanitizer was only referenced by toString() method.

minwoox

Thanks for the clean up! 👍 👍 👍

ikhoon added the defect label Mar 19, 2024

ikhoon added this to the 1.28.0 milestone Mar 19, 2024

github-actions bot requested review from jrhee17, minwoox and trustin March 19, 2024 13:26

ikhoon changed the title ~~Fix a bug where the default HeadersSanitizer in logging decorators~~ Fix a bug where the default HeadersSanitizer does not work in logging decorators Mar 19, 2024

ikhoon added 2 commits March 19, 2024 22:30

remove shouldBuildLogWriter

8423005

remove cruft

cd0203c

ikhoon commented Mar 20, 2024

View reviewed changes

jrhee17 approved these changes Mar 20, 2024

View reviewed changes

minwoox approved these changes Mar 22, 2024

View reviewed changes

jrhee17 added the breaking change label Apr 2, 2024

jrhee17 merged commit 42edbb2 into line:main Apr 2, 2024
18 checks passed

ikhoon deleted the default-logging-sanitizer branch June 28, 2024 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug where the default `HeadersSanitizer` does not work in logging decorators #5519

Fix a bug where the default `HeadersSanitizer` does not work in logging decorators #5519

ikhoon commented Mar 19, 2024

github-actions bot commented Mar 19, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024

ikhoon Mar 20, 2024

jrhee17 left a comment

jrhee17 Mar 20, 2024

ikhoon Mar 21, 2024

jrhee17 Mar 20, 2024

ikhoon Mar 21, 2024

jrhee17 Mar 20, 2024

ikhoon Mar 21, 2024 •

edited

Loading

jrhee17 Mar 21, 2024

jrhee17 Mar 20, 2024

ikhoon Mar 21, 2024

minwoox left a comment

Fix a bug where the default HeadersSanitizer does not work in logging decorators #5519

Fix a bug where the default HeadersSanitizer does not work in logging decorators #5519

Conversation

ikhoon commented Mar 19, 2024

github-actions bot commented Mar 19, 2024 • edited Loading

🔍 Build Scan® (commit: cd0203c)

codecov bot commented Mar 19, 2024

Codecov Report

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Fix a bug where the default `HeadersSanitizer` does not work in logging decorators #5519

Fix a bug where the default `HeadersSanitizer` does not work in logging decorators #5519

github-actions bot commented Mar 19, 2024 •

edited

Loading

🔍 Build Scan® (commit: `cd0203c`)

ikhoon Mar 21, 2024 •

edited

Loading