serenity
serenity copied to clipboard
SerenityOS
Browser: URLs containing new lines (CRLF) render poorly in Browser UI
Note the text overlap in the address bar.
Edit: Note that the HTTP/1.1 [...] portion of the URL is from a test file used to test the HTTP header injection issue (#6557). It is unrelated to this issue - it's simply the test file I was using initially. HTTP header injection is no longer possible and unrelated to this issue.
<!DOCTYPE html>
<html>
<head>
</head>
<body>
<a href="http://10.1.1.115:1337/test HTTP/1.1
Abcd: efgh
Ijkl: mnop
">Click me</a>
</body>
</html>
This can also be used to perform some mildly amusing URL spoofing attacks in the status bar and address bar.
<!DOCTYPE html>
<html>
<head>
</head>
<body>
<a href="http://example.com/?
http://google.com/
">Click me</a>
</body>
</html>
Tweaking the number of new lines and spaces in the URL could probably make the address bar URL look more realistic.
For example, for long URLs, the address bad scrolls to the right.
<!DOCTYPE html>
<html>
<head>
</head>
<body>
<a href="http://example.com/? http://google.com/
">Click me</a>
</body>
</html>
Fix
The approach taken in #6557 was to URL encode characters such as 0x0a and 0x0d to resolve issues with CRLF new line injection in HTTP requests. This change applied only to ResourceLoader and did not patch rendering of the URL in Browser UI or status bar. A similar approach should be applied here.
Note that there are likely mane other characters which are problematic for rendering (such a tab 0x09).
This same issue can be seen when pasting js code with newlines in the js console. Maybe TextEditor should replace newlines with whitespace in single line mode?
This same issue can be seen when pasting js code with newlines in the js console. Maybe TextEditor should replace newlines with whitespace in single line mode?
That seems reasonable for text editor widgets.
However, that seems like a different issue for Browser as URLs should be URL encoded in the address bar and status bar. Issues with CRLF exist across a lot of the code base. This is one of the more "interesting" bugs.
The approach taken in #6557 was to URL encode characters such as 0x0a and 0x0d to resolve issues with CRLF new line injection in HTTP requests. This change applied only to ResourceLoader and did not patch rendering of the URL in Browser UI or status bar. A similar approach should be applied here.
It could maybe be the best approch to just patch URL::to_string() to esacpe control characters (such as CR, LF, ...; but not HTML control characters like /, &, ...), even if you do not call to_string_encoded(). I think this is a unique possibility with URLs as we do not modify the semantics of the URL as it points to the same resource by definition. This is unlike other cases (like #7277, #7279, #7280), where you sometimes have to be careful not to change something for semantic reasons.
When the URL widget is pre-filled by following a link characters not allowed in an URL (such as newline or space) should be URL-encoded.
It is not clear what to do with characters typed/pasted by the user. Some browsers ignore newlines completely when pasted (ie it's possible to paste wrapped URL and it just works). Other characters may be accepted and encoded once the user requests navigating to the entered URL.
