jnioche opened a new issue, #2197:
URL: https://github.com/apache/stormcrawler/issues/2197
### Version
main branch
### Describe what's wrong
## Summary
Whenever `BasicURLNormalizer` rebuilds a URL, the query string is
percent-encoded into the
path: the `?` becomes `%3F`, and the URL is left with no query at all. The
same rebuild
percent-encodes the `%` of any escape that is already in the URL, so `%21`
comes back as
`%2521`.
## Reproduction
```java
BasicURLNormalizer normalizer = new BasicURLNormalizer();
normalizer.configure(new HashMap<>(), new ObjectMapper().readTree("{}"));
String url = "HTTP://A.Example/p?b=2&a=1";
System.out.println(normalizer.filter(URLUtil.toURL(url), new Metadata(),
url));
```
| input | actual | expected |
|---|---|---|
| `HTTP://A.Example/p?b=2&a=1` | `http://a.example/p%3Fb=2&a=1` |
`http://a.example/p?b=2&a=1` |
| `http://a.example/%7euser?x=1` | `http://a.example/~user%3Fx=1` |
`http://a.example/~user?x=1` |
| `http://a.example/p?x=1` | `http://a.example/p?x=1` | unchanged, correct |
With `{"queryElementsToRemove":["utm_source"]}`, the double-encoding shows
up as well —
`~!q` is encoded to `~%21q` by the query rebuild, then to `~%2521q` by the
URL rebuild,
which decodes back to `~%21q` rather than to `~!q`:
| input | actual |
|---|---|
| `http://a.example/r?u=https%3A%2F%2Fb.example&t=~!q` |
`http://a.example/r%3Ft=~%2521q&u=https%253A%252F%252Fb.example` |
## When it happens
Only when the URL is rebuilt, i.e. when `hasChanged` is true:
- the scheme is not lowercase,
- the host is not lowercase, or is converted by `hostIDNtoASCII`,
- `unescapePath` / `escapePath` changed the path, e.g. `%7e` → `~`, or an
illegal
character had to be escaped.
A URL that needs none of these keeps its query, which is why the bug is easy
to miss: it
only bites the URLs that most need normalising.
## Cause
In `filter()`:
```java
String file = theUrl.getFile(); // path AND query
...
String file2 = unescapePath(file); // so the query is escaped as if it
were a path
file2 = escapePath(file2);
if (!file.equals(file2)) {
hasChanged = true;
}
if (hasChanged) {
URI uri = new URI(
protocol,
null, // userInfo
host,
port,
file2, // path ← path + query is passed as the path
null, // query
null // fragment
);
urlToFilter = uri.toString();
}
```
`URL.getFile()` returns path + `?` + query, and the multi-argument `URI`
constructor
percent-encodes any character that is not legal in the component it is given
— so the `?`
that separates the query becomes `%3F`, and an existing `%` becomes `%25`.
## Impact
The rewritten URL points at a different resource: a crawler that fetches
`http://a.example/p%3Fb=2&a=1` asks the server for a path that almost
certainly does not
exist. Any deduplication, revisit or comparison keyed on the normalised URL
is affected too,
since the same page reached with a mixed-case host normalises to a different
string than the
page reached in lowercase.
## Suggested fix
Escape the path only, and keep the query out of the `URI` constructor — split
`theUrl.getFile()` on its first `?`, pass the path through
`unescapePath`/`escapePath` as
today, and append `"?" + query` to the result verbatim. That also stops the
query being
double-encoded, since the query is no longer passed through path escaping at
all.
### Error message and/or stacktrace
N/A
### How to reproduce
See code above
### 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]
