Display of logo of the Reconciliation service a which column is reconciled against

[ayushrai206]

Fixes #4824

Changes proposed in this pull request:

• We have a logo of the service against which the column is reconciled beside the recon stats bar
• We also have the logo beside the column name during schema building(if the column is reconciled against a service which has a logo )

[Abbe98]

I'm a little worried about this one, should we trust reconciliation services to the extent that we inject content this way? It kinda defeats the sandboxing work done in other places.

[wetneb]

Thanks for bringing this up! We already include external images in the Wikibase selection dialog, so it wouldn't be a new threat, but it's still worth considering of course.
Looking at this stackoverflow post, the following concerns are brought up:

• user tracking: that's already possible whenever the user fetches the manifest of the reconciliation service or does any other actions with it, but maybe those images would be requested more often.
• 404 errors: in that case, the logo just wouldn't be displayed, and that's probably fine?
• change of content of the logo: I think that's fine
• browser vulnerability exploit: that's something that seems difficult to avoid and probably affects a lot of other places.

In general, I wonder if it would make sense to channel all calls to reconciliation services via the backend. It would have some latency consequences (for instance for suggest services) but perhaps it would be overall a good move:

• we would have a tighter control over the headers that are exposed to the reconciliation service, preventing user-agent leak for instance
• we wouldn't have to worry about services supporting CORS anymore
• the hostname of the reconciliation service would only need to be resolvable from one place, the backend. I have seen people being confused about bugs which were due to the fact that only the browser or the server were able to resolve the hostname, in situations where the server isn't running on the same machine as the browser
• in this particular case, we'd be able to sanitize (such as thumbnail) the image before serving it to the frontend, although this would likely require some pretty heavy external libraries

But of course such a move is a pretty big task and I would say it's outside the scope of this issue/PR.

[Abbe98]

There is a lot of attack surface here, first one could just point to a SVG image as SVG support <script> elements and (more importantly given browser-context) various "on" events.

Secondly someone could inject something other than an image and do all kinds of nasty things.

https://security.stackexchange.com/questions/135513/what-could-an-img-src-xss-do

We already include external images in the Wikibase selection dialog, so it wouldn't be a new threat

It wouldn't be a new threat to users of the Wikibase extension to the rest of us it would be new(AFAIK).

[wetneb]

Yes I also thought about SVG, but as your link indicates, including an SVG image via <img src="..." /> does not execute the JS inside: it would only work if the SVG was embedded directly as an <svg> tag.

Secondly someone could inject something other than an image and do all kinds of nasty things.

what are you thinking about? If it's not an image than the browser will not fall back on just including whatever is served there as HTML code, right?

[Abbe98]

https://security.stackexchange.com/questions/135513/what-could-an-img-src-xss-do

All the answers apply here not only the accepted one about SVG.

[wetneb]

Alright, so from what I can tell we are safe as long that we validate that the string is a valid URL with HTTP(S) protocol. We could additionally require that the host of the URL matches that of the reconciliation service, if we want to avoid triggering CSRF attacks on vulnerable services.

Does that seem like an appropriate mitigation to you, or do you see other possible issues?

[wetneb]

@Abbe98 are you happy with the mitigation I proposed above? I would like to propose to @ayushrai206 that we go ahead with this, with the URL and host validation.

[Abbe98]

I think passing the URL through new URL() is enough.

[wetneb]

Makes sense. Closing this PR in favor of #6156 then