NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 253 forks source link

Automatically download and embed icon from iconUrl #9344

Open chrisraygill opened 4 years ago

chrisraygill commented 4 years ago

Problem: At some point nuget.org will stop accepting packages with iconUrls in favor of migrating package authors to embedded icons. This will require package authors to modify their current metadata practices.

Solution: Automatically download the image from the iconUrl and embed it into the nupkg at pack time. This will achieve the same benefits of embedded icons (because the icon will be embedded), while making the transition a seamless experience to the package author.

rrelyea commented 4 years ago

@dominoFire @karann-msft - you folks should understand this idea and see if it fits well /c: @agr @aortiz-msft

Nirmal4G commented 4 years ago

This should've been the solution from the begining when Packaging added embedding files at the root for icon, Release notes, License file, etc...

Then again, the problem will be what happens when the URL fails? like 404 - not found!!

Should the pack fail or silently continue with no icon?

agr commented 4 years ago

I have a security concern about that: it enables the SSRF exploitation. Imagine a public GitHub/ADO/whatever setup where PRs require successful build in order to get merged with builds running on repo owner infrastructure and builds produce a NuGet package. Now, someone makes a change making IconUrl pointing to a URL that is not accessible for the outsider, but accessible for the build pipeline and sends PR for that repository. CI pipeline picks it up, builds, packs the package and sends the request the user was not supposed to be able to send.

For that reason the server side process that retrieves external icons runs on an isolated network to prevent any possibility of it accessing resource it is not supposed to access.

loic-sharma commented 3 years ago

@agr That's an excellent point. However, you can already do this attack using MSBuild in the project file. We're not making this existing attack vector better or worse.

ghost commented 2 years ago

Issue is missing Type label, remember to add a Type label