Skip to content

Commit

Permalink
internal/{frontend,vuln}: make vuln search case-insensitive
Browse files Browse the repository at this point in the history 
Vulnerability search (by Go ID, CVE or GHSA) is now case-insensitive.

Also fixes the regular expression for GHSAs which was too permissive.

Change-Id: I2d53bfe41a1f8f78f6dd8774d3c9accc4461d101
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/506517
Run-TryBot: Tatiana Bradley <[email protected]>
TryBot-Result: kokoro <[email protected]>
Reviewed-by: Jamal Carvalho <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
  • Loading branch information
@tatianab
tatianab committed Jun 28, 2023
1 parent d85a217 commit a9d9eed
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 54 deletions.
13 changes: 7 additions & 6 deletions internal/frontend/search.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ func searchRequestRedirectPath(ctx context.Context, ds internal.DataSource, quer
if urlSchemeIdx > -1 {
query = query[urlSchemeIdx+3:]
}
if vulnSupport && vuln.IsGoID(query) {
return fmt.Sprintf("/vuln/%s?q", query)
if id, ok := vuln.CanonicalGoID(query); vulnSupport && ok {
return fmt.Sprintf("/vuln/%s?q", id)
}
requestedPath := path.Clean(query)
if !strings.Contains(requestedPath, "/") || mode == searchModeVuln {
Expand Down Expand Up @@ -388,14 +388,15 @@ func searchVulnModule(ctx context.Context, mode, query string, client *vuln.Clie
func searchVulnAlias(ctx context.Context, mode, cq string, vc *vuln.Client) (_ *searchAction, err error) {
defer derrors.Wrap(&err, "searchVulnAlias(%q, %q)", mode, cq)

if mode != searchModeVuln || !vuln.IsAlias(cq) || vc == nil {
alias, ok := vuln.CanonicalAlias(cq)
if mode != searchModeVuln || !ok || vc == nil {
return nil, nil
}
id, err := vc.ByAlias(ctx, cq)
goID, err := vc.ByAlias(ctx, alias)
if err != nil {
return nil, &serverError{status: derrors.ToStatus(err)}
}
return &searchAction{redirectURL: "/vuln/" + id}, nil
return &searchAction{redirectURL: "/vuln/" + goID}, nil
}

// searchMode reports whether the search performed should be in package or
Expand All @@ -413,7 +414,7 @@ func searchMode(r *http.Request) string {
case searchModeVuln:
return searchModeVuln
default:
if vuln.IsAlias(q) {
if _, ok := vuln.CanonicalAlias(q); ok {
return searchModeVuln
}
if shouldDefaultToSymbolSearch(q) {
Expand Down
19 changes: 13 additions & 6 deletions internal/frontend/search_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func TestDetermineSearchAction(t *testing.T) {
// See TestSearchVulnAlias in this file for more tests.
{
name: "vuln alias",
query: "q=GHSA-aaaa-bbbb-cccc&m=vuln",
query: "q=GHSA-cccc-ffff-gggg&m=vuln",
wantRedirect: "/vuln/GO-1990-0001",
},
{
Expand All @@ -119,18 +119,18 @@ func TestDetermineSearchAction(t *testing.T) {
{
// We turn on vuln mode if the query matches a vuln alias.
name: "vuln alias not vuln mode",
query: "q=GHSA-aaaa-bbbb-cccc",
query: "q=GHSA-cccc-ffff-gggg",
wantRedirect: "/vuln/GO-1990-0001",
},
{
name: "vuln alias with no match",
query: "q=GHSA-aaaa-bbbb-dddd",
query: "q=GHSA-cccc-ffff-xxxx",
wantStatus: http.StatusNotFound,
},
{
// An explicit mode overrides that.
name: "vuln alias symbol mode",
query: "q=GHSA-aaaa-bbbb-cccc?m=symbol",
query: "q=GHSA-cccc-ffff-gggg?m=symbol",
wantTemplate: "search",
},
{
Expand Down Expand Up @@ -502,7 +502,7 @@ func TestNewSearchResult(t *testing.T) {
got := newSearchResult(&test.in, false, pr)
test.want.CommitTime = "unknown"
if diff := cmp.Diff(&test.want, got); diff != "" {
t.Errorf("mimatch (-want, +got):\n%s", diff)
t.Errorf("mismatch (-want, +got):\n%s", diff)
}
})
}
Expand Down Expand Up @@ -540,6 +540,7 @@ func TestSearchRequestRedirectPath(t *testing.T) {
{"non-existent path does not redirect", "github.com/non-existent", "", ""},
{"trim URL scheme from query", "https://golang.org/x/tools", "/golang.org/x/tools", ""},
{"Go vuln redirects", "GO-1969-0720", "/vuln/GO-1969-0720?q", ""},
{"Lower-case Go vuln redirects", "go-1969-0720", "/vuln/GO-1969-0720?q", ""},
{"not a Go vuln", "somepkg/GO-1969-0720", "", ""},
// Just setting the search mode to vuln does not cause a redirect.
{"search mode is vuln", "searchmodevuln", "", searchModeVuln},
Expand Down Expand Up @@ -593,7 +594,13 @@ func TestSearchVulnAlias(t *testing.T) {
{
name: "one match",
mode: searchModeVuln,
query: "GHSA-aaaa-bbbb-cccc",
query: "GHSA-cccc-ffff-gggg",
wantURL: "/vuln/GO-1990-0001",
},
{
name: "one match - case insensitive",
mode: searchModeVuln,
query: "gHSa-ccCc-fFFF-gGgG",
wantURL: "/vuln/GO-1990-0001",
},
} {
Expand Down
4 changes: 2 additions & 2 deletions internal/frontend/vulns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ func newVulnPage(ctx context.Context, url *url.URL, vc *vuln.Client) (*vulnPage,
template: "vuln/list",
title: "Vulnerability Reports"}, nil
default: // the path should be "/<ID>", e.g. "/GO-2021-0001".
id := strings.TrimPrefix(path, "/")
if !vuln.IsGoID(id) {
id, ok := vuln.CanonicalGoID(strings.TrimPrefix(path, "/"))
if !ok {
if url.Query().Has("q") {
return nil, derrors.NotFound
}
Expand Down
14 changes: 13 additions & 1 deletion internal/frontend/vulns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
)

var testEntries = []*osv.Entry{
{ID: "GO-1990-0001", Details: "a", Aliases: []string{"CVE-2000-1", "GHSA-aaaa-bbbb-cccc"}},
{ID: "GO-1990-0001", Details: "a", Aliases: []string{"CVE-2000-1", "GHSA-cccc-ffff-gggg"}},
{ID: "GO-1990-0002", Details: "b", Aliases: []string{"CVE-2000-1", "GHSA-1111-2222-3333"}},
{ID: "GO-1990-0010", Details: "c"},
{ID: "GO-1991-0001", Details: "d"},
Expand Down Expand Up @@ -104,6 +104,18 @@ func TestNewVulnPage(t *testing.T) {
title: "GO-1990-0002",
},
},
{
name: "vuln entry page - case insensitive",
url: "https://pkg.go.dev/vuln/go-1990-0002",
want: &vulnPage{
page: &VulnEntryPage{
Entry: testEntries[1],
AliasLinks: aliasLinks(testEntries[1]),
},
template: "vuln/entry",
title: "GO-1990-0002",
},
},
}

for _, tc := range tcs {
Expand Down
49 changes: 38 additions & 11 deletions internal/vuln/regexp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,48 @@

package vuln

import "regexp"
import (
"regexp"
"strings"
)

const (
ci = "(?i)" // case-insensitive
goRE = "^GO-[0-9]{4}-[0-9]{4,}$"
cveRE = "^CVE-[0-9]{4}-[0-9]+$"
// Regexp adapted from https://github.com/github/advisory-database.
ghsaRE = "^(GHSA)((-[23456789cfghjmpqrvwx]{4}){3})$"
)

// Case-insensitive regexps for vuln IDs/aliases.
var (
goRegexp = regexp.MustCompile("^GO-[0-9]{4}-[0-9]{4,}$")
cveRegexp = regexp.MustCompile("^CVE-[0-9]{4}-[0-9]+$")
ghsaRegexp = regexp.MustCompile("^GHSA-.{4}-.{4}-.{4}$")
goID = regexp.MustCompile(ci + goRE)
cveID = regexp.MustCompile(ci + cveRE)
ghsaID = regexp.MustCompile(ci + ghsaRE)
)

// IsGoID returns whether s is a valid Go vulnerability ID.
func IsGoID(s string) bool {
return goRegexp.MatchString(s)
// Canonical returns the canonical form of the given Go ID string
// by correcting the case.
//
// If no canonical form can be found, returns false.
func CanonicalGoID(id string) (_ string, ok bool) {
if goID.MatchString(id) {
return strings.ToUpper(id), true
}
return "", false
}

// IsAlias returns whether s is a valid vulnerability alias
// (CVE or GHSA).
func IsAlias(s string) bool {
return cveRegexp.MatchString(s) || ghsaRegexp.MatchString(s)
// Canonical returns the canonical form of the given alias ID string
// (a CVE or GHSA id) by correcting the case.
//
// If no canonical form can be found, returns false.
func CanonicalAlias(id string) (_ string, ok bool) {
if cveID.MatchString(id) {
return strings.ToUpper(id), true
}
parts := ghsaID.FindStringSubmatch(id)
if len(parts) != 4 {
return "", false
}
return strings.ToUpper(parts[1]) + strings.ToLower(parts[2]), true
}
106 changes: 78 additions & 28 deletions internal/vuln/regexp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,65 +6,115 @@ package vuln

import "testing"

func TestIsGoID(t *testing.T) {
func TestCanonicalGoID(t *testing.T) {
tests := []struct {
id string
want bool
id string
wantID string
wantOK bool
}{
{
id: "GO-1999-0001",
want: true,
id: "GO-1999-0001",
wantID: "GO-1999-0001",
wantOK: true,
},
{
id: "GO-2023-12345678",
want: true,
id: "GO-1999-000111",
wantID: "GO-1999-000111",
wantOK: true,
},
{
id: "GO-2023-123",
want: false,
id: "go-1999-0001",
wantID: "GO-1999-0001",
wantOK: true,
},
{
id: "GO-abcd-0001",
want: false,
id: "GO-1999",
wantID: "",
wantOK: false,
},
{
id: "CVE-1999-0001",
want: false,
id: "GHSA-cfgh-2345-rwxq",
wantID: "",
wantOK: false,
},
{
id: "CVE-1999-000123",
wantID: "",
wantOK: false,
},
{
id: "ghsa-Cfgh-2345-Rwxq",
wantID: "",
wantOK: false,
},
{
id: "cve-1999-000123",
wantID: "",
wantOK: false,
},
{
id: "cve-ghsa-go",
wantID: "",
wantOK: false,
},
}
for _, tt := range tests {
t.Run(tt.id, func(t *testing.T) {
got := IsGoID(tt.id)
if got != tt.want {
t.Errorf("IsGoID(%s) = %t, want %t", tt.id, got, tt.want)
gotID, gotOK := CanonicalGoID(tt.id)
if gotID != tt.wantID || gotOK != tt.wantOK {
t.Errorf("CanonicalGoID(%s) = (%s, %t), want (%s, %t)", tt.id, gotID, gotOK, tt.wantID, tt.wantOK)
}
})
}
}

func TestIsAlias(t *testing.T) {
func TestCanonicalAlias(t *testing.T) {
tests := []struct {
id string
want bool
id string
wantID string
wantOK bool
}{
{
id: "GO-1999-0001",
want: false,
id: "GO-1999-0001",
wantID: "",
wantOK: false,
},
{
id: "GHSA-cfgh-2345-rwxq",
wantID: "GHSA-cfgh-2345-rwxq",
wantOK: true,
},
{
id: "CVE-1999-000123",
wantID: "CVE-1999-000123",
wantOK: true,
},
{
id: "go-1999-0001",
wantID: "",
wantOK: false,
},
{
id: "ghsa-Cfgh-2345-Rwxq",
wantID: "GHSA-cfgh-2345-rwxq",
wantOK: true,
},
{
id: "GHSA-abcd-1234-efgh",
want: true,
id: "cve-1999-000123",
wantID: "CVE-1999-000123",
wantOK: true,
},
{
id: "CVE-1999-000123",
want: true,
id: "abc-CVE-1999-0001",
wantID: "",
wantOK: false,
},
}
for _, tt := range tests {
t.Run(tt.id, func(t *testing.T) {
got := IsAlias(tt.id)
if got != tt.want {
t.Errorf("IsAlias(%s) = %t, want %t", tt.id, got, tt.want)
gotID, gotOK := CanonicalAlias(tt.id)
if gotID != tt.wantID || gotOK != tt.wantOK {
t.Errorf("CanonicalAlias(%s) = (%s, %t), want (%s, %t)", tt.id, gotID, gotOK, tt.wantID, tt.wantOK)
}
})
}
Expand Down

0 comments on commit a9d9eed

Please sign in to comment.