Browse Source

logpolicy: fix nil pointer dereference with invalid TS_LOG_TARGET

When TS_LOG_TARGET is set to an invalid URL, url.Parse returns an error
and nil pointer, which caused a panic when accessing u.Host.

Now we check the error from url.Parse and log a helpful message while
falling back to the default log host.

Fixes #17792

Signed-off-by: Andrew Dunham <[email protected]>
Andrew Dunham 3 months ago
parent
commit
208a32af5b
2 changed files with 54 additions and 4 deletions
  1. 10 4
      logpolicy/logpolicy.go
  2. 44 0
      logpolicy/logpolicy_test.go

+ 10 - 4
logpolicy/logpolicy.go

@@ -640,10 +640,16 @@ func (opts Options) init(disableLogging bool) (*logtail.Config, *Policy) {
 
 		logHost := logtail.DefaultHost
 		if val := getLogTarget(); val != "" {
-			opts.Logf("You have enabled a non-default log target. Doing without being told to by Tailscale staff or your network administrator will make getting support difficult.")
-			conf.BaseURL = val
-			u, _ := url.Parse(val)
-			logHost = u.Host
+			u, err := url.Parse(val)
+			if err != nil {
+				opts.Logf("logpolicy: invalid TS_LOG_TARGET %q: %v; using default log host", val, err)
+			} else if u.Host == "" {
+				opts.Logf("logpolicy: invalid TS_LOG_TARGET %q: missing host; using default log host", val)
+			} else {
+				opts.Logf("You have enabled a non-default log target. Doing without being told to by Tailscale staff or your network administrator will make getting support difficult.")
+				conf.BaseURL = val
+				logHost = u.Host
+			}
 		}
 
 		if conf.HTTPC == nil {

+ 44 - 0
logpolicy/logpolicy_test.go

@@ -84,3 +84,47 @@ func TestOptions(t *testing.T) {
 		})
 	}
 }
+
+// TestInvalidLogTarget is a test for #17792
+func TestInvalidLogTarget(t *testing.T) {
+	defer resetLogTarget()
+
+	tests := []struct {
+		name      string
+		logTarget string
+	}{
+		{
+			name:      "invalid_url_no_scheme",
+			logTarget: "not a url at all",
+		},
+		{
+			name:      "malformed_url",
+			logTarget: "ht!tp://invalid",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			resetLogTarget()
+			os.Setenv("TS_LOG_TARGET", tt.logTarget)
+
+			opts := Options{
+				Collection: "test.log.tailscale.io",
+				Logf:       t.Logf,
+			}
+
+			// This should not panic even with invalid log target
+			config, policy := opts.init(false)
+			if policy == nil {
+				t.Fatal("expected non-nil policy")
+			}
+			defer policy.Close()
+
+			// When log target is invalid, it should fall back to the invalid value
+			// but not crash. BaseURL should remain empty
+			if config.BaseURL != "" {
+				t.Errorf("got BaseURL=%q, want empty", config.BaseURL)
+			}
+		})
+	}
+}