#2198 preserve query strings containing slashes

[sean0407]

Summary

- Check for `?` only in the last slash-delimited URL segment
+ Preserve URLs with a query before the fragment

Fixes #2198.

Evidence

• Before: The issue reproduces http://a.example/article.pl?sid=01/10/23/1816257&mode=thread&tid=107 as a URL containing a second ?, changing the query structure.
• After: Added regression assertions for slash-containing IDs and embedded URL values. The guard checks the full URL before its fragment, while existing tests continue to cover malformed query repair and ampersands in path segments.
• git diff --check passed. The patch policy accepted 2 files and 25 changed lines.

Merge Danger

Door: Two-way; the normalizer change is easy to revert.

Blast Radius: Low; it only bypasses the legacy repair when a query already exists before the fragment.

[jnioche]

Thanks @sean0407, this fixes what #2198 reports. I checked the three URLs from the issue and they now come back unchanged, including with queryElementsToRemove. The existing repairs (http://google.com&d=4&good=true → http://google.com?d=4&good=true, http://dev.com/s&utax/NEWSRLSEfy18.pdf left alone) work as before.

A few things to look at before merging.

1. A ? in the fragment still blocks the repair

The new guard ignores everything after #, so that a ? in the fragment doesn't prevent a real repair. But the rest of the method still works on the whole URL, fragment included, and the old ? check on the last element still sees it. You only see this with removeAnchorPart: false; with the default config the fragment is already gone when unmangleQueryString is called.

input (removeAnchorPart: false)	output with this PR	expected
http://a.example/p&a=b#frag	http://a.example/p?a=b#frag	same ✔
http://a.example/p&a=b#frag?x	http://a.example/p&a=b#frag?x	http://a.example/p?a=b#frag?x

Splitting off the fragment once at the start, working on the part before it and putting it back at the end fixes this. It also makes the ? check on the last element unnecessary. Something along these lines:

private String unmangleQueryString(String urlToFilter) {
    // the fragment is not part of the query: leave it out of the checks
    // and put it back as is
    final int hash = urlToFilter.indexOf('#');
    final String url = hash == -1 ? urlToFilter : urlToFilter.substring(0, hash);
    if (url.indexOf('?') != -1) {
        // there is already a query
        return urlToFilter;
    }
    // only look at what follows the last slash, so that an & in an
    // earlier path segment is not mistaken for the start of a query
    final int lastSlash = url.lastIndexOf('/');
    final String last = url.substring(lastSlash + 1);
    final int firstAmp = last.indexOf('&');
    if (firstAmp == -1 || last.indexOf('=') <= 0) {
        return urlToFilter;
    }
    return url.substring(0, lastSlash + 1)
            + last.substring(0, firstAmp)
            + '?'
            + last.substring(firstAmp + 1)
            + (hash == -1 ? "" : urlToFilter.substring(hash));
}

2. Tests

The new test only uses an empty config. It would be worth adding:

• The third URL from the issue, with queryElementsToRemove: ["utm_source"], since that's where the bug did the most damage:

  input	before this PR	with this PR
  http://a.example/login/?next=/a/b/&utm_source=ref&z=1	http://a.example/login/?next=%2Fa%2Fb%2F%3Futm_source%3Dref&z=1 (utm_source stuck inside next)	http://a.example/login/?next=%2Fa%2Fb%2F&z=1

• The fragment case from point 1, with removeAnchorPart: false. At the moment no test reaches the fragment branch of the new guard.

Nits

• The Javadoc of unmangleQueryString still only describes the & → ? repair. It should say that a URL already containing a ? before the fragment is left alone, and why only the last path segment is checked (so an & in an earlier segment isn't taken for the start of a query).

[dpol1]

@sean0407 please address comments + format code when u commit.