Request logging discards custom headers if no content is written

[wader]

Version: master at 0798459e44f82a8262ffec93ae173181227ab176

I noticed this when working on porting a webdav plugin to Caddy 2 and the OPTIONS request did not return a Allow header.

Reproduction:

# Caddyfile
:8080
log stdout
header Custom-Header "My value"

$ caddy run -config Caddyfile
{"level":"info","ts":1587411474.820839,"logger":"http.log.access","msg":"handled request","request":{"method":"GET","uri":"/","proto":"HTTP/1.1","remote_addr":"127.0.0.1:52586","host":"0:8080","headers":{"User-Agent":["curl/7.64.1"],"Accept":["*/*"]}},"common_log":"127.0.0.1 - - [20/Apr/2020:21:37:54 +0200] \"GET / HTTP/1.1\" 0 0","latency":0.000023796,"size":0,"status":0,"resp_headers":{"Custom-Header":["My value"],"Server":["Caddy"]}}

$ curl -v http://0:8080
*   Trying 0.0.0.0...
* TCP_NODELAY set
* Connected to 0 (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: 0:8080
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Server: Caddy
< Date: Mon, 20 Apr 2020 19:48:32 GMT
< Content-Length: 0
<
* Connection #0 to host 0 left intact
* Closing connection 0

If I remove the log directive:

$ curl -v http://0:8080
*   Trying 0.0.0.0...
* TCP_NODELAY set
* Connected to 0 (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: 0:8080
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Custom-Header: My value
< Server: Caddy
< Date: Mon, 20 Apr 2020 19:49:21 GMT
< Content-Length: 0
<
* Connection #0 to host 0 left intact
* Closing connection 0

In both cases the custom header appear in the request log.

I've tried to track the issue down and it seems to be because of the header create and copy done at https://github.com/caddyserver/caddy/blob/master/modules/caddyhttp/responsewriter.go#L151 If i change to use w.Header() directly it works.

Caddy 1 have similar code but seem to not suffer from the issue for some reason.

[mholt]

Interesting! I wonder if I have already fixed this actually, on the h2c branch. Am mobile now but can check later unless you beat me to it.

PS. Did you know I already ported the webdav plugin to v2? https://GitHub.com/mholt/caddy-webdav

[wader]

Oh thanks, did not know. Mine is also based on hacdias webdav plugin, have kind of working version at https://github.com/wader/caddy2-webdav

Looking at the comment in yours it feels like it indeed would be a better idea to integrate more with the caddy auth stuff instead of using the built-in things in github.com/hacdias/webdav/v3

[wader]

You'r right, work fine in the h2c branch. Is it something in the go stdlib http server code that does WriteHeader if it has not been done and by wrapping and making a new map it will still keep using the "old" map instance?

[mholt]

@wader Very interesting! I thought that bug was only limited to HTTP/2 Trailers. Thanks for testing it out! I might have to cherry pick that commit before the 2.0 release.

It turns out that maintaining a second header map was not necessary and was in fact buggy :P

Edit: Oh, and yes, let's leave all the request matching and auth logic up to respective parts of Caddy's HTTP server; ideally the webdav handler should only do webdav.

That said, sorry about the duplicated effort. I meant to make a PR soon, in fact I only got around to telling @hacdias about it today! (I just needed to start using it on a server about a week ago.)

What do you want to do now?

[wader]

Aha good!

No problem. Learned a lot about caddy internals :) guess I could help out with your implementation if it's needed? for my own needs i only use webdav with full read/write access behind http basic auth (done by traefik atm, it routes other things also).

It would be nice if using webdav under a path would be more intuitive. For example that you would not need to explicitly tell webdav what to prefix with. Maybe hard to do? Also thought about support X-Forwarded-Prefix (other others?) but maybe that could be done more general?

[mholt]

No problem. Learned a lot about caddy internals :) guess I could help out with your implementation if it's needed? for my own needs i only use webdav with full read/write access behind http basic auth (done by traefik atm, it routes other things also).

Sure, would be happy to open it up for collaboration. In fact, I'd be fine with merging it back upstream if @hacdias is open to that.

(Hmm, any way we can get you to have a pure Caddy stack? 🤔 )

It would be nice if using webdav under a path would be more intuitive. For example that you would not need to explicitly tell webdav what to prefix with.

What do you mean? Something like any requests within /prefix are served with webdav, even though the prefix directory does not exist on disk?

[francislavoie]

(Hmm, any way we can get you to have a pure Caddy stack? 🤔 )

I think we need the v2 equivalent of https://github.com/lucaslorentz/caddy-docker-proxy for that.

Basically - read the labels of spun up docker containers and push new server config blocks with a reverse_proxy handler with some specified host/port. Should also remove those servers blocks when the containers shut down.

(I actually have a usecase where I'd use this at work, we're thinking of using traefik for it soon, i.e. for use with GitLab Review Apps, see this guide)

[wader]

(Hmm, any way we can get you to have a pure Caddy stack? 🤔 )

😄Currently i use traefik for https, certificate acquisition and to dynamically route traffic to a bunch docker containers based on container labels. Not sure what the state of that is for caddy now a days? some years ago i was close to start doing it after having issues with nginx-proxy and https.

What do you mean? Something like any requests within /prefix are served with webdav, even though the prefix directory does not exist on disk?

Maybe an example makes it more clear. Here is my current config using caddy 2 and my webdav plugin. Notice the double mention of a and b. But now when i think about it... maybe i could have just have one route /* { ... }? had a lot trouble getting it to work, maybe i was also confused by an issue i noticed that the webdav directive did not get the config argument if it started with a slash like webdav /a { ... }?

route /a/* {
    webdav a {
        scope /path/to/a_root
    }
}

route /b/* {
    webdav b {
        scope /path/to/b_root
    }
}

Getting late in Sweden, bed time!

[mholt]

You could probably simplify that config to:

webdav /a/* {
    scope /path/to/a_root
}
webdav /b/* {
    scope /path/to/b_root
}

right?

[wader]

Should that work with your new webdav plugin? I think i struggled with the thing i mention above about leading slash and that it did not work with wildcards. Now i look it seems to use https://github.com/caddyserver/caddy/blob/v1/caddyhttp/httpserver/path.go#L36 which seem to be prefix check only... also i guess than means my plugin actually ends up building with parts of caddy v1? 😨

I will look into using your plugin as soon as possible hehe

[mholt]

@wader I guess I don't understand what you're trying to do.

Now i look it seems to use https://github.com/caddyserver/caddy/blob/v1/caddyhttp/httpserver/path.go#L36 which seem to be prefix check only..

Is that not what you wanted:

For example that you would not need to explicitly tell webdav what to prefix with.

The prefix has to go somewhere, I'm not sure how it can magically know which paths to serve webdav on and which not to?

[mholt]

Fixed in 026937fab54de4a840e25e676cd8998030a6778a

[wader]

@wader I guess I don't understand what you're trying to do.

Now i look it seems to use https://github.com/caddyserver/caddy/blob/v1/caddyhttp/httpserver/path.go#L36 which seem to be prefix check only..

Is that not what you wanted:

For example that you would not need to explicitly tell webdav what to prefix with.

The prefix has to go somewhere, I'm not sure how it can magically know which paths to serve webdav on and which not to?

Sorry for the confusion. Yes and I think the main confusing i had first when i tried to use the webdav directive was if the argument to it is a route prefix matcher, route wildcard matcher or base URL/prefix to pass to the webdav server. In hacdias webdav server it seems to be both a route prefix matcher and a base URL (that is used as Prefix for the webdav handler) which probably makes the most sense. And I can't see any obvious way to get a sane prefix from a wildcard matcher unless it happens to be matching just a prefix.

[wader]

@mholt about https://github.com/mholt/caddy-webdav let me know if you want help and have design ideas. For example it looks like prefix is missing, that i would like. Should it be just a prefix option like webdav /some/prefix/* { prefix /some/prefix }?

[mholt]

Matchers are independent of handlers in Caddy 2 -- handlers don't need to (and shouldn't -- with few exceptions) do their own request matching. Caddy will do that for them.

In Caddy 2, path matchers are exact unless you use a * character: https://caddyserver.com/docs/caddyfile/matchers#path-matchers

[wader]

Aha thanks that probably explain the config behaviour i noticed where things starting with slash turned into a match config and was not passed to the directive config parser.

Ok so then the webdav prefix needs to configured separately.

Maybe should move this discussion to the webdav plugin repo?

[mholt]

@wader Yeah, let's continue the discussion over on the other repo!