requested fixes: allowableQuery regex, and sanitize queries with @ in the name

Author: savil

internal/devpkg/package.go

@@ -270,7 +270,7 @@ func (p *Package) FullPackageAttributePath() (string, error) {
 }
 
 // NormalizedPackageAttributePath returns an attribute path normalized by nix
-// lookupNixInfo. This is useful for comparing different attribute paths that may
+// search. This is useful for comparing different attribute paths that may
 // point to the same package. Note, it may be an expensive call.
 func (p *Package) NormalizedPackageAttributePath() (string, error) {
 	if p.normalizedPackageAttributePathCache != "" {
@@ -284,7 +284,7 @@ func (p *Package) NormalizedPackageAttributePath() (string, error) {
 	return p.normalizedPackageAttributePathCache, nil
 }
 
-// normalizePackageAttributePath calls nix lookupNixInfo to find the normalized attribute
+// normalizePackageAttributePath calls nix search to find the normalized attribute
 // path. It may be an expensive call (~100ms).
 func (p *Package) normalizePackageAttributePath() (string, error) {
 	var query string

internal/nix/search.go

@@ -118,13 +118,23 @@ type CachedSearchResult struct {
 	Query string `json:"query"`
 }
 
+var allowableQuery = regexp.MustCompile("^github:NixOS/nixpkgs/[0-9a-f]{40}#[^#]+$")
+
+func isAllowableQuery(query string) bool {
+	return allowableQuery.MatchString(query)
+}
+
 // SearchNixpkgsAttribute is a wrapper around searchSystem that caches results.
 // NOTE: we should be very conservative in where we use this function. `nix search`
 // accepts generalized `installable regex` as arguments but is slow. For certain
 // queries of the form `nixpkgs/<commit-hash>#attribute`, we can know for sure that
 // once `nix search` returns a valid result, it will always be the very same result.
 // Hence we can cache it locally and answer future queries fast, by not calling `nix search`.
 func SearchNixpkgsAttribute(query string) (map[string]*Info, error) {
+	if !isAllowableQuery(query) {
+		return nil, errors.Errorf("invalid query: %s, must match regex: %s", query, allowableQuery)
+	}
+
 	key := cacheKey(query)
 
 	// Check if the query was already cached, and return the result if so
@@ -170,10 +180,10 @@ func filecacheNeedsUpdate(err error) bool {
 // cacheKey sanitizes the search query to be a valid unix filename.
 // This cache key is used as the filename to store the cache value, and having a
 // representation of the query is important for debuggability.
-func cacheKey(input string) string {
+func cacheKey(query string) string {
 	// Replace disallowed characters with underscores.
-	re := regexp.MustCompile(`[:/#+]`)
-	sanitized := re.ReplaceAllString(input, "_")
+	re := regexp.MustCompile(`[:/#@+]`)
+	sanitized := re.ReplaceAllString(query, "_")
 
 	// Remove any remaining invalid characters.
 	sanitized = regexp.MustCompile(`[^\w\.-]`).ReplaceAllString(sanitized, "")

internal/nix/search_test.go

@@ -13,6 +13,10 @@ func TestSearchCacheKey(t *testing.T) {
 			"github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go_1_19",
 			"github_NixOS_nixpkgs_8670e496ffd093b60e74e7fa53526aa5920d09eb_go_1_19",
 		},
+		{
+			"github:nixos/nixpkgs/7d0ed7f2e5aea07ab22ccb338d27fbe347ed2f11#emacsPackages.@",
+			"github_nixos_nixpkgs_7d0ed7f2e5aea07ab22ccb338d27fbe347ed2f11_emacsPackages._",
+		},
 	}
 
 	for _, testCase := range testCases {
@@ -24,3 +28,35 @@ func TestSearchCacheKey(t *testing.T) {
 		})
 	}
 }
+
+func TestIsAllowableQuery(t *testing.T) {
+	testCases := []struct {
+		in       string
+		expected bool
+	}{
+		{
+			"github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go_1_19",
+			true,
+		},
+		{
+			"github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb",
+			false,
+		},
+		{
+			"github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#",
+			false,
+		},
+		{
+			"github:NixOS/nixpkgs/nixpkgs-unstable#go_1_19",
+			false,
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.in, func(t *testing.T) {
+			out := isAllowableQuery(testCase.in)
+			if out != testCase.expected {
+				t.Errorf("got %t, want %t", out, testCase.expected)
+			}
+		})
+	}
+}