badges
Support logoSize=auto for custom logos
Addresses #11092
I've added support for logoSize=auto for custom logos. This involves adding the image-width package 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.
PR title conventions - https://github.com/badges/shields/blob/master/CONTRIBUTING.md#running-service-tests-in-pull-requests
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)?
| Messages | |
|---|---|
| :book: | :sparkles: Thanks for your contribution to Shields, @paulbrimicombe! |
Generated by :no_entry_sign: dangerJS against 6c9f94907b79c8a65bf3baf8946db1829190ef81
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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)?
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 https://github.com/badges/shields/pull/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 image-size to calculate image sizes on our server may introduce potential security issues. Here is a security issue they had before: https://github.com/image-size/image-size/security/advisories/GHSA-m5qc-5hw7-8vg7.
Thanks for your comments — @chris48s and @LitoMore . I'm happy to close this if you're worried about security issues.
Looking back over https://github.com/badges/shields/pull/9191 the reason why we didn't apply this to custom logos was due to the performance considerations necessary for dealing with raster images.
Is there an option to support this for custom SVG images only perhaps (without using image-size)? If that's acceptable, let me know.
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