Skip to content

fix: Allow access log to log paths jetty rejects#9788

Closed
Mahoney wants to merge 1 commit intodropwizard:release/5.0.xfrom
Mahoney-forks:access-request-logging-bad-path
Closed

fix: Allow access log to log paths jetty rejects#9788
Mahoney wants to merge 1 commit intodropwizard:release/5.0.xfrom
Mahoney-forks:access-request-logging-bad-path

Conversation

@Mahoney
Copy link
Copy Markdown
Contributor

@Mahoney Mahoney commented Jan 16, 2025

Tentative fix for #9773

Problem:

When running with access request logging (via logback-access), a request such as this:
GET /..;/manager/html
results in no access request log entry and a stacktrace in the logs.

See #9773

Solution:

Use ch.qos.logback.access:logback-access-jetty12's provided ways of adapting Jetty classes to the Servlet API to allow using logbakc access.

@Mahoney Mahoney requested a review from a team as a code owner January 16, 2025 13:47
@github-actions github-actions bot added this to the 5.0.0 milestone Jan 16, 2025
@Mahoney
Copy link
Copy Markdown
Contributor Author

Mahoney commented Jan 16, 2025

To state the obvious I have failed to add a test that actually demonstrates this works... just putting this here as a tentative direction of travel rather than asking for it to be merged. If someone can point me at an appropriate way to write an integration test proving this works that would be awesome.

Tentative fix for dropwizard#9773

The test change proves that logback-access's `ResponseWrapper` is not brilliant. Should probably be fixed in logback-access, but could be fixed
@Mahoney Mahoney force-pushed the access-request-logging-bad-path branch from d9d0c18 to 559b38a Compare January 16, 2025 14:02
@Mahoney
Copy link
Copy Markdown
Contributor Author

Mahoney commented Jan 16, 2025

Actually I see I've effectively reverted #8008 / #7965 here.

I do think that would have been better solved upstream in logback-access.

@Mahoney Mahoney closed this Jan 16, 2025
@Mahoney
Copy link
Copy Markdown
Contributor Author

Mahoney commented Jan 16, 2025

I think if we can get qos-ch/logback-access#23 merged and released then this PR would become relevant again.

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.

1 participant