Merge pull request #184 from david-bezero/remove-bluemonday

Remove bluemonday and fix double-escaping

go.mod

@@ -9,7 +9,6 @@ require (
 	github.com/go-pkgz/repeater v1.1.3
 	github.com/go-pkgz/rest v1.18.2
 	github.com/golang-jwt/jwt v3.2.2+incompatible
-	github.com/microcosm-cc/bluemonday v1.0.26
 	github.com/nullrocks/identicon v0.0.0-20180626043057-7875f45b0022
 	github.com/stretchr/testify v1.8.4
 	go.etcd.io/bbolt v1.3.8
@@ -21,12 +20,10 @@ require (
 require (
 	cloud.google.com/go/compute v1.23.3 // indirect
 	cloud.google.com/go/compute/metadata v0.2.3 // indirect
-	github.com/aymerick/douceur v0.2.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/golang/snappy v0.0.4 // indirect
 	github.com/google/uuid v1.1.1 // indirect
-	github.com/gorilla/css v1.0.1 // indirect
 	github.com/klauspost/compress v1.17.4 // indirect
 	github.com/montanaflynn/stats v0.7.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
@@ -43,7 +40,6 @@ require (
 	github.com/xdg-go/stringprep v1.0.4 // indirect
 	github.com/youmark/pkcs8 v0.0.0-20201027041543-1326539a0a0a // indirect
 	golang.org/x/crypto v0.17.0 // indirect
-	golang.org/x/net v0.19.0 // indirect
 	golang.org/x/sync v0.5.0 // indirect
 	golang.org/x/sys v0.15.0 // indirect
 	golang.org/x/text v0.14.0 // indirect

go.sum

@@ -7,8 +7,6 @@ github.com/ajg/form v1.5.1 h1:t9c7v8JUKu/XxOGBU0yjNpaMloxGEJhUkqFRq0ibGeU=
 github.com/ajg/form v1.5.1/go.mod h1:uL1WgH+h2mgNtvBq0339dVnzXdBETtL2LeUXaIv25UY=
 github.com/andybalholm/brotli v1.0.4 h1:V7DdXeJtZscaqfNuAdSRuRFzuiKlHSC/Zh3zl9qY3JY=
 github.com/andybalholm/brotli v1.0.4/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig=
-github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuPk=
-github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
@@ -61,8 +59,6 @@ github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY=
 github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
-github.com/gorilla/css v1.0.1 h1:ntNaBIghp6JmvWnxbZKANoLyuXTPZ4cAMlo6RyhlbO8=
-github.com/gorilla/css v1.0.1/go.mod h1:BvnYkspnSzMmwRK+b8/xgNPLiIuNZr6vbZBTPQ2A3b0=
 github.com/gorilla/websocket v1.4.2 h1:+/TMaTYc4QFitKJxsQ7Yye35DkWvkdLcvGKqM+x0Ufc=
 github.com/gorilla/websocket v1.4.2/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=
 github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
@@ -82,8 +78,6 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/mattn/go-colorable v0.1.7/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
 github.com/mattn/go-isatty v0.0.12/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Kysco4FUpU=
-github.com/microcosm-cc/bluemonday v1.0.26 h1:xbqSvqzQMeEHCqMi64VAs4d8uy6Mequs3rQ0k/Khz58=
-github.com/microcosm-cc/bluemonday v1.0.26/go.mod h1:JyzOCs9gkyQyjs+6h10UEVSe02CGwkhd72Xdqh78TWs=
 github.com/montanaflynn/stats v0.0.0-20171201202039-1bf9dbcd8cbe/go.mod h1:wL8QJuTMNUDYhXwkmfOly8iTdp5TEcJFWZD2D7SIkUc=
 github.com/montanaflynn/stats v0.7.1 h1:etflOAAHORrCC44V+aR6Ftzort912ZU+YLiSTuV8eaE=
 github.com/montanaflynn/stats v0.7.1/go.mod h1:etXPPgVO6n31NxCd9KQUMvCM+ve0ruNzt6R8Bnaayow=

provider/custom_server.go

@@ -2,6 +2,7 @@ package provider
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"html/template"
 	"net"
@@ -12,7 +13,6 @@ import (
 	"time"
 
 	goauth2 "github.com/go-oauth2/oauth2/v4/server"
-	"github.com/microcosm-cc/bluemonday"
 	"golang.org/x/oauth2"
 
 	"github.com/go-pkgz/auth/avatar"
@@ -160,16 +160,19 @@ func (c *CustomServer) handleUserInfo(w http.ResponseWriter, r *http.Request) {
 	}
 	userID := ti.GetUserID()
 
-	p := bluemonday.UGCPolicy()
-	ava := p.Sanitize(fmt.Sprintf(c.URL+"/avatar?user=%s", userID))
-	res := fmt.Sprintf(`{
-					"id": "%s",
-					"name":"%s",
-					"picture":"%s"
-					}`, userID, userID, ava)
+	user := token.User{
+		ID:      userID,
+		Name:    userID,
+		Picture: fmt.Sprintf(c.URL+"/avatar?user=%s", url.QueryEscape(userID)),
+	}
+	res, err := json.Marshal(user)
+	if err != nil {
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
 
 	w.Header().Set("Content-Type", "application/json; charset=utf-8")
-	if _, err := w.Write([]byte(res)); err != nil {
+	if _, err := w.Write(res); err != nil {
 		w.WriteHeader(http.StatusInternalServerError)
 		return
 	}

provider/custom_server_test.go

@@ -43,6 +43,9 @@ func TestCustomProvider(t *testing.T) {
 		L:       logger.Std,
 	}
 
+	var loginUsername string
+	var capturedUser token.User
+
 	sopts := CustomServerOpt{
 		URL:           "http://127.0.0.1:9096",
 		L:             logger.Std,
@@ -61,7 +64,7 @@ func TestCustomProvider(t *testing.T) {
 			jar.SetCookies(u, r.Cookies())
 
 			form := url.Values{}
-			form.Add("username", "admin")
+			form.Add("username", loginUsername)
 			form.Add("password", "pwd1234")
 
 			req, err := http.NewRequest("POST", "", strings.NewReader(form.Encode()))
@@ -87,9 +90,7 @@ func TestCustomProvider(t *testing.T) {
 			claims, err := params.JwtService.Parse(resp.Cookies()[0].Value)
 			assert.NoError(t, err)
 
-			assert.Equal(t, token.User{Name: "admin", ID: "admin",
-				Picture: "http://127.0.0.1:9096/avatar?user=admin", IP: ""}, *claims.User)
-
+			capturedUser = *claims.User
 		},
 	}
 
@@ -120,13 +121,16 @@ func TestCustomProvider(t *testing.T) {
 	client := &http.Client{Jar: jar, Timeout: time.Second * 10}
 
 	// check non-admin, permanent
+	loginUsername = "admin"
 	resp, err := client.Get("http://127.0.0.1:8080/auth/customprov/login?site=my-test-site")
 	require.Nil(t, err)
 	assert.Equal(t, 200, resp.StatusCode)
 	body, err := io.ReadAll(resp.Body)
 	assert.NoError(t, err)
 	t.Logf("resp %s", string(body))
 	t.Logf("headers: %+v", resp.Header)
+	assert.Equal(t, token.User{Name: "admin", ID: "admin",
+		Picture: "http://127.0.0.1:9096/avatar?user=admin", IP: ""}, capturedUser)
 
 	// check avatar
 	resp, err = client.Get("http://127.0.0.1:9096/avatar?user=dev_user")
@@ -137,6 +141,18 @@ func TestCustomProvider(t *testing.T) {
 	assert.Equal(t, 960, len(body))
 	t.Logf("headers: %+v", resp.Header)
 
+	// check malicious user ID
+	loginUsername = "attack"
+	resp, err = client.Get("http://127.0.0.1:8080/auth/customprov/login?site=my-test-site")
+	require.Nil(t, err)
+	assert.Equal(t, 200, resp.StatusCode)
+	body, err = io.ReadAll(resp.Body)
+	assert.NoError(t, err)
+	t.Logf("resp %s", string(body))
+	t.Logf("headers: %+v", resp.Header)
+	// user ID in picture URL is encoded
+	assert.Equal(t, "http://127.0.0.1:9096/avatar?user=none%26attack%3Dvalue%22%3E%3Cscript%3Enasty+stuff%3C%2Fscript%3E", capturedUser.Picture)
+
 	// check default login page
 	prov.LoginPageHandler = nil
 	resp, err = client.Get("http://127.0.0.1:8080/auth/customprov/login?site=my-test-site")
@@ -196,10 +212,13 @@ func initGoauth2Srv(t *testing.T) *goauth2.Server {
 		if r.ParseForm() != nil {
 			return "", fmt.Errorf("no username and password in request")
 		}
-		if r.Form.Get("username") != "admin" || r.Form.Get("password") != "pwd1234" {
-			return "", fmt.Errorf("wrong creds")
+		if r.Form.Get("username") == "admin" && r.Form.Get("password") == "pwd1234" {
+			return "admin", nil
+		}
+		if r.Form.Get("username") == "attack" && r.Form.Get("password") == "pwd1234" {
+			return "none&attack=value\"><script>nasty stuff</script>", nil
 		}
-		return "admin", nil
+		return "", fmt.Errorf("wrong creds")
 	})
 
 	srv.SetInternalErrorHandler(func(err error) (re *errors.Response) {

provider/verify.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/go-pkgz/rest"
 	"github.com/golang-jwt/jwt"
-	"github.com/microcosm-cc/bluemonday"
 
 	"github.com/go-pkgz/auth/avatar"
 	"github.com/go-pkgz/auth/logger"
@@ -134,9 +133,7 @@ func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) {
 // GET /login?site=site&user=name&[email protected]
 func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request) {
 
-	user, address := r.URL.Query().Get("user"), r.URL.Query().Get("address")
-	user = e.sanitize(user)
-	address = e.sanitize(address)
+	user, address, site := r.URL.Query().Get("user"), r.URL.Query().Get("address"), r.URL.Query().Get("site")
 
 	if user == "" || address == "" {
 		rest.SendErrorJSON(w, r, e.L, http.StatusBadRequest, fmt.Errorf("wrong request"), "can't get user and address")
@@ -150,7 +147,7 @@ func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request)
 		},
 		SessionOnly: r.URL.Query().Get("session") != "" && r.URL.Query().Get("session") != "0",
 		StandardClaims: jwt.StandardClaims{
-			Audience:  e.sanitize(r.URL.Query().Get("site")),
+			Audience:  site,
 			ExpiresAt: time.Now().Add(30 * time.Minute).Unix(),
 			NotBefore: time.Now().Add(-1 * time.Minute).Unix(),
 			Issuer:    e.Issuer,
@@ -179,10 +176,10 @@ func (e VerifyHandler) sendConfirmation(w http.ResponseWriter, r *http.Request)
 		Token   string
 		Site    string
 	}{
-		User:    user,
-		Address: address,
+		User:    trim(user),
+		Address: trim(address),
 		Token:   tkn,
-		Site:    r.URL.Query().Get("site"),
+		Site:    site,
 	}
 	buf := bytes.Buffer{}
 	if err = emailTmpl.Execute(&buf, tmplData); err != nil {
@@ -212,14 +209,8 @@ Confirmation for {{.User}} {{.Address}}, site {{.Site}}
 Token: {{.Token}}
 `
 
-func (e VerifyHandler) sanitize(inp string) string {
-	p := bluemonday.UGCPolicy()
-	res := p.Sanitize(inp)
-	res = template.HTMLEscapeString(res)
-	res = strings.ReplaceAll(res, "&", "&")
-	res = strings.ReplaceAll(res, """, "\"")
-	res = strings.ReplaceAll(res, "'", "'")
-	res = strings.ReplaceAll(res, "\n", "")
+func trim(inp string) string {
+	res := strings.ReplaceAll(inp, "\n", "")
 	res = strings.TrimSpace(res)
 	if len(res) > 128 {
 		return res[:128]

provider/verify_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
@@ -59,7 +60,7 @@ func TestVerifyHandler_LoginSendConfirm(t *testing.T) {
 	assert.Equal(t, "test", e.Name())
 }
 
-func TestVerifyHandler_LoginSendConfirmRejected(t *testing.T) {
+func TestVerifyHandler_LoginSendConfirmEscapesBadInput(t *testing.T) {
 
 	emailer := mockSender{}
 	e := VerifyHandler{
@@ -77,20 +78,22 @@ func TestVerifyHandler_LoginSendConfirmRejected(t *testing.T) {
 
 	handler := http.HandlerFunc(e.LoginHandler)
 	rr := httptest.NewRecorder()
-	badUser := "%3C%21DOCTYPE%20html%3E%20%0A%3Chtml%3E%20%0A%3Chead%3E%0A%3Cmeta%20name%3D%22viewport%22%20content%3D%22width%3Ddevice-width%2C%20initial-scale%3D1%22%3E%0A%3Ctitle%3E%20Login%20Page%20%3C%2Ftitle%3E%0A%3Cstyle%3E%20%0ABody%20%7B%0A%20%20font-family%3A%20Calibri%2C%20Helvetica%2C%20sans-serif%3B%0A%20%20background-color%3A%20pink%3B%0A%7D%0Abutton%20%7B%20%0A%20%20%20%20%20%20%20background-color%3A%20%234CAF50%3B%20%0A%20%20%20%20%20%20%20width%3A%20100%25%3B%0A%20%20%20%20%20%20%20%20color%3A%20orange%3B%20%0A%20%20%20%20%20%20%20%20padding%3A%2015px%3B%20%0A%20%20%20%20%20%20%20%20margin%3A%2010px%200px%3B%20%0A%20%20%20%20%20%20%20%20border%3A%20none%3B%20%0A%20%20%20%20%20%20%20%20cursor%3A%20pointer%3B%20%0A%20%20%20%20%20%20%20%20%20%7D%20%0A%20form%20%7B%20%0A%20%20%20%20%20%20%20%20border%3A%203px%20solid%20%23f1f1f1%3B%20%0A%20%20%20%20%7D%20%0A%20input%5Btype%3Dtext%5D%2C%20input%5Btype%3Dpassword%5D%20%7B%20%0A%20%20%20%20%20%20%20%20width%3A%20100%25%3B%20%0A%20%20%20%20%20%20%20%20margin%3A%208px%200%3B%0A%20%20%20%20%20%20%20%20padding%3A%2012px%2020px%3B%20%0A%20%20%20%20%20%20%20%20display%3A%20inline-block%3B%20%0A%20%20%20%20%20%20%20%20border%3A%202px%20solid%20green%3B%20%0A%20%20%20%20%20%20%20%20box-sizing%3A%20border-box%3B%20%0A%20%20%20%20%7D%0A%20button%3Ahover%20%7B%20%0A%20%20%20%20%20%20%20%20opacity%3A%200.7%3B%20%0A%20%20%20%20%7D%20%0A%20%20.cancelbtn%20%7B%20%0A%20%20%20%20%20%20%20%20width%3A%20auto%3B%20%0A%20%20%20%20%20%20%20%20padding%3A%2010px%2018px%3B%0A%20%20%20%20%20%20%20%20margin%3A%2010px%205px%3B%0A%20%20%20%20%7D%20%0A%20%20%20%20%20%20%0A%20%20%20%0A%20.container%20%7B%20%0A%20%20%20%20%20%20%20%20padding%3A%2025px%3B%20%0A%20%20%20%20%20%20%20%20background-color%3A%20lightblue%3B%0A%20%20%20%20%7D%20%0A%3C%2Fstyle%3E%20%0A%3C%2Fhead%3E%20%20%0A%3Cbody%3E%20%20%0A%20%20%20%20%3Ccenter%3E%20%3Ch1%3E%20Student%20Login%20Form%20%3C%2Fh1%3E%20%3C%2Fcenter%3E%20%0A%20%20%20%20%3Cform%3E%0A%20%20%20%20%20%20%20%20%3Cdiv%20class%3D%22container%22%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Clabel%3EUsername%20%3A%20%3C%2Flabel%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22text%22%20placeholder%3D%22Enter%20Username%22%20name%3D%22username%22%20required%3E%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Clabel%3EPassword%20%3A%20%3C%2Flabel%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22password%22%20placeholder%3D%22Enter%20Password%22%20name%3D%22password%22%20required%3E%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cbutton%20type%3D%22submit%22%3ELogin%3C%2Fbutton%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22checkbox%22%20checked%3D%22checked%22%3E%20Remember%20me%20%0A%20%20%20%20%20%20%20%20%20%20%20%20%3Cbutton%20type%3D%22button%22%20class%3D%22cancelbtn%22%3E%20Cancel%3C%2Fbutton%3E%20%0A%20%20%20%20%20%20%20%20%20%20%20%20Forgot%20%3Ca%20href%3D%22%23%22%3E%20password%3F%20%3C%2Fa%3E%20%0A%20%20%20%20%20%20%20%20%3C%2Fdiv%3E%20%0A%20%20%20%20%3C%2Fform%3E%20%20%20%0A%3C%2Fbody%3E%20%20%20%0A%3C%2Fhtml%3E%0A%0A%20%0A"
-	req, err := http.NewRequest("GET", "/[email protected]&user="+badUser+"&site=remark42", http.NoBody)
+	badData := "<html><script>nasty stuff</script>&lt;escaped&gt;</html>"
+	req, err := http.NewRequest("GET", "/[email protected]&user="+url.QueryEscape(badData)+"&site="+url.QueryEscape(badData), http.NoBody)
 	require.NoError(t, err)
 	handler.ServeHTTP(rr, req)
 	assert.Equal(t, 200, rr.Code)
 	assert.Equal(t, "[email protected]", emailer.to)
-	assert.Contains(t, emailer.text, "Password :    [email protected] remark42 token:")
+	expectedEscaped := "&lt;html&gt;&lt;script&gt;nasty stuff&lt;/script&gt;&amp;lt;escaped&amp;gt;&lt;/html&gt;"
+	assert.Contains(t, emailer.text, expectedEscaped+" [email protected] "+expectedEscaped+" token:")
 
 	tknStr := strings.Split(emailer.text, " token:")[1]
 	tkn, err := e.TokenService.Parse(tknStr)
 	assert.NoError(t, err)
 	t.Logf("%s %+v", tknStr, tkn)
-	assert.Equal(t, "&lt;h1&gt; Student Login Form &lt;/h1&gt;              &lt;div&gt;             Username :                          Password :   ::[email protected]", tkn.Handshake.ID)
-	assert.Equal(t, "remark42", tkn.Audience)
+	// not escaped in these fields as they are not rendered as HTML
+	assert.Equal(t, badData+"::[email protected]", tkn.Handshake.ID)
+	assert.Equal(t, badData, tkn.Audience)
 	assert.True(t, tkn.ExpiresAt > tkn.NotBefore)
 
 	assert.Equal(t, "test", e.Name())