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

[bluebox-steven]

Issue Details

I wrote a simple string manipulation plugin to alter placeholders with lower/upper case by appending a .lower or .upper suffix. It works by adding a custom Replacer provider mapping function. Testing showed it worked fine with static placeholders, however, it wasn't triggering on dynamic placeholders such as http.request.header.*, http.request.uri.query.*, etc. After further debugging, the issue stems from the implementation of dynamic placeholders in the addHTTPVarsToReplacer function. The dynamic placeholders are replaced whether it finds a set key or not (returning empty string if not found). However, this means that custom providers further down the chain which may want to hook into a dynamic placeholder will never be called.

ie. http.request.header.x-custom-header.upper will always be stopped in the addHTTPVarsToReplacer provider chain and return empty even though it doesn't exist.

I propose that all Replacer logic handling dynamic placeholders return false if there isn't a key match, thereby allowing other providers to apply their own logic. ie.

			// request header fields
			if strings.HasPrefix(key, reqHeaderReplPrefix) {
				field := key[len(reqHeaderReplPrefix):]
				vals := req.Header[textproto.CanonicalMIMEHeaderKey(field)]
				// always return true, since the header field might
				// be present only in some requests
				return strings.Join(vals, ","), true
			}

becomes:

			// request header fields
			if strings.HasPrefix(key, reqHeaderReplPrefix) {
				field := key[len(reqHeaderReplPrefix):]
				vals := req.Header[textproto.CanonicalMIMEHeaderKey(field)]
				if len(vals) == 0 {
					return "", false
				}
				return strings.Join(vals, ","), true
			}

Based on my testing I see no adverse effects and resolves the chaining issue, but rather than adding a pull request I thought it's best to check what the feedback on a change like this would be, and whether it's something that would be accepted.

Thanks.

Assistance Disclosure

AI not used

If AI was used, describe the extent to which it was used.

No response

[francislavoie]

I think technically that would be a breaking change because it would mean the placeholder doesn't get replaced if the value is unknown in some cases, so instead of empty string some users would end up with the {http.request.header.whatever} in whatever they use the placeholder in (unless ReplaceAll is being used which would replace with empty string even if the placeholder has no value).

It's hard to judge how much that truly matters (we don't have any way to track how much that occurs in the wild), but it would be a difference.

[francislavoie]

What we could do is detect if there's a dot in the header/query key which should be invalid under normal circumstances I think, and return false there, which would let you do what you want with .lower.

[bluebox-steven]

Thanks for the quick feedback @francislavoie - appreciate it.

I think detecting a dot would work for headers and cookies since it isn't allowed in keys by RFC, but it is valid in query keys, so not sure it's workable for all situations.

Perhaps a less impactful change in the meantime would be allowing prioritizing providers so I can get my custom provider in before the addHTTPVarsToReplacer provider. For example, a MapWithPriority(mapFunc ReplacerFunc, priority int) function. So if the priority is by default:

• globalDefaultReplacementProvider{} is priority 0
• fileReplacementProvider{} is 1
• fromStatic is 2
• addHTTPVarsToReplacer is 3

Then I could use MapWithPriority(func(key string) (any, bool) {}, 3) which will shift addHTTPVarsToReplacer down to priority 4.