Repository: ariga/atlas
Pull request #2108 (2023-09-21 20:36:03)

cmd/atlas: omit url strings from parse errors
Before this PR, passing Atlas an invalid URL returned an error that includes the problematic URL (in `url.Error.URL`):

```shell
$ atlas schema inspect -u "  mysql://user:pass@:3306"

Error: parse "  mysql://user:pass@:3306": first path segment in URL cannot contain colon
```

This might be useful in local development, but it should be avoided, as Atlas is mostly used in CI/CD environments. This PR changes the behavior by introducing a new function (`sqlclient.ParseURL`) that returns parse errors without the raw URLs, and it is used by the various cmd packages. For example:

```
$ atlas schema inspect -u "  mysql://user:pass@:3306"

Error: first path segment in URL cannot contain colon
```

giautm: I think we should add golint rule to block url.Parse for all places
a8m: > I think we should add golint rule to block url.Parse for all places

It’s only for database URLs. For other URLs, like directory path, we use url.Parse and return the full error. 

Patch:
diff --git a/cmd/atlas/internal/cloudapi/client.go b/cmd/atlas/internal/cloudapi/client.go
index 58b1eb12..bbfa5567 100644
--- a/cmd/atlas/internal/cloudapi/client.go
+++ b/cmd/atlas/internal/cloudapi/client.go
@@ -12,12 +12,12 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"net/url"
 	"runtime"
 	"strings"
 	"time"
 
 	"ariga.io/atlas/sql/migrate"
+	"ariga.io/atlas/sql/sqlclient"
 
 	"github.com/vektah/gqlparser/v2/gqlerror"
 )
@@ -288,14 +288,11 @@ func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 
 // RedactedURL returns a URL string with the userinfo redacted.
 func RedactedURL(s string) (string, error) {
-	u, err := url.Parse(s)
-	switch err := err.(type) {
-	case nil:
-		return u.Redacted(), nil
-	case *url.Error:
-		err.URL = ""
+	u, err := sqlclient.ParseURL(s)
+	if err != nil {
+		return "", err
 	}
-	return "", err
+	return u.Redacted(), nil
 }
 
 // version of the CLI set by cmdapi.
diff --git a/cmd/atlas/internal/cmdapi/cmdapi.go b/cmd/atlas/internal/cmdapi/cmdapi.go
index 5a9d072f..3f6de54c 100644
--- a/cmd/atlas/internal/cmdapi/cmdapi.go
+++ b/cmd/atlas/internal/cmdapi/cmdapi.go
@@ -420,7 +420,7 @@ func (c *stateReaderConfig) Exported() (*cmdext.StateReaderConfig, error) {
 		parsed = make([]*url.URL, len(c.urls))
 	)
 	for i, u := range c.urls {
-		if parsed[i], err = url.Parse(u); err != nil {
+		if parsed[i], err = sqlclient.ParseURL(u); err != nil {
 			return nil, err
 		}
 	}
diff --git a/cmd/atlas/internal/cmdext/cmdext.go b/cmd/atlas/internal/cmdext/cmdext.go
index 558ddb64..e13a9bb3 100644
--- a/cmd/atlas/internal/cmdext/cmdext.go
+++ b/cmd/atlas/internal/cmdext/cmdext.go
@@ -86,7 +86,7 @@ func RuntimeVar(c *hcl.EvalContext, block *hclsyntax.Block) (cty.Value, error) {
 	if diags := gohcl.DecodeBody(block.Body, c, &args); diags.HasErrors() {
 		return cty.NilVal, errorf("decoding body: %v", diags)
 	}
-	u, err := url.Parse(args.URL)
+	u, err := sqlclient.ParseURL(args.URL)
 	if err != nil {
 		return cty.NilVal, errorf("parsing url: %v", err)
 	}
@@ -769,7 +769,7 @@ func (l EntLoader) MigrateDiff(ctx context.Context, opts *MigrateDiffOptions) er
 	if opts.Dev.URL.Schema == "" {
 		return errNotSchemaURL
 	}
-	u, err := url.Parse(opts.To[0])
+	u, err := sqlclient.ParseURL(opts.To[0])
 	if err != nil {
 		return nil
 	}
diff --git a/sql/sqlclient/client.go b/sql/sqlclient/client.go
index 8f677ceb..a8c53bfa 100644
--- a/sql/sqlclient/client.go
+++ b/sql/sqlclient/client.go
@@ -167,6 +167,19 @@ func (f URLParserFunc) ParseURL(u *url.URL) *URL {
 	return f(u)
 }
 
+// ParseURL is similar to url.Parse but returns errors without
+// the raw URL attached to avoid printing userinfo in errors.
+func ParseURL(s string) (*url.URL, error) {
+	u, err := url.Parse(s)
+	if err != nil {
+		if err1, ok := err.(*url.Error); ok {
+			err = err1.Err
+		}
+		return nil, err
+	}
+	return u, nil
+}
+
 var drivers sync.Map
 
 type (
@@ -184,7 +197,7 @@ var ErrUnsupported = errors.New("sql/sqlclient: driver does not support changing
 
 // Open opens an Atlas client by its provided url string.
 func Open(ctx context.Context, s string, opts ...OpenOption) (*Client, error) {
-	u, err := url.Parse(s)
+	u, err := ParseURL(s)
 	if err != nil {
 		return nil, fmt.Errorf("sql/sqlclient: parse open url: %w", err)
 	}


Test patch:
diff --git a/cmd/atlas/internal/cloudapi/client_test.go b/cmd/atlas/internal/cloudapi/client_test.go
index e51a259f..ff219d36 100644
--- a/cmd/atlas/internal/cloudapi/client_test.go
+++ b/cmd/atlas/internal/cloudapi/client_test.go
@@ -145,6 +145,6 @@ func TestRedactedURL(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "mysql://user:xxxxx@:3306/db", u)
 	u, err = RedactedURL("\\n mysql://user:pass@:3306/db")
-	require.EqualError(t, err, `parse "": first path segment in URL cannot contain colon`)
+	require.EqualError(t, err, `first path segment in URL cannot contain colon`)
 	require.Empty(t, u)
 }
diff --git a/cmd/atlas/internal/cmdapi/migrate_test.go b/cmd/atlas/internal/cmdapi/migrate_test.go
index 45659a32..b6ade08d 100644
--- a/cmd/atlas/internal/cmdapi/migrate_test.go
+++ b/cmd/atlas/internal/cmdapi/migrate_test.go
@@ -983,9 +983,9 @@ env {
 			"--url", "    sqlite://my:pass@file.db",
 			"--var", "cloud_url="+srv.URL,
 		)
-		// URL errors are reported to the client, but not deployment set.
-		require.EqualError(t, err, `sql/sqlclient: parse open url: parse "    sqlite://my:pass@file.db": first path segment in URL cannot contain colon`)
-		require.Equal(t, `Error: parse "": first path segment in URL cannot contain colon`, *reports.Error)
+		// Open errors are reported without URLs.
+		require.EqualError(t, err, `sql/sqlclient: parse open url: first path segment in URL cannot contain colon`)
+		require.Equal(t, `Error: sql/sqlclient: parse open url: first path segment in URL cannot contain colon`, *reports.Error)
 	})
 
 	t.Run("LocalDirectory", func(t *testing.T) {
diff --git a/sql/sqlclient/client_test.go b/sql/sqlclient/client_test.go
index e5409654..c783532c 100644
--- a/sql/sqlclient/client_test.go
+++ b/sql/sqlclient/client_test.go
@@ -67,6 +67,21 @@ func TestOpen_Errors(t *testing.T) {
 	c, err = sqlclient.Open(context.Background(), "unknown://")
 	require.EqualError(t, err, `sql/sqlclient: unknown driver "unknown". See: https://atlasgo.io/url`)
 	require.Nil(t, c)
+
+	// URLs are not attached to errors.
+	_, err = sqlclient.Open(context.Background(), " postgres://user:pass:3306/")
+	require.EqualError(t, err, "sql/sqlclient: parse open url: first path segment in URL cannot contain colon")
+	_, err = sqlclient.Open(context.Background(), "scheme://hello world")
+	require.EqualError(t, err, `sql/sqlclient: parse open url: invalid character " " in host name`)
+}
+
+func TestParseURL(t *testing.T) {
+	_, err := sqlclient.ParseURL("boring ://")
+	require.EqualError(t, err, "first path segment in URL cannot contain colon")
+	_, err = sqlclient.ParseURL("\bboring://foo.com:3000")
+	require.EqualError(t, err, "net/url: invalid control character in URL")
+	_, err = sqlclient.ParseURL("boring:// : @foo.com:3000")
+	require.EqualError(t, err, "net/url: invalid userinfo")
 }
 
 func TestClient_AddClosers(t *testing.T) {