fix(scan): tolerate ref path items when parsing openapi specs (#309)

paths was typed as a map of operation structs, so a spec with a $ref string path item (valid in openapi 3.1) failed to decode and the whole spec was dropped with no finding. decode path items into a generic tree and extract per-operation security manually, treating a non-array security value as absent so it inherits the global default rather than being reported as an anonymous endpoint.
2026-07-28 14:37:01 -07:00 · 2026-07-22 12:49:31 -07:00
parent bf8c6a8643
commit e2da1bc5d8
2 changed files with 156 additions and 11 deletions
@@ -79,12 +79,21 @@ const (
 // openapiSpec is the minimal slice of an openapi/swagger document we care about:
 // the version banner, info block, top-level security and the path map. unknown
 // fields are ignored by both json and yaml decoders.
+//
+// path items decode into a bare interface{} rather than a typed operation struct
+// because openapi 3.1 allows a path item to be a "$ref" to a shared item instead
+// of a set of operations. a strongly typed sibling map (map[string]rawOpsStruct)
+// makes both json and yaml fail the whole document the moment one path item is a
+// $ref string next to another path item with real get/post operations, which
+// silently drops an otherwise valid, enumerable spec. interface{} accepts either
+// shape without erroring, and operationSecurity below sorts out what's actually
+// an operation object.
 type openapiSpec struct {
-	OpenAPI  string                       `json:"openapi" yaml:"openapi"`
-	Swagger  string                       `json:"swagger" yaml:"swagger"`
-	Info     openapiInfo                  `json:"info" yaml:"info"`
-	Security []map[string][]string        `json:"security" yaml:"security"`
-	Paths    map[string]map[string]rawOps `json:"paths" yaml:"paths"`
+	OpenAPI  string                            `json:"openapi" yaml:"openapi"`
+	Swagger  string                            `json:"swagger" yaml:"swagger"`
+	Info     openapiInfo                       `json:"info" yaml:"info"`
+	Security []map[string][]string             `json:"security" yaml:"security"`
+	Paths    map[string]map[string]interface{} `json:"paths" yaml:"paths"`
 }

 type openapiInfo struct {
@@ -92,10 +101,58 @@ type openapiInfo struct {
 	Version string `json:"version" yaml:"version"`
 }

-// rawOps captures the per-operation security block. a pointer so an absent block
-// (inherit global) is distinct from an explicit empty one (security: [] = public).
-type rawOps struct {
-	Security *[]map[string][]string `json:"security" yaml:"security"`
+// operationSecurity pulls the per-operation security block out of a decoded path
+// item entry. it reports present=false when the entry isn't an operation object
+// at all (a $ref path item, or a security key that was never declared), which the
+// caller treats as "inherit global" the same as an absent security key.
+func operationSecurity(op interface{}) (reqs []map[string][]string, present bool) {
+	obj, ok := op.(map[string]interface{})
+	if !ok {
+		return nil, false
+	}
+	raw, ok := obj["security"]
+	if !ok {
+		return nil, false
+	}
+	// a security key whose value isn't a list (null, or a malformed scalar or
+	// object) is not a usable requirement block. treat it as absent and inherit
+	// the global default rather than fabricate an anonymous high-severity finding
+	// from garbage, which also matches how the old typed decoder handled a null.
+	if _, ok := raw.([]interface{}); !ok {
+		return nil, false
+	}
+	return toSecurityReqs(raw), true
+}
+
+// toSecurityReqs converts a decoded "security" value (a list of scheme->scopes
+// requirement objects) into the typed form securityAllowsAnonymous expects.
+// malformed entries are skipped rather than treated as a parse failure, since by
+// this point the document has already passed the openapi/swagger version check.
+func toSecurityReqs(raw interface{}) []map[string][]string {
+	arr, ok := raw.([]interface{})
+	if !ok {
+		return nil
+	}
+	reqs := make([]map[string][]string, 0, len(arr))
+	for _, item := range arr {
+		obj, ok := item.(map[string]interface{})
+		if !ok {
+			continue
+		}
+		req := make(map[string][]string, len(obj))
+		for scheme, scopesRaw := range obj {
+			scopesArr, _ := scopesRaw.([]interface{})
+			scopes := make([]string, 0, len(scopesArr))
+			for _, s := range scopesArr {
+				if str, ok := s.(string); ok {
+					scopes = append(scopes, str)
+				}
+			}
+			req[scheme] = scopes
+		}
+		reqs = append(reqs, req)
+	}
+	return reqs
 }

 // OpenAPI probes the candidate spec paths concurrently and, on the first hit,
@@ -294,8 +351,8 @@ func specToResult(spec *openapiSpec) *OpenAPIResult {
 			}
 			// an explicit block decides on its own; an absent one inherits global.
 			var unauth bool
-			if op.Security != nil {
-				unauth = securityAllowsAnonymous(*op.Security)
+			if reqs, present := operationSecurity(op); present {
+				unauth = securityAllowsAnonymous(reqs)
 			} else {
 				unauth = globalAllowsAnon
 			}
@@ -268,6 +268,94 @@ func TestOpenAPI_YAMLSpec(t *testing.T) {
 	}
 }

+// a spec with a $ref path item (a shared item defined elsewhere, valid since
+// openapi 3.1) sitting next to a normal operation. /pet can't be resolved without
+// fetching the referenced document, but /users must still be enumerated: the
+// $ref entry must not fail the whole document's decode.
+const openapiJSONWithRef = `{
+  "openapi": "3.1.0",
+  "info": {"title": "Ref API", "version": "1.0"},
+  "paths": {
+    "/pet": {"$ref": "#/components/pathItems/Pet"},
+    "/users": {"get": {"summary": "list"}}
+  }
+}`
+
+// TestOpenAPI_RefPathItemDoesNotDropSpec locks a real regression: the old
+// map[string]rawOps decoded every path item as a strict operation-set struct, so
+// a $ref path item (its value is a plain string, not an object) failed both the
+// json and yaml unmarshal for the *entire* document, and an otherwise valid,
+// enumerable spec was silently rejected as "not a spec".
+func TestOpenAPI_RefPathItemDoesNotDropSpec(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/openapi.json" {
+			_, _ = w.Write([]byte(openapiJSONWithRef))
+			return
+		}
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	defer srv.Close()
+
+	result, err := OpenAPI(srv.URL, 5*time.Second, 4, "")
+	if err != nil {
+		t.Fatalf("OpenAPI: %v", err)
+	}
+	if result == nil {
+		t.Fatal("expected a result despite the $ref path item, got nil")
+	}
+	if _, ok := hasEndpoint(result, "/users", http.MethodGet); !ok {
+		t.Errorf("expected /users GET to be enumerated, got %+v", result.Endpoints)
+	}
+}
+
+// a globally-secured spec whose operation carries a security key that isn't a
+// list (here null). the interface{} extraction must treat this as absent and let
+// the operation inherit the enforced global requirement, exactly as the old typed
+// decoder did, not read it as an empty (public) block and fabricate a high finding.
+const openapiJSONNullOpSecurity = `{
+  "openapi": "3.0.1",
+  "info": {"title": "Null Sec API", "version": "1.0"},
+  "security": [{"bearerAuth": []}],
+  "paths": {
+    "/weird": {"get": {"summary": "malformed security", "security": null}}
+  }
+}`
+
+// TestOpenAPI_NonListOpSecurityInheritsGlobal locks the empty-vs-absent boundary
+// against a false positive: a non-array security value must inherit the global
+// requirement (authenticated, medium), not decode to an empty block (anonymous,
+// high). the pre-interface{} struct decoded null to a nil pointer and inherited;
+// the extraction has to preserve that or every malformed security key becomes a
+// spurious high-severity unauthenticated finding.
+func TestOpenAPI_NonListOpSecurityInheritsGlobal(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/openapi.json" {
+			_, _ = w.Write([]byte(openapiJSONNullOpSecurity))
+			return
+		}
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	defer srv.Close()
+
+	result, err := OpenAPI(srv.URL, 5*time.Second, 4, "")
+	if err != nil {
+		t.Fatalf("OpenAPI: %v", err)
+	}
+	if result == nil {
+		t.Fatal("expected a result, got nil")
+	}
+	ep, ok := hasEndpoint(result, "/weird", http.MethodGet)
+	if !ok {
+		t.Fatal("expected /weird GET to be enumerated")
+	}
+	if ep.Unauth {
+		t.Error("a non-list security value should inherit the global requirement, not read as public")
+	}
+	if result.Severity != openapiSevMedium {
+		t.Errorf("expected medium severity, got %q", result.Severity)
+	}
+}
+
 // TestOpenAPI_NoSpecExposed confirms a server with no spec at any candidate path
 // produces no result.
 func TestOpenAPI_NoSpecExposed(t *testing.T) {