Support build avatars sourced from non-GitHub URLs #1441

[sagar-pathak]

Description of proposed changes

Users will now be able to define a path to a custom avatar that is not hosted on GitHub through the config file.

Related issue(s)

Related Issue: #1441

• Checks pass
• If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR
• (to be done by a Nextstrain team member) Create preview PRs on downstream repositories.

[jameshadfield]

The changes look ok to me and work well on the datasets I've manually adjusted.

I think React's escaping of strings is enough to prevent XSS but would appreciate 👀 from @joverlee521 and @genehack on this. And also whether to allow base64 encoded images etc, however I think we already allow these in the markdown footer, right?

[genehack]

I think React's escaping of strings is enough to prevent XSS

From what I understand, with the way this is done (setting the src prop value via interpolation), React covers the XSS angle here. (To be clear, I did not try to inject something malicious, like a Base64-encoded hunk of JS, but I don't think that will work in an <img> element the way it would with a <a href.

And also whether to allow base64 encoded images etc, however I think we already allow these in the markdown footer, right?

I think supporting Base64-encoded images would be good; we may need to indicate something about how to do this in some docs somewhere, if we don't already?

I did leave one tweak suggestion on the PR that I think should be added, as a belt-and-suspenders check that somebody didn't stick an array or an object in the JSON instead of the expected string (the same way we check the buildUrl before the regex parsing is attempted).

[sagar-pathak]

I added the check for the string and tested it with a base64 image, which works. However, I'm unsure how we will add validation for base64 images since they will be received as a string. @genehack @jameshadfield

[sagar-pathak]

Any updates? @joverlee521 @genehack

[tsibley]

This will allow visitor tracking by third-parties. Do we intend to allow that?

[sagar-pathak]

This will allow visitor tracking by third-parties. Do we intend to allow that?

Hi @tsibley, thank you for your response. Would you mind explaining more on this? Do you mean by injecting code through the build_avatar third-parties can track visitors?

[jameshadfield]

This will allow visitor tracking by third-parties.

More so than allowing outbound src images in markdown footers etc?

[tsibley]

More so than allowing outbound src images in markdown footers etc?

Nope!

Do you mean by injecting code through the build_avatar third-parties can track visitors?

Not code, HTTP requests.

[sagar-pathak]

Not code, HTTP requests.

Wouldn't it be the same in the URL field for maintainers? Sorry, I didn't quite understand. Could you please help me clarify?

[corneliusroemer]

@sagar-pathak The URL isn't loaded by default, it requires a click IIUC.

The avatar would always be loaded.

Do we have precedent for loading such resources? If so, this PR might not worsen things. However if this would be a first it would be significant.

It might even require adding a consent notice in the EU to allow the request (if it uses for tracking purposes - but we don't know the purpose so we'd sort of have to err on the side of caution)

@sagar-pathak I appreciate you suggesting this change, could you explain your use case? Maybe there are other, more privacy preserving ways to achieve the same goal.

Update: I missed that this was an issue in our issue tracker made by @huddlej

[sagar-pathak]

@corneliusroemer Thanks, that makes perfect sense. I will check what best practices are there to handle such situations.

[jameshadfield]

Thanks @sagar-pathak - much appreciated!

[cornelius] Do we have precedent for loading such resources?

Yes, we already allow (and load) images with outbound src in markdown footers & narratives.