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

Case-folding of "ProductCode" field #2558

Open
denelon opened this issue Sep 29, 2022 · 3 comments
Open

Case-folding of "ProductCode" field #2558

denelon opened this issue Sep 29, 2022 · 3 comments
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Milestone

Comments

@denelon
Copy link
Contributor

denelon commented Sep 29, 2022

Brief description of your issue

The client calls a REST source and passes in the productCode in lowercase. It's doing "Exact" matching when it probably should be doing "caseInsesnsitive" as referenced in:

Steps to reproduce

Add a manifest with a product code containing an upper case productCode to the REST source.
Attempt to get a matching result with the client.

Expected behavior

A match is returned

Actual behavior

No match is returned.

Environment

winget --info                                                                   in pwsh at 10:54:42
Windows Package Manager (Preview) v1.4.2161-preview
Copyright (c) Microsoft Corporation. All rights reserved.

Windows: Windows.Desktop v10.0.25212.1000
System Architecture: X64
Package: Microsoft.DesktopAppInstaller v1.19.2161.0

Logs: %LOCALAPPDATA%\Packages\Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\LocalState\DiagOutputDir

Links
---------------------------------------------------------------------------
Privacy Statement   https://aka.ms/winget-privacy
License Agreement   https://aka.ms/winget-license
Third Party Notices https://aka.ms/winget-3rdPartyNotice
Homepage            https://aka.ms/winget
Windows Store Terms https://www.microsoft.com/en-us/storedocs/terms-of-sale

Group Policy                               State
---------------------------------------------------
Enable Windows App Installer Hash Override Disabled
@denelon denelon added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 29, 2022
@ghost ghost added the Needs-Triage Issue need to be triaged label Sep 29, 2022
@denelon denelon removed the Needs-Triage Issue need to be triaged label Sep 29, 2022
@jantari
Copy link

jantari commented Oct 5, 2022

I can't read C++ but in the spirit of Hacktoberfest I tried to look into this. I've found what I think are two possible sources of this problem, depending on what is called by what when:

First, this comment and code in WorkflowBase.cpp seems to imply ProductCodes are intentionally always searched "Exact":

// Regardless of match type, always use an exact match for the system reference strings.
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::PackageFamilyName, MatchType::Exact, query));
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::ProductCode, MatchType::Exact, query));
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::Id, matchType, query));
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::Name, matchType, query));
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::Moniker, matchType, query));

and then further down the same file we have some more interesting logic that always searches for ProductCodes with MatchType "Exact", but does make an exception for portable packages specifically and treats those correctly with CaseInsensitive:

else if (!installer.ProductCode.empty())
{
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::ProductCode, MatchType::Exact, installer.ProductCode));
}
else if (installer.EffectiveInstallerType() == Manifest::InstallerTypeEnum::Portable)
{
const auto& productCode = Utility::MakeSuitablePathPart(manifest.Id + "_" + source.GetIdentifier());
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::ProductCode, MatchType::CaseInsensitive, Utility::Normalize(productCode)));
}

Then there's also some sussy stuff in ARPCorrelation.cpp, but to me it reads like these searches are only comparing the current ARP state to a cached/prior ARP state so maybe this code doesn't actually interact with a remote source:

for (const auto& installer : manifest.Installers)
{
if (!installer.ProductCode.empty())
{
// Add each ProductCode only once
if (productCodes.insert(installer.ProductCode).second)
{
manifestSearchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::ProductCode, MatchType::Exact, installer.ProductCode));
}
}
for (const auto& appsAndFeaturesEntry : installer.AppsAndFeaturesEntries)
{
if (!appsAndFeaturesEntry.DisplayName.empty())
{
manifestSearchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::NormalizedNameAndPublisher, MatchType::Exact,
appsAndFeaturesEntry.DisplayName,
appsAndFeaturesEntry.Publisher.empty() ? defaultPublisher : appsAndFeaturesEntry.Publisher));
}
// Add each ProductCode and UpgradeCode only once;
if (!appsAndFeaturesEntry.ProductCode.empty() && upgradeCodes.insert(appsAndFeaturesEntry.ProductCode).second)
{
manifestSearchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::ProductCode, MatchType::Exact, appsAndFeaturesEntry.ProductCode));
}
if (!appsAndFeaturesEntry.UpgradeCode.empty() && upgradeCodes.insert(appsAndFeaturesEntry.UpgradeCode).second)
{
manifestSearchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::UpgradeCode, MatchType::Exact, appsAndFeaturesEntry.UpgradeCode));
}
}
}

If the issue really is in WorkflowBase.cpp where I pointed to I could open a PR, but my C++ skills won't go much beyond replacing MatchType::Exact with MatchType::CaseInsensitive so if there's a lot of other impacted or related code, or special exceptions would have to be made I couldn't do that.

@denelon
Copy link
Contributor Author

denelon commented Oct 5, 2022

@ryfu-msft the origin of this came from winget-cli-restsource. Does this look like it's sufficient, or would more work be involved (test cases impacted)?

@denelon denelon added this to the v1.5-Client milestone Feb 14, 2023
@denelon denelon modified the milestones: v1.5-Client, v.Next-Client Apr 18, 2023
@jantari
Copy link

jantari commented Feb 18, 2024

Is there an update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

No branches or pull requests

2 participants