vmod_querysort treats `/a?&` as different than `/a`

[DemiMarie]

Expected Behavior

vmod_querysort strips consecutive ampersands, but doesn’t convert /a? or /a?&&&&& to /a.

Current Behavior

vmod_querysort should convert /a?&&& (any number of &, including 0) to /a.

Possible Solution

In https://github.com/varnishcache/varnish-cache/blob/95e41dfa584d108e444949534c7ce5801cffeacc/vmod/vmod_std_querysort.c#L69-L77, do something like (untested):

	const char *ce;

	/* Split :query from :url */
	cu = strchr(url, '?');
	if (cu == NULL)
 		return (url);

	ce = cu + 1;
	while (*ce == '&')
		ce++;
	if (*ce == '\0') {
		int return_size = cu - url;
		r = WS_Copy(ctx->ws, url, return_size + 1);
		if (r == NULL)
			return (url); /* or return an error */
		r[return_size] = 0;
	}

Steps to Reproduce (for bugs)

Use vmod_querysort with a URI matching the regular expression \?&*$.

Context

This might lead to a suboptimal cache-hit rate. I noticed this when looking at the Wikimedia Foundation’s version of this vmod.

Varnish Cache version

No response

Operating system

No response

Source of binary packages used (if any)

No response

[dridi]

Disclaimer, I will suggest a VMOD I authored.

You can achieve the desired result and more with vmod_querystring: https://git.sr.ht/~dridi/vmod-querystring

[walid-git]

@DemiMarie Would you be able to submit a PR with your suggested fix ?

[DemiMarie]

@walid-git I think so, yes. I’m no expert on authoring VMODs but I can at least make sure it compiles and passes some basic tests.

[walid-git]

@walid-git I think so, yes. I’m no expert on authoring VMODs but I can at least make sure it compiles and passes some basic tests.

That would be great. Please have a look at pdiff and WS_VSB* functions that might help here.