Merge pull request #1969 from mikejgray/fix/serve-config-validation-chain

Fix serve config validation being skipped when base-url is set

cmd/serve.go

@@ -348,7 +348,17 @@ func execServe(c *cli.Context) error {
 		return fmt.Errorf("config option visitor-attachment-daily-bandwidth-limit must be lower than %d", math.MaxInt)
 	}
 
-	// Check values
+	// Check values; checks that don't return stay out of the else-if chain, which would end there
+	if baseURL != "" {
+		u, err := url.Parse(baseURL)
+		if err != nil {
+			return fmt.Errorf("if set, base-url must be a valid URL, e.g. https://ntfy.mydomain.com: %v", err)
+		} else if u.Scheme != "http" && u.Scheme != "https" {
+			return errors.New("if set, base-url must be a valid URL starting with http:// or https://, e.g. https://ntfy.mydomain.com")
+		} else if u.Path != "" {
+			return fmt.Errorf("if set, base-url must not have a path (%s), as hosting ntfy on a sub-path is not supported, e.g. https://ntfy.mydomain.com", u.Path)
+		}
+	}
 	if databaseURL != "" && !strings.HasPrefix(databaseURL, "postgres://") && !strings.HasPrefix(databaseURL, "postgresql://") {
 		return errors.New("if database-url is set, it must start with postgres:// or postgresql://")
 	} else if databaseURL != "" && (authFile != "" || cacheFile != "" || webPushFile != "") {
@@ -391,15 +401,6 @@ func execServe(c *cli.Context) error {
 		return errors.New("if smtp-server-listen is set, smtp-server-domain must also be set")
 	} else if attachmentCacheDir != "" && baseURL == "" {
 		return errors.New("if attachment-cache-dir is set, base-url must also be set")
-	} else if baseURL != "" {
-		u, err := url.Parse(baseURL)
-		if err != nil {
-			return fmt.Errorf("if set, base-url must be a valid URL, e.g. https://ntfy.mydomain.com: %v", err)
-		} else if u.Scheme != "http" && u.Scheme != "https" {
-			return errors.New("if set, base-url must be a valid URL starting with http:// or https://, e.g. https://ntfy.mydomain.com")
-		} else if u.Path != "" {
-			return fmt.Errorf("if set, base-url must not have a path (%s), as hosting ntfy on a sub-path is not supported, e.g. https://ntfy.mydomain.com", u.Path)
-		}
 	} else if upstreamBaseURL != "" && !strings.HasPrefix(upstreamBaseURL, "http://") && !strings.HasPrefix(upstreamBaseURL, "https://") {
 		return errors.New("if set, upstream-base-url must start with http:// or https://")
 	} else if upstreamBaseURL != "" && strings.HasSuffix(upstreamBaseURL, "/") {
@@ -420,11 +421,8 @@ func execServe(c *cli.Context) error {
 		return errors.New("if stripe-secret-key is set, stripe-webhook-key and base-url must also be set")
 	} else if twilioAccount != "" && (twilioAuthToken == "" || twilioPhoneNumber == "" || twilioVerifyService == "" || baseURL == "" || (authFile == "" && databaseURL == "")) {
 		return errors.New("if twilio-account is set, twilio-auth-token, twilio-phone-number, twilio-verify-service, base-url, and auth-file (or database-url) must also be set")
-	} else if messageSizeLimit > server.DefaultMessageSizeLimit {
-		log.Warn("message-size-limit is greater than 4K, this is not recommended and largely untested, and may lead to issues with some clients")
-		if messageSizeLimit > 5*1024*1024 {
-			return errors.New("message-size-limit cannot be higher than 5M")
-		}
+	} else if messageSizeLimit > 5*1024*1024 {
+		return errors.New("message-size-limit cannot be higher than 5M")
 	} else if !server.WebPushAvailable && (webPushPrivateKey != "" || webPushPublicKey != "" || webPushFile != "") {
 		return errors.New("cannot enable WebPush, support is not available in this build (nowebpush)")
 	} else if webPushExpiryWarningDuration > 0 && webPushExpiryWarningDuration > webPushExpiryDuration {
@@ -446,6 +444,9 @@ func execServe(c *cli.Context) error {
 	} else if runtime.GOOS == "windows" && listenUnix != "" {
 		return errors.New("listen-unix is not supported on Windows")
 	}
+	if messageSizeLimit > server.DefaultMessageSizeLimit {
+		log.Warn("message-size-limit is greater than 4K, this is not recommended and largely untested, and may lead to issues with some clients")
+	}
 	if cacheDuration > 0 && attachmentExpiryDuration > cacheDuration {
 		log.Warn("attachment-expiry-duration (%s) is longer than cache-duration (%s); attachments never outlive their message, so they will be deleted after %s", attachmentExpiryDurationStr, cacheDurationStr, cacheDurationStr)
 	}

cmd/serve_test.go

@@ -691,6 +691,19 @@ func TestCLI_Serve_AttachmentExpiryLongerThanCacheDurationWarning(t *testing.T)
 	require.NotContains(t, buf.String(), "attachment-expiry-duration")
 }
 
+func TestCLI_Serve_ValidationAfterBaseURLAndMessageSizeLimit(t *testing.T) {
+	configFile := newEmptyFile(t) // Avoid issues with existing server.yml file on system
+	// A valid base-url used to end the checks, so the ones after it never ran; an invalid
+	// auth-default-access fails right after the checks, so the server never starts
+	app, _, _, _ := newTestApp()
+	err := app.Run([]string{"ntfy", "serve", "--config=" + configFile, "--auth-default-access=invalid", "--base-url=https://ntfy.example.com", "--upstream-base-url=https://ntfy.example.com"})
+	require.ErrorContains(t, err, "base-url and upstream-base-url cannot be identical")
+	// The same for a message-size-limit above 4K
+	app, _, _, _ = newTestApp()
+	err = app.Run([]string{"ntfy", "serve", "--config=" + configFile, "--auth-default-access=invalid", "--message-size-limit=8k", "--visitor-prefix-bits-ipv4=0"})
+	require.ErrorContains(t, err, "visitor-prefix-bits-ipv4 must be between 1 and 32")
+}
+
 func newEmptyFile(t *testing.T) string {
 	filename := filepath.Join(t.TempDir(), "empty")
 	require.Nil(t, os.WriteFile(filename, []byte{}, 0600))

docs/releases.md

@@ -2299,6 +2299,7 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release
 * Admin API: return 4xx errors instead of a 500 for invalid input, and remove a deleted user's web push subscriptions
 * Web app: Deleted notifications no longer come back when a poll replays messages published in the same second as the last one ([#2098](https://github.com/binwiederhier/ntfy/issues/2098), thanks to [@mattford63](https://github.com/mattford63) and [@arminus](https://github.com/arminus) for reporting)
 * Web app: A message replayed after a reconnect keeps its read state and no longer notifies again, and an older version of a sequence no longer replaces a newer one ([#2090](https://github.com/binwiederhier/ntfy/issues/2090), thanks to [@tobykaspar79](https://github.com/tobykaspar79) for reporting)
+* Check the whole server config again when `base-url` or a `message-size-limit` above 4K is set; most checks after them were skipped, so an invalid config (e.g. `upstream-base-url` equal to `base-url`) now fails to start ([#1969](https://github.com/binwiederhier/ntfy/pull/1969), thanks to [@mikejgray](https://github.com/mikejgray) for the contribution)
 
 ### ntfy iOS app v1.8.0 (UNRELEASED)