Support logoSize=auto for custom logos #11093
Conversation
|
You're not modifying any services - its all core - so there’s no need to run any of the service tests here. Before talking about the code (there are a few things that jump out to me, but I don't think its useful to start commenting on the diff at this stage), the first thing I want to get into is: Looking back over #9191 the reason why we didn't apply this to custom logos was due to the performance considerations necessary for dealing with raster images. Looking over https://www.npmjs.com/package/image-size one of the bullet points is "Minimal memory footprint - reads only image headers" @LitoMore do you have any thoughts on this as a general approach? (ignoring the code in this PR itself for now). Was this a library you considered? |
|
We have been attacked before with endpoint badges. Using |
|
Thanks for your comments — @chris48s and @LitoMore . I'm happy to close this if you're worried about security issues.
Is there an option to support this for custom SVG images only perhaps (without using |
|
Thanks. Security is a great thing to consider here. That said, I actually don't think that issue is a reason to discount image-size. Sometimes software has security issues. It happens to the best of us. It is unrealistic to think that every package we pull in has had zero security issues ever. What I really want to know in terms of security is: If there are security issues, will they be handled well upstream? Looking at image-size: The project has a healthy release cadence and they handled this issue by publishing a security advisory, fixing the issue and also backporting that fix back to the previous major version. So I'm not actually super worried by that. |
|
Then I think this should be fine, since the base64 image will not be too large, given the URL length limit. |
|
@paulbrimicombe I'll try and get back to you with some proper feedback on the code. I've had a really quick glance but I need to spend a bit more time on this in order to give you (as a first-time contributor) a proper steer on where next. I have quite a bit going on right now so I am probably going to struggle to find that time over the next week or so. If I haven't replied to this thread in a week can you comment so I get a ping in my notifications. Ta |
|
🚀 Updated review app: https://pr-11093-badges-shields.fly.dev |
| if (logoSize === 'auto') { | ||
| const [, base64Image] = overrideLogoSvgBase64.split(';base64,') | ||
| const dimensions = base64Image | ||
| ? imageSize(Buffer.from(base64Image, 'base64')) |
There was a problem hiding this comment.
Feels like there is a risk imageSize could throw here on spurious inputs and I don't want that to be an unhandled exception. There's no guarantee what the user has entered here is intelligible.
I think we should wrap the bit where we attempt to decode the base64 and parse the image in a try/catch and either
- re-throw any decode/parse exception as an
InvalidParameteror - just ignore it and don't attempt to apply a custom
logoWidth(but attempt to render the badge with whatever input we've been given)
| logoWidth = | ||
| (dimensions.width / dimensions.height) * DEFAULT_LOGO_HEIGHT |
There was a problem hiding this comment.
The calculation result is possibly Infinity or NaN. We might need to validate the value before applying it.
Addresses #11092
I've added support for
logoSize=autofor custom logos. This involves adding theimage-widthpackage as a dependency. I've added some unit tests but if you'd like more testing, do please indicate what you'd like me to add.Can someone give me some guidance as to which tests I should be running for this change (i.e. what to put in the
[]in the PR title)?