config: validate URLs at config load time #1468

simonpasquier · 2018-07-13T13:02:58Z

brian-brazil · 2018-07-13T13:16:27Z

config/config_test.go

+	if err != nil {
+		t.Fatal(err)
+	}
+	require.Equal(t, "<secret>\n", string(c), "SecretURL not properly elidedin YAML.")


missing space

squat · 2018-07-13T13:28:22Z

notify/impl.go

-		alias  = hashKey(key)
-		alerts = types.Alerts(as...)
+		msg       interface{}
+		apiURL, _ = url.Parse(n.conf.APIURL.String())


For consistency and clarity for future readers we could use same comment here about checking at conf load time

mxinden · 2018-07-13T13:32:18Z

notify/impl.go

@@ -759,8 +763,10 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 		data     = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...)
 		tmplText = tmplText(n.tmpl, data, &err)
 		tmplHTML = tmplHTML(n.tmpl, data, &err)
-		url      = fmt.Sprintf("%sv2/room/%s/notification?auth_token=%s", n.conf.APIURL, n.conf.RoomID, n.conf.AuthToken)
+		// n.conf.APIURL is already checked at configuration load time.
+		url, _ = url.Parse(n.conf.APIURL.String())


I am a bit confused here, why not just n.conf.APIURL is this not already a URL?

n.conf.APIURL is a pointer so I didn't want to modify it.

you could also do

u := *c.conf.APIURL u.Path += fmt.Sprintf("v2/room/%s/notification?auth_token=%s", n.conf.RoomID, n.conf.AuthToken) // ... resp, err := ctxhttp.Post(ctx, c, u.String(), contentTypeJSON, &buf)

since we've assigned something to variable name url, we've shadowed the reference to the package net/url in this scope. Having url's named u is annoying, but at some point someone will want to use net/url later in this package..

mxinden · 2018-07-13T13:40:12Z

@simonpasquier There are a couple more URL strings inside notifier.go. Would it make sense to replace them as well?

simonpasquier · 2018-07-13T13:43:08Z

@mxinden absolutely! Not sure how I managed to miss them...

brian-brazil · 2018-07-13T13:46:31Z

The pushover url is a template, not a url.

simonpasquier · 2018-07-13T13:49:49Z

@brian-brazil yep, I replied too fast. This isn't the Pushover API URL...

stuartnelson3

couple relatively unimportant comments

stuartnelson3 · 2018-07-17T08:05:00Z

notify/impl.go

@@ -759,8 +763,10 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 		data     = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...)
 		tmplText = tmplText(n.tmpl, data, &err)
 		tmplHTML = tmplHTML(n.tmpl, data, &err)
-		url      = fmt.Sprintf("%sv2/room/%s/notification?auth_token=%s", n.conf.APIURL, n.conf.RoomID, n.conf.AuthToken)
+		// n.conf.APIURL is already checked at configuration load time.
+		url, _ = url.Parse(n.conf.APIURL.String())


you could also do

u := *c.conf.APIURL u.Path += fmt.Sprintf("v2/room/%s/notification?auth_token=%s", n.conf.RoomID, n.conf.AuthToken) // ... resp, err := ctxhttp.Post(ctx, c, u.String(), contentTypeJSON, &buf)

stuartnelson3 · 2018-07-17T08:07:35Z

notify/impl.go

@@ -759,8 +763,10 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 		data     = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...)
 		tmplText = tmplText(n.tmpl, data, &err)
 		tmplHTML = tmplHTML(n.tmpl, data, &err)
-		url      = fmt.Sprintf("%sv2/room/%s/notification?auth_token=%s", n.conf.APIURL, n.conf.RoomID, n.conf.AuthToken)
+		// n.conf.APIURL is already checked at configuration load time.
+		url, _ = url.Parse(n.conf.APIURL.String())


since we've assigned something to variable name url, we've shadowed the reference to the package net/url in this scope. Having url's named u is annoying, but at some point someone will want to use net/url later in this package..

Signed-off-by: Simon Pasquier <[email protected]>

simonpasquier · 2018-07-24T15:48:42Z

notify/impl.go

@@ -802,8 +802,7 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 		tmplText = tmplText(n.tmpl, data, &err)
 		tmplHTML = tmplHTML(n.tmpl, data, &err)
 		roomid   = tmplText(n.conf.RoomID)
-		// n.conf.APIURL is already checked at configuration load time.
-		url, _ = url.Parse(n.conf.APIURL.String())
+		url      = *n.conf.APIURL


This isn't a deep copy, needs to be fixed.

simonpasquier · 2018-07-24T15:49:28Z

notify/impl.go

@@ -798,8 +802,9 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 		tmplText = tmplText(n.tmpl, data, &err)
 		tmplHTML = tmplHTML(n.tmpl, data, &err)
 		roomid   = tmplText(n.conf.RoomID)
-		url      = fmt.Sprintf("%sv2/room/%s/notification?auth_token=%s", n.conf.APIURL, roomid, n.conf.AuthToken)
+		apiURL   = *n.conf.APIURL


This isn't a deep copy, needs to be fixed.

Signed-off-by: Simon Pasquier <[email protected]>

* config: validate URLs at config load time Signed-off-by: Simon Pasquier <[email protected]> * Address Brian and Lucas comments Signed-off-by: Simon Pasquier <[email protected]> * Shallow copy of URL instead of reparsing it Signed-off-by: Simon Pasquier <[email protected]> * Unshadow net/url package Signed-off-by: Simon Pasquier <[email protected]> * Make a deep-copy of URL struct Signed-off-by: Simon Pasquier <[email protected]>

simonpasquier requested review from stuartnelson3 and brian-brazil July 13, 2018 13:02

brian-brazil reviewed Jul 13, 2018

View reviewed changes

squat reviewed Jul 13, 2018

View reviewed changes

mxinden reviewed Jul 13, 2018

View reviewed changes

stuartnelson3 reviewed Jul 17, 2018

View reviewed changes

simonpasquier added 3 commits July 24, 2018 13:54

config: validate URLs at config load time

132fd5f

Signed-off-by: Simon Pasquier <[email protected]>

Address Brian and Lucas comments

348dcba

Signed-off-by: Simon Pasquier <[email protected]>

Shallow copy of URL instead of reparsing it

a078112

Signed-off-by: Simon Pasquier <[email protected]>

simonpasquier force-pushed the sanitize-urls branch from 6570755 to b5001b3 Compare July 24, 2018 11:59

Unshadow net/url package

353e2b9

Signed-off-by: Simon Pasquier <[email protected]>

simonpasquier force-pushed the sanitize-urls branch from b5001b3 to 353e2b9 Compare July 24, 2018 13:21

simonpasquier commented Jul 24, 2018

View reviewed changes

Make a deep-copy of URL struct

d440d3a

Signed-off-by: Simon Pasquier <[email protected]>

mxinden approved these changes Jul 26, 2018

View reviewed changes

stuartnelson3 approved these changes Jul 26, 2018

View reviewed changes

stuartnelson3 merged commit 0ccc7c9 into prometheus:master Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: validate URLs at config load time #1468

config: validate URLs at config load time #1468

simonpasquier commented Jul 13, 2018

brian-brazil Jul 13, 2018

squat Jul 13, 2018

mxinden Jul 13, 2018

simonpasquier Jul 13, 2018

stuartnelson3 Jul 17, 2018

stuartnelson3 Jul 17, 2018

mxinden commented Jul 13, 2018

simonpasquier commented Jul 13, 2018

brian-brazil commented Jul 13, 2018

simonpasquier commented Jul 13, 2018

stuartnelson3 left a comment

stuartnelson3 Jul 17, 2018

stuartnelson3 Jul 17, 2018

simonpasquier Jul 24, 2018

simonpasquier Jul 24, 2018

config: validate URLs at config load time #1468

config: validate URLs at config load time #1468

Conversation

simonpasquier commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Jul 13, 2018

simonpasquier commented Jul 13, 2018

brian-brazil commented Jul 13, 2018

simonpasquier commented Jul 13, 2018

stuartnelson3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment