Unexpected behaviour if caddyhttp.Route is provisioned twice

[jesper-chainalysis]

Let me just start with saying, that this is not something that was observed during normal Caddy operation. It's a corner case that arose because I'm using caddyhttp.Route from another Caddy module.

The TL;DR of the issue is that because the middleware field in caddyhttp.Route is appended to directly here, then if we ever call ProvisionHandlers more than once, the pre-compiled handlers will be added to the middleware field again, instead of reflecting the current Handlers.

For a more convoluted example, and how I discovered it. What I do is, I have a Caddy module that embeds the caddyhttp.Route struct and I call Provision on that embedding to trigger all the JSON magic that creates the Route object. A little later I look at the Handlers field, and in some cases I add a Rewrite.Rewrite handler in front. Then I call ProvisionHandlers, followed by Compile. Which I expected would have worked, but because of the pre-compile thing, the Rewrite handler is not at the top, and never hit because the last Handler is terminating.

There is an easy fix though, which restores the expected behaviour without affecting Caddy's current behaviour:

diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go
index 6f237149..35e63a55 100644
--- a/modules/caddyhttp/routes.go
+++ b/modules/caddyhttp/routes.go
@@ -159,10 +159,17 @@ func (r *Route) ProvisionHandlers(ctx caddy.Context, metrics *Metrics) error {
                r.Handlers = append(r.Handlers, handler.(MiddlewareHandler))
        }

+       // Create a new middleware slice so we can call ProvisionHandlers multiple times
+       newMiddleware := []Middleware{}
+
        // pre-compile the middleware handler chain
        for _, midhandler := range r.Handlers {
-               r.middleware = append(r.middleware, wrapMiddleware(ctx, midhandler, metrics))
+               newMiddleware = append(newMiddleware, wrapMiddleware(ctx, midhandler, metrics))
        }
+
+       // Assign the new middleware slice to the route
+       r.middleware = newMiddleware
+
        return nil
 }

I would be more than happy to raise a PR with that small change. I just want to make sure that I haven't misunderstood what is going on.

[mholt]

Hmm, I guess that should be fine. Want to submit a PR?

If it does break anything we'd appreciate your help to maintain patches :)

[jbro]

Only glad to help :)

I've created a PR here

(Sorry for mixing accounts, I accidentally created the issue with my work account)