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.

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

Support build avatars sourced from non-GitHub URLs #1441 #1942

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sagar-pathak
Copy link

@sagar-pathak sagar-pathak commented Feb 17, 2025

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

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants