From b257f456ba13bae83da25cda689687e8ae943d5d Mon Sep 17 00:00:00 2001
From: Rob Watson <rob@netflux.io>
Date: Thu, 10 Apr 2025 22:00:37 +0200
Subject: [PATCH] feat(config): tighten RTMP URL validation

---
 internal/app/integration_test.go              |  4 ++--
 internal/config/service.go                    |  8 +++++---
 internal/config/service_test.go               | 20 +++++++++++++------
 ...n-url.yml => destination-url-not-rtmp.yml} |  0
 .../testdata/destination-url-not-valid.yml    |  6 ++++++
 5 files changed, 27 insertions(+), 11 deletions(-)
 rename internal/config/testdata/{invalid-destination-url.yml => destination-url-not-rtmp.yml} (100%)
 create mode 100644 internal/config/testdata/destination-url-not-valid.yml

diff --git a/internal/app/integration_test.go b/internal/app/integration_test.go
index 448aaf2..0601e2c 100644
--- a/internal/app/integration_test.go
+++ b/internal/app/integration_test.go
@@ -325,7 +325,7 @@ func TestIntegrationDestinationValidations(t *testing.T) {
 			contents := getContents()
 
 			assert.True(t, contentsIncludes(contents, "Configuration update failed:"), "expected to see config update error")
-			assert.True(t, contentsIncludes(contents, "validate: destination URL must start with"), "expected to see config update error")
+			assert.True(t, contentsIncludes(contents, "validate: destination URL must be an RTMP URL"), "expected to see invalid RTMP URL error")
 		},
 		10*time.Second,
 		time.Second,
@@ -345,7 +345,7 @@ func TestIntegrationDestinationValidations(t *testing.T) {
 			contents := getContents()
 
 			assert.True(t, contentsIncludes(contents, "Configuration update failed:"), "expected to see config update error")
-			assert.True(t, contentsIncludes(contents, "validate: destination URL must start with"), "expected to see config update error")
+			assert.True(t, contentsIncludes(contents, "validate: destination URL must be an RTMP URL"), "expected to see invalid RTMP URL error")
 		},
 		10*time.Second,
 		time.Second,
diff --git a/internal/config/service.go b/internal/config/service.go
index 3169759..afec59d 100644
--- a/internal/config/service.go
+++ b/internal/config/service.go
@@ -5,9 +5,9 @@ import (
 	_ "embed"
 	"errors"
 	"fmt"
+	"net/url"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"gopkg.in/yaml.v3"
 )
@@ -204,8 +204,10 @@ func validate(cfg Config) error {
 	urlCounts := make(map[string]int)
 
 	for _, dest := range cfg.Destinations {
-		if !strings.HasPrefix(dest.URL, "rtmp://") {
-			err = errors.Join(err, fmt.Errorf("destination URL must start with rtmp://"))
+		if u, urlErr := url.Parse(dest.URL); urlErr != nil {
+			err = errors.Join(err, fmt.Errorf("invalid destination URL: %w", urlErr))
+		} else if u.Scheme != "rtmp" {
+			err = errors.Join(err, errors.New("destination URL must be an RTMP URL"))
 		}
 
 		urlCounts[dest.URL]++
diff --git a/internal/config/service_test.go b/internal/config/service_test.go
index e719c69..91531a5 100644
--- a/internal/config/service_test.go
+++ b/internal/config/service_test.go
@@ -25,8 +25,11 @@ var configLogfile []byte
 //go:embed testdata/no-logfile.yml
 var configNoLogfile []byte
 
-//go:embed testdata/invalid-destination-url.yml
-var configInvalidDestinationURL []byte
+//go:embed testdata/destination-url-not-rtmp.yml
+var configDestinationURLNotRTMP []byte
+
+//go:embed testdata/destination-url-not-valid.yml
+var configDestinationURLNotValid []byte
 
 //go:embed testdata/multiple-invalid-destination-urls.yml
 var configMultipleInvalidDestinationURLs []byte
@@ -120,14 +123,19 @@ func TestConfigServiceReadConfig(t *testing.T) {
 			},
 		},
 		{
-			name:        "invalid destination URL",
-			configBytes: configInvalidDestinationURL,
-			wantErr:     "destination URL must start with rtmp://",
+			name:        "destination URL is not rtmp scheme",
+			configBytes: configDestinationURLNotRTMP,
+			wantErr:     "destination URL must be an RTMP URL",
+		},
+		{
+			name:        "destination URL is not valid",
+			configBytes: configDestinationURLNotValid,
+			wantErr:     `invalid destination URL: parse "rtmp://rtmp.example.com/%%+": invalid URL escape "%%+"`,
 		},
 		{
 			name:        "multiple invalid destination URLs",
 			configBytes: configMultipleInvalidDestinationURLs,
-			wantErr:     "destination URL must start with rtmp://\ndestination URL must start with rtmp://",
+			wantErr:     "destination URL must be an RTMP URL\ndestination URL must be an RTMP URL",
 		},
 	}
 
diff --git a/internal/config/testdata/invalid-destination-url.yml b/internal/config/testdata/destination-url-not-rtmp.yml
similarity index 100%
rename from internal/config/testdata/invalid-destination-url.yml
rename to internal/config/testdata/destination-url-not-rtmp.yml
diff --git a/internal/config/testdata/destination-url-not-valid.yml b/internal/config/testdata/destination-url-not-valid.yml
new file mode 100644
index 0000000..77ffd1b
--- /dev/null
+++ b/internal/config/testdata/destination-url-not-valid.yml
@@ -0,0 +1,6 @@
+---
+logfile:
+  enabled: true
+  path: test.log
+destinations:
+- url: rtmp://rtmp.example.com/%%+