Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Message view gives error on malformed image URL #4449

Closed
gnprice opened this issue Jan 30, 2021 · 0 comments · Fixed by #4450
Closed

Message view gives error on malformed image URL #4449

gnprice opened this issue Jan 30, 2021 · 0 comments · Fixed by #4450
Assignees
@gnprice
Labels
a-message list

Comments

@gnprice
Copy link
Member

gnprice commented Jan 30, 2021

Recently I've occasionally been seeing a red error banner at the top of the message list (the one that says "Oh no! Something went wrong. Try again?".) I've only seen it when looking at #test here on czo -- so presumably there's something exciting happening in some message there that triggers whatever bug is involved.

Then yesterday I was looking at adb logcat output in order to do some performance measurements (for #4448), and I hit the same symptom and saw this in the log:

01-28 22:00:15.545 23892 23927 E ReactNativeJS: WebView error
01-28 22:00:15.553 23892 23927 E ReactNativeJS:     message: "Uncaught TypeError: Failed to construct 'URL': Invalid URL"
01-28 22:00:15.553 23892 23927 E ReactNativeJS:     source: "file:///android_asset/webview/index.html"
01-28 22:00:15.553 23892 23927 E ReactNativeJS:     line: 263
01-28 22:00:15.553 23892 23927 E ReactNativeJS:     column: 22
01-28 22:00:15.553 23892 23927 E ReactNativeJS:     error: {}
01-28 22:00:15.554 23892 23927 E ReactNativeJS: index.android.bundle:559:1322
01-28 22:00:15.554 23892 23927 E ReactNativeJS: [email protected]:2170:2490
01-28 22:00:15.554 23892 23927 E ReactNativeJS: [email protected]:2143:2114
01-28 22:00:15.554 23892 23927 E ReactNativeJS: [email protected]:2145:5068
01-28 22:00:15.554 23892 23927 E ReactNativeJS: [email protected]:27:3751
01-28 22:00:15.554 23892 23927 E ReactNativeJS: index.android.bundle:27:812
01-28 22:00:15.554 23892 23927 E ReactNativeJS: [email protected]:27:2958
01-28 22:00:15.554 23892 23927 E ReactNativeJS: [email protected]:27:784
01-28 22:00:15.554 23892 23927 E ReactNativeJS: callFunctionReturnFlushedQueue@[native code]

Specifically, I'd just narrowed to the topic "abc" on that stream. That should make this bug now highly reproducible.

The error message is "Uncaught TypeError: Failed to construct 'URL': Invalid URL". So apparently we're trying to construct a URL object, the URL isn't valid, and we're throwing an error. This is likely to cause other functionality in the message list to break, because whatever the code was trying to do at the time is going to get interrupted in an incomplete state.

We don't have great source-line mapping for this traceback. It's line 263 of "file:///android_asset/webview/index.html" -- which is the URL we make up (in MessageList.js) for the one page the webview shows, consisting of HTML built on the fly.

But that HTML we build starts with a few lines of stuff like <html><head>…, and then copies in the giant string literal in generatedEs3.js. So the line number should be not too far off from line numbers in that generated file.

And looking there for new URL, there's three of them, of which one is around the right spot:

  var rewriteImageUrls = function rewriteImageUrls(auth, element) {
    var realm = auth.realm;
    var imageTags = [].concat(element instanceof HTMLImageElement ? [element] : [], Array.from(element.getElementsByTagName('img')));
    imageTags.forEach(function (img) {
      var actualSrc = img.getAttribute('src');

      if (actualSrc == null) {
        return;
      }

      var fixedSrc = new URL(actualSrc, realm);

That's line 247 there, so 16 lines off, which fits.

I think the bug here is simply that we're taking this URL from the HTML the server rendered for the message body, and counting on it to be a well-formed URL, and that's apparently not something the server guarantees (or successfully guarantees.)

Specifically it looks like the message that triggers this is probably this one:
https://chat.zulip.org/#narrow/stream/7-test-here/topic/abc/near/693721

Its Markdown source looks like this:

(deleted) [Screenshot-2019-02-04-at-1.43.30-AM.png](https://chat. [Click to join video call](https://meet.jit.si//211922917196696) zulip.org/user_uploads/2/c2/F5oyoXEQinkG1zCjM38pTeWD/Screenshot-2019-02-04-at-1.43.30-AM.png)

Sure is an invalid URL there -- it's trying to have a hostname that starts with chat. [, which is not going to fly. Looks like something that got garbled during compose by an interaction between the "Add video call" button, and an image link that was already there. In general Zulip certainly has to handle arbitrary nonsense that might be in a message's source.

In principle the server, when it's turning part of that Markdown source into an HTML img element, perhaps should notice if the URL isn't a well-formed URL string and should refuse to produce such an img. But it looks like the server doesn't do that, or anyway didn't when this message was sent (and rendered to HTML) a couple of years ago. Inspecting in the browser finds this element:
<img src="https://chat.%20[Click%20to%20join%20video%20call](https://meet.jit.si//211922917196696)%20zulip.org/user_uploads/2/c2/F5oyoXEQinkG1zCjM38pTeWD/Screenshot-2019-02-04-at-1.43.30-AM.png">

So, we as the client need to be prepared for that in turn. When a URL inside a message body is invalid, we should pass over it gracefully and not let it disrupt anything else we're doing.
@gnprice gnprice self-assigned this Jan 30, 2021
This was referenced Jan 30, 2021
Merged
Closed
Gautam-Arora24 pushed a commit to Gautam-Arora24/zulip-mobile that referenced this issue Feb 3, 2021
@gnprice @Gautam-Arora24
msglist: Gracefully ignore ill-formed image URLs.
5a6f035 
Recently I've occasionally been seeing a red error banner at the top
of the message list (the one that says "Oh no! Something went
wrong. Try again?".)  I've only seen it when looking at #**test here**
on czo -- so figured there must be an oddity in some message there
that triggered whatever bug it was.

And indeed, it turns out that the trigger is a message with an image
element where the image's URL isn't a well-formed URL.  We'd been
implicitly assuming that that couldn't happen -- the server is after
all generating this HTML, and could prevent that.  But it turns out
that it doesn't.

Quite possibly the server should and in the future will refuse to
generate such elements.  But the Zulip server renders messages to HTML
immediately at send time, and in general we never go back and
re-render old messages.  (That's because if we did, then every subtle
change in Zulip's Markdown format, or even changes in its
implementation that were intended to have no effect on behavior, would
carry a risk of silently corrupting old messages.)  So even if the
server becomes stricter about this in the future, clients will still
need to handle messages with this kind of malformedness indefinitely.

(For that matter, in fact, it's possible the server already has made
that change in the two years since the message I found that triggers
this issue was sent.)

From the code, it looks like this bug has been here since we started
explicitly absolutizing these URLs, in 7ef0aeb in 2019.  Yet I was
pretty confident I hadn't seen it before, when I saw it sometime in
the last few weeks; and every time I've seen it since then, it's been
on the same stream and likely due to the same one message there.
So that's a little reassuring that this isn't a super common issue
to hit, at least.

Fixes: zulip#4449
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gnprice gnprice

Labels
a-message list
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

msglist: Gracefully ignore ill-formed image URLs. gnprice/zulip-mobile
1 participant
@gnprice