From 97a89a1cd8957ba90d08867b0902c56252d6f367 Mon Sep 17 00:00:00 2001 From: briskt <3172830+briskt@users.noreply.github.com> Date: Wed, 17 Sep 2025 14:17:48 +0800 Subject: [PATCH 1/5] new test helper newPasscode() --- apikey_test.go | 14 ++------------ totp_test.go | 12 ++++++++++++ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/apikey_test.go b/apikey_test.go index 1659d33..b6bc1d2 100644 --- a/apikey_test.go +++ b/apikey_test.go @@ -13,7 +13,6 @@ import ( "testing" "time" - uuid "github.com/satori/go.uuid" "golang.org/x/crypto/bcrypt" ) @@ -346,12 +345,7 @@ func (ms *MfaSuite) TestAppRotateApiKey() { key := user.ApiKey must(db.Store(config.ApiKeyTable, key)) - totp := TOTP{ - UUID: uuid.NewV4().String(), - ApiKey: key.Key, - EncryptedTotpKey: mustEncryptLegacy(key, "plain text TOTP key"), - } - must(db.Store(ms.app.GetConfig().TotpTable, totp)) + totp := ms.newPasscode(key) newKey := newTestKey() must(db.Store(config.ApiKeyTable, newKey)) @@ -513,11 +507,7 @@ func (ms *MfaSuite) TestApiKey_ReEncryptTOTPs() { must(newKey.Activate()) must(ms.app.GetDB().Store(ms.app.GetConfig().ApiKeyTable, newKey)) - must(storage.Store(ms.app.GetConfig().TotpTable, TOTP{ - UUID: uuid.NewV4().String(), - ApiKey: oldKey.Key, - EncryptedTotpKey: mustEncryptLegacy(oldKey, "plain text TOTP key"), - })) + _ = ms.newPasscode(oldKey) complete, incomplete, err := newKey.ReEncryptTOTPs(storage, oldKey) ms.NoError(err) diff --git a/totp_test.go b/totp_test.go index 0bc1eb5..cdf97e5 100644 --- a/totp_test.go +++ b/totp_test.go @@ -7,6 +7,8 @@ import ( "net/http" "net/http/httptest" "strings" + + uuid "github.com/satori/go.uuid" ) func (ms *MfaSuite) TestAppCreateTOTP() { @@ -111,3 +113,13 @@ func (ms *MfaSuite) TestNewTOTP() { ms.NoError(err) ms.Equal(got.Key, plainText, "EncryptedTotpKey isn't correct") } + +func (ms *MfaSuite) newPasscode(key ApiKey) TOTP { + totp := TOTP{ + UUID: uuid.NewV4().String(), + ApiKey: key.Key, + EncryptedTotpKey: mustEncryptLegacy(key, "plain text TOTP key"), + } + must(ms.app.db.Store(ms.app.GetConfig().TotpTable, totp)) + return totp +} From 5199daeb31c536b7c116612fd8ed3d2bb2083317 Mon Sep 17 00:00:00 2001 From: briskt <3172830+briskt@users.noreply.github.com> Date: Wed, 17 Sep 2025 14:18:40 +0800 Subject: [PATCH 2/5] implement DELETE /totp/{uuid} endpoint --- README.md | 2 -- api.go | 5 ++++- openapi.yaml | 18 +++++++++++++++ router/main.go | 12 ++++++---- totp.go | 49 +++++++++++++++++++++++++++++++++++++++++ totp_test.go | 59 +++++++++++++++++++++++++++++++++++++++++++++++++- 6 files changed, 137 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 0d99337..a65436f 100644 --- a/README.md +++ b/README.md @@ -46,8 +46,6 @@ This endpoint has not yet been proven in production use. Proceed at your own ris `DELETE /totp/{uuid}` -Coming soon. - ### Validate TOTP Passcode `POST /totp/{uuid}/validate` diff --git a/api.go b/api.go index e724740..c7bf519 100644 --- a/api.go +++ b/api.go @@ -6,7 +6,10 @@ import ( "net/http" ) -const IDParam = "id" +const ( + IDParam = "id" + UUIDParam = "uuid" +) // simpleError is a custom error type that can be JSON-encoded for API responses type simpleError struct { diff --git a/openapi.yaml b/openapi.yaml index 20b0501..2aeff2d 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -687,3 +687,21 @@ paths: example: "otpauth://totp/idp:john_smith?secret=0123456789ABCDEF0123456789ABCDEF&issuer=SIL%20IdP" 401: $ref: "#/components/responses/UnauthorizedError" + /totp/{uuid}: + delete: + summary: Delete a passcode (TOTP) + parameters: + - in: path + name: uuid + schema: + type: string + format: uuid + required: true + description: The unique identifier for the passcode. + responses: + "204": + description: Success + "404": + description: Not found + "401": + $ref: "#/components/responses/UnauthorizedError" diff --git a/router/main.go b/router/main.go index a546385..351056e 100644 --- a/router/main.go +++ b/router/main.go @@ -37,6 +37,14 @@ func getRoutes(app *mfa.App) []route { Pattern: "POST /api-key", HandlerFunc: app.CreateApiKey, }, + { + Pattern: "POST /totp", + HandlerFunc: app.CreateTOTP, + }, + { + Pattern: "DELETE /totp/{" + mfa.UUIDParam + "}", + HandlerFunc: app.DeleteTOTP, + }, { Pattern: "POST /webauthn/register", HandlerFunc: app.BeginRegistration, @@ -65,9 +73,5 @@ func getRoutes(app *mfa.App) []route { Pattern: "DELETE /webauthn/credential/{" + mfa.IDParam + "}/", HandlerFunc: app.DeleteCredential, }, - { - Pattern: "POST /totp", - HandlerFunc: app.CreateTOTP, - }, } } diff --git a/totp.go b/totp.go index b6704d2..8052123 100644 --- a/totp.go +++ b/totp.go @@ -8,12 +8,17 @@ import ( "fmt" "image/png" "io" + "log" "net/http" + "strings" "github.com/google/uuid" "github.com/pquerna/otp/totp" ) +// TOTPTablePK is the primary key in the TOTP DynamoDB table +const TOTPTablePK = "uuid" + // TOTP contains data to represent a Time-based One-Time Passcode (token). The ID and encrypted fields are persisted in // DynamoDB. The others are non-encrypted and are short-lived. type TOTP struct { @@ -39,6 +44,11 @@ type TOTP struct { OTPAuthURL string `dynamodbav:"-" json:"-"` } +// debugString is used by the debugger to show useful TOTP information in watched variables +func (t TOTP) debugString() string { + return fmt.Sprintf("UUID: %s, Key: %s, ApiKey: %s", t.UUID, t.Key, t.ApiKey) +} + // CreateTOTPRequestBody defines the JSON request body for the CreateTOTP endpoint type CreateTOTPRequestBody struct { Issuer string `json:"issuer"` @@ -151,6 +161,45 @@ func newTOTP(db *Storage, apiKey ApiKey, issuer, name string) (TOTP, error) { return t, nil } +// DeleteTOTP is the http handler to delete a passcode. +func (a *App) DeleteTOTP(w http.ResponseWriter, r *http.Request) { + const notFound = "TOTP not found" + + id := r.PathValue(UUIDParam) + + key, err := getAPIKey(r) + if err != nil { + jsonResponse(w, fmt.Errorf("API Key not found in request context: %w", err), http.StatusInternalServerError) + return + } + + var t TOTP + err = a.db.Load(envConfig.TotpTable, TOTPTablePK, id, &t) + if err != nil { + if strings.Contains(err.Error(), "does not exist") { + jsonResponse(w, notFound, http.StatusNotFound) + } else { + log.Printf("error loading TOTP: %s", err) + jsonResponse(w, "Internal server error", http.StatusInternalServerError) + } + return + } + + if key.Key != t.ApiKey { + jsonResponse(w, notFound, http.StatusNotFound) + return + } + + err = a.db.Delete(envConfig.TotpTable, TOTPTablePK, id) + if err != nil { + log.Printf("Failed to delete TOTP: %s", err) + jsonResponse(w, "Failed to delete TOTP", http.StatusInternalServerError) + return + } + + jsonResponse(w, nil, http.StatusNoContent) +} + // authTOTP is a just a placeholder for TOTP. It takes the verified API Key and returns it as an authenticated User // for later use. func authTOTP(apiKey ApiKey) (User, error) { diff --git a/totp_test.go b/totp_test.go index cdf97e5..9e29c1d 100644 --- a/totp_test.go +++ b/totp_test.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "strings" uuid "github.com/satori/go.uuid" @@ -38,7 +39,7 @@ func (ms *MfaSuite) TestAppCreateTOTP() { ms.Run(tt.name, func() { response := httptest.NewRecorder() ms.app.CreateTOTP(response, tt.request) - ms.Equalf(tt.wantStatus, response.Code, "incorrect http status, body %s", response.Body.String()) + ms.Equalf(tt.wantStatus, response.Code, "incorrect http status, response body: %s", response.Body.String()) if tt.wantStatus == http.StatusOK { var responseBody CreateTOTPResponseBody @@ -114,6 +115,62 @@ func (ms *MfaSuite) TestNewTOTP() { ms.Equal(got.Key, plainText, "EncryptedTotpKey isn't correct") } +func (ms *MfaSuite) TestAppDeleteTOTP() { + key := newTestKey() + otherKey := newTestKey() + totp := ms.newPasscode(key) + + ctxWithAPIKey := context.WithValue(context.Background(), UserContextKey, key) + ctxWithOtherAPIKey := context.WithValue(context.Background(), UserContextKey, otherKey) + + requestWithCorrectID := &http.Request{ + Method: http.MethodDelete, + URL: &url.URL{Path: "/totp/" + totp.UUID}, + } + requestWithCorrectID = requestWithCorrectID.WithContext(ctxWithAPIKey) + + requestWithWrongKey := requestWithCorrectID.WithContext(ctxWithOtherAPIKey) + + requestWithWrongUUID := &http.Request{ + Method: http.MethodDelete, + URL: &url.URL{Path: "/totp/" + uuid.NewV4().String()}, + } + requestWithWrongUUID = requestWithWrongUUID.WithContext(ctxWithAPIKey) + + tests := []struct { + name string + request *http.Request + wantStatus int + }{ + { + name: "wrong UUID", + request: requestWithWrongUUID, + wantStatus: http.StatusNotFound, + }, + { + name: "correct UUID, wrong key", + request: requestWithWrongKey, + wantStatus: http.StatusNotFound, + }, + { + name: "correct UUID, correct key", + request: requestWithCorrectID, + wantStatus: http.StatusNoContent, + }, + } + for _, tt := range tests { + ms.Run(tt.name, func() { + mux := &http.ServeMux{} + mux.HandleFunc("DELETE /totp/{"+UUIDParam+"}", ms.app.DeleteTOTP) + + response := httptest.NewRecorder() + mux.ServeHTTP(response, tt.request) + + ms.Equalf(tt.wantStatus, response.Code, "incorrect http status, response body: %s", response.Body.String()) + }) + } +} + func (ms *MfaSuite) newPasscode(key ApiKey) TOTP { totp := TOTP{ UUID: uuid.NewV4().String(), From 2a89fb04e25022569e334ec773a30e8013843a8f Mon Sep 17 00:00:00 2001 From: briskt <3172830+briskt@users.noreply.github.com> Date: Thu, 18 Sep 2025 11:57:47 +0800 Subject: [PATCH 3/5] don't send error detail to client --- totp.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/totp.go b/totp.go index 8052123..cb4140c 100644 --- a/totp.go +++ b/totp.go @@ -164,12 +164,14 @@ func newTOTP(db *Storage, apiKey ApiKey, issuer, name string) (TOTP, error) { // DeleteTOTP is the http handler to delete a passcode. func (a *App) DeleteTOTP(w http.ResponseWriter, r *http.Request) { const notFound = "TOTP not found" + const internalServerError = "Internal server error" id := r.PathValue(UUIDParam) key, err := getAPIKey(r) if err != nil { - jsonResponse(w, fmt.Errorf("API Key not found in request context: %w", err), http.StatusInternalServerError) + log.Printf("API Key not found in request context: %s", err) + jsonResponse(w, internalServerError, http.StatusInternalServerError) return } @@ -180,7 +182,7 @@ func (a *App) DeleteTOTP(w http.ResponseWriter, r *http.Request) { jsonResponse(w, notFound, http.StatusNotFound) } else { log.Printf("error loading TOTP: %s", err) - jsonResponse(w, "Internal server error", http.StatusInternalServerError) + jsonResponse(w, internalServerError, http.StatusInternalServerError) } return } From cd41169d5732fdfae4d831d703410cb7cab8e5ba Mon Sep 17 00:00:00 2001 From: briskt <3172830+briskt@users.noreply.github.com> Date: Thu, 18 Sep 2025 11:58:03 +0800 Subject: [PATCH 4/5] move mux init outside test loop --- totp_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/totp_test.go b/totp_test.go index 9e29c1d..4fb2aa3 100644 --- a/totp_test.go +++ b/totp_test.go @@ -137,6 +137,9 @@ func (ms *MfaSuite) TestAppDeleteTOTP() { } requestWithWrongUUID = requestWithWrongUUID.WithContext(ctxWithAPIKey) + mux := &http.ServeMux{} + mux.HandleFunc("DELETE /totp/{"+UUIDParam+"}", ms.app.DeleteTOTP) + tests := []struct { name string request *http.Request @@ -160,9 +163,6 @@ func (ms *MfaSuite) TestAppDeleteTOTP() { } for _, tt := range tests { ms.Run(tt.name, func() { - mux := &http.ServeMux{} - mux.HandleFunc("DELETE /totp/{"+UUIDParam+"}", ms.app.DeleteTOTP) - response := httptest.NewRecorder() mux.ServeHTTP(response, tt.request) From 91666c28c27153593a9b74a5fb34a16be4d399f1 Mon Sep 17 00:00:00 2001 From: briskt <3172830+briskt@users.noreply.github.com> Date: Thu, 18 Sep 2025 13:22:24 +0800 Subject: [PATCH 5/5] another place where SonarQube flagged as vulnerable to injection attack --- api.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api.go b/api.go index c7bf519..189dbfc 100644 --- a/api.go +++ b/api.go @@ -4,6 +4,7 @@ import ( "encoding/json" "log" "net/http" + "strings" ) const ( @@ -37,7 +38,12 @@ func jsonResponse(w http.ResponseWriter, body interface{}, status int) { if data != nil { jBody, err = json.Marshal(data) if err != nil { - log.Printf("failed to marshal response body to json: %s", err) + + // SonarQube flagged this as vulnerable to injection attacks. Rather than exhaustively search for places + // where user input is inserted into the error message, I'll just sanitize it as recommended. + sanitizedError := strings.ReplaceAll(strings.ReplaceAll(err.Error(), "\n", "_"), "\r", "_") + + log.Printf("failed to marshal response body to json: %s", sanitizedError) w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write([]byte("failed to marshal response body to json")) return