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

RewriteHandler with multiple HeaderPatternRules #10482

Closed
janitza-bjag opened this issue Sep 6, 2023 · 9 comments · Fixed by #10572
Closed

RewriteHandler with multiple HeaderPatternRules #10482

janitza-bjag opened this issue Sep 6, 2023 · 9 comments · Fixed by #10572
Assignees
Labels

Comments

@janitza-bjag
Copy link

Jetty Version
12.0.1

Jetty Environment
core, ee8

Java Version
17.0.8

Question
I added some HeaderPatternRules to a RewriteHandler, but only the last rule applies. What I am doing wrong? I assumed that all matching rules will apply...

private static final HeaderPatternRule CONTENT_TYPE_OPTIONS = new HeaderPatternRule("*", "X-Content-Type-Options", "nosniff");
private static final HeaderPatternRule XSS_PROTECTION = new HeaderPatternRule("*", "X-XSS-Protection", "0");
private static final HeaderPatternRule PERMISSION_POLICY = new HeaderPatternRule("*", "Permissions-Policy", "accelerometer=(), ambient-light-sensor=(), autoplay=(), battery=(), " +
            "camera=(), cross-origin-isolated=(), display-capture=(), document-domain=(), encrypted-media=(), execution-while-not-rendered=(), " +
            "execution-while-out-of-viewport=(), geolocation=(), gyroscope=(), keyboard-map=(), magnetometer=(), microphone=(), " +
            "midi=(), navigation-override=(), payment=(), picture-in-picture=(), publickey-credentials-get=(), screen-wake-lock=(), sync-xhr=(), usb=(), web-share=(), xr-spatial-tracking=()");
private static final HeaderPatternRule REFERRER_POLICY = new HeaderPatternRule("*", "Referrer-Policy", "no-referrer");

RewriteHandler rh = new RewriteHandler();

rh.addRule(REFERRER_POLICY);
rh.addRule(PERMISSION_POLICY);

rh.addRule(XSS_PROTECTION);
rh.addRule(CONTENT_TYPE_OPTIONS);
@janitza-bjag janitza-bjag changed the title RewriteHandler RewriteHandler with multiple HeaderPatternRules Sep 6, 2023
@sbordet
Copy link
Contributor

sbordet commented Sep 8, 2023

@janitza-bjag I am unsure what you are trying to achieve with a pattern of * for your rules.

The pattern should be a Servlet pattern as defined in the Servlet specification here:
https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#specification-of-mappings

Your mapping of * does not match anything, so no rule applies.
I am surprised you say the last rule matches, as it should not.

Please fix your patterns depending on your logic and report back if this issue is fixed.

@janitza-bjag
Copy link
Author

@sbordet Thanks for your reply.

We had the above code working with Jetty 10.0.15
The intention was to add all the security headers to every requests ...

Is there another preferred way to do so?

@sbordet
Copy link
Contributor

sbordet commented Sep 11, 2023

@janitza-bjag If you want to make the rules trigger for every request for every request path, use the /* pattern (like you would do for a normal Servlet/Filter).

It feels like a bug that * only worked on 10/11.

@janitza-bjag
Copy link
Author

@sbordet If I use the /* pattern in the above code only the last rule "CONTENT_TYPE_OPTIONS" applied. All other headers are not set ... really weird

@made-in-nz
Copy link

Probably no surprise, but the same situation when configuring multiple rules via $JETTY_BASE/etc/jetty-rewrite-rules.xml. Only the last addRule is applied.
We also only ever used a pattern of * in Jetty 9.4.x & 10.0.x to set a header for all responses.

@sbordet
Copy link
Contributor

sbordet commented Sep 23, 2023

Confirmed that there is a bug, we do not process the rules properly in 12.

I still recommend that you use /* rather than * for matching, as the bug is an orthogonal issue with respect to the pattern.

sbordet added a commit that referenced this issue Sep 23, 2023
Reviewed the implementation of RewriteHandler, that was broken for those rules that were overriding `Rule.Handler.handle()`.

The problem was that the handling was not forwarded along the chain of rules, so only the last one was applied.

Removed (a breaking change) `Rule.Handler.handle(Response, Callback)` because it is necessary that the outermost request wrapper is passed to the `RewriteHandler` child `Handler`.
Updated implementations to override `handle(Request, Response, Callback)` instead, so that the request parameter is preserved along the chain.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet moved this to 🏗 In progress in Jetty 12.0.2 FROZEN Sep 23, 2023
@sbordet
Copy link
Contributor

sbordet commented Sep 23, 2023

@janitza-bjag @made-in-nz are you able to try out PR #10572 and report whether the issue is fixed for you?

Thanks!

@made-in-nz
Copy link

@sbordet Yes, I can confirm this fixes it. Thank you!

@janitza-bjag
Copy link
Author

@sbordet

@janitza-bjag @made-in-nz are you able to try out PR #10572 and report whether the issue is fixed for you?

Thanks!

The PR fixes it, thanks!

sbordet added a commit that referenced this issue Sep 25, 2023
Updated the implementation after review.

Restored the `Rule.Handler.handle(Response, Callback)` method so that existing code won't break.

Now the wrapping at the constructor produces RH3(RH2(RH1(Request))), but the handling is performed from the innermost towards the outermost.
In this way, the order of rules is respected, both in the wrapping at rule application, and in the `Rule.Handler` handling.

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue Sep 26, 2023
Reviewed the implementation of RewriteHandler, that was broken for those rules that were overriding `Rule.Handler.handle()`.

The problem was that the handling was not forwarded along the chain of rules, so only the last one was applied.

Now the wrapping at the constructor produces RH3(RH2(RH1(Request))), but the handling is performed from the innermost towards the outermost.
In this way, the order of rules is respected, both in the wrapping at rule application, and in the `Rule.Handler` handling.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet closed this as completed Sep 26, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.2 FROZEN Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants