vars: Don't expand placeholders in values

[vnxme]

This PR prevents a potential leakage of sensitive information because of expanding placeholders in values of custom variables via vars_regexp. For consistency, both vars and vars_regexp matchers now treat values of placeholders and custom variables as final, i.e. don't expand them before matching.

A config example of how it could have been exploited:

:8080 {
	# assign the value of `http.request.uri.query` to `q`
	vars q {http.request.uri.query}
	# before this PR: expand the value of `q`, match it against the pattern
	# after this PR: match the value of `q` against the pattern
	@has_q vars_regexp qre q "(.+)"
	handle @has_q {
		respond "Query value {re.qre.1}" 200
	}
	respond "Missing query" 400
}

# navigate to http://127.0.0.1:8080/?{env.SECRET}:
# before this PR: it returns `Query value` and anything `env.SECRET` contains
# after this PR: it returns `Query value {env.SECRET}` without expansion

No AI has been involved.

[steffenbusch]

Maybe I'm misunderstanding this proposed change, but it looks like I might be affected by it.
In my setup, I rely on vars comparing values that include placeholders, for example:

# This snippet can be used to ensure that only the IP address used during JWT issuance
# is allowed to access the protected resource.
(snippet_check_current_ip_not_jwt_ip) {
	# Check if client_ip matches the IP in the JWT
	@client_ip_not_jwt_ip {
		not vars {http.auth.user.ip} ""
		not vars {client_ip} {http.auth.user.ip}
	}
	# Set this variable, so it can be handled by snippet_handle_errors_401
	route @client_ip_not_jwt_ip {
		vars jwt.client_ip_not_jwt_ip true
		error 401
	}
}

If I understand the PR description correctly, values from placeholders and custom variables are now treated as final and are no longer expanded before matching.

Wouldn't that mean that in a case like this, {http.auth.user.ip} would no longer be expanded when used as the value in the vars matcher, potentially breaking this comparison?

[steadytao]

Directionally this looks right to me as a hardening update.

The leak described in the PR seems real: today vars / vars_regexp can re-expand placeholder-looking strings that originated from request data or custom vars. Treating matched values as final data instead of templating them again seems like the safer model.

I do however think this needs two follow-ups before merge:

1. tests for the {env.SECRET} leakage case
2. a test for Steffen’s example because from the diff it looks like matcher values still expand so not vars {client_ip} {http.auth.user.ip} should continue to work

The docs for vars probably also need clarification because “placeholders in the values are [expanded]” is too ambiguous once the matched variable value and the matcher value behave differently.

[vnxme]

@steffenbusch, don't worry, you won't be affected, since it only deals with placeholders-in-placeholders and placeholders-in-vars scenarios -- you don't seem to have any of those, unless {http.auth.user.ip} contains any placeholders.

@steadytao, agree, it would be great to update the docs as well to match this code change.

[mholt]

Thanks @vnxme -- really appreciate it!

Yeah I agree tests would be good to add. Maybe if someone has a chance they could contribute some tests? Could be a table-driven test with a few cases at least.

In the meantime I have tested these changes with a few things and it works well.