Merge pull request #87 from OpenVPN/fix/baseurl-validation-require-https

Implement base URL validation

Author: arslanbekov

cloudconnexa/cloudconnexa.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -27,6 +28,96 @@ const (
 	DefaultMaxTokenResponseSize int64 = 1 << 20
 )
 
+// ClientOptions provides optional configuration for the API client.
+type ClientOptions struct {
+	// AllowInsecureHTTP permits HTTP connections for loopback addresses only
+	// (localhost, 127.0.0.1, ::1). This is intended for local development and testing.
+	// WARNING: HTTP connections to non-loopback addresses are always rejected.
+	AllowInsecureHTTP bool
+}
+
+// validateBaseURL validates the base URL for the API client.
+// It ensures the URL is well-formed and uses HTTPS unless allowHTTP is true
+// and the host is a loopback address (per RFC 9700 OAuth security best practices).
+func validateBaseURL(rawURL string, allowHTTP bool) (string, error) {
+	if rawURL == "" {
+		return "", fmt.Errorf("%w: URL cannot be empty", ErrInvalidBaseURL)
+	}
+
+	parsed, err := url.Parse(rawURL)
+	if err != nil {
+		return "", fmt.Errorf("%w: %v", ErrInvalidBaseURL, err)
+	}
+
+	// Validate scheme is present
+	if parsed.Scheme == "" {
+		return "", fmt.Errorf("%w: URL must include scheme (e.g., https://)", ErrInvalidBaseURL)
+	}
+
+	// Normalize scheme to lowercase
+	scheme := strings.ToLower(parsed.Scheme)
+
+	// Validate host is present
+	if parsed.Host == "" {
+		return "", fmt.Errorf("%w: URL must include a host", ErrInvalidBaseURL)
+	}
+
+	// Reject URLs with embedded credentials (security: prevents logging leaks)
+	if parsed.User != nil {
+		return "", fmt.Errorf("%w: URL must not contain credentials", ErrInvalidBaseURL)
+	}
+
+	// Check scheme - only allow http or https
+	switch scheme {
+	case "https":
+		// HTTPS is always allowed
+	case "http":
+		if !allowHTTP {
+			return "", ErrHTTPSRequired
+		}
+		// Even with allowHTTP, only permit loopback addresses
+		if !isLoopbackHost(parsed.Host) {
+			return "", fmt.Errorf("%w: HTTP is only allowed for localhost/127.0.0.1/::1", ErrHTTPSRequired)
+		}
+	default:
+		return "", fmt.Errorf("%w: unsupported scheme %q; only https is allowed", ErrInvalidBaseURL, scheme)
+	}
+
+	// Normalize: strip path, query, fragment - only keep scheme://host
+	normalized := fmt.Sprintf("%s://%s", scheme, parsed.Host)
+	return normalized, nil
+}
+
+// isLoopbackHost checks if the host is a loopback address.
+// This includes localhost, 127.0.0.0/8 range, and ::1.
+func isLoopbackHost(host string) bool {
+	// Extract hostname without port using net.SplitHostPort for robustness
+	hostname := host
+
+	// Try to split host and port
+	if h, _, err := net.SplitHostPort(host); err == nil {
+		hostname = h
+	}
+
+	// Strip brackets from IPv6 (may remain if no port was present, e.g., "[::1]")
+	hostname = strings.TrimPrefix(hostname, "[")
+	hostname = strings.TrimSuffix(hostname, "]")
+	hostname = strings.ToLower(hostname)
+
+	// Check common loopback names
+	if hostname == "localhost" || hostname == "::1" {
+		return true
+	}
+
+	// Check 127.0.0.0/8 range
+	ip := net.ParseIP(hostname)
+	if ip != nil {
+		return ip.IsLoopback()
+	}
+
+	return false
+}
+
 // Client represents a CloudConnexa API client with all service endpoints.
 type Client struct {
 	client *http.Client
@@ -87,19 +178,36 @@ func (e ErrClientResponse) StatusCode() int { return e.status }
 func (e ErrClientResponse) Body() string { return e.body }
 
 // NewClient creates a new CloudConnexa API client with the given credentials.
+// The baseURL must use HTTPS. For development with localhost HTTP, use NewClientWithOptions.
 // It authenticates using OAuth2 client credentials flow and returns a configured client.
 func NewClient(baseURL, clientID, clientSecret string) (*Client, error) {
+	return NewClientWithOptions(baseURL, clientID, clientSecret, nil)
+}
+
+// NewClientWithOptions creates a new CloudConnexa API client with custom options.
+// It authenticates using OAuth2 client credentials flow and returns a configured client.
+func NewClientWithOptions(baseURL, clientID, clientSecret string, opts *ClientOptions) (*Client, error) {
 	if clientID == "" || clientSecret == "" {
 		return nil, ErrCredentialsRequired
 	}
 
+	allowHTTP := false
+	if opts != nil {
+		allowHTTP = opts.AllowInsecureHTTP
+	}
+
+	normalizedURL, err := validateBaseURL(baseURL, allowHTTP)
+	if err != nil {
+		return nil, err
+	}
+
 	values := map[string]string{"grant_type": "client_credentials", "scope": "default"}
 	jsonData, err := json.Marshal(values)
 	if err != nil {
 		return nil, err
 	}
 
-	tokenURL := fmt.Sprintf("%s/api/v1/oauth/token", strings.TrimRight(baseURL, "/"))
+	tokenURL := fmt.Sprintf("%s/api/v1/oauth/token", normalizedURL)
 	req, err := http.NewRequest(http.MethodPost, tokenURL, bytes.NewBuffer(jsonData))
 
 	if err != nil {
@@ -140,7 +248,7 @@ func NewClient(baseURL, clientID, clientSecret string) (*Client, error) {
 
 	c := &Client{
 		client:            httpClient,
-		BaseURL:           baseURL,
+		BaseURL:           normalizedURL,
 		Token:             credentials.AccessToken,
 		UserAgent:         userAgent,
 		ReadRateLimiter:   rate.NewLimiter(rate.Every(1*time.Second), 1),

cloudconnexa/cloudconnexa_test.go

@@ -60,7 +60,10 @@ func TestNewClient(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			client, err := NewClient(tt.baseURL, tt.clientID, tt.clientSecret)
+			// Use NewClientWithOptions with AllowInsecureHTTP for localhost test server
+			client, err := NewClientWithOptions(tt.baseURL, tt.clientID, tt.clientSecret, &ClientOptions{
+				AllowInsecureHTTP: true,
+			})
 
 			if tt.wantErr {
 				assert.Error(t, err, "NewClient should return an error for invalid credentials")
@@ -267,8 +270,118 @@ func TestNewClient_TokenResponseOverLimit(t *testing.T) {
 	}))
 	defer server.Close()
 
-	_, err := NewClient(server.URL, "client-id", "client-secret")
+	_, err := NewClientWithOptions(server.URL, "client-id", "client-secret", &ClientOptions{
+		AllowInsecureHTTP: true,
+	})
 
 	assert.Error(t, err, "NewClient should fail for oversized OAuth response")
 	assert.True(t, errors.Is(err, ErrResponseTooLarge), "Error should be ErrResponseTooLarge, got: %v", err)
 }
+
+// TestValidateBaseURL tests the validateBaseURL function that validates base URL parameters.
+func TestValidateBaseURL(t *testing.T) {
+	tests := []struct {
+		name      string
+		url       string
+		allowHTTP bool
+		wantErr   error
+		wantURL   string
+	}{
+		// Valid HTTPS URLs
+		{"valid https", "https://api.example.com", false, nil, "https://api.example.com"},
+		{"https with port", "https://api.example.com:443", false, nil, "https://api.example.com:443"},
+		{"https with path stripped", "https://api.example.com/v1/", false, nil, "https://api.example.com"},
+		{"https trailing slash stripped", "https://api.example.com/", false, nil, "https://api.example.com"},
+
+		// Invalid: HTTP without allowHTTP
+		{"http not allowed", "http://api.example.com", false, ErrHTTPSRequired, ""},
+
+		// Valid: HTTP with allowHTTP for loopback
+		{"http localhost allowed", "http://localhost:8080", true, nil, "http://localhost:8080"},
+		{"http 127.0.0.1 allowed", "http://127.0.0.1:8080", true, nil, "http://127.0.0.1:8080"},
+		{"http 127.0.0.2 allowed", "http://127.0.0.2:9999", true, nil, "http://127.0.0.2:9999"},
+		{"http ::1 allowed", "http://[::1]:8080", true, nil, "http://[::1]:8080"},
+
+		// Invalid: HTTP with allowHTTP for non-loopback
+		{"http remote rejected even with allowHTTP", "http://api.example.com", true, ErrHTTPSRequired, ""},
+
+		// Invalid URLs
+		{"empty url", "", false, ErrInvalidBaseURL, ""},
+		{"missing scheme", "api.example.com", false, ErrInvalidBaseURL, ""},
+		{"ftp scheme", "ftp://api.example.com", false, ErrInvalidBaseURL, ""},
+		{"missing host", "https://", false, ErrInvalidBaseURL, ""},
+
+		// Security: credentials in URL rejected
+		{"url with userinfo", "https://user:pass@api.example.com", false, ErrInvalidBaseURL, ""},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := validateBaseURL(tt.url, tt.allowHTTP)
+			if tt.wantErr != nil {
+				assert.Error(t, err)
+				assert.True(t, errors.Is(err, tt.wantErr), "expected error %v, got %v", tt.wantErr, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.wantURL, result)
+			}
+		})
+	}
+}
+
+// TestIsLoopbackHost tests the isLoopbackHost function that checks for loopback addresses.
+func TestIsLoopbackHost(t *testing.T) {
+	tests := []struct {
+		host     string
+		expected bool
+	}{
+		{"localhost", true},
+		{"localhost:8080", true},
+		{"LOCALHOST", true},
+		{"127.0.0.1", true},
+		{"127.0.0.1:8080", true},
+		{"127.0.0.2", true},
+		{"127.255.255.255", true},
+		{"[::1]", true},
+		{"[::1]:8080", true},
+		{"::1", true},
+		{"api.example.com", false},
+		{"192.168.1.1", false},
+		{"10.0.0.1", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.host, func(t *testing.T) {
+			result := isLoopbackHost(tt.host)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+// TestNewClient_HTTPSRequired tests that NewClient rejects HTTP URLs by default.
+func TestNewClient_HTTPSRequired(t *testing.T) {
+	_, err := NewClient("http://api.example.com", "id", "secret")
+	assert.Error(t, err)
+	assert.True(t, errors.Is(err, ErrHTTPSRequired), "expected ErrHTTPSRequired, got: %v", err)
+}
+
+// TestNewClientWithOptions_AllowHTTPForLocalhost tests that NewClientWithOptions allows HTTP for localhost.
+func TestNewClientWithOptions_AllowHTTPForLocalhost(t *testing.T) {
+	server := setupMockServer()
+	defer server.Close()
+
+	client, err := NewClientWithOptions(server.URL, "test-id", "test-secret", &ClientOptions{
+		AllowInsecureHTTP: true,
+	})
+	assert.NoError(t, err)
+	assert.NotNil(t, client)
+}
+
+// TestNewClientWithOptions_HTTPRemoteRejected tests that HTTP to non-localhost is always rejected.
+func TestNewClientWithOptions_HTTPRemoteRejected(t *testing.T) {
+	_, err := NewClientWithOptions("http://api.example.com", "test-id", "test-secret", &ClientOptions{
+		AllowInsecureHTTP: true,
+	})
+	assert.Error(t, err)
+	assert.True(t, errors.Is(err, ErrHTTPSRequired), "expected ErrHTTPSRequired, got: %v", err)
+}

cloudconnexa/errors.go

@@ -10,3 +10,9 @@ var ErrEmptyID = errors.New("id cannot be empty")
 
 // ErrResponseTooLarge is returned when a response body exceeds the configured size limit.
 var ErrResponseTooLarge = errors.New("response body exceeds maximum allowed size")
+
+// ErrInvalidBaseURL is returned when the base URL is malformed or cannot be parsed.
+var ErrInvalidBaseURL = errors.New("invalid base URL")
+
+// ErrHTTPSRequired is returned when HTTP is used but HTTPS is required for security.
+var ErrHTTPSRequired = errors.New("HTTPS required: HTTP is not allowed for OAuth credentials")

cloudconnexa/vpn_regions_test.go

@@ -42,7 +42,9 @@ func TestVPNRegionsService_List(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client, err := NewClient(server.URL, "test", "test")
+	client, err := NewClientWithOptions(server.URL, "test", "test", &ClientOptions{
+		AllowInsecureHTTP: true,
+	})
 	assert.NoError(t, err)
 	regions, err := client.VPNRegions.List()
 
@@ -73,7 +75,9 @@ func TestVPNRegionsService_GetByID(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client, err := NewClient(server.URL, "test", "test")
+	client, err := NewClientWithOptions(server.URL, "test", "test", &ClientOptions{
+		AllowInsecureHTTP: true,
+	})
 	assert.NoError(t, err)
 
 	// Test existing region