Skip to content

Fixes #12300: add null check in RewriteHandler.doStart() and unit test #13358

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 21, 2025
Merged

Fixes #12300: add null check in RewriteHandler.doStart() and unit test #13358

merged 3 commits into from
Jul 21, 2025

Conversation

sanjerai
Copy link
Contributor

Added null check in RewriteHandler#doStart() to prevent misuse when no child Handler is set. included a test verifying the behavior.

@joakime joakime requested a review from sbordet July 16, 2025 19:33
@joakime joakime added the Bug For general bugs on Jetty side label Jul 16, 2025
@joakime joakime moved this to 👀 In review in Jetty 12.1.1 - FROZEN Jul 16, 2025
@joakime
Copy link
Contributor

joakime commented Jul 16, 2025

@sanjerai we need a signed Eclipse ECA in order to merge this. This is a requirement by the Eclipse Foundation Legal department.

See: https://github.com/jetty/jetty.project/blob/jetty-12.0.x/CONTRIBUTING.md#eclipse-contributor-agreement

@sanjerai sanjerai force-pushed the fix-rewritehandler-nullcheck branch from 5b897e2 to bad3dc6 Compare July 16, 2025 19:35
@sanjerai
Copy link
Contributor Author

@joakime thank you. i have signed the ECA

@joakime
Copy link
Contributor

joakime commented Jul 16, 2025

Last build fails due to checkstyle violation.

[INFO] --- checkstyle:3.6.0:check (checkstyle-check) @ jetty-rewrite ---
[INFO] Starting audit...
[WARN] /home/jenkins/agent/workspace/jetty.project_PR-13358/jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RewriteHandler.java:147:47: '{' at column 47 should be on a new line. [LeftCurly]
[WARN] /home/jenkins/agent/workspace/jetty.project_PR-13358/jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RewriteHandler.java:148:35: '{' at column 35 should be on a new line. [LeftCurly]
[WARN] /home/jenkins/agent/workspace/jetty.project_PR-13358/jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/RewriteHandlerTest.java:128:49: '{' at column 49 should be on a new line. [LeftCurly]
Audit done.
[WARNING] src/main/java/org/eclipse/jetty/rewrite/handler/RewriteHandler.java:[147,47] (blocks) LeftCurly: '{' at column 47 should be on a new line.
[WARNING] src/main/java/org/eclipse/jetty/rewrite/handler/RewriteHandler.java:[148,35] (blocks) LeftCurly: '{' at column 35 should be on a new line.
[WARNING] src/test/java/org/eclipse/jetty/rewrite/handler/RewriteHandlerTest.java:[128,49] (blocks) LeftCurly: '{' at column 49 should be on a new line.

You can replicate this on your local machine by running a command line build.

$ mvn -Pci clean install -DskipTests

Also, if you want to integrate your IDE's checkstyle checker, the checkstyle rules are present in git as well.
See: https://github.com/jetty/jetty.project/blob/jetty-12.1.x/build/build-resources/src/main/resources/jetty-checkstyle.xml

@sbordet
Copy link
Contributor

sbordet commented Jul 17, 2025

@sanjerai @joakime RewriteHandler can have a null child, for example in those cases where all the rules are "terminating" rules, such as redirects, or validity rules that produce an error response.

This PR would be a breaking behavior change for those cases where RewriteHandler has a null child.

@gregw opinions?

@sbordet
Copy link
Contributor

sbordet commented Jul 17, 2025

@sanjerai what is the exact use case that prompted you to issue this PR?

sbordet
sbordet requested changes Jul 17, 2025
View reviewed changes
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RewriteHandler.java Outdated
@Override
protected void doStart() throws Exception
{
if (getHandler() == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

See the other comment, I think having a null child Handler could be a legit use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbordet in that case should i change it to a warning log only

@sanjerai
Copy link
Contributor Author

@sbordet i did changes for this Bug

@sbordet
Copy link
Contributor

sbordet commented Jul 17, 2025

@sanjerai I think a better fix would be to change LastRuleHandler.handle() and throw the IllegalStateException there.

@sbordet sbordet linked an issue Jul 17, 2025 that may be closed by this pull request
Use of RewriteHandler.LastRuleHandler without a child handler should produce a clear error message. #12300
Closed
@sanjerai sanjerai force-pushed the fix-rewritehandler-nullcheck branch from c32fb45 to de5bf67 Compare July 17, 2025 16:11
@sbordet sbordet self-requested a review July 17, 2025 21:30
@gregw
Copy link
Contributor

gregw commented Jul 18, 2025

@sanjerai @joakime RewriteHandler can have a null child, for example in those cases where all the rules are "terminating" rules, such as redirects, or validity rules that produce an error response.

This PR would be a breaking behavior change for those cases where RewriteHandler has a null child.

@gregw opinions?

I do not think it is an error to have a null Handler. If it was, we should fix in Handler.Wrapper, not just here.

I'm fine to add some debug to note this case, but not to throw ISE.

@sanjerai
Copy link
Contributor Author

@gregw @sbordet i hope we are fine with ISE from LastRuleHandler (PR code updated)
this should take care of scenario where handler is null in case of a non terminating Rule

@sbordet sbordet requested a review from gregw July 18, 2025 16:22
@gregw
Copy link
Contributor

gregw commented Jul 21, 2025

I'm good with an ISE.

@gregw gregw merged commit 99886f5 into jetty:jetty-12.1.x Jul 21, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.1.1 - FROZEN Jul 21, 2025
@olamy olamy mentioned this pull request Jul 30, 2025
Merged
@sbordet sbordet moved this to ✅ Done in Jetty 12.1.0 - (FROZEN) Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@gregw gregw Awaiting requested review from gregw

Assignees
No one assigned
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Jetty 12.1.0 - (FROZEN)
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use of RewriteHandler.LastRuleHandler without a child handler should produce a clear error message.
4 participants
@sanjerai @joakime @sbordet @gregw