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

Make dotnet package verification env var value comparison case insensitive #11876

Closed
erdembayar opened this issue Jun 13, 2022 · 2 comments · Fixed by NuGet/NuGet.Client#4688
Closed

Comments

@erdembayar
Copy link
Contributor

NuGet Product Used

dotnet.exe

Product Version

n/a

Worked before?

No response

Impact

No response

Repro Steps & Context

You are comparing the env var and bool true value, while only converting the latter to uppercase.

Console.WriteLine(IsEnabled(null));
Console.WriteLine(IsEnabled("true"));
Console.WriteLine(IsEnabled("TRUE"));

static object IsEnabled(string signVerifyEnvVariable)
{
    if (!string.IsNullOrEmpty(signVerifyEnvVariable))
    {
        if (signVerifyEnvVariable.Equals(bool.TrueString.ToUpperInvariant(), StringComparison.Ordinal))
        {
            return true;
        }

        // other values are unsupported
        return false;
    }
    return false;
}

This would print:

False
False
True

which is incorrect and should be:

False
True
True

instead.

Verbose Logs

No response

@zivkan
Copy link
Member

zivkan commented Jun 13, 2022

Why not use StringComparison.OrdinalIgnoreCase?

ToUpperInvarient() will definitely cause a memory allocation, whereas passing the strings we already have into a different string comparer might not.

@zivkan
Copy link
Member

zivkan commented Jun 22, 2022

@erdembayar the title of this issue does not match the implementation of the PR that closed it.

@erdembayar erdembayar changed the title Package Signing Verification Env variable value should be converted to upper case before for comparison Make dotnet package verification env var value comparison case insensitive Jun 22, 2022
@kartheekp-ms kartheekp-ms added this to the 6.3 milestone Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants