close
Skip to content

vars: Add matcher placeholder handling tests#7640

Merged
mholt merged 2 commits into
caddyserver:masterfrom
steadytao:fix/vars-matcher-tests
Apr 10, 2026
Merged

vars: Add matcher placeholder handling tests#7640
mholt merged 2 commits into
caddyserver:masterfrom
steadytao:fix/vars-matcher-tests

Conversation

@steadytao
Copy link
Copy Markdown
Member

Adds focused regression coverage for #7629.

Summary

This adds table-driven tests for matcher-side placeholder handling in vars and vars_regexp.

The main goal is to cover the case where already-resolved values contain placeholder syntax such as {env.SECRET} and must not be re-expanded by the matcher. It also adds a compatibility case to make sure matcher-side values still expand placeholders as before.

Behaviour

Added coverage to verify that:

  • VarsMatcher does not re-expand resolved values from literal vars
  • VarsMatcher does not re-expand resolved values from placeholder-key lookups
  • matcher values in VarsMatcher still expand placeholders
  • MatchVarsRE does not re-expand resolved values from literal vars
  • MatchVarsRE does not re-expand resolved values from placeholder-key lookups

These tests exercise the matcher boundary directly by injecting already-resolved values into VarsCtxKey which matches the behavior changed in #7629.

Assistance Disclosure

No AI was used.

Copy link
Copy Markdown
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thank you!

Happy to merge this as-is; one other thought I had is, do you think it's worth adding a test where a placeholder that accesses the query string is used? e.g. {http.request.uri.query.foo} -- obviously it shouldn't matter, but it should go to document that we've covered that case as well. (I actually got a duplicate report that used the query string, instead of the header, I presume because they did not realize the patch was universal.)

@mholt mholt added this to the v2.11.3 milestone Apr 10, 2026
@steadytao
Copy link
Copy Markdown
Member Author

Do you think it's worth adding a test where a placeholder that accesses the query string is used? e.g. {http.request.uri.query.foo}

Would be worth. Will do so now.

Copy link
Copy Markdown
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Beautiful. Thanks!

@mholt mholt merged commit 7dcc041 into caddyserver:master Apr 10, 2026
27 checks passed
@steadytao steadytao deleted the fix/vars-matcher-tests branch April 10, 2026 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants