feat: clamp glob cache minimums (TTL≥1s, entries≥16) and strip default ports in normalizeHost

Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/a2999beb-54fd-42e7-b1f4-c50844590385

Author: Copilot

CHANGELOG.md

@@ -12,9 +12,13 @@ See [Conventional Commits](https://www.conventionalcommits.org/) for commit mess
 - **`pkg/filesystem.GetGlobMatches`**: always returns a non-nil `[]string{}` (never `nil`).
   Callers must use `len(result) == 0` to detect no matches instead of `result == nil`.
   The cache is now bounded and configurable via three environment variables:
-  - `ATMOS_FS_GLOB_CACHE_MAX_ENTRIES` (default `1024`) — maximum number of cached glob patterns.
-  - `ATMOS_FS_GLOB_CACHE_TTL` (default `5m`) — time-to-live for each cache entry.
+  - `ATMOS_FS_GLOB_CACHE_MAX_ENTRIES` (default `1024`, minimum `16`) — maximum number of cached glob patterns.
+  - `ATMOS_FS_GLOB_CACHE_TTL` (default `5m`, minimum `1s`) — time-to-live for each cache entry.
+    Values below the respective minimums are clamped up rather than rejected.
   - `ATMOS_FS_GLOB_CACHE_EMPTY` (default `1`) — set to `0` to skip caching patterns that match no files.
+- **`pkg/http.normalizeHost`**: now strips default ports (`:443`, `:80`) in addition to
+  lower-casing and removing trailing dots, so that `api.github.com:443` is treated
+  identically to `api.github.com` for allowlist matching.
 
 ### Added
 

pkg/filesystem/doc.go

@@ -13,9 +13,9 @@
 // The glob LRU cache is configurable at startup via environment variables:
 //
 //   - ATMOS_FS_GLOB_CACHE_MAX_ENTRIES – maximum number of cached patterns
-//     (default: 1024).
+//     (default: 1024, minimum: 16; values below 16 are clamped up).
 //   - ATMOS_FS_GLOB_CACHE_TTL – TTL per entry as a Go duration string, e.g.
-//     "10m" (default: 5m).
+//     "10m" (default: 5m, minimum: 1s; values below 1s are clamped up).
 //   - ATMOS_FS_GLOB_CACHE_EMPTY – set to "0" or "false" to disable caching
 //     of empty (no-match) results (default: "1" = enabled).
 //

pkg/filesystem/glob.go

@@ -24,6 +24,16 @@ const (
 	// defaultGlobCacheTTL is the default time-to-live for each cache entry.
 	// Override at startup with ATMOS_FS_GLOB_CACHE_TTL (e.g. "10m", "30s").
 	defaultGlobCacheTTL = 5 * time.Minute
+
+	// minGlobCacheTTL is the minimum accepted TTL value.  Values parsed from
+	// ATMOS_FS_GLOB_CACHE_TTL that are positive but below this floor are clamped up.
+	// A sub-second TTL would make the cache nearly useless and cause excessive I/O.
+	minGlobCacheTTL = time.Second
+
+	// minGlobCacheMaxEntries is the minimum accepted LRU capacity.  Values parsed
+	// from ATMOS_FS_GLOB_CACHE_MAX_ENTRIES that are positive but below this floor
+	// are clamped up to prevent near-empty caches that evict on nearly every call.
+	minGlobCacheMaxEntries = 16
 )
 
 // globCacheEntry holds a cached glob result together with its expiry timestamp.
@@ -78,6 +88,9 @@ func applyGlobCacheConfig() {
 	//nolint:forbidigo // Direct env lookup required for cache configuration.
 	if v := os.Getenv("ATMOS_FS_GLOB_CACHE_MAX_ENTRIES"); v != "" {
 		if n, err := strconv.Atoi(v); err == nil && n > 0 {
+			if n < minGlobCacheMaxEntries {
+				n = minGlobCacheMaxEntries
+			}
 			maxEntries = n
 		}
 	}
@@ -86,6 +99,9 @@ func applyGlobCacheConfig() {
 	//nolint:forbidigo // Direct env lookup required for cache configuration.
 	if v := os.Getenv("ATMOS_FS_GLOB_CACHE_TTL"); v != "" {
 		if d, err := time.ParseDuration(v); err == nil && d > 0 {
+			if d < minGlobCacheTTL {
+				d = minGlobCacheTTL
+			}
 			ttl = d
 		}
 	}
@@ -133,8 +149,8 @@ func init() {
 // Callers should always use len(result) == 0 to detect no matches, not a nil comparison.
 //
 // Caching policy:
-//   - Results are bounded to a configurable LRU (default 1024 entries, ATMOS_FS_GLOB_CACHE_MAX_ENTRIES).
-//   - Each entry expires after a configurable TTL (default 5 minutes, ATMOS_FS_GLOB_CACHE_TTL).
+//   - Results are bounded to a configurable LRU (default 1024 entries, minimum 16, ATMOS_FS_GLOB_CACHE_MAX_ENTRIES).
+//   - Each entry expires after a configurable TTL (default 5 minutes, minimum 1s, ATMOS_FS_GLOB_CACHE_TTL).
 //   - Empty results are cached by default; set ATMOS_FS_GLOB_CACHE_EMPTY=0 to disable.
 //   - Cached slices are cloned on read, so callers may safely mutate the returned slice.
 func GetGlobMatches(pattern string) ([]string, error) {

pkg/filesystem/glob_atomic_test.go

@@ -614,6 +614,43 @@ func TestApplyGlobCacheConfig_InvalidInputsClamped(t *testing.T) {
 			wantTTL:        defaultTTL,
 			wantMaxEntries: 256,
 		},
+		// Values below the minimum should be clamped up, not rejected.
+		{
+			name:           "TTL_below_minimum_clamped_to_1s",
+			ttlEnv:         "100ms",
+			wantTTL:        time.Second,
+			wantMaxEntries: defaultMaxEntries,
+		},
+		{
+			name:           "TTL_500ms_clamped_to_1s",
+			ttlEnv:         "500ms",
+			wantTTL:        time.Second,
+			wantMaxEntries: defaultMaxEntries,
+		},
+		{
+			name:           "maxEntries_below_minimum_clamped_to_16",
+			maxEntriesEnv:  "5",
+			wantTTL:        defaultTTL,
+			wantMaxEntries: 16,
+		},
+		{
+			name:           "maxEntries_15_clamped_to_16",
+			maxEntriesEnv:  "15",
+			wantTTL:        defaultTTL,
+			wantMaxEntries: 16,
+		},
+		{
+			name:           "maxEntries_exactly_16_accepted",
+			maxEntriesEnv:  "16",
+			wantTTL:        defaultTTL,
+			wantMaxEntries: 16,
+		},
+		{
+			name:           "TTL_exactly_1s_accepted",
+			ttlEnv:         "1s",
+			wantTTL:        time.Second,
+			wantMaxEntries: defaultMaxEntries,
+		},
 	}
 
 	for _, tc := range cases {

pkg/http/client.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/url"
 	"os"
@@ -186,14 +187,24 @@ type GitHubAuthenticatedTransport struct {
 }
 
 // normalizeHost canonicalizes a hostname for allowlist comparison:
-// it lower-cases the string and strips a trailing dot (FQDN form).
-// Port stripping is explicitly NOT performed here; callers should pass
-// the result of url.URL.Hostname() (which already has the port removed)
-// rather than the raw url.URL.Host field.  This avoids incorrectly
-// truncating IPv6 literals (e.g., [::1]) which contain colons.
+// it lower-cases the string, strips a trailing dot (FQDN form), and removes
+// default HTTP/HTTPS ports (:80 and :443) so that "api.github.com:443" is
+// treated identically to "api.github.com".
+//
+// net.SplitHostPort is used to handle IPv6 literals safely (e.g., "[::1]:443"
+// is split to "::1" and "443", and the brackets are dropped for comparison).
+// Non-default ports (e.g., :8443) are preserved unchanged.
+//
+// Note: in the hot path, callers pass url.URL.Hostname() which already strips
+// the port, making the port-stripping here a defence-in-depth measure.
 func normalizeHost(host string) string {
 	host = strings.ToLower(host)
 	host = strings.TrimSuffix(host, ".")
+	// Strip default ports so that "api.github.com:443" matches "api.github.com".
+	// Also strip any trailing dot from the host part (handles "api.github.com.:443").
+	if h, port, err := net.SplitHostPort(host); err == nil && (port == "443" || port == "80") {
+		host = strings.TrimSuffix(h, ".")
+	}
 	return host
 }
 

pkg/http/client_test.go

@@ -1147,6 +1147,20 @@ func TestNormalizeHost(t *testing.T) {
 		// Upper-case + trailing dot.
 		{"API.GITHUB.COM.", "api.github.com"},
 		{"", ""},
+		// Default port 443 should be stripped.
+		{"api.github.com:443", "api.github.com"},
+		// Default port 80 should be stripped.
+		{"api.github.com:80", "api.github.com"},
+		// Non-default port should be preserved.
+		{"api.github.com:8443", "api.github.com:8443"},
+		// Port 443 + upper-case: both normalised.
+		{"API.GITHUB.COM:443", "api.github.com"},
+		// Port 443 + trailing dot: trailing dot stripped then port stripped.
+		{"api.github.com.:443", "api.github.com"},
+		// IPv6 with default port: brackets are stripped by net.SplitHostPort.
+		{"[::1]:443", "::1"},
+		// IPv6 with non-default port: preserved (with brackets stripped by SplitHostPort).
+		{"[::1]:8080", "[::1]:8080"},
 	}
 
 	for _, tt := range tests {
@@ -1167,6 +1181,11 @@ func TestIsGitHubHost_CaseAndTrailingDot(t *testing.T) {
 		"API.GITHUB.COM.",
 		"Raw.GitHubUserContent.com",
 		"UPLOADS.GITHUB.COM",
+		// Port variants: default port should be stripped before matching.
+		"api.github.com:443",
+		"API.GITHUB.COM:443",
+		"uploads.github.com:443",
+		"raw.githubusercontent.com:80",
 	}
 	for _, h := range positives {
 		assert.True(t, isGitHubHost(h), "expected %q to be allowed", h)
@@ -1176,6 +1195,9 @@ func TestIsGitHubHost_CaseAndTrailingDot(t *testing.T) {
 		"GITHUB.EXAMPLE.COM",
 		"EXAMPLE.GITHUB.COM",
 		"github.com",
+		// Port variants on disallowed hosts should still be denied.
+		"github.example.com:443",
+		"example.github.com:443",
 	}
 	for _, h := range negatives {
 		assert.False(t, isGitHubHost(h), "expected %q to be denied", h)