search

microsoft / winget-pkgs

The Microsoft community Windows Package Manager manifest repository
MIT License
8.75k stars 4.56k forks source link

Moderation needed. #14621

Closed KaranKad closed 3 years ago

KaranKad commented 3 years ago

Describe issue

People are submitting bad or duplicate manifests without checking if the app already exists or not in this repository. Some examples Duplicate pr's - #14546 #14606 #14498 #14605 #14610 Bad pr's - #14393 #14362 #14389 #14551 #14523 Overwriting existing manifests with half of the information - #14544 #14382 Luckily not every pr is merged.

Solution

Create a group of active contributors who know what they are doing, with ability to close a pr so they can prevent bad or duplicate pr's from getting in.

OfficialEsco commented 3 years ago
  1. voidtools.everything #14367 broke the repo because it got added under the same name but small K in the foldername...
  2. voidtools.Everything #14381 got overwritten and deleted one installer.
  3. 7zip.7zip #14408 changed the InstallerType to exe which would make the validation fail but it didn't.
  4. VSRevoGroup.RevoUninstaller #14428 duplicate
  5. MarcinSzeniak.BCUninstaller #14437 duplicate
  6. PointPlanck.FileBot #14283 duplicate, but FileBot.FileBot only had one manifest so i deleted that one instead
  7. ZoomVideoCommunications,Inc.ZoomMeetingsInstaller #14522 duplicate and bad PackageIdentifier
  8. TimKosse.Filezilla #14486 duplicate
  9. Cloudflare,Inc.CloudflareWARP #14587 duplicate and bad PackageIdentifier
  10. PowerSoftwareLtd.PowerISO #14582 duplicate
  11. TonecInc.InternetDownloadManagerinstaller #14495 bad PackageVersion
  12. obsproject.com.OBSStudio #14623 duplicate
  13. Unity #14571 duplicate and bad PackageIdentifier
  14. GoogleLLC.GoogleBackupandSync #14555 duplicate
  15. Google.GoogleEarth #14422 duplicate
  16. DuongDieuPhap.ImageGlass #14626 duplicate/overwritten
  17. AkeoConsulting.Rufus #14406 duplicate
  18. Foundry376,LLC.Mailspring #14405 duplicate
  19. OpenMediaLLC.4KVideoDownloader #14652 duplicate
  20. Speccy.PiriformSoftwareLtd.Speccy #14671 and #15041 duplicate
  21. Revert "Manifest Update fore Piriform.Speccy v1.32" #14682 and #15037 reverting correct manifest to incorrect information
  22. Guilded,Inc.Guilded #14672 bad PackageIdentifier
  23. Surfshark.Surfshark #14679 duplicate
  24. DuongDieuPhap.ImageGlass #14746 duplicate
  25. SaladTechnologies.Salad #14749 duplicate
  26. uvncbvba.UltraVnc #14699 wrong publisher (moved UltraVNC.UltraVNC to this)
KaranKad commented 3 years ago
  1. (BraveSoftwareInc.BraveSoftwareUpdate) #14691 duplicate with bad PackageIdentifier
  2. Funkin'Team.FridayNightFunkin' #14716 bad PackageIdentifier
  3. DolphinEmu.DolphinEmulatorProject #14741 duplicate
  4. DuongDieuPhap.ImageGlass #14746 duplicate
  5. ppy.osu #14747 duplicate
  6. lunar_client.lunarclient #14759 bad PackageIdentifier
  7. NitroPDFIncNitroPDFPtyLtd.PrimoPDF #14763 bad PackageIdentifier and download url
  8. Moonsworth,LLC.LunarClient #14767 bad PackageIdentifier
  9. DonHOdon.h@free.fr.Notepad++ #14977 duplicate with bad PackageIdentifier
  10. Microsoft.VisualStudio #14970 just a really bad pr
  11. CPUID,Inc.CPUIDCPU-Z #14979 duplicate with bad PackageIdentifier
  12. kite.kite #14986 #14620 submitting same pr twice.
  13. CyotekLtd.CyotekWebCopy #14991 duplicate Publisher
  14. CPUID,Inc.CPUIDpowerMAX #14989 bad PackageIdentifier (Fixed)
  15. CyotekLtd.CyotekWebCopy #14993 #14991 submitting same pr twice. (Fixed)
  16. Irium.Webcam #15000 duplicate. (deleted) (Fixed)
  17. AprelTech,LLC.SilentInstallBuilder #14992 bad PackageIdentifier (Fixed)
  18. Process.explorer #15005 #15006 submitting same pr twice.
  19. GoogleLLC.GoogleUpdate #15007 duplicate with bad PackageIdentifier (Fixed)
  20. Mumble.Mumble134 #15011 bad PackageIdentifier
  21. ScratchFoundation.Scratch3 #15014 bad duplicate (Fixed)
  22. U.Freiberger.RobotKarol30 #15030 bad PackageIdentifier
  23. FamatechCorp.AdvancedIPScanner #15028 bad PackageIdentifier
  24. ParagonSoftware.APFS #15055 duplicate Publisher (Fixed)
  25. SonyInteractiveEntertainmentInc.PSRemotePlay #15057 bad PackageIdentifier (Fixed by #15093)
  26. fi.skyjake.Lagrange #15058 bad PackageIdentifier (Fixed)
  27. AppleInc.iCloud #15046 duplicate publisher
  28. TheIridiumAuthors.IridiumBrowser #15097 #15096 submitting same pr twice. (Fixed)
  29. BitSum.ProcessLasso.Beta #15119 bad PackageIdentifier
  30. BitsumLLC.ProcessLasso #15120 duplicate Publisher
  31. BraveSoftwareInc.BraveSoftwareUpdate #15126 duplicate
  32. KakaoCorp.KakaoTalk #15157 bad PackageIdentifier
  33. Kakao.PotPlayer #15167 duplicate (Fixed)
  34. RaspberryPi.RaspberryPiImager #15184 duplicate publisher (Fixed)
  35. Pylo.MCreator.Installer #15173 bad PackageIdentifier
  36. OracleCorporation.OracleVMVirtualBox #15203 duplicate (Fixed)
  37. m2070.samsung #15209 #15210 submitting same pr twice.
  38. DiscordInc.Discord #15228 duplicate (Fixed)
  39. Discord.Discord #15222 Overwriting existing pr (Fixed)
jedieaston commented 3 years ago

I really really think that any new PackageIdentifer should have to be checked by someone on the winget team (or if they want to start a recognized contributor system Iā€™d throw my hat in the ring). Fully automated adding of brand new packages will have tons of duplicates.

Touchcreator commented 3 years ago

What can I do to make my PackageIdentifier better?

KaranKad commented 3 years ago

What can I do to make my PackageIdentifier better?

Oh! my bad, I checked the download url https://github.com/CreativeDo/ZXPInstaller and saw that you submitted the PakageIdentifier as guideguide.ZXPInstaller, I thought it should have been CreativeDo.ZXPInstaller but now I checked their actual GitHub page.

Touchcreator commented 3 years ago

What can I do to make my PackageIdentifier better?

Oh! my bad, I checked the download url https://github.com/CreativeDo/ZXPInstaller and saw that you submitted the PakageIdentifier as guideguide.ZXPInstaller, I thought it should have been CreativeDo.ZXPInstaller but now I checked their actual GitHub page.

oh ok

denelon commented 3 years ago

I'll talk with the team tomorrow when everyone is back from holiday. One of the options could be requiring a "second" approver on a "new" manifest in a "new" directory. The bot has a concept that might work for that scenario. I just don't want to put too much friction and time delay for people submitting manifests, nor too much pressure on "moderators".

We've got a feature on the backlog to detect duplicates. It's more of a warning than a blocking action. We have some expected "valid" rename scenarios.

I also put something similar on the wingetcreate backlog too.

denelon commented 3 years ago

Would one of you like to take a stab at "How to submit a manifest" as an Issue? I'll pin it if it's got decent directions on finding the right installer type, testing, etc...

OfficialEsco commented 3 years ago

One of the options could be requiring a "second" approver on a "new" manifest in a "new" directory. The bot has a concept that might work for that scenario. I just don't want to put too much friction and time delay for people submitting manifests, nor too much pressure on "moderators".

If possible, people with XX amount of commits should skip the second approval since they most likely already learned how the repo/packages works.

We've got a feature on the backlog to detect duplicates. It's more of a warning than a blocking action. We have some expected "valid" rename scenarios.

I also put something similar on the wingetcreate backlog too.

Would one of you like to take a stab at "How to submit a manifest" as an Issue? I'll pin it if it's got decent directions on finding the right installer type, testing, etc...

People use wingetcreate which skips ALL of the pre checks, they don't even look at the PR comments. I don't even think they do winget search to check if their program is already added..

jedieaston commented 3 years ago

Maybe wingetcreate manifests should require manual review? I know we are trying not to waste the moderator's time, but since they are committing known bad metadata by default (for example, not updating the ProductCode attribute), the bot doesn't realize it and then someone who knows that the bug exists has to go back and fix all of the errors (or live with the metadata being wrong, which is a tragedy ;D).

ocdtrekkie commented 3 years ago

@denelon I am kinda appalled we are where we are right now, and Winget was announced as being GA/1.0 when core issues like this on the trust and safety side haven't been addressed.

I am absolutely baffled how this repo doesn't link to anything about what release channels or versions should be selected in the package manager, or any sort of common behavior about how some choices should be made. There's nothing telling me why I should believe I can trust anything the automated pipeline has accepted into the repo, or that the downloads are in any way safe. Right now, using WinGet would be at best no safer than randomly grabbing installer files off popup ads and running them.

I am not sure anything can fix this repo short of a "nuke it from orbit and start over" approach, but nobody security-side at Microsoft seems to have noticed this repo is here? I found a policy document on the Docs site, but it looks more like legal boilerplate, and lacks any cohesive answers on questions which are crucial to package managers.

But in the most severe case, in light of these glaring and painful issues, it's concerning your primary focus is preventing delay in approvals. Shut the bots down and start the manual review process, and roll this back to beta until there's clear documentation about how packages should be added or not.

OfficialEsco commented 3 years ago

@ocdtrekkie every PR up until #12424 (22 days ago) have been manually/semi manually approved, and every url is being run trough VirusTotal so there is some sort of protection in place.

And i do very much agree that Microsoft have not thought very much about this since one idiot could ruin the whole thing with a fully automated system, every Package Manager have some kind of Manual Approval/Maintainer of that Program.

ocdtrekkie commented 3 years ago

What would happen if I sent a PR that caused the Chrome package to install Firefox instead? Would any checks prohibit it from being automatically merged? There's a very wide variety of things you can do without pushing an executable that will flag on VirusTotal.

But there's a lot of issues here Microsoft doesn't even seem to be prepared for. Even if you knew for sure every manifest was legitimate and correct, use of WinGet would still lead the Windows platform to a worse security posture than it has now: Without any ownership by either Microsoft or official app developer channels, WinGet package manifests may or may not be updated in a timely manner, if at all, and without any practices or policy about architectures, release channels, deployment configurations, etc. users may be getting 32-bit versions on their 64-bit machine when 64-bit versions exist, or be stuck on very old versions, or get broken releases instead of stable ones.

Third party package managers like PDQ generally are going to maintain a restricted list of packages they maintain, because they're taking responsibility for ensuring they're pushing the latest, working updates of a given package they support. That approach would be possible for Microsoft, but it would need to be much more restrictive, and manually reviewed.

The other alternative is one Microsoft has tried before, it's the Windows Store: Only support software released to the repository by the developer of said package. Obviously it's a lot less useful if major brands don't participate, but then the responsibility of keeping the deployment working and up to date is on the developers themselves.

Look at Chocolately's moderation document: https://docs.chocolatey.org/en-us/community-repository/moderation/ It covers everything. Release channels. Processor architectures. Licensing questions. Packages are at minimum tied to specific maintainers responsible for them, such that you can trust it hasn't been changed by someone else in the past hour since you looked at the repo. And of course, everything is robustly and manually reviewed by humans before anything is made available to deploy to anyone's computers.

KaranKad commented 3 years ago

I think this repo is too free for anyone to edit/add things the way they like, which should not be the case at all. I would rather have people tell the application they want here, by creating a issue rather than do it themselves (because they do it badly). After V1 announcement it was kind of overwhelming and frustrating here. Soo many duplicate/bad manifests one after other. There is documentation on how to submit a manifest correctly but no one seems to read it.

I think we should stop accepting new manifests ASAP for a while and spend time correcting bad manifests with very little information. Automation has eased process for moderator but with its negative consequences. This can be fixed by

  1. Not allowing anyone (unknown) to add new manifests. Like seriously anyone can edit or delete a manifest right now.
  2. Have moderators. Only moderators should be allowed to make changes to this repo. While it's good that people can add applications they want, it could be also done by them requesting some moderator to add it for them.
  3. Automation system is only good if used properly. Automation system is one of the reason for this issue. But, by only allowing moderators to submit new application (knowing manifests are going to be good), automation can be put at good use and could save someone's time.

Optional I really think winget should have an official site for package viewing similar to chocolatey with chat option, where other people can give direct feedback about particular packages

But currently my main point is that Moderation should be taken seriously. Group of moderators should be created because this is not a one person job and so that winget's repository remains sane. Decision for this issue needs to be made immediately!

@denelon What do you think about it?

ocdtrekkie commented 3 years ago

"So now, when we face a choice between adding features and resolving security issues, we need to choose security." - Bill Gates

https://www.wired.com/2002/01/bill-gates-trustworthy-computing/

As it stands right now, WinGet adds a feature at an exceptional security cost. If Microsoft isn't interested in implementing a review team, I sincerely believe Microsoft should look at it's options to roll back and deprecate WinGet.

denelon commented 3 years ago

The "automated merge" has been stopped.

We're making changes to the wingetcreate tool to detect duplicates, and require "update" vs. "new" for existing packages.

denelon commented 3 years ago

@KaranKad I created a discussion topic for Moderation.

KaranKad commented 3 years ago

Yes! please think about creating a moderation team. I think its the best solution. For manifests to be perfect, only moderators should be allowed submission of new or updating existing manifests. Because people submitting new applications using wingetcreate, don't really do it that well either.

Teraskull commented 3 years ago

Would also be nice to moderate ShortDescription, to detect and avoid stuff like <appname> setup or <appname> installer, like seen here:

https://github.com/microsoft/winget-pkgs/blob/master/manifests/t/TeaSpeak/TeaClient/1.5.3-2/TeaSpeak.TeaClient.locale.en-US.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/v/Vromans/ChordPro/0.977/Vromans.ChordPro.locale.en-US.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/f/FACEITLTD/FACEITAC/2.0/FACEITLTD.FACEITAC.locale.en-US.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/g/Google/ChromeCanary/1.3.36.82/Google.ChromeCanary.locale.en-US.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/i/IObit/ProtectedFolder/4.3.0.51/IObit.ProtectedFolder.locale.en-US.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/DebaucheeOpenSourceGroup/Barrier/2.3.2/DebaucheeOpenSourceGroup.Barrier.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/r/Racket/Racket/8.1/Racket.Racket.locale.en-US.yaml

Or adding the PackageIdentifier in ShortDescription, like here: https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/Debian/Debian/9.0/Debian.Debian.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/b/Bookry/Wavebox/10.0.462.2/Bookry.Wavebox.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/DimitriVanHeesch/Doxygen/1.9.1/DimitriVanHeesch.Doxygen.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/DJI/DJIAssistant2FPV/2.0.2/DJI.DJIAssistant2FPV.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/DJI/DJIAssistant2ForMG/2.0.18/DJI.DJIAssistant2ForMG.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/dnGrep/dnGrep/v2.9.256.0/dnGrep.dnGrep.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/DolphinEmu/DolphinEmu/5.0/DolphinEmu.DolphinEmu.yaml https://github.com/microsoft/winget-pkgs/blob/master/manifests/d/DrewNaylor/guinget/0.1.1/DrewNaylor.guinget.yaml

Or just adding the PackageName in ShortDescription, like here: https://github.com/microsoft/winget-pkgs/blob/master/manifests/p/PowerSoftware/AnyBurn/5.2.0.0/PowerSoftware.AnyBurn.locale.en-US.yaml

denelon commented 3 years ago

@jedieaston since you volunteered, we've added the logic so that when you "approve" a PR by reviewing it, the "Moderator-Approved" label is added, and if the "Azure-Pipeline-Passed" and the "Validation-Completed" labels are present, the PR will be merged. Please test this when you get a chance.

jedieaston commented 3 years ago

I've approved 3 PRs and they've merged successfully, thanks!

Jaifroid commented 3 years ago

Just to say, that as the developer of an app I was about to submit here, I found someone else had an open PR on the app only hours before, with an erroneous link that would have caused validation issues as soon as I push a new version of the app.

Fortunately I was able to get in touch with this person and ask them to close their PR so I could submit one with the proper information. I have to say I was extremely surprised to find that someone conpletely unrelated to my app (albeit well intentioned) was able to fill out all the information, accept the CLA, etc., on my behalf without my knowing anything about it. Winget devs, how didn't you think this through? At the very least there needs to be a check that the person submitting a manifest has the right to accept agreements, etc.

Contrast this with the rigorous checks of organizations on the Microsoft Store -- it took several months for us to get the right documentation to open an org account on the MS Store -- and you see the problem immediately. And what are you going to do about "inconvenient" but legal issues such as age ratings, etc.? IMHO winget should have started with packages already validated and approved in the MS Store, and then opened up more gradually from there with the right checks in place. A pain, yes, but this is Windows, not Linux. Linux has like 3% desktop market share, mostly used by responsible devs, compared to Windows 80%-ish, used by everyone from 8-year-olds to 80-year-olds.

KaranKad commented 3 years ago

@Jaifroid Currently people submit pr's for application they would like to have here. That is unless the developer of the application would like to step in and manage information for their application themselves, which is honestly great. Once this feature at #100 is implemented, respective applications developer will be the only one (along with moderators mabye) who can manage updates and information for their application. And for applications that are not submitted by their developers, moderation team was assigned after this issue was created to ensure that manifest content is correct and is not merged into main branch until a moderator approves it.

Jaifroid commented 3 years ago

@KaranKad Thanks for your detailed reply. #100 is essential -- good to know it's coming.

d3vel0per commented 3 years ago

Maybe wingetcreate manifests should require manual review? I know we are trying not to waste the moderator's time, but since they are committing known bad metadata by default (for example, not updating the ProductCode attribute), the bot doesn't realize it and then someone who knows that the bug exists has to go back and fix all of the errors (or live with the metadata being wrong, which is a tragedy ;D).

Albeit a little unrelated but having submitted a few packages recently. The process of generating ProductCode itself is convoluted and not very well defined at least to my knowledge.

denelon commented 3 years ago

@KaranKad I've created a project to track work for the next "milestone". I've marked this work item "In Progress". I didn't want to close it until the Issues listed above have been corrected. Let me know when you feel like it's safe to close this. I don't believe we have figured out our final solution for moderation, but we do have something in place currently to help. We will continue to refine how moderation should work for the community repository. We will continue by gathering feedback from the community, and creating new Issues as the program needs to adapt.

Thank you for taking the time to report the issue and provide specifics.

lychichem commented 3 years ago

Would one of you like to take a stab at "How to submit a manifest" as an Issue? I'll pin it if it's got decent directions on finding the right installer type, testing, etc...

Sadly, it's useless... In my culture, there is a slang "too long to have look at". This will also fit for all the EULA, issue, template, etc. You cannot imagine everyone will first look through all the documents before they take action, especially at this public repo. There are many people, who know little about how to contribute and respect others' works, trying to write something. We should find a more efficent way, maybe peer review.

ocdtrekkie commented 3 years ago

Would one of you like to take a stab at "How to submit a manifest" as an Issue? I'll pin it if it's got decent directions on finding the right installer type, testing, etc...

Sadly, it's useless... In my culture, there is a slang "too long to have look at". This will also fit for all the EULA, issue, template, etc. You cannot imagine everyone will first look through all the documents before they take action, especially at this public repo. There are many people, who know little about how to contribute and respect others' works, trying to write something. We should find a more efficent way, maybe peer review.

The goal of such a document need not be that everyone exhaustively be expected to read it, but that it's there for both contributors and moderators and serves as the bible to use when determining the answer to controversial decisions.

gumadozucia commented 3 years ago

It would be nice if the moderation policy included a section on submitting non-stable versions of software.

Currently 7zip alpha 21.02 is offered as the upgrade for 7zip 19.00. IMHO this should not be a default, as users are likely to prefer stable versions. What happens if there's another release from the 19.xx branch later? Will winget select it as the default or will it keep the 21.xx alpha branch? Similarly, VLC 3.0.15 is offered as an upgrade for 3.0.14. 3.0.15 has not been released to the wider public yet and VLC's website still serves 3.0.14 in the download section.

OfficialEsco commented 3 years ago

It would be nice if the moderation policy included a section on submitting non-stable versions of software.

Currently 7zip alpha 21.02 is offered as the upgrade for 7zip 19.00. IMHO this should not be a default, as users are likely to prefer stable versions. What happens if there's another release from the 19.xx branch later? Will winget select it as the default or will it keep the 21.xx alpha branch? Similarly, VLC 3.0.15 is offered as an upgrade for 3.0.14. 3.0.15 has not been released to the wider public yet and VLC's website still serves 3.0.14 in the download section.

7-Zip have caused the issue them self by how they made the EXE and MSI, as you can CLEARLY see 7-Zip Alpha is not in the Stable branch, it is under the Alpha Branch. Mainly this is a issue on https://github.com/microsoft/winget-cli or 7-Zip dev which we are trying to work around.

Feel free to try to fix it and make it compatible šŸ¤·ā€ā™‚ļø

VLC is actually published under the public branch http://download.videolan.org/pub/videolan/vlc/ 2021-06-09

gumadozucia commented 3 years ago

Thank you. I appreciate all the hard work you and other moderators have put into this project.

I fully agree that as these issues result from developers' mistakes/inconsistencies, it would be unreasonable to expect the moderation team to do a lot of extra work on every submission.

VLC still for some reason puts 3.0.14 as latest despite having put 3.0.15 in the releases section, so they might be behaving like Firefox in the 3.x era, which put release candidates as releases on ftp and replaced them if any issues were found during that stage - having to confirm such cases for every application would be a nightmare.

denelon commented 3 years ago

@KaranKad is it safe to close this issue now? I just wanted to leave it open long enough for the issues identified way up top to be resolved before closing it. If it's still being worked on, I'm happy to keep it open šŸ˜Š

KaranKad commented 3 years ago

I think it can be closed now.