From 1bbb3bf454a9c949e8b4bd7047b2510fd0f1fe73 Mon Sep 17 00:00:00 2001
From: mlsmaycon <mlsmaycon@gmail.com>
Date: Wed, 12 Aug 2026 09:44:33 +0000
Subject: [PATCH] [client] Clear MDM upload URL when the policy empties; cut
 complexity

Two review follow-ups on the MDM debug-bundle upload override:

- applyMDMPolicy returned early on an empty policy before reaching the
  clear-on-absent path, so a policy that dropped every key left a stale
  upload target directing bundles on a reused Config. Resolve the override
  unconditionally, up front, so an absent, empty, or invalid value fails
  closed to "" (falling back to the management-supplied or default target).
- Extract the resolution into mdmDebugBundleUploadURL, dropping the outer
  function's cognitive complexity from 26 to 20 (SonarCloud gate is 25).

Tests cover the empty-policy and invalid-URL clearing paths.
---
 client/internal/profilemanager/config.go      | 44 ++++++++++++-------
 client/internal/profilemanager/config_test.go | 15 +++++++
 2 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go
index 16a781907..b56677eef 100644
--- a/client/internal/profilemanager/config.go
+++ b/client/internal/profilemanager/config.go
@@ -691,6 +691,14 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 // for the key, so per-field rejection of user writes still applies).
 func (config *Config) applyMDMPolicy(policy *mdm.Policy) {
 	config.policy = policy
+
+	// DebugBundleUploadURL is a runtime-only override re-derived from MDM on
+	// every apply. Resolve it unconditionally (before the IsEmpty early return)
+	// so a policy that drops the key, becomes empty, or carries an invalid
+	// value can never leave a previously-enforced upload target active on a
+	// reused Config instance.
+	config.DebugBundleUploadURL = mdmDebugBundleUploadURL(policy)
+
 	if policy.IsEmpty() {
 		return
 	}
@@ -767,22 +775,28 @@ func (config *Config) applyMDMPolicy(policy *mdm.Policy) {
 		logApplied(mdm.KeyLazyConnection, state)
 	}
 
-	if v, ok := policy.GetString(mdm.KeyBundleUploadURL); ok {
-		// Must be a well-formed https URL with a host, matching the client's
-		// remote-job upload-URL validation. Invalid values are skipped so a
-		// bad policy cannot break bundle uploads. The URL is not logged: it
-		// can embed credentials or signed query tokens.
-		if u, err := url.Parse(v); err != nil || u.Scheme != "https" || u.Host == "" {
-			log.Warnf("MDM debug bundle upload URL is invalid (must be an https URL with a host); keeping previous value")
-		} else {
-			config.DebugBundleUploadURL = v
-			logApplied(mdm.KeyBundleUploadURL, "")
-		}
-	} else {
-		// The key was dropped from the policy: clear any stale override so it
-		// can never keep directing uploads to a previously-enforced host.
-		config.DebugBundleUploadURL = ""
+}
+
+// mdmDebugBundleUploadURL resolves the MDM-enforced debug-bundle upload URL
+// override from the policy, returning the empty string when the policy does
+// not carry a valid KeyBundleUploadURL. An absent or invalid value fails
+// closed to "" so it falls back to the management-supplied or default upload
+// target rather than a previously-enforced one. The URL is never logged: it
+// can embed credentials or signed query tokens (KeyBundleUploadURL is in
+// mdm.SecretKeys).
+func mdmDebugBundleUploadURL(policy *mdm.Policy) string {
+	v, ok := policy.GetString(mdm.KeyBundleUploadURL)
+	if !ok {
+		return ""
 	}
+	// Must be a well-formed https URL with a host, matching the client's
+	// remote-job upload-URL validation.
+	if u, err := url.Parse(v); err != nil || u.Scheme != "https" || u.Host == "" {
+		log.Warnf("MDM debug bundle upload URL is invalid (must be an https URL with a host); ignoring the override")
+		return ""
+	}
+	log.Infof("MDM override %s = ********** (secret)", mdm.KeyBundleUploadURL)
+	return v
 }
 
 // parseURL parses and validates the URL for the named service. The URL
diff --git a/client/internal/profilemanager/config_test.go b/client/internal/profilemanager/config_test.go
index 3b04ceed6..248920b5e 100644
--- a/client/internal/profilemanager/config_test.go
+++ b/client/internal/profilemanager/config_test.go
@@ -332,6 +332,21 @@ func TestApplyMDMPolicyRemoteJobs(t *testing.T) {
 		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{mdm.KeyRemoteJobsAllowed: true}))
 		assert.Empty(t, cfg.DebugBundleUploadURL, "the stale upload URL override must be cleared")
 	})
+
+	t.Run("an empty replacement policy clears a previously-applied override", func(t *testing.T) {
+		cfg := &Config{DebugBundleUploadURL: "https://old.example.com"}
+		// A policy that becomes empty entirely hits the IsEmpty early return;
+		// the override must still be cleared rather than surviving on the
+		// reused Config instance.
+		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{}))
+		assert.Empty(t, cfg.DebugBundleUploadURL, "the stale upload URL override must be cleared when the policy empties")
+	})
+
+	t.Run("an invalid upload URL clears a previously-applied override (fail closed)", func(t *testing.T) {
+		cfg := &Config{DebugBundleUploadURL: "https://old.example.com"}
+		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{mdm.KeyBundleUploadURL: "not-a-url"}))
+		assert.Empty(t, cfg.DebugBundleUploadURL, "an invalid override must fail closed, not keep the stale target")
+	})
 }
 
 func TestUpdateOldManagementURL(t *testing.T) {