-
Notifications
You must be signed in to change notification settings - Fork 566
✨ feat: Refactor scorecard serve cmd #4665
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
3060b2a
8768959
341ca36
0180be5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,107 +15,316 @@ | |
package cmd | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
"html/template" | ||
stdlog "log" | ||
"net/http" | ||
"os" | ||
"os/signal" | ||
"sort" | ||
"strings" | ||
"syscall" | ||
"time" | ||
|
||
"github.com/spf13/cobra" | ||
|
||
"github.com/ossf/scorecard/v5/clients/githubrepo" | ||
"github.com/ossf/scorecard/v5/clients/ossfuzz" | ||
"github.com/ossf/scorecard/v5/checker" | ||
"github.com/ossf/scorecard/v5/clients" | ||
"github.com/ossf/scorecard/v5/clients/localdir" | ||
pmc "github.com/ossf/scorecard/v5/cmd/internal/packagemanager" | ||
docs "github.com/ossf/scorecard/v5/docs/checks" | ||
"github.com/ossf/scorecard/v5/log" | ||
"github.com/ossf/scorecard/v5/options" | ||
"github.com/ossf/scorecard/v5/pkg/scorecard" | ||
"github.com/ossf/scorecard/v5/policy" | ||
) | ||
|
||
// TODO(cmd): Determine if this should be exported. | ||
type server struct { | ||
logger *log.Logger | ||
opts *options.Options | ||
} | ||
|
||
type scorecardRequest struct { | ||
Repo string `json:"repo"` | ||
Local string `json:"local,omitempty"` | ||
NPM string `json:"npm,omitempty"` | ||
PyPI string `json:"pypi,omitempty"` | ||
RubyGems string `json:"rubygems,omitempty"` | ||
Nuget string `json:"nuget,omitempty"` | ||
Checks []string `json:"checks,omitempty"` | ||
Commit string `json:"commit,omitempty"` | ||
CommitDepth int `json:"commit_depth,omitempty"` | ||
ShowDetails bool `json:"show_details,omitempty"` | ||
Format string `json:"format,omitempty"` | ||
LogLevel string `json:"log_level,omitempty"` | ||
Probes []string `json:"probes,omitempty"` | ||
FileMode string `json:"file_mode,omitempty"` | ||
PolicyFile string `json:"policy_file,omitempty"` | ||
} | ||
|
||
func newServer(logger *log.Logger, opts *options.Options) *server { | ||
return &server{ | ||
logger: logger, | ||
opts: opts, | ||
} | ||
} | ||
|
||
func (s *server) handleScorecard(w http.ResponseWriter, r *http.Request) { | ||
var req scorecardRequest | ||
if r.Method == http.MethodPost { | ||
if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
http.Error(w, "invalid request body", http.StatusBadRequest) | ||
return | ||
} | ||
spencerschrock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
req.Repo = r.URL.Query().Get("repo") | ||
req.Local = r.URL.Query().Get("local") | ||
req.NPM = r.URL.Query().Get("npm") | ||
req.PyPI = r.URL.Query().Get("pypi") | ||
req.RubyGems = r.URL.Query().Get("rubygems") | ||
req.Nuget = r.URL.Query().Get("nuget") | ||
req.Checks = strings.Split(r.URL.Query().Get("checks"), ",") | ||
req.Commit = r.URL.Query().Get("commit") | ||
req.ShowDetails = r.URL.Query().Get("show_details") == "true" | ||
req.Format = r.URL.Query().Get("format") | ||
req.LogLevel = r.URL.Query().Get("log_level") | ||
req.Probes = strings.Split(r.URL.Query().Get("probes"), ",") | ||
req.FileMode = r.URL.Query().Get("file_mode") | ||
req.PolicyFile = r.URL.Query().Get("policy_file") | ||
} | ||
|
||
// Set options | ||
s.opts.Repo = req.Repo | ||
s.opts.Local = req.Local | ||
s.opts.NPM = req.NPM | ||
s.opts.PyPI = req.PyPI | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now we have one we should create a new struct for each request. either through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. |
||
s.opts.RubyGems = req.RubyGems | ||
s.opts.Nuget = req.Nuget | ||
s.opts.Commit = req.Commit | ||
if s.opts.Commit == "" { | ||
s.opts.Commit = clients.HeadSHA | ||
} | ||
s.opts.CommitDepth = req.CommitDepth | ||
s.opts.ShowDetails = req.ShowDetails | ||
s.opts.Format = req.Format | ||
if s.opts.Format == "" { | ||
s.opts.Format = options.FormatDefault | ||
} | ||
s.opts.LogLevel = req.LogLevel | ||
if s.opts.LogLevel == "" { | ||
s.opts.LogLevel = "info" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I have fixed it. |
||
} | ||
s.opts.FileMode = req.FileMode | ||
if s.opts.FileMode == "" { | ||
s.opts.FileMode = options.FileModeArchive | ||
} else if s.opts.FileMode != options.FileModeArchive && s.opts.FileMode != options.FileModeGit { | ||
http.Error(w, fmt.Sprintf("unsupported file mode: %s", s.opts.FileMode), http.StatusBadRequest) | ||
return | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this validation is already covered by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. It has been removed. |
||
s.opts.PolicyFile = req.PolicyFile | ||
s.opts.ChecksToRun = req.Checks | ||
|
||
// Validate options | ||
if err := s.opts.Validate(); err != nil { | ||
http.Error(w, fmt.Sprintf("invalid options: %v", err), http.StatusBadRequest) | ||
return | ||
} | ||
|
||
p := &pmc.PackageManagerClient{} | ||
// Set repo from package managers | ||
pkgResp, err := fetchGitRepositoryFromPackageManagers(s.opts.NPM, s.opts.PyPI, s.opts.RubyGems, s.opts.Nuget, p) | ||
if err != nil { | ||
http.Error(w, fmt.Sprintf("fetchGitRepositoryFromPackageManagers: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
if pkgResp.exists { | ||
s.opts.Repo = pkgResp.associatedRepo | ||
} | ||
|
||
pol, err := policy.ParseFromFile(s.opts.PolicyFile) | ||
if err != nil { | ||
http.Error(w, fmt.Sprintf("readPolicy: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
|
||
ctx := r.Context() | ||
|
||
var repo clients.Repo | ||
if s.opts.Local != "" { | ||
repo, err = localdir.MakeLocalDirRepo(s.opts.Local) | ||
if err != nil { | ||
http.Error(w, fmt.Sprintf("making local dir: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we expect people to pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error also stems from my naive approach of simply copying the logic from |
||
} else { | ||
repo, err = makeRepo(s.opts.Repo) | ||
if err != nil { | ||
http.Error(w, fmt.Sprintf("making remote repo: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
} | ||
|
||
// Read docs | ||
checkDocs, err := docs.Read() | ||
if err != nil { | ||
http.Error(w, fmt.Sprintf("cannot read yaml file: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
|
||
var requiredRequestTypes []checker.RequestType | ||
if s.opts.Local != "" { | ||
requiredRequestTypes = append(requiredRequestTypes, checker.FileBased) | ||
} | ||
if !strings.EqualFold(s.opts.Commit, clients.HeadSHA) { | ||
requiredRequestTypes = append(requiredRequestTypes, checker.CommitBased) | ||
} | ||
|
||
enabledChecks, err := policy.GetEnabled(pol, s.opts.Checks(), requiredRequestTypes) | ||
stdlog.Printf("DEBUG: enabledChecks = %#v", enabledChecks) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be removed? was this for your testing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. Sorry for my carelessness. Done removing. |
||
if err != nil { | ||
http.Error(w, fmt.Sprintf("GetEnabled: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
|
||
checks := make([]string, 0, len(enabledChecks)) | ||
for c := range enabledChecks { | ||
checks = append(checks, c) | ||
} | ||
|
||
enabledProbes := s.opts.Probes() | ||
|
||
opts := []scorecard.Option{ | ||
scorecard.WithLogLevel(log.ParseLevel(s.opts.LogLevel)), | ||
scorecard.WithCommitSHA(s.opts.Commit), | ||
scorecard.WithCommitDepth(s.opts.CommitDepth), | ||
scorecard.WithProbes(enabledProbes), | ||
scorecard.WithChecks(checks), | ||
} | ||
|
||
if strings.EqualFold(s.opts.FileMode, options.FileModeGit) { | ||
opts = append(opts, scorecard.WithFileModeGit()) | ||
} | ||
|
||
repoResult, err := scorecard.Run(ctx, repo, opts...) | ||
if err != nil { | ||
s.logger.Error(err, "scorecard.Run") | ||
http.Error(w, fmt.Sprintf("scorecard.Run: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
|
||
repoResult.Metadata = append(repoResult.Metadata, s.opts.Metadata...) | ||
|
||
// Sort by name | ||
sort.Slice(repoResult.Checks, func(i, j int) bool { | ||
return repoResult.Checks[i].Name < repoResult.Checks[j].Name | ||
}) | ||
|
||
// Return results | ||
w.Header().Set("Content-Type", "application/json") | ||
if err := repoResult.AsJSON2(w, checkDocs, &scorecard.AsJSON2ResultOption{ | ||
LogLevel: log.ParseLevel(s.opts.LogLevel), | ||
Details: s.opts.ShowDetails, | ||
Annotations: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we would probably want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! I've added |
||
}); err != nil { | ||
s.logger.Error(err, "writing JSON response") | ||
http.Error(w, fmt.Sprintf("failed to format results: %v", err), http.StatusInternalServerError) | ||
return | ||
} | ||
} | ||
|
||
// CORS middleware for net/http | ||
func corsMiddleware(next http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Header().Set("Access-Control-Allow-Origin", "*") | ||
w.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS") | ||
w.Header().Set("Access-Control-Allow-Headers", "Accept, Content-Type") | ||
if r.Method == http.MethodOptions { | ||
w.WriteHeader(http.StatusOK) | ||
return | ||
} | ||
next.ServeHTTP(w, r) | ||
}) | ||
} | ||
|
||
// Recover middleware for net/http | ||
func recoverMiddleware(next http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
defer func() { | ||
if err := recover(); err != nil { | ||
stdlog.Printf("panic: %v", err) | ||
http.Error(w, "internal server error", http.StatusInternalServerError) | ||
} | ||
}() | ||
next.ServeHTTP(w, r) | ||
}) | ||
} | ||
|
||
// Logger middleware for net/http | ||
func loggerMiddleware(next http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
start := time.Now() | ||
next.ServeHTTP(w, r) | ||
stdlog.Printf("%s %s %s", r.Method, r.URL, time.Since(start)) | ||
Check failureCode scanning / CodeQL Log entries created from user input High
This log entry depends on a
user-provided value Error loading related location Loading |
||
}) | ||
} | ||
|
||
func serveCmd(o *options.Options) *cobra.Command { | ||
return &cobra.Command{ | ||
Use: "serve", | ||
Short: "Serve the scorecard program over http", | ||
Long: ``, | ||
Run: func(cmd *cobra.Command, args []string) { | ||
Long: `Start an HTTP server to run scorecard checks on repositories with REST API support.`, | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
logger := log.NewLogger(log.ParseLevel(o.LogLevel)) | ||
srv := newServer(logger, o) | ||
|
||
t, err := template.New("webpage").Parse(tpl) | ||
if err != nil { | ||
// TODO(log): Should this actually panic? | ||
logger.Error(err, "parsing webpage template") | ||
panic(err) | ||
} | ||
|
||
http.HandleFunc("/", func(rw http.ResponseWriter, r *http.Request) { | ||
repoParam := r.URL.Query().Get("repo") | ||
const length = 3 | ||
s := strings.SplitN(repoParam, "/", length) | ||
if len(s) != length { | ||
rw.WriteHeader(http.StatusBadRequest) | ||
} | ||
repo, err := githubrepo.MakeGithubRepo(repoParam) | ||
if err != nil { | ||
rw.WriteHeader(http.StatusBadRequest) | ||
} | ||
ctx := r.Context() | ||
repoClient := githubrepo.CreateGithubRepoClient(ctx, logger) | ||
ossFuzzRepoClient, err := ossfuzz.CreateOSSFuzzClientEager(ossfuzz.StatusURL) | ||
if err != nil { | ||
logger.Error(err, "initializing clients") | ||
rw.WriteHeader(http.StatusInternalServerError) | ||
} | ||
defer ossFuzzRepoClient.Close() | ||
repoResult, err := scorecard.Run(ctx, repo, | ||
scorecard.WithCommitDepth(o.CommitDepth), | ||
scorecard.WithRepoClient(repoClient), | ||
scorecard.WithOSSFuzzClient(ossFuzzRepoClient), | ||
) | ||
if err != nil { | ||
logger.Error(err, "running enabled scorecard checks on repo") | ||
rw.WriteHeader(http.StatusInternalServerError) | ||
} | ||
|
||
if r.Header.Get("Content-Type") == "application/json" { | ||
if err := repoResult.AsJSON(o.ShowDetails, log.ParseLevel(o.LogLevel), rw); err != nil { | ||
// TODO(log): Improve error message | ||
logger.Error(err, "") | ||
rw.WriteHeader(http.StatusInternalServerError) | ||
} | ||
return | ||
} | ||
if err := t.Execute(rw, repoResult); err != nil { | ||
// TODO(log): Improve error message | ||
logger.Error(err, "") | ||
mux := http.NewServeMux() | ||
mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { | ||
if r.Method == http.MethodGet || r.Method == http.MethodPost { | ||
srv.handleScorecard(w, r) | ||
} else { | ||
http.NotFound(w, r) | ||
} | ||
}) | ||
mux.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) { | ||
w.WriteHeader(http.StatusOK) | ||
}) | ||
|
||
// Compose middlewares: logger -> recover -> cors -> mux | ||
handler := loggerMiddleware(recoverMiddleware(corsMiddleware(mux))) | ||
|
||
port := os.Getenv("PORT") | ||
if port == "" { | ||
port = "8080" | ||
} | ||
logger.Info("Listening on localhost:" + port + "\n") | ||
//nolint:gosec // unused. | ||
err = http.ListenAndServe(fmt.Sprintf("0.0.0.0:%s", port), nil) | ||
if err != nil { | ||
// TODO(log): Should this actually panic? | ||
logger.Error(err, "listening and serving") | ||
panic(err) | ||
|
||
httpServer := &http.Server{ | ||
Addr: fmt.Sprintf("0.0.0.0:%s", port), | ||
Handler: handler, | ||
} | ||
|
||
// Graceful shutdown | ||
done := make(chan os.Signal, 1) | ||
signal.Notify(done, os.Interrupt, syscall.SIGINT, syscall.SIGTERM) | ||
|
||
go func() { | ||
logger.Info("Server starting on port " + port) | ||
if err := httpServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||
logger.Error(err, "server error") | ||
} | ||
}() | ||
|
||
<-done | ||
logger.Info("Shutting down server...") | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
defer cancel() | ||
|
||
if err := httpServer.Shutdown(ctx); err != nil { | ||
return fmt.Errorf("server shutdown: %w", err) | ||
} | ||
|
||
return nil | ||
}, | ||
} | ||
} | ||
|
||
const tpl = ` | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<meta charset="UTF-8"> | ||
<title>Scorecard Results for: {{.Repo}}</title> | ||
</head> | ||
<body> | ||
{{range .Checks}} | ||
<div> | ||
<p>{{ .Name }}: {{ .Pass }}</p> | ||
</div> | ||
{{end}} | ||
</body> | ||
</html>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the CLI, policy file is a local file. How do we expect to pass a policy file to a server, with a URI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm late. I rarely use this policy file. I think that it should be a cmd parameter rather than a server parameter, so now I've temporarily removed it.