jnioche opened a new issue, #2198: URL: https://github.com/apache/stormcrawler/issues/2198
### Version main branch ### Describe what's wrong ## Summary `unmangleQueryString` is meant to repair a URL whose query starts with `&` instead of `?` (`http://foo.example&a=b` → `http://foo.example?a=b`). It decides whether a query is already present by looking at the last `/`-separated element of the URL only. When a query parameter value contains a `/`, the real `?` sits in an earlier element and is not seen, so the check passes and the first `&` of a perfectly valid query is rewritten to `?`. Everything after that point stops being a query parameter: it becomes part of the value of the parameter before it. It is on by default (`unmangleQueryString` defaults to `true`). ## Reproduction With an empty `params` block, so nothing else touches the query: ```java BasicURLNormalizer normalizer = new BasicURLNormalizer(); normalizer.configure(new HashMap<>(), new ObjectMapper().readTree("{}")); String url = "http://a.example/article.pl?sid=01/10/23/1816257&mode=thread&tid=107";; System.out.println(normalizer.filter(URLUtil.toURL(url), new Metadata(), url)); ``` | input | actual | expected | |---|---|---| | `http://a.example/article.pl?sid=01/10/23/1816257&mode=thread&tid=107` | `http://a.example/article.pl?sid=01/10/23/1816257?mode=thread&tid=107` | unchanged | | `http://a.example/p?u=http://b.example/x&z=1` | `http://a.example/p?u=http://b.example/x?z=1` | unchanged | | `http://a.example/login/?next=/a/b/&utm_source=ref&z=1` | `http://a.example/login/?next=/a/b/?utm_source=ref&z=1` | unchanged | Any URL carrying a path-like or URL-like value is affected — `next=`, `url=`, `redirect=`, `return_to=`, and ids such as `sid=01/10/23/1816257`. ## Consequence for parameter removal With `{"queryElementsToRemove":["utm_source"]}`, the third URL above becomes: ``` http://a.example/login/?next=%2Fa%2Fb%2F%3Futm_source%3Dref&z=1 ``` `utm_source` is still there, and is no longer removable: by the time the query is parsed it is part of the value of `next`, not a parameter of its own. So the bug also silently defeats `queryElementsToRemove` and `removeHashes` for these URLs. ## Cause ```java private String unmangleQueryString(String urlToFilter) { String[] pathElements = urlToFilter.split("/"); final String lastPathElement = pathElements[pathElements.length - 1]; int firstAmp = lastPathElement.indexOf('&'); if (firstAmp == -1) { return urlToFilter; } int firstQuestionMark = lastPathElement.indexOf('?'); // ← only the last element if (firstQuestionMark == -1 && lastPathElement.indexOf("=") > 0) { pathElements[pathElements.length - 1] = lastPathElement.replaceFirst("&", "?"); return String.join("/", pathElements); } return urlToFilter; } ``` For `http://a.example/article.pl?sid=01/10/23/1816257&mode=thread&tid=107`, splitting on `/` makes the last element `1816257&mode=thread&tid=107`. It holds an `&`, no `?` and an `=`, so it looks exactly like a query that lost its `?`. ## Impact The rewritten URL points at a different resource, since the server sees a single parameter whose value now contains the rest of the query. Redirect and login URLs, which are precisely the ones carrying a URL as a parameter value, are the most affected. ## Suggested fix Decide whether a query is present by looking at the whole URL rather than at its last `/` element, and leave the URL alone when there is already a `?`: ```java final int hash = urlToFilter.indexOf('#'); final String beforeFragment = hash == -1 ? urlToFilter : urlToFilter.substring(0, hash); if (beforeFragment.indexOf('?') != -1) { return urlToFilter; // already has a query, nothing to unmangle } // ... unchanged from here: last element, first '&', '=' check, replaceFirst("&", "?") ``` Keeping the rest of the method as it is preserves the repair it was written for, and the restriction to the last element still stops an `&` in the path from being mistaken for the start of a query. The fragment is excluded from the check so that a `?` inside `#…` does not suppress a genuine repair. These must keep working, and do with the change above: | input | output | |---|---| | `http://a.example&a=b` | `http://a.example?a=b` | | `http://a.example/p&a=b&c=d` | `http://a.example/p?a=b&c=d` | | `http://a.example/p?a=1&b=2` | unchanged | ### Error message and/or stacktrace N/A ### How to reproduce With an empty `params` block, so nothing else touches the query: ```java BasicURLNormalizer normalizer = new BasicURLNormalizer(); normalizer.configure(new HashMap<>(), new ObjectMapper().readTree("{}")); String url = "http://a.example/article.pl?sid=01/10/23/1816257&mode=thread&tid=107";; System.out.println(normalizer.filter(URLUtil.toURL(url), new Metadata(), url)); ``` | input | actual | expected | |---|---|---| | `http://a.example/article.pl?sid=01/10/23/1816257&mode=thread&tid=107` | `http://a.example/article.pl?sid=01/10/23/1816257?mode=thread&tid=107` | unchanged | | `http://a.example/p?u=http://b.example/x&z=1` | `http://a.example/p?u=http://b.example/x?z=1` | unchanged | | `http://a.example/login/?next=/a/b/&utm_source=ref&z=1` | `http://a.example/login/?next=/a/b/?utm_source=ref&z=1` | unchanged | Any URL carrying a path-like or URL-like value is affected — `next=`, `url=`, `redirect=`, `return_to=`, and ids such as `sid=01/10/23/1816257`. ### Additional context _No response_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
