Skip to content

Commit

Permalink
Remove bluemonday and fix double-escaping
Browse files Browse the repository at this point in the history
  • Loading branch information
david-bezero authored and paskal committed Nov 30, 2023
1 parent d481655 commit 4480c33
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 41 deletions.
4 changes: 0 additions & 4 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ require (
github.com/go-pkgz/repeater v1.1.3
github.com/go-pkgz/rest v1.17.0
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.7
Expand All @@ -21,12 +20,10 @@ require (
require (
cloud.google.com/go/compute v1.23.1 // 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.0 // indirect
github.com/klauspost/compress v1.17.1 // indirect
github.com/montanaflynn/stats v0.7.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
Expand All @@ -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.14.0 // indirect
golang.org/x/net v0.17.0 // indirect
golang.org/x/sync v0.4.0 // indirect
golang.org/x/sys v0.13.0 // indirect
golang.org/x/text v0.13.0 // indirect
Expand Down
6 changes: 0 additions & 6 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down Expand Up @@ -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.0 h1:BQqNyPTi50JCFMTw/b67hByjMVXZRwGha6wxVGkeihY=
github.com/gorilla/css v1.0.0/go.mod h1:Dn721qIggHpt4+EFCcTLTU/vk5ySda2ReITrtgBl60c=
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=
Expand All @@ -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=
Expand Down
21 changes: 12 additions & 9 deletions provider/custom_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package provider

import (
"context"
"encoding/json"
"fmt"
"html/template"
"net"
Expand All @@ -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"
Expand Down Expand Up @@ -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", 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
}
Expand Down
23 changes: 7 additions & 16 deletions provider/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -134,9 +133,7 @@ func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) {
// GET /login?site=site&user=name&address=someone@example.com
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")
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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]
Expand Down
15 changes: 9 additions & 6 deletions provider/verify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -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{
Expand All @@ -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", "/login?address=blah@user.com&user="+badUser+"&site=remark42", http.NoBody)
badData := "<html><script>nasty stuff</script>&lt;escaped&gt;</html>"
req, err := http.NewRequest("GET", "/login?address=blah@user.com&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, "blah@user.com", emailer.to)
assert.Contains(t, emailer.text, "Password : blah@user.com 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+" blah@user.com "+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 : ::blah@user.com", 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+"::blah@user.com", tkn.Handshake.ID)
assert.Equal(t, badData, tkn.Audience)
assert.True(t, tkn.ExpiresAt > tkn.NotBefore)

assert.Equal(t, "test", e.Name())
Expand Down

0 comments on commit 4480c33

Please sign in to comment.