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

[Logging] Masking authorization header #822

Merged
merged 7 commits into from
Dec 7, 2023

Conversation

amarlankri
Copy link
Contributor

Add option to LoggingInterceptor to mask sensitive data of header request from the log.

Description

The solution implemented is described in #818.

Motivation and Context

Currently, all the headers of the request are logged:

this.logger.log(
{
message,
method,
body: maskedBody,
headers,
},
ctx,
);

However, those headers may contain sensitive data. Especially, the authorization header may contain a JWT which can encode sensitive data, readable by anyone once decoded. So, the logging interceptor should provide a way to mask request headers.

Fix #818

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@amarlankri amarlankri self-assigned this Dec 6, 2023
@amarlankri amarlankri force-pushed the logging/feat/header-mask branch from 759c2ec to 30a3469 Compare December 6, 2023 09:17
@amarlankri amarlankri marked this pull request as ready for review December 6, 2023 09:19
Copy link
Contributor

@LeKer29 LeKer29 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @amarlankri ! :)

requestHeader: {
password: true, // Mask the header 'password' in the request
authorization: (header: string | string[]) => {
... // Handle the header value to keep non sensitve data for instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
... // Handle the header value to keep non sensitve data for instance
... // Handle the header value to keep non sensitive data for instance

if (typeof mask === 'function') {
return {
...maskedHeaders,
[headerKey]: mask(headerValue),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a user-defined function, so it's very error-prone. I think we should catch the exception during the function execution.


this.logger.log(
{
message,
method,
body: maskedBody,
headers,
headers: maskedHeaders,
Copy link
Contributor

Choose a reason for hiding this comment

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

In case of exception, we could log the error message instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure to understand your proposition. You mean passing the message of the exception as the value of the header?

Copy link
Contributor

Choose a reason for hiding this comment

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

This always logs the masked headers. I meant we should have an another flow to log the exception occured during the header masking.

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 proposed an implementation in my last commit. Tell me if you thought about another solution.

@amarlankri amarlankri requested a review from qhdinh December 6, 2023 15:13

return {
...maskedHeaders,
[headerKey]: this.maskingPlaceholder,
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought of logging the error and leaving the hearder value unmasked. But masking with default place holder is a better solution I think.

@amarlankri amarlankri force-pushed the logging/feat/header-mask branch from e2233d7 to 502c8e7 Compare December 7, 2023 08:53
@amarlankri amarlankri requested a review from g-ongenae December 7, 2023 08:54
@g-ongenae g-ongenae merged commit 13ee221 into algoan:master Dec 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LoggingInterceptor] Masking authorization header
5 participants