Skip to content

caddyhttp: let dotted dynamic placeholders fall through#7766

Closed
puneetdixit200 wants to merge 4 commits into
caddyserver:masterfrom
puneetdixit200:fix/7373-replacer-dynamic-fallback
Closed

caddyhttp: let dotted dynamic placeholders fall through#7766
puneetdixit200 wants to merge 4 commits into
caddyserver:masterfrom
puneetdixit200:fix/7373-replacer-dynamic-fallback

Conversation

@puneetdixit200
Copy link
Copy Markdown

@puneetdixit200 puneetdixit200 commented May 22, 2026

Fixes #7373

Summary

  • Add Replacer.MapFirst so modules can explicitly register a custom replacement provider before existing providers.
  • Use that precedence hook for dynamic HTTP placeholder composition instead of treating dotted header/query placeholder names as an implicit extension delimiter.
  • Preserve existing known-empty behavior for missing header/query placeholders, including dotted query keys that can be real query parameters.

Tests

  • GOMODCACHE=/tmp/codex-caddy-gomodcache GOCACHE=/tmp/codex-caddy-gocache go test . -run TestReplacerMapFirst -count=1
  • GOMODCACHE=/tmp/codex-caddy-gomodcache GOCACHE=/tmp/codex-caddy-gocache go test ./modules/caddyhttp -run 'TestHTTPVarReplacement|TestHTTPVarReplacementCustomProviderPrecedence' -count=1\n- GOMODCACHE=/tmp/codex-caddy-gomodcache GOCACHE=/tmp/codex-caddy-gocache go test ./modules/caddyhttp -count=1\n- git diff --check\n\n## Assistance Disclosure\nAI/LLM assistance from OpenAI GPT-5 was used for implementation and test workflow support. I reviewed, authored, and verified the submitted changes.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 22, 2026

CLA assistant check
All committers have signed the CLA.

@steadytao
Copy link
Copy Markdown
Member

I do not think this really fixes #7373. The underlying issue is that custom replacer providers cannot reliably compose with dynamic HTTP placeholders because addHTTPVarsToReplacer claims the key before later providers can inspect it. This PR only makes only one guessed convention work; missing header/query placeholders containing . fall through.

That means it works for a suffix style like {http.request.header.foo.upper} but it does not solve the general composition problem and it is ambiguous because dotted header names and dotted query keys can be real keys. A valid absent key like {http.request.header.foo.bar} would now be treated as unknown/fallback instead of a known-empty dynamic placeholder.

I think this needs a clearer extension mechanism rather than using . as an implicit delimiter inside dynamic placeholder names. Also, the dependency bump is already on current master so I will update this branch. CLA is still pending too.

steadytao and others added 2 commits May 23, 2026 07:14
Add a MapFirst hook so custom replacer providers can opt into running before built-in request placeholders instead of relying on dotted header or query names falling through.

Assisted-by: OpenAI GPT-5
@steadytao
Copy link
Copy Markdown
Member

Thanks but I am closing this PR. The CLA is still unsigned, you did not address my concerns and the updated patch still does not address the core issue in #7373. Adding MapFirst() proves that provider order can be overridden but it does not define the placeholder composition model we need for dynamic HTTP placeholders and it does not resolve the ambiguity around valid dotted header/query keys.

I will not merge a broad core replacer API change as a partial fix here. Please do not attempt to reopen this PR. If you want to continue working on #7373, please discuss the intended design in the issue first so we can agree on the approach before another implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

declined 🚫 Not a fit for this project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replacer's addHTTPVarsToReplacer should return false on unset dynamic placeholder keys

3 participants