Skip to content

Commit 174198e

Browse files
hfhf
authored
feat: cover 100% of crypto with tests (#1892)
Covers a 100% of the `crypto` package with tests.
1 parent 0cfd177 commit 174198e

File tree

14 files changed

+325
-251
lines changed

14 files changed

+325
-251
lines changed

‎Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ migrate_test: ## Run database migrations for test.
3636

3737
test: build ## Run tests.
3838
go test $(CHECK_FILES) -coverprofile=coverage.out -coverpkg ./... -p 1 -race -v -count=1
39+
./hack/coverage.sh
3940

4041
vet: # Vet the code
4142
go vet $(CHECK_FILES)

‎hack/coverage.sh

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
FAIL=false
2+
3+
for PKG in "crypto"
4+
do
5+
UNCOVERED_FUNCS=$(go tool cover -func=coverage.out | grep "^github.com/supabase/auth/internal/$PKG/" | grep -v '100.0%$')
6+
UNCOVERED_FUNCS_COUNT=$(echo "$UNCOVERED_FUNCS" | wc -l)
7+
8+
if [ "$UNCOVERED_FUNCS_COUNT" -gt 1 ] # wc -l counts +1 line
9+
then
10+
echo "Package $PKG not covered 100% with tests. $UNCOVERED_FUNCS_COUNT functions need more tests. This is mandatory."
11+
echo "$UNCOVERED_FUNCS"
12+
FAIL=true
13+
fi
14+
done
15+
16+
if [ "$FAIL" = "true" ]
17+
then
18+
exit 1
19+
else
20+
exit 0
21+
fi

‎internal/api/hooks.go

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,12 @@ import (
1414
"time"
1515

1616
"github.com/gofrs/uuid"
17-
"github.com/supabase/auth/internal/observability"
17+
"github.com/sirupsen/logrus"
18+
standardwebhooks "github.com/standard-webhooks/standard-webhooks/libraries/go"
1819

1920
"github.com/supabase/auth/internal/conf"
20-
"github.com/supabase/auth/internal/crypto"
21-
22-
"github.com/sirupsen/logrus"
2321
"github.com/supabase/auth/internal/hooks"
24-
22+
"github.com/supabase/auth/internal/observability"
2523
"github.com/supabase/auth/internal/storage"
2624
)
2725

@@ -103,7 +101,7 @@ func (a *API) runHTTPHook(r *http.Request, hookConfig conf.ExtensibilityPointCon
103101
}
104102
msgID := uuid.Must(uuid.NewV4())
105103
currentTime := time.Now()
106-
signatureList, err := crypto.GenerateSignatures(hookConfig.HTTPHookSecrets, msgID, currentTime, inputPayload)
104+
signatureList, err := generateSignatures(hookConfig.HTTPHookSecrets, msgID, currentTime, inputPayload)
107105
if err != nil {
108106
return nil, err
109107
}
@@ -382,3 +380,26 @@ func (a *API) runHook(r *http.Request, conn *storage.Connection, hookConfig conf
382380

383381
return response, nil
384382
}
383+
384+
func generateSignatures(secrets []string, msgID uuid.UUID, currentTime time.Time, inputPayload []byte) ([]string, error) {
385+
SymmetricSignaturePrefix := "v1,"
386+
// TODO(joel): Handle asymmetric case once library has been upgraded
387+
var signatureList []string
388+
for _, secret := range secrets {
389+
if strings.HasPrefix(secret, SymmetricSignaturePrefix) {
390+
trimmedSecret := strings.TrimPrefix(secret, SymmetricSignaturePrefix)
391+
wh, err := standardwebhooks.NewWebhook(trimmedSecret)
392+
if err != nil {
393+
return nil, err
394+
}
395+
signature, err := wh.Sign(msgID.String(), currentTime, inputPayload)
396+
if err != nil {
397+
return nil, err
398+
}
399+
signatureList = append(signatureList, signature)
400+
} else {
401+
return nil, errors.New("invalid signature format")
402+
}
403+
}
404+
return signatureList, nil
405+
}

‎internal/api/magic_link.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error {
8383
if isNewUser {
8484
// User either doesn't exist or hasn't completed the signup process.
8585
// Sign them up with temporary password.
86-
password, err := crypto.GeneratePassword(config.Password.RequiredCharacters, 33)
87-
if err != nil {
88-
return internalServerError("error creating user").WithInternalError(err)
89-
}
86+
password := crypto.GeneratePassword(config.Password.RequiredCharacters, 33)
9087

9188
signUpParams := &SignupParams{
9289
Email: params.Email,

‎internal/api/mail.go

Lines changed: 20 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,7 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
8787

8888
var url string
8989
now := time.Now()
90-
otp, err := crypto.GenerateOtp(config.Mailer.OtpLength)
91-
if err != nil {
92-
// OTP generation must always succeed
93-
panic(err)
94-
}
90+
otp := crypto.GenerateOtp(config.Mailer.OtpLength)
9591

9692
hashedToken := crypto.GenerateTokenHash(params.Email, otp)
9793

@@ -300,19 +296,18 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
300296
}
301297

302298
func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *models.User, flowType models.FlowType) error {
299+
var err error
300+
303301
config := a.config
304302
maxFrequency := config.SMTP.MaxFrequency
305303
otpLength := config.Mailer.OtpLength
306304

307-
if err := validateSentWithinFrequencyLimit(u.ConfirmationSentAt, maxFrequency); err != nil {
305+
if err = validateSentWithinFrequencyLimit(u.ConfirmationSentAt, maxFrequency); err != nil {
308306
return err
309307
}
310308
oldToken := u.ConfirmationToken
311-
otp, err := crypto.GenerateOtp(otpLength)
312-
if err != nil {
313-
// OTP generation must succeeed
314-
panic(err)
315-
}
309+
otp := crypto.GenerateOtp(otpLength)
310+
316311
token := crypto.GenerateTokenHash(u.GetEmail(), otp)
317312
u.ConfirmationToken = addFlowPrefixToToken(token, flowType)
318313
now := time.Now()
@@ -342,11 +337,8 @@ func (a *API) sendInvite(r *http.Request, tx *storage.Connection, u *models.User
342337
otpLength := config.Mailer.OtpLength
343338
var err error
344339
oldToken := u.ConfirmationToken
345-
otp, err := crypto.GenerateOtp(otpLength)
346-
if err != nil {
347-
// OTP generation must succeed
348-
panic(err)
349-
}
340+
otp := crypto.GenerateOtp(otpLength)
341+
350342
u.ConfirmationToken = crypto.GenerateTokenHash(u.GetEmail(), otp)
351343
now := time.Now()
352344
if err = a.sendEmail(r, tx, u, mail.InviteVerification, otp, "", u.ConfirmationToken); err != nil {
@@ -382,15 +374,12 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m
382374
}
383375

384376
oldToken := u.RecoveryToken
385-
otp, err := crypto.GenerateOtp(otpLength)
386-
if err != nil {
387-
// OTP generation must succeed
388-
panic(err)
389-
}
377+
otp := crypto.GenerateOtp(otpLength)
378+
390379
token := crypto.GenerateTokenHash(u.GetEmail(), otp)
391380
u.RecoveryToken = addFlowPrefixToToken(token, flowType)
392381
now := time.Now()
393-
if err = a.sendEmail(r, tx, u, mail.RecoveryVerification, otp, "", u.RecoveryToken); err != nil {
382+
if err := a.sendEmail(r, tx, u, mail.RecoveryVerification, otp, "", u.RecoveryToken); err != nil {
394383
u.RecoveryToken = oldToken
395384
if errors.Is(err, EmailRateLimitExceeded) {
396385
return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
@@ -422,11 +411,8 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u
422411
}
423412

424413
oldToken := u.ReauthenticationToken
425-
otp, err := crypto.GenerateOtp(otpLength)
426-
if err != nil {
427-
// OTP generation must succeed
428-
panic(err)
429-
}
414+
otp := crypto.GenerateOtp(otpLength)
415+
430416
u.ReauthenticationToken = crypto.GenerateTokenHash(u.GetEmail(), otp)
431417
now := time.Now()
432418

@@ -452,6 +438,7 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u
452438
}
453439

454440
func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.User, flowType models.FlowType) error {
441+
var err error
455442
config := a.config
456443
otpLength := config.Mailer.OtpLength
457444

@@ -462,11 +449,8 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U
462449
}
463450

464451
oldToken := u.RecoveryToken
465-
otp, err := crypto.GenerateOtp(otpLength)
466-
if err != nil {
467-
// OTP generation must succeed
468-
panic(err)
469-
}
452+
otp := crypto.GenerateOtp(otpLength)
453+
470454
token := crypto.GenerateTokenHash(u.GetEmail(), otp)
471455
u.RecoveryToken = addFlowPrefixToToken(token, flowType)
472456

@@ -501,22 +485,16 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models
501485
return err
502486
}
503487

504-
otpNew, err := crypto.GenerateOtp(otpLength)
505-
if err != nil {
506-
// OTP generation must succeed
507-
panic(err)
508-
}
488+
otpNew := crypto.GenerateOtp(otpLength)
489+
509490
u.EmailChange = email
510491
token := crypto.GenerateTokenHash(u.EmailChange, otpNew)
511492
u.EmailChangeTokenNew = addFlowPrefixToToken(token, flowType)
512493

513494
otpCurrent := ""
514495
if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" {
515-
otpCurrent, err = crypto.GenerateOtp(otpLength)
516-
if err != nil {
517-
// OTP generation must succeed
518-
panic(err)
519-
}
496+
otpCurrent = crypto.GenerateOtp(otpLength)
497+
520498
currentToken := crypto.GenerateTokenHash(u.GetEmail(), otpCurrent)
521499
u.EmailChangeTokenCurrent = addFlowPrefixToToken(currentToken, flowType)
522500
}

‎internal/api/mfa.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -393,10 +393,9 @@ func (a *API) challengePhoneFactor(w http.ResponseWriter, r *http.Request) error
393393
return tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(factor.LastChallengedAt, config.MFA.Phone.MaxFrequency))
394394
}
395395
}
396-
otp, err := crypto.GenerateOtp(config.MFA.Phone.OtpLength)
397-
if err != nil {
398-
panic(err)
399-
}
396+
397+
otp := crypto.GenerateOtp(config.MFA.Phone.OtpLength)
398+
400399
challenge, err := factor.CreatePhoneChallenge(ipAddress, otp, config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey)
401400
if err != nil {
402401
return internalServerError("error creating SMS Challenge")

‎internal/api/mfa_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() {
525525
} else if v.factorType == models.Phone {
526526
friendlyName := uuid.Must(uuid.NewV4()).String()
527527
numDigits := 10
528-
otp, err := crypto.GenerateOtp(numDigits)
528+
otp := crypto.GenerateOtp(numDigits)
529529
require.NoError(ts.T(), err)
530530
phone := fmt.Sprintf("+%s", otp)
531531
f = models.NewPhoneFactor(ts.TestUser, phone, friendlyName)

‎internal/api/phone.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use
7777
now := time.Now()
7878

7979
var otp, messageID string
80-
var err error
8180

8281
if testOTP, ok := config.Sms.GetTestOTP(phone, now); ok {
8382
otp = testOTP
@@ -93,10 +92,8 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use
9392
return "", tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, "SMS rate limit exceeded")
9493
}
9594
}
96-
otp, err = crypto.GenerateOtp(config.Sms.OtpLength)
97-
if err != nil {
98-
return "", internalServerError("error generating otp").WithInternalError(err)
99-
}
95+
otp = crypto.GenerateOtp(config.Sms.OtpLength)
96+
10097
if config.Hook.SendSMS.Enabled {
10198
input := hooks.SendSMSInput{
10299
User: user,

0 commit comments

Comments
 (0)