Data sources: Don't fail if URL doesn't specify protocol (#24497)

pkg/api/common_test.go

@@ -1,15 +1,18 @@
 package api
 
 import (
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"path/filepath"
+	"testing"
 
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/middleware"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/auth"
 	. "github.com/smartystreets/goconvey/convey"
+	"github.com/stretchr/testify/require"
 	"gopkg.in/macaron.v1"
 )
 
@@ -82,12 +85,18 @@ func (sc *scenarioContext) fakeReq(method, url string) *scenarioContext {
 func (sc *scenarioContext) fakeReqWithParams(method, url string, queryParams map[string]string) *scenarioContext {
 	sc.resp = httptest.NewRecorder()
 	req, err := http.NewRequest(method, url, nil)
+	// TODO: Depend on sc.t
+	if sc.t != nil {
+		require.NoError(sc.t, err)
+	} else if err != nil {
+		panic(fmt.Sprintf("Making request failed: %s", err))
+	}
+
 	q := req.URL.Query()
 	for k, v := range queryParams {
 		q.Add(k, v)
 	}
 	req.URL.RawQuery = q.Encode()
-	So(err, ShouldBeNil)
 	sc.req = req
 
 	return sc
@@ -114,6 +123,7 @@ func (sc *scenarioContext) fakeReqNoAssertionsWithCookie(method, url string, coo
 }
 
 type scenarioContext struct {
+	t                    *testing.T
 	m                    *macaron.Macaron
 	context              *models.ReqContext
 	resp                 *httptest.ResponseRecorder

pkg/api/dataproxy.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/grafana/grafana/pkg/api/datasource"
 	"github.com/grafana/grafana/pkg/api/pluginproxy"
 	"github.com/grafana/grafana/pkg/infra/metrics"
 	"github.com/grafana/grafana/pkg/models"
@@ -37,7 +38,7 @@ func (hs *HTTPServer) ProxyDataSourceRequest(c *models.ReqContext) {
 
 	proxy, err := pluginproxy.NewDataSourceProxy(ds, plugin, c, proxyPath, hs.Cfg)
 	if err != nil {
-		if errors.Is(err, pluginproxy.URLValidationError{}) {
+		if errors.Is(err, datasource.URLValidationError{}) {
 			c.JsonApiErr(400, fmt.Sprintf("Invalid data source URL: %q", ds.Url), err)
 		} else {
 			c.JsonApiErr(500, "Failed creating data source proxy", err)

pkg/api/datasource/validation.go

@@ -0,0 +1,50 @@
+package datasource
+
+import (
+	"fmt"
+	"net/url"
+	"regexp"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+)
+
+var logger = log.New("datasource")
+
+// URLValidationError represents an error from validating a data source URL.
+type URLValidationError struct {
+	error
+
+	url string
+}
+
+// Error returns the error message.
+func (e URLValidationError) Error() string {
+	return fmt.Sprintf("Validation of data source URL %q failed: %s", e.url, e.error.Error())
+}
+
+// Unwrap returns the wrapped error.
+func (e URLValidationError) Unwrap() error {
+	return e.error
+}
+
+// reURL is a regexp to detect if a URL specifies the protocol. We match also strings where the actual protocol is
+// missing (i.e., "://"), in order to catch these as invalid when parsing.
+var reURL = regexp.MustCompile("^[^:]*://")
+
+// ValidateURL validates a data source URL.
+//
+// If successful, the valid URL object is returned, otherwise an error is returned.
+func ValidateURL(urlStr string) (*url.URL, error) {
+	// Make sure the URL starts with a protocol specifier, so parsing is unambiguous
+	if !reURL.MatchString(urlStr) {
+		logger.Debug(
+			"Data source URL doesn't specify protocol, so prepending it with http:// in order to make it unambiguous")
+		urlStr = fmt.Sprintf("http://%s", urlStr)
+	}
+	u, err := url.Parse(urlStr)
+	if err != nil {
+		return nil, URLValidationError{error: err, url: urlStr}
+	}
+
+	return u, nil
+}

pkg/api/datasources.go

@@ -3,10 +3,10 @@ package api
 import (
 	"encoding/json"
 	"fmt"
-	"net/url"
 	"sort"
 
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
+	"github.com/grafana/grafana/pkg/api/datasource"
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/models"
@@ -14,7 +14,6 @@ import (
 	"github.com/grafana/grafana/pkg/plugins/backendplugin"
 	"github.com/grafana/grafana/pkg/plugins/datasource/wrapper"
 	"github.com/grafana/grafana/pkg/util"
-	"github.com/grafana/grafana/pkg/util/errutil"
 )
 
 func GetDataSources(c *models.ReqContext) Response {
@@ -131,10 +130,8 @@ func DeleteDataSourceByName(c *models.ReqContext) Response {
 
 func validateURL(u string) Response {
 	if u != "" {
-		_, err := url.Parse(u)
-		if err != nil {
-			return Error(400, fmt.Sprintf("Validation error, invalid URL: %q", u), errutil.Wrapf(err,
-				"invalid data source URL %q", u))
+		if _, err := datasource.ValidateURL(u); err != nil {
+			return Error(400, fmt.Sprintf("Validation error, invalid URL: %q", u), err)
 		}
 	}
 

pkg/api/datasources_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/grafana/grafana/pkg/models"
+	"github.com/stretchr/testify/assert"
 
 	"github.com/grafana/grafana/pkg/bus"
 	. "github.com/smartystreets/goconvey/convey"
@@ -56,3 +57,107 @@ func TestDataSourcesProxy(t *testing.T) {
 		})
 	})
 }
+
+// Adding data sources with invalid URLs should lead to an error.
+func TestAddDataSource_InvalidURL(t *testing.T) {
+	defer bus.ClearBusHandlers()
+
+	sc := setupScenarioContext("/api/datasources")
+	// TODO: Make this an argument to setupScenarioContext
+	sc.t = t
+
+	sc.m.Post(sc.url, Wrap(func(c *models.ReqContext) Response {
+		return AddDataSource(c, models.AddDataSourceCommand{
+			Name: "Test",
+			Url:  "invalid:url",
+		})
+	}))
+
+	sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec()
+
+	assert.Equal(t, 400, sc.resp.Code)
+}
+
+// Adding data sources with URLs not specifying protocol should work.
+func TestAddDataSource_URLWithoutProtocol(t *testing.T) {
+	defer bus.ClearBusHandlers()
+
+	const name = "Test"
+	const url = "localhost:5432"
+
+	// Stub handler
+	bus.AddHandler("sql", func(cmd *models.AddDataSourceCommand) error {
+		assert.Equal(t, name, cmd.Name)
+		assert.Equal(t, url, cmd.Url)
+
+		cmd.Result = &models.DataSource{}
+		return nil
+	})
+
+	sc := setupScenarioContext("/api/datasources")
+	// TODO: Make this an argument to setupScenarioContext
+	sc.t = t
+
+	sc.m.Post(sc.url, Wrap(func(c *models.ReqContext) Response {
+		return AddDataSource(c, models.AddDataSourceCommand{
+			Name: name,
+			Url:  url,
+		})
+	}))
+
+	sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec()
+
+	assert.Equal(t, 200, sc.resp.Code)
+}
+
+// Updating data sources with invalid URLs should lead to an error.
+func TestUpdateDataSource_InvalidURL(t *testing.T) {
+	defer bus.ClearBusHandlers()
+
+	sc := setupScenarioContext("/api/datasources/1234")
+	// TODO: Make this an argument to setupScenarioContext
+	sc.t = t
+
+	sc.m.Put(sc.url, Wrap(func(c *models.ReqContext) Response {
+		return AddDataSource(c, models.AddDataSourceCommand{
+			Name: "Test",
+			Url:  "invalid:url",
+		})
+	}))
+
+	sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec()
+
+	assert.Equal(t, 400, sc.resp.Code)
+}
+
+// Updating data sources with URLs not specifying protocol should work.
+func TestUpdateDataSource_URLWithoutProtocol(t *testing.T) {
+	defer bus.ClearBusHandlers()
+
+	const name = "Test"
+	const url = "localhost:5432"
+
+	// Stub handler
+	bus.AddHandler("sql", func(cmd *models.AddDataSourceCommand) error {
+		assert.Equal(t, name, cmd.Name)
+		assert.Equal(t, url, cmd.Url)
+
+		cmd.Result = &models.DataSource{}
+		return nil
+	})
+
+	sc := setupScenarioContext("/api/datasources/1234")
+	// TODO: Make this an argument to setupScenarioContext
+	sc.t = t
+
+	sc.m.Put(sc.url, Wrap(func(c *models.ReqContext) Response {
+		return AddDataSource(c, models.AddDataSourceCommand{
+			Name: name,
+			Url:  url,
+		})
+	}))
+
+	sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec()
+
+	assert.Equal(t, 200, sc.resp.Code)
+}

pkg/api/pluginproxy/ds_proxy.go

@@ -16,6 +16,7 @@ import (
 	"github.com/opentracing/opentracing-go"
 	"golang.org/x/oauth2"
 
+	"github.com/grafana/grafana/pkg/api/datasource"
 	"github.com/grafana/grafana/pkg/bus"
 	glog "github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/login/social"
@@ -31,20 +32,6 @@ var (
 	client = newHTTPClient()
 )
 
-type URLValidationError struct {
-	error
-
-	url string
-}
-
-func (e URLValidationError) Error() string {
-	return fmt.Sprintf("Validation of URL %q failed: %s", e.url, e.error.Error())
-}
-
-func (e URLValidationError) Unwrap() error {
-	return e.error
-}
-
 type DataSourceProxy struct {
 	ds        *models.DataSource
 	ctx       *models.ReqContext
@@ -86,9 +73,9 @@ func (lw *logWrapper) Write(p []byte) (n int, err error) {
 // NewDataSourceProxy creates a new Datasource proxy
 func NewDataSourceProxy(ds *models.DataSource, plugin *plugins.DataSourcePlugin, ctx *models.ReqContext,
 	proxyPath string, cfg *setting.Cfg) (*DataSourceProxy, error) {
-	targetURL, err := url.Parse(ds.Url)
+	targetURL, err := datasource.ValidateURL(ds.Url)
 	if err != nil {
-		return nil, URLValidationError{error: err, url: ds.Url}
+		return nil, err
 	}
 
 	return &DataSourceProxy{

pkg/api/pluginproxy/ds_proxy_test.go

@@ -567,7 +567,25 @@ func TestNewDataSourceProxy_InvalidURL(t *testing.T) {
 	plugin := plugins.DataSourcePlugin{}
 	_, err := NewDataSourceProxy(&ds, &plugin, &ctx, "api/method", &cfg)
 	require.Error(t, err)
-	assert.True(t, strings.HasPrefix(err.Error(), `Validation of URL "://host/root" failed`))
+	assert.True(t, strings.HasPrefix(err.Error(), `Validation of data source URL "://host/root" failed`))
+}
+
+func TestNewDataSourceProxy_ProtocolLessURL(t *testing.T) {
+	ctx := models.ReqContext{
+		Context: &macaron.Context{
+			Req: macaron.Request{},
+		},
+		SignedInUser: &models.SignedInUser{OrgRole: models.ROLE_EDITOR},
+	}
+	ds := models.DataSource{
+		Type: "test",
+		Url:  "127.0.01:5432",
+	}
+	cfg := setting.Cfg{}
+	plugin := plugins.DataSourcePlugin{}
+	_, err := NewDataSourceProxy(&ds, &plugin, &ctx, "api/method", &cfg)
+
+	require.NoError(t, err)
 }
 
 type CloseNotifierResponseRecorder struct {