From 6829882a0327a31fd10fee865be04aa79594765c Mon Sep 17 00:00:00 2001 From: Entlein Date: Wed, 29 Apr 2026 14:49:28 +0200 Subject: [PATCH] fix(dynamicpathdetector): include Internal in endpoint merge key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HTTPEndpoint.Equal distinguishes endpoints by (Endpoint, Direction, Internal), but MergeDuplicateEndpoints was keying only on (Endpoint, Direction). Three places leaked the bug: 1. getEndpointKey — duplicate-detection at the top of the loop would collapse two endpoints that differ only in Internal. 2. The wildcard-after-specific sweep — a :0/x Internal=false wildcard could absorb a previously-recorded :443/x Internal=true sibling because the sweep only checked path + direction. 3. The wildcardKey lookup for specific-after-wildcard ordering — same omission, so a specific-port entry could fold into a wildcard with a different Internal. Result: profile entries that the runtime semantically considers distinct (Internal=true vs false) were silently merged into one, losing the internal/external distinction for that path. Fix: include Internal in getEndpointKey, the sweep predicate, and wildcardKey, all keyed identically so the map lookups stay consistent. 4 new regression tests pin all three spots plus a positive sanity check that matching-Internal merges still happen. Existing tests unchanged. Flagged on upstream review of kubescape/storage#316. --- .../dynamicpathdetector/analyze_endpoints.go | 38 +++--- .../tests/analyze_endpoints_test.go | 116 ++++++++++++++++++ 2 files changed, 139 insertions(+), 15 deletions(-) diff --git a/pkg/registry/file/dynamicpathdetector/analyze_endpoints.go b/pkg/registry/file/dynamicpathdetector/analyze_endpoints.go index a0eba25a4..721406ecf 100644 --- a/pkg/registry/file/dynamicpathdetector/analyze_endpoints.go +++ b/pkg/registry/file/dynamicpathdetector/analyze_endpoints.go @@ -109,18 +109,22 @@ func splitEndpointPortAndPath(endpoint string) (string, string) { } // MergeDuplicateEndpoints folds duplicates and merges same-path specific-port -// endpoints into a wildcard-port (:0) sibling. The folding is symmetric: +// endpoints into a wildcard-port (:0) sibling. Folding is symmetric and is +// keyed on the same triple HTTPEndpoint.Equal compares — (Endpoint, +// Direction, Internal). An Internal=false endpoint will therefore NOT merge +// with an Internal=true sibling even if their path and direction match. // // - If a specific-port endpoint is encountered AFTER its :0 sibling, the // specific-port methods/headers are merged INTO the wildcard entry. // - If a specific-port endpoint is encountered BEFORE its :0 sibling, it // is initially recorded; when the wildcard arrives we sweep `seen` for -// same-(path, direction) specific-port siblings, fold them into the -// wildcard, and remove them from the output. +// same-(path, direction, Internal) specific-port siblings, fold them +// into the wildcard, and remove them from the output. // // This contract was tightened on the back of upstream review on // kubescape/storage#316 — a single :0 entry must NOT cause unrelated -// concrete-port endpoints to be wildcarded; only same-path siblings fold. +// concrete-port endpoints to be wildcarded; only same-path same-Internal +// siblings fold. func MergeDuplicateEndpoints(endpoints []*types.HTTPEndpoint) []*types.HTTPEndpoint { seen := make(map[string]*types.HTTPEndpoint) var newEndpoints []*types.HTTPEndpoint @@ -137,30 +141,30 @@ func MergeDuplicateEndpoints(endpoints []*types.HTTPEndpoint) []*types.HTTPEndpo if isWildcardPort(port) { // Wildcard arriving after specific-port siblings — sweep `seen` - // for any same-(path, direction) specific-port entries already - // recorded, fold them into the wildcard, then drop them from - // the output slice. - absorbed := false + // for any same-(path, direction, Internal) specific-port entries + // already recorded, fold them into the wildcard, then drop them + // from the output slice. for k, e := range seen { ePort, ePath := splitEndpointPortAndPath(e.Endpoint) - if isWildcardPort(ePort) || ePath != pathPart || e.Direction != endpoint.Direction { + if isWildcardPort(ePort) || ePath != pathPart || + e.Direction != endpoint.Direction || e.Internal != endpoint.Internal { continue } endpoint.Methods = MergeStrings(endpoint.Methods, e.Methods) mergeHeaders(endpoint, e) delete(seen, k) newEndpoints = removeEndpoint(newEndpoints, e) - absorbed = true } seen[key] = endpoint newEndpoints = append(newEndpoints, endpoint) - _ = absorbed continue } - // Specific port: if a wildcard sibling for the same (path, direction) - // is already in `seen`, fold this entry into it. - wildcardKey := fmt.Sprintf(":0%s|%s", pathPart, endpoint.Direction) + // Specific port: if a wildcard sibling for the same + // (path, direction, Internal) is already in `seen`, fold this entry + // into it. The wildcardKey shape MUST match getEndpointKey exactly so + // the lookup hits the same map slot the wildcard was inserted under. + wildcardKey := fmt.Sprintf(":0%s|%s|%t", pathPart, endpoint.Direction, endpoint.Internal) if existing, found := seen[wildcardKey]; found { existing.Methods = MergeStrings(existing.Methods, endpoint.Methods) mergeHeaders(existing, endpoint) @@ -186,8 +190,12 @@ func removeEndpoint(s []*types.HTTPEndpoint, target *types.HTTPEndpoint) []*type return s } +// getEndpointKey returns a key that uniquely identifies an HTTPEndpoint by +// the same fields HTTPEndpoint.Equal compares: Endpoint, Direction, Internal. +// Keep this in sync with the wildcardKey shape constructed in +// MergeDuplicateEndpoints — the two MUST hash identical entries identically. func getEndpointKey(endpoint *types.HTTPEndpoint) string { - return fmt.Sprintf("%s|%s", endpoint.Endpoint, endpoint.Direction) + return fmt.Sprintf("%s|%s|%t", endpoint.Endpoint, endpoint.Direction, endpoint.Internal) } func mergeHeaders(existing, new *types.HTTPEndpoint) { diff --git a/pkg/registry/file/dynamicpathdetector/tests/analyze_endpoints_test.go b/pkg/registry/file/dynamicpathdetector/tests/analyze_endpoints_test.go index 3f7700839..460d4cd12 100644 --- a/pkg/registry/file/dynamicpathdetector/tests/analyze_endpoints_test.go +++ b/pkg/registry/file/dynamicpathdetector/tests/analyze_endpoints_test.go @@ -449,3 +449,119 @@ func TestMergeDuplicateEndpoints_NoWildcardKeepsAllSpecificPorts(t *testing.T) { assert.Equal(t, 3, len(result), "no wildcard sibling => all specific-port endpoints must be kept") } + +// --------------------------------------------------------------------------- +// Internal-field isolation tests. +// +// `HTTPEndpoint.Equal` distinguishes endpoints by (Endpoint, Direction, +// Internal). The merge key in `MergeDuplicateEndpoints` and the wildcard +// sweep must therefore also distinguish Internal — otherwise an +// internally-originating endpoint can absorb an externally-originating one +// (or vice versa) just because they share path + direction. +// +// Flagged by upstream review on kubescape/storage#316 (matthyx). +// --------------------------------------------------------------------------- + +// TestMergeDuplicateEndpoints_InternalFieldDistinguishesDuplicates asserts +// that two endpoints differing ONLY in Internal are NOT collapsed by the +// duplicate-key check at the top of the merge loop. +func TestMergeDuplicateEndpoints_InternalFieldDistinguishesDuplicates(t *testing.T) { + external := &types.HTTPEndpoint{ + Endpoint: ":443/login", + Methods: []string{"POST"}, + Direction: "outbound", + Internal: false, + } + internal := &types.HTTPEndpoint{ + Endpoint: ":443/login", + Methods: []string{"GET"}, + Direction: "outbound", + Internal: true, + } + + result := dynamicpathdetector.MergeDuplicateEndpoints([]*types.HTTPEndpoint{external, internal}) + + assert.Equal(t, 2, len(result), + "endpoints with different Internal must NOT merge (HTTPEndpoint.Equal distinguishes Internal)") + // Each output must keep its own Internal value and methods. + for _, ep := range result { + switch ep.Internal { + case false: + assert.Equal(t, []string{"POST"}, ep.Methods, "external endpoint methods must not be polluted") + case true: + assert.Equal(t, []string{"GET"}, ep.Methods, "internal endpoint methods must not be polluted") + } + } +} + +// TestMergeDuplicateEndpoints_InternalFieldGuardsWildcardAbsorbingPrior +// pins the wildcard-after-specific path: an Internal=false :0 wildcard must +// NOT sweep up a previously-recorded Internal=true specific-port sibling. +func TestMergeDuplicateEndpoints_InternalFieldGuardsWildcardAbsorbingPrior(t *testing.T) { + specificInternal := &types.HTTPEndpoint{ + Endpoint: ":443/login", + Methods: []string{"POST"}, + Direction: "outbound", + Internal: true, + } + wildcardExternal := &types.HTTPEndpoint{ + Endpoint: ":0/login", + Methods: []string{"GET"}, + Direction: "outbound", + Internal: false, + } + + result := dynamicpathdetector.MergeDuplicateEndpoints([]*types.HTTPEndpoint{specificInternal, wildcardExternal}) + + assert.Equal(t, 2, len(result), + "wildcard with Internal=false must NOT absorb a specific-port sibling with Internal=true") +} + +// TestMergeDuplicateEndpoints_InternalFieldGuardsSpecificFoldingIntoWildcard +// pins the wildcard-first path: a previously-recorded Internal=true wildcard +// must NOT absorb a later Internal=false specific-port sibling. +func TestMergeDuplicateEndpoints_InternalFieldGuardsSpecificFoldingIntoWildcard(t *testing.T) { + wildcardInternal := &types.HTTPEndpoint{ + Endpoint: ":0/login", + Methods: []string{"GET"}, + Direction: "outbound", + Internal: true, + } + specificExternal := &types.HTTPEndpoint{ + Endpoint: ":443/login", + Methods: []string{"POST"}, + Direction: "outbound", + Internal: false, + } + + result := dynamicpathdetector.MergeDuplicateEndpoints([]*types.HTTPEndpoint{wildcardInternal, specificExternal}) + + assert.Equal(t, 2, len(result), + "specific-port with Internal=false must NOT fold into a wildcard sibling with Internal=true") +} + +// TestMergeDuplicateEndpoints_InternalFieldMatching_StillFolds is the +// positive sanity check: when Internal DOES match, the existing +// path+direction merge contract still holds. A regression here would mean +// the Internal guard accidentally blocks legitimate folding. +func TestMergeDuplicateEndpoints_InternalFieldMatching_StillFolds(t *testing.T) { + wildcard := &types.HTTPEndpoint{ + Endpoint: ":0/login", + Methods: []string{"GET"}, + Direction: "outbound", + Internal: true, + } + specific := &types.HTTPEndpoint{ + Endpoint: ":443/login", + Methods: []string{"POST"}, + Direction: "outbound", + Internal: true, + } + + result := dynamicpathdetector.MergeDuplicateEndpoints([]*types.HTTPEndpoint{wildcard, specific}) + + assert.Equal(t, 1, len(result), "matching Internal => specific-port sibling still folds into wildcard") + assert.Equal(t, ":0/login", result[0].Endpoint) + assert.True(t, result[0].Internal, "merged endpoint must preserve Internal=true") + assert.ElementsMatch(t, []string{"GET", "POST"}, result[0].Methods) +}