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

Guide @ServerFilter @FilterMethod guide #1408

Merged
merged 16 commits into from
Jan 15, 2024
Merged

Guide @ServerFilter @FilterMethod guide #1408

merged 16 commits into from
Jan 15, 2024

Conversation

sdelamo
Copy link
Contributor

@sdelamo sdelamo commented Jan 5, 2024

No description provided.

@sdelamo sdelamo requested a review from timyates January 5, 2024 12:05
@sdelamo sdelamo changed the title Guide @ServeFilter @FilterMethod guide Guide @ServerFilter @FilterMethod guide Jan 5, 2024
@sdelamo sdelamo requested a review from graemerocher January 5, 2024 12:06
@sdelamo sdelamo requested a review from timyates January 5, 2024 13:57
if (LOG.isTraceEnabled()) {
HttpHeaders headers = request.getHeaders()
for (String headerName : headers.names()) {
if (headerName.equalsIgnoreCase(HttpHeaders.AUTHORIZATION)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we should be encouraging unsafe patterns like this, how do you know what other headers might contain sensitive values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer if I remove the part where we skip logging the Authorization header and instead log every HTTP header in the guide?

I think showing users to be careful about what they log is better than logging everything.

Please note that in core, we skip logging some headers. Of course, what header is sensitive is application specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps you could use HeaderUtils since it already does some 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.

@sdelamo sdelamo requested a review from graemerocher January 8, 2024 23:53
@sdelamo sdelamo merged commit 8b3b55a into master Jan 15, 2024
2 checks passed
@sdelamo sdelamo deleted the server-filter-request branch January 15, 2024 09:07
AndreaLaGrotteria pushed a commit to AndreaLaGrotteria/micronaut-guides that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants