Attribution should not use innerHTML=

[mike-marcacci]

Motivation

While working on #7914 to add rel="noopener" to cross-domain links, I noticed that attribution control links are directly injected using innerHTML=. For me this raises a couple of questions:

1. How can I get rel="noopener" added to these links? As it stands, this would have to change on the data source side. Is HTML really the best way to communicate attribution?
2. Using innerHTML= is a potential security issue. While mapbox is obviously a trusted 3rd party, there are certain use cases where having this opportunity for remote execution would be very undesirable. This is especially important if using third party sources – until I read the source code just now, I thought I was simply consuming their data, and didn't realize that I was essentially giving them full access to the contents of my page.

Design Alternatives

1. Parse the HTML on the client side, and reconstruct DOM links accordingly. It looks like mapbox-gl-native already does this to make its attribution links. This seems a bit silly to do, but is probably the best way of fixing this while being backwards-compatible.
2. The most straightforward fix is to change the source attribution format to separately describe the titles and URLs of each source. However, this would require a change to the spec and break all sorts of existing implementations.

[ryanhamley]

Thanks for pointing this out @mike-marcacci I've been playing around with this a bit and confirmed that it's possible to force a map to execute arbitrary Javascript via the attribution control.

Some form of HTML parsing/sanitizing is probably the best solution here or anywhere it's possible for the user to add content with innerHTML.

[1ec5]

Parse the HTML on the client side, and reconstruct DOM links accordingly. It looks like mapbox-gl-native already does this to make its attribution links. This seems a bit silly to do, but is probably the best way of fixing this while being backwards-compatible.

For context, the iOS and macOS map SDK use a platform-standard parser to convert the HTML to RTF. On macOS, we display the RTF string as rich text in a native label control, replacing any links with native buttons. On iOS, we pick out the links and display them individually as buttons. The conversion to RTF was necessary because we wanted to use a native control rather than an embedded Web browser. It’s essentially a workaround for the lack of a structured attribution mechanism in the TileJSON spec: mapbox/tilejson-spec#20. But it’s worth noting that the iOS/macOS implementation is still pretty flexible: you can go so far as to include a remote image in the attribution string and it’ll work. The only thing that’s truly disabled is JavaScript.

[mourner]

Looks like Mapbox.js had a similar issue and it was solved there by using a special google-caja-based client-side sanitizer. However, it is 10kb minified/gzipped and adopting it would mean a 6% bundle increase. Perhaps we should indeed explore a simpler alternative for this (meaning writing our own attribution parser).

[mourner]

Looks like the most advanced and well-developed modern JS sanitizer is https://github.com/cure53/DOMPurify, which would be a 6 KB (gzipped) overhead, however this may be an overkill for an attribution string. We could instead try designing a simple whitelist-based sanitizer if we can come up with a good enough spec of what is allowed. Then we would use something similar to this code.

[trollepierre]

Hello, I'm having this issue (same than @mike-marcacci with Lighthouse #7914 ) and I would like to know what is needed to fix this bug 🙏 ?
Thanks 😄