diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index c01108cb5f0..4b0855da675 100644 --- a/pkg/api/common_test.go +++ b/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 diff --git a/pkg/api/dataproxy.go b/pkg/api/dataproxy.go index 4dcea44b5bd..5c438ab5d0a 100644 --- a/pkg/api/dataproxy.go +++ b/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) diff --git a/pkg/api/datasource/validation.go b/pkg/api/datasource/validation.go new file mode 100644 index 00000000000..a924f49f502 --- /dev/null +++ b/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 +} diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go index 25a80b985f5..e99cdd6f670 100644 --- a/pkg/api/datasources.go +++ b/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) } } diff --git a/pkg/api/datasources_test.go b/pkg/api/datasources_test.go index 6e52a27758b..26962ad30f1 100644 --- a/pkg/api/datasources_test.go +++ b/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) +} diff --git a/pkg/api/pluginproxy/ds_proxy.go b/pkg/api/pluginproxy/ds_proxy.go index e9cfda3481c..8b20fb81b1f 100644 --- a/pkg/api/pluginproxy/ds_proxy.go +++ b/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{ diff --git a/pkg/api/pluginproxy/ds_proxy_test.go b/pkg/api/pluginproxy/ds_proxy_test.go index 4d5698cc433..7b91feca7a7 100644 --- a/pkg/api/pluginproxy/ds_proxy_test.go +++ b/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 {