Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix URL query encoding #78

Merged
merged 2 commits into from
Dec 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
// one time passcodes in a Google Authenticator compatible manner.
//
// When adding a TOTP for a user, you must store the "secret" value
// persistently. It is recommend to store the secret in an encrypted field in your
// persistently. It is recommended to store the secret in an encrypted field in your
// datastore. Due to how TOTP works, it is not possible to store a hash
// for the secret value like you would a password.
//
Expand Down Expand Up @@ -57,6 +57,7 @@
//
// Validating a TOTP passcode is very easy, just prompt the user for a passcode
// and retrieve the associated user's previously stored secret.
//
// import "github.com/pquerna/otp/totp"
//
// passcode := promptForPasscode()
Expand Down
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
github.com/boombuler/barcode v1.0.0 h1:s1TvRnXwL2xJRaccrdcBQMZxq6X7DvsMogtmJeHDdrc=
github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8=
github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc h1:biVzkmvwrH8WK8raXaxBx6fRVTlJILwEwQGL1I/ByEI=
github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8=
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
Expand Down
5 changes: 3 additions & 2 deletions hotp/hotp.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package hotp

import (
"github.com/pquerna/otp"
"github.com/pquerna/otp/internal"
"io"

"crypto/hmac"
Expand Down Expand Up @@ -186,7 +187,7 @@ func Generate(opts GenerateOpts) (*otp.Key, error) {
opts.Rand = rand.Reader
}

// otpauth://totp/Example:alice@google.com?secret=JBSWY3DPEHPK3PXP&issuer=Example
// otpauth://hotp/Example:alice@google.com?secret=JBSWY3DPEHPK3PXP&issuer=Example

v := url.Values{}
if len(opts.Secret) != 0 {
Expand All @@ -208,7 +209,7 @@ func Generate(opts GenerateOpts) (*otp.Key, error) {
Scheme: "otpauth",
Host: "hotp",
Path: "/" + opts.Issuer + ":" + opts.AccountName,
RawQuery: v.Encode(),
RawQuery: internal.EncodeQuery(v),
}

return otp.NewKeyFromURL(u.String())
Expand Down
17 changes: 12 additions & 5 deletions hotp/hotp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,17 +148,24 @@ func TestGenerate(t *testing.T) {
Issuer: "SnakeOil",
AccountName: "alice@example.com",
})
require.NoError(t, err, "generate basic TOTP")
require.NoError(t, err, "generate basic HOTP")
require.Equal(t, "SnakeOil", k.Issuer(), "Extracting Issuer")
require.Equal(t, "alice@example.com", k.AccountName(), "Extracting Account Name")
require.Equal(t, 16, len(k.Secret()), "Secret is 16 bytes long as base32.")

k, err = Generate(GenerateOpts{
Issuer: "Snake Oil",
AccountName: "alice@example.com",
})
require.NoError(t, err, "issuer with a space in the name")
require.Contains(t, k.String(), "issuer=Snake%20Oil")

k, err = Generate(GenerateOpts{
Issuer: "SnakeOil",
AccountName: "alice@example.com",
SecretSize: 20,
})
require.NoError(t, err, "generate larger TOTP")
require.NoError(t, err, "generate larger HOTP")
require.Equal(t, 32, len(k.Secret()), "Secret is 32 bytes long as base32.")

k, err = Generate(GenerateOpts{
Expand All @@ -178,9 +185,9 @@ func TestGenerate(t *testing.T) {
k, err = Generate(GenerateOpts{
Issuer: "SnakeOil",
AccountName: "alice@example.com",
SecretSize: 17, // anything that is not divisable by 5, really
SecretSize: 17, // anything that is not divisible by 5, really
})
require.NoError(t, err, "Secret size is valid when length not divisable by 5.")
require.NoError(t, err, "Secret size is valid when length not divisible by 5.")
require.NotContains(t, k.Secret(), "=", "Secret has no escaped characters.")

k, err = Generate(GenerateOpts{
Expand All @@ -190,6 +197,6 @@ func TestGenerate(t *testing.T) {
})
require.NoError(t, err, "Secret generation failed")
sec, err := b32NoPadding.DecodeString(k.Secret())
require.NoError(t, err, "Secret wa not valid base32")
require.NoError(t, err, "Secret was not valid base32")
require.Equal(t, sec, []byte("helloworld"), "Specified Secret was not kept")
}
35 changes: 35 additions & 0 deletions internal/encode.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package internal

import (
"net/url"
"sort"
"strings"
)

// EncodeQuery is a copy-paste of url.Values.Encode, except it uses %20 instead
// of + to encode spaces. This is necessary to correctly render spaces in some
// authenticator apps, like Google Authenticator.
func EncodeQuery(v url.Values) string {
if v == nil {
return ""
}
var buf strings.Builder
keys := make([]string, 0, len(v))
for k := range v {
keys = append(keys, k)
}
sort.Strings(keys)
for _, k := range keys {
vs := v[k]
keyEscaped := url.PathEscape(k) // changed from url.QueryEscape
for _, v := range vs {
if buf.Len() > 0 {
buf.WriteByte('&')
}
buf.WriteString(keyEscaped)
buf.WriteByte('=')
buf.WriteString(url.PathEscape(v)) // changed from url.QueryEscape
}
}
return buf.String()
}
3 changes: 2 additions & 1 deletion totp/totp.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package totp
import (
"github.com/pquerna/otp"
"github.com/pquerna/otp/hotp"
"github.com/pquerna/otp/internal"
"io"

"crypto/rand"
Expand Down Expand Up @@ -199,7 +200,7 @@ func Generate(opts GenerateOpts) (*otp.Key, error) {
Scheme: "otpauth",
Host: "totp",
Path: "/" + opts.Issuer + ":" + opts.AccountName,
RawQuery: v.Encode(),
RawQuery: internal.EncodeQuery(v),
}

return otp.NewKeyFromURL(u.String())
Expand Down
11 changes: 9 additions & 2 deletions totp/totp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ func TestGenerate(t *testing.T) {
require.Equal(t, "alice@example.com", k.AccountName(), "Extracting Account Name")
require.Equal(t, 32, len(k.Secret()), "Secret is 32 bytes long as base32.")

k, err = Generate(GenerateOpts{
Issuer: "Snake Oil",
AccountName: "alice@example.com",
})
require.NoError(t, err, "issuer with a space in the name")
require.Contains(t, k.String(), "issuer=Snake%20Oil")

k, err = Generate(GenerateOpts{
Issuer: "SnakeOil",
AccountName: "alice@example.com",
Expand All @@ -139,9 +146,9 @@ func TestGenerate(t *testing.T) {
k, err = Generate(GenerateOpts{
Issuer: "SnakeOil",
AccountName: "alice@example.com",
SecretSize: 13, // anything that is not divisable by 5, really
SecretSize: 13, // anything that is not divisible by 5, really
})
require.NoError(t, err, "Secret size is valid when length not divisable by 5.")
require.NoError(t, err, "Secret size is valid when length not divisible by 5.")
require.NotContains(t, k.Secret(), "=", "Secret has no escaped characters.")

k, err = Generate(GenerateOpts{
Expand Down