caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
57.43k stars 4k forks source link

matchers: support `null` value in expression matcher #6320

Open mohammed90 opened 4 months ago

mohammed90 commented 4 months ago

The plain header match (non-CEL) supports checking for the absence of a header. The corresponding CEL matcher couldn't achieve the same because it expects a string a slice in the value of header->value map. In the plain header matcher, the absence of a value is represented by a nil in-place of zero-slice as the value of the header key in the map. The nil only equals nil when iterating through the map. We needed an analogous value, i.e. null.

My attempt here is to add support for null. I'm stumbling in the dark as I'm not familiar with the cel-go module, but something works, so am I close?

Here are the checks I've run:

~ $ # Before: using `@without `header({"X-Custom-Header": ""})``
~ $ curl -k https://localhost
with
~ $ curl -k -H "X-Custom-Header:" https://localhost
with
~ $ curl -k -H "X-Custom-Header: d" https://localhost
with

~ $ # Before: using `@without `header({"X-Custom-Header": [] })``
2024-05-17 1:20:57  ~ $ curl -k https://localhost
with
~ $ curl -k -H "X-Custom-Header:" https://localhost
with
~ $ curl -k -H "X-Custom-Header: val" https://localhost
without

~ $ # After: using ``
~ $ curl -k https://localhost
without
~ $ curl -k -H "X-Custom-Header:" https://localhost
without
~ $ curl -k -H "X-Custom-Header: val" https://localhost
with

So, works?

mohammed90 commented 4 months ago

So, works?

But breaks other stuff 🤔 https://github.com/caddyserver/caddy/actions/runs/9120010471/job/25076559154?pr=6320#step:9:568

francislavoie commented 4 months ago

I think this is making all strings nullable? I think that's probably not what we want :thinking: We might need to make a specific function to handle null but only for handle (or a different set of handlers). It's crashing with a SIGSEGV which is kinda wild but I guess not totally surprising with null lol

@TristonianJones FYI in case you have any suggestions. The gist of it is we're trying to allow header({"X-Customer-Token": null}) I think (i.e. null values in "JSON-like" config, but probably only for this one matcher/function)

TristonianJones commented 4 months ago

I think this is making all strings nullable? I think that's probably not what we want 🤔 We might need to make a specific function to handle null but only for handle (or a different set of handlers). It's crashing with a SIGSEGV which is kinda wild but I guess not totally surprising with null lol

@TristonianJones FYI in case you have any suggestions. The gist of it is we're trying to allow header({"X-Customer-Token": null}) I think (i.e. null values in "JSON-like" config, but probably only for this one matcher/function)

Hi @francislavoie, I think part of this issue is that the internal types are map[string]string and map[string][]string even though the CEL type is a map(string,dyn). If you want to use null to represent absent, then you can absolutely do this; however, doing so would require handling null everywhere it could appear, at least as far as the macro is concerned. It's usually a bad idea to relax the constraints conditionally, in one case, but not all cases. Can you omit the key and use 'key-name' in map instead as the expression for testing existence?