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

✨ Support Nuget Central Package Management #4369

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 113 additions & 29 deletions checks/raw/pinned_dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import (
"github.com/ossf/scorecard/v5/checks/fileparser"
sce "github.com/ossf/scorecard/v5/errors"
"github.com/ossf/scorecard/v5/finding"
"github.com/ossf/scorecard/v5/internal/csproj"
"github.com/ossf/scorecard/v5/internal/dotnet/csproj"
"github.com/ossf/scorecard/v5/internal/dotnet/properties"
"github.com/ossf/scorecard/v5/remediation"
)

Expand All @@ -38,6 +39,11 @@ type dotnetCsprojLockedData struct {
LockedModeSet bool
}

type NugetPostProcessData struct {
CsprojConfigs []dotnetCsprojLockedData
CpmConfig properties.CentralPackageManagementConfig
Comment on lines 41 to +44
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not export this type

type nugetPostProcessData struct {

}

// PinningDependencies checks for (un)pinned dependencies.
func PinningDependencies(c *checker.CheckRequest) (checker.PinningDependenciesData, error) {
var results checker.PinningDependenciesData
Expand Down Expand Up @@ -67,14 +73,87 @@ func PinningDependencies(c *checker.CheckRequest) (checker.PinningDependenciesDa
return checker.PinningDependenciesData{}, err
}

if unpinnedNugetDependencies := getUnpinnedNugetDependencies(&results); len(unpinnedNugetDependencies) > 0 {
if err := processCsprojLockedMode(c, unpinnedNugetDependencies); err != nil {
return checker.PinningDependenciesData{}, err
}
// Nuget Post Processing
if err := postProcessNugetDependencies(c, &results); err != nil {
return checker.PinningDependenciesData{}, err
}

return results, nil
}

func postProcessNugetDependencies(c *checker.CheckRequest,
pinningDependenciesData *checker.PinningDependenciesData,
) error {
if unpinnedDependencies := getUnpinnedNugetDependencies(pinningDependenciesData); len(unpinnedDependencies) > 0 {
var nugetPostProcessData NugetPostProcessData
if err := retrieveNugetCentralPackageManagement(c, &nugetPostProcessData); err != nil {
return err
}
if err := retrieveCsprojConfig(c, &nugetPostProcessData); err != nil {
return err
}
if nugetPostProcessData.CpmConfig.IsCPMEnabled {
collectPostProcessNugetCPMDependencies(unpinnedDependencies, &nugetPostProcessData)
} else {
collectPostProcessNugetCsprojDependencies(unpinnedDependencies, &nugetPostProcessData)
}
}
return nil
Comment on lines +87 to +101
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: in go it's common to return early to reduce nesting: https://danp.net/posts/reducing-go-nesting/

so you could do something like this if you want (but I won't block on it):

unpinnedDependencies := getUnpinnedNugetDependencies(pinningDependenciesData)
if len(unpinnedDependencies) == 0 {
	return nil
}

// the rest of the function

}

func collectPostProcessNugetCPMDependencies(unpinnedNugetDependencies []*checker.Dependency,
postProcessingData *NugetPostProcessData,
) {
packageVersions := postProcessingData.CpmConfig.PackageVersions

numFixedVersions, unfixedVersions := countFixedVersions(packageVersions)
// if all dependencies are fixed to specific versions, pin all dependencies
if numFixedVersions == len(packageVersions) {
Comment on lines +109 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the variable is called numFixedVersions but it should really be numUnfixedVersions ? this logic needs fixed

pinAllNugetDependencies(unpinnedNugetDependencies)
Comment on lines +104 to +112
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is CPM all-or-nothing for managing (direct) dependencies, or can you manage only a subset centrally? For example, can you pin foo in a Directory.Packages.props and then have a project with packages foo (with no version because CPM) and bar (with a version range)? As written, this PR may give this "pinned" status even though bar can float.

I want to make sure I understand the feature. CPM is just a configuration feature so you don't need to specify versions in more than one place? The reason I'm asking is to see if any of the rules (pinned to specific version) should also apply to all project files?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CPM is all or nothing for an individual project. By enabling it in Directory.Packages.props its on for all projects but individual projects can still opt-out of it. Once enabled, you'll receive an error if any PackageReference declares a version or if any PackageReference has no corresponding PackageVersion item.

} else {
// if some or all dependencies are not fixed to specific versions, update the remediation
for i := range unpinnedNugetDependencies {
(unpinnedNugetDependencies)[i].Remediation.Text = (unpinnedNugetDependencies)[i].Remediation.Text +
": some of dependency versions are not fixes to specific versions: " + unfixedVersions
}
}
}

func retrieveNugetCentralPackageManagement(c *checker.CheckRequest, nugetPostProcessData *NugetPostProcessData) error {
if err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
Pattern: "Directory.*.props",
Comment on lines +123 to +124
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Jeff's comment matter here? If you have the pattern broad for the purposes of the test harness, I can help with that.

I'm not sure if it matters in this context, but be aware that the official name for the CPM file is Directory.Packages.props and on case sensitive file systems, it won't get imported if the case of the file name is incorrect.

CaseSensitive: false,
}, processDirectoryPropsFile, nugetPostProcessData, c.Dlogger); err != nil {
return err
}

return nil
}

func processDirectoryPropsFile(path string, content []byte, args ...interface{}) (bool, error) {
pdata, ok := args[0].(*NugetPostProcessData)
if !ok {
// panic if it is not correct type
panic(fmt.Sprintf("expected type NugetPostProcessData, got %v", reflect.TypeOf(args[0])))
}

cpmConfig, err := properties.GetCentralPackageManagementConfig(path, content)
if err != nil {
dl, ok := args[1].(checker.DetailLogger)
if !ok {
// panic if it is not correct type
panic(fmt.Sprintf("expected type checker.DetailLogger, got %v", reflect.TypeOf(args[1])))
}

dl.Warn(&checker.LogMessage{
Text: fmt.Sprintf("malformed properties file: %v", err),
})
return true, nil
}
pdata.CpmConfig = cpmConfig
return false, nil
}

func getUnpinnedNugetDependencies(pinningDependenciesData *checker.PinningDependenciesData) []*checker.Dependency {
var unpinnedNugetDependencies []*checker.Dependency
nugetDependencies := getDependenciesByType(pinningDependenciesData, checker.DependencyUseTypeNugetCommand)
Expand All @@ -98,30 +177,25 @@ func getDependenciesByType(p *checker.PinningDependenciesData,
return deps
}

func processCsprojLockedMode(c *checker.CheckRequest, dependencies []*checker.Dependency) error {
csprojDeps, err := collectCsprojLockedModeData(c)
if err != nil {
return err
}
unlockedCsprojDeps, unlockedPath := countUnlocked(csprojDeps)

// none of the csproject files set RestoreLockedMode. Keep the same status of the nuget dependencies
if unlockedCsprojDeps == len(csprojDeps) {
return nil
}

// all csproj files set RestoreLockedMode, update the dependency pinning status of all nuget dependencies to pinned
if unlockedCsprojDeps == 0 {
pinAllNugetDependencies(dependencies)
} else {
func collectPostProcessNugetCsprojDependencies(unpinnedNugetDependencies []*checker.Dependency,
postProcessingData *NugetPostProcessData,
) {
unlockedCsprojDeps, unlockedPath := countUnlocked(postProcessingData.CsprojConfigs)
switch unlockedCsprojDeps {
case len(postProcessingData.CsprojConfigs):
// none of the csproject files set RestoreLockedMode. Keep the same status of the nuget dependencies
return
case 0:
// all csproj files set RestoreLockedMode, update the dependency pinning status of all nuget dependencies to pinned
pinAllNugetDependencies(unpinnedNugetDependencies)
default:
// only some csproj files are locked, keep the same status of the nuget dependencies but create a remediation
for i := range dependencies {
(dependencies)[i].Remediation.Text = (dependencies)[i].Remediation.Text +
for i := range unpinnedNugetDependencies {
(unpinnedNugetDependencies)[i].Remediation.Text = (unpinnedNugetDependencies)[i].Remediation.Text +
": some of your csproj files set the RestoreLockedMode property to true, " +
"while other do not set it: " + unlockedPath
}
}
return nil
}

func pinAllNugetDependencies(dependencies []*checker.Dependency) {
Expand All @@ -133,16 +207,15 @@ func pinAllNugetDependencies(dependencies []*checker.Dependency) {
}
}

func collectCsprojLockedModeData(c *checker.CheckRequest) ([]dotnetCsprojLockedData, error) {
var csprojDeps []dotnetCsprojLockedData
func retrieveCsprojConfig(c *checker.CheckRequest, nugetPostProcessData *NugetPostProcessData) error {
if err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
Pattern: "*.csproj",
CaseSensitive: false,
}, analyseCsprojLockedMode, &csprojDeps, c.Dlogger); err != nil {
return nil, err
}, analyseCsprojLockedMode, &nugetPostProcessData.CsprojConfigs, c.Dlogger); err != nil {
return err
}

return csprojDeps, nil
return nil
}

func analyseCsprojLockedMode(path string, content []byte, args ...interface{}) (bool, error) {
Expand Down Expand Up @@ -186,6 +259,17 @@ func countUnlocked(csprojFiles []dotnetCsprojLockedData) (int, string) {
return len(unlockedPaths), strings.Join(unlockedPaths, ", ")
}

func countFixedVersions(packages []properties.NugetPackage) (int, string) {
var unfixedVersions []string
Comment on lines +262 to +263
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this counts unfixed versions


for i := range packages {
if !packages[i].IsFixed {
unfixedVersions = append(unfixedVersions, packages[i].Version)
}
}
return len(unfixedVersions), strings.Join(unfixedVersions, ", ")
}

func dataAsPinnedDependenciesPointer(data interface{}) *checker.PinningDependenciesData {
pdata, ok := data.(*checker.PinningDependenciesData)
if !ok {
Expand Down
Loading
Loading