kyle/mcias
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix SEC-11: use json.Marshal for audit details

Browse Source 
- Add internal/audit package with JSON() and JSONWithRoles() helpers
  that use json.Marshal instead of fmt.Sprintf with %q
- Replace all fmt.Sprintf audit detail construction in:
  - internal/server/server.go (10 occurrences)
  - internal/ui/handlers_auth.go (4 occurrences)
  - internal/grpcserver/auth.go (4 occurrences)
- Add tests for the helpers including edge-case Unicode,
  null bytes, special characters, and odd argument counts
- Fix broken {"roles":%v} formatting that produced invalid JSON

Security: Audit log detail strings are now constructed via
json.Marshal, which correctly handles all Unicode edge cases
(U+2028, U+2029, null bytes, etc.) that fmt.Sprintf with %q
may mishandle. This prevents potential log injection or parsing
issues in audit event consumers.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Kyle Isom
2026-03-13 00:46:00 -07:00
parent 586d4e3355
commit 3b17f7f70b
5 changed files with 217 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
internal/audit/detail.go Normal file
View File

@@ -0,0 +1,33 @@
// Package audit provides helpers for constructing audit log detail strings.
package audit
import "encoding/json"
// JSON builds a JSON details string from key-value pairs for audit logging.
// Uses json.Marshal for safe encoding rather than fmt.Sprintf with %q,
// which is fragile for edge-case Unicode.
func JSON(pairs ...string) string {
if len(pairs)%2 != 0 {
return "{}"
}
m := make(map[string]string, len(pairs)/2)
for i := 0; i < len(pairs); i += 2 {
m[pairs[i]] = pairs[i+1]
}
b, err := json.Marshal(m)
if err != nil {
return "{}"
}
return string(b)
}
// JSONWithRoles builds a JSON details string that includes a "roles" key
// mapped to a string slice. This produces a proper JSON array for the value.
func JSONWithRoles(roles []string) string {
m := map[string][]string{"roles": roles}
b, err := json.Marshal(m)
if err != nil {
return "{}"
}
return string(b)
}

163
internal/audit/detail_test.go Normal file
View File

@@ -0,0 +1,163 @@
package audit
import (
"encoding/json"
"testing"
)
func TestJSON(t *testing.T) {
tests := []struct {
name string
pairs []string
verify func(t *testing.T, result string)
}{
{
name: "single pair",
pairs: []string{"username", "alice"},
verify: func(t *testing.T, result string) {
var m map[string]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON: %v", err)
}
if m["username"] != "alice" {
t.Fatalf("expected alice, got %s", m["username"])
}
},
},
{
name: "multiple pairs",
pairs: []string{"jti", "abc-123", "reason", "logout"},
verify: func(t *testing.T, result string) {
var m map[string]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON: %v", err)
}
if m["jti"] != "abc-123" {
t.Fatalf("expected abc-123, got %s", m["jti"])
}
if m["reason"] != "logout" {
t.Fatalf("expected logout, got %s", m["reason"])
}
},
},
{
name: "special characters in values",
pairs: []string{"username", "user\"with\\quotes"},
verify: func(t *testing.T, result string) {
var m map[string]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON for special chars: %v", err)
}
if m["username"] != "user\"with\\quotes" {
t.Fatalf("unexpected value: %s", m["username"])
}
},
},
{
name: "unicode edge cases",
pairs: []string{"username", "user\u2028line\u2029sep"},
verify: func(t *testing.T, result string) {
var m map[string]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON for unicode: %v", err)
}
if m["username"] != "user\u2028line\u2029sep" {
t.Fatalf("unexpected value: %s", m["username"])
}
},
},
{
name: "null bytes in value",
pairs: []string{"data", "before\x00after"},
verify: func(t *testing.T, result string) {
var m map[string]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON for null bytes: %v", err)
}
if m["data"] != "before\x00after" {
t.Fatalf("unexpected value: %q", m["data"])
}
},
},
{
name: "odd number of args returns empty object",
pairs: []string{"key"},
verify: func(t *testing.T, result string) {
if result != "{}" {
t.Fatalf("expected {}, got %s", result)
}
},
},
{
name: "no args returns empty object",
pairs: nil,
verify: func(t *testing.T, result string) {
if result != "{}" {
t.Fatalf("expected {}, got %s", result)
}
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
result := JSON(tc.pairs...)
tc.verify(t, result)
})
}
}
func TestJSONWithRoles(t *testing.T) {
tests := []struct {
name string
roles []string
verify func(t *testing.T, result string)
}{
{
name: "multiple roles",
roles: []string{"admin", "editor"},
verify: func(t *testing.T, result string) {
var m map[string][]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON: %v", err)
}
if len(m["roles"]) != 2 || m["roles"][0] != "admin" || m["roles"][1] != "editor" {
t.Fatalf("unexpected roles: %v", m["roles"])
}
},
},
{
name: "empty roles",
roles: []string{},
verify: func(t *testing.T, result string) {
var m map[string][]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON: %v", err)
}
if len(m["roles"]) != 0 {
t.Fatalf("expected empty roles, got %v", m["roles"])
}
},
},
{
name: "roles with special characters",
roles: []string{"role\"special"},
verify: func(t *testing.T, result string) {
var m map[string][]string
if err := json.Unmarshal([]byte(result), &m); err != nil {
t.Fatalf("invalid JSON: %v", err)
}
if m["roles"][0] != "role\"special" {
t.Fatalf("unexpected role: %s", m["roles"][0])
}
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
result := JSONWithRoles(tc.roles)
tc.verify(t, result)
})
}
}

9
internal/grpcserver/auth.go
View File

@@ -13,6 +13,7 @@ import (
"google.golang.org/protobuf/types/known/timestamppb"
mciasv1 "git.wntrmute.dev/kyle/mcias/gen/mcias/v1"
"git.wntrmute.dev/kyle/mcias/internal/audit"
"git.wntrmute.dev/kyle/mcias/internal/auth"
"git.wntrmute.dev/kyle/mcias/internal/crypto"
"git.wntrmute.dev/kyle/mcias/internal/model"
@@ -42,7 +43,7 @@ func (a *authServiceServer) Login(ctx context.Context, req *mciasv1.LoginRequest
// Security: run dummy Argon2 to equalise timing for unknown users.
_, _ = auth.VerifyPassword("dummy", auth.DummyHash())
a.s.db.WriteAuditEvent(model.EventLoginFail, nil, nil, ip, //nolint:errcheck // audit failure is non-fatal
fmt.Sprintf(`{"username":%q,"reason":"unknown_user"}`, req.Username))
audit.JSON("username", req.Username, "reason", "unknown_user"))
return nil, status.Error(codes.Unauthenticated, "invalid credentials")
}
@@ -129,7 +130,7 @@ func (a *authServiceServer) Login(ctx context.Context, req *mciasv1.LoginRequest
a.s.db.WriteAuditEvent(model.EventLoginOK, &acct.ID, nil, ip, "") //nolint:errcheck
a.s.db.WriteAuditEvent(model.EventTokenIssued, &acct.ID, nil, ip, //nolint:errcheck
fmt.Sprintf(`{"jti":%q}`, claims.JTI))
audit.JSON("jti", claims.JTI))
return &mciasv1.LoginResponse{
Token: tokenStr,
@@ -145,7 +146,7 @@ func (a *authServiceServer) Logout(ctx context.Context, _ *mciasv1.LogoutRequest
return nil, status.Error(codes.Internal, "internal error")
}
a.s.db.WriteAuditEvent(model.EventTokenRevoked, nil, nil, peerIP(ctx), //nolint:errcheck
fmt.Sprintf(`{"jti":%q,"reason":"logout"}`, claims.JTI))
audit.JSON("jti", claims.JTI, "reason", "logout"))
return &mciasv1.LogoutResponse{}, nil
}
@@ -186,7 +187,7 @@ func (a *authServiceServer) RenewToken(ctx context.Context, _ *mciasv1.RenewToke
}
a.s.db.WriteAuditEvent(model.EventTokenRenewed, &acct.ID, nil, peerIP(ctx), //nolint:errcheck
fmt.Sprintf(`{"old_jti":%q,"new_jti":%q}`, claims.JTI, newClaims.JTI))
audit.JSON("old_jti", claims.JTI, "new_jti", newClaims.JTI))
return &mciasv1.RenewTokenResponse{
Token: newTokenStr,

21
internal/server/server.go
View File

@@ -19,6 +19,7 @@ import (
"net"
"net/http"
"git.wntrmute.dev/kyle/mcias/internal/audit"
"git.wntrmute.dev/kyle/mcias/internal/auth"
"git.wntrmute.dev/kyle/mcias/internal/config"
"git.wntrmute.dev/kyle/mcias/internal/crypto"
@@ -214,7 +215,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) {
// Security: return a generic error whether the user exists or not.
// Always run a dummy Argon2 check to prevent timing-based user enumeration.
_, _ = auth.VerifyPassword("dummy", auth.DummyHash())
s.writeAudit(r, model.EventLoginFail, nil, nil, fmt.Sprintf(`{"username":%q,"reason":"unknown_user"}`, req.Username))
s.writeAudit(r, model.EventLoginFail, nil, nil, audit.JSON("username", req.Username, "reason", "unknown_user"))
middleware.WriteError(w, http.StatusUnauthorized, "invalid credentials", "unauthorized")
return
}
@@ -315,7 +316,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) {
}
s.writeAudit(r, model.EventLoginOK, &acct.ID, nil, "")
s.writeAudit(r, model.EventTokenIssued, &acct.ID, nil, fmt.Sprintf(`{"jti":%q}`, claims.JTI))
s.writeAudit(r, model.EventTokenIssued, &acct.ID, nil, audit.JSON("jti", claims.JTI))
writeJSON(w, http.StatusOK, loginResponse{
Token: tokenStr,
@@ -330,7 +331,7 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) {
middleware.WriteError(w, http.StatusInternalServerError, "internal error", "internal_error")
return
}
s.writeAudit(r, model.EventTokenRevoked, nil, nil, fmt.Sprintf(`{"jti":%q,"reason":"logout"}`, claims.JTI))
s.writeAudit(r, model.EventTokenRevoked, nil, nil, audit.JSON("jti", claims.JTI, "reason", "logout"))
w.WriteHeader(http.StatusNoContent)
}
@@ -376,7 +377,7 @@ func (s *Server) handleRenew(w http.ResponseWriter, r *http.Request) {
return
}
s.writeAudit(r, model.EventTokenRenewed, &acct.ID, nil, fmt.Sprintf(`{"old_jti":%q,"new_jti":%q}`, claims.JTI, newClaims.JTI))
s.writeAudit(r, model.EventTokenRenewed, &acct.ID, nil, audit.JSON("old_jti", claims.JTI, "new_jti", newClaims.JTI))
writeJSON(w, http.StatusOK, loginResponse{
Token: newTokenStr,
@@ -482,7 +483,7 @@ func (s *Server) handleTokenIssue(w http.ResponseWriter, r *http.Request) {
actorID = &a.ID
}
}
s.writeAudit(r, model.EventTokenIssued, actorID, &acct.ID, fmt.Sprintf(`{"jti":%q}`, claims.JTI))
s.writeAudit(r, model.EventTokenIssued, actorID, &acct.ID, audit.JSON("jti", claims.JTI))
writeJSON(w, http.StatusOK, loginResponse{
Token: tokenStr,
@@ -502,7 +503,7 @@ func (s *Server) handleTokenRevoke(w http.ResponseWriter, r *http.Request) {
return
}
s.writeAudit(r, model.EventTokenRevoked, nil, nil, fmt.Sprintf(`{"jti":%q}`, jti))
s.writeAudit(r, model.EventTokenRevoked, nil, nil, audit.JSON("jti", jti))
w.WriteHeader(http.StatusNoContent)
}
@@ -597,7 +598,7 @@ func (s *Server) handleCreateAccount(w http.ResponseWriter, r *http.Request) {
return
}
s.writeAudit(r, model.EventAccountCreated, nil, &acct.ID, fmt.Sprintf(`{"username":%q}`, acct.Username))
s.writeAudit(r, model.EventAccountCreated, nil, &acct.ID, audit.JSON("username", acct.Username))
writeJSON(w, http.StatusCreated, accountToResponse(acct))
}
@@ -712,7 +713,7 @@ func (s *Server) handleSetRoles(w http.ResponseWriter, r *http.Request) {
return
}
s.writeAudit(r, model.EventRoleGranted, grantedBy, &acct.ID, fmt.Sprintf(`{"roles":%v}`, req.Roles))
s.writeAudit(r, model.EventRoleGranted, grantedBy, &acct.ID, audit.JSONWithRoles(req.Roles))
w.WriteHeader(http.StatusNoContent)
}
@@ -745,7 +746,7 @@ func (s *Server) handleGrantRole(w http.ResponseWriter, r *http.Request) {
return
}
s.writeAudit(r, model.EventRoleGranted, grantedBy, &acct.ID, fmt.Sprintf(`{"role":"%s"}`, req.Role))
s.writeAudit(r, model.EventRoleGranted, grantedBy, &acct.ID, audit.JSON("role", req.Role))
w.WriteHeader(http.StatusNoContent)
}
@@ -774,7 +775,7 @@ func (s *Server) handleRevokeRole(w http.ResponseWriter, r *http.Request) {
return
}
s.writeAudit(r, model.EventRoleRevoked, revokedBy, &acct.ID, fmt.Sprintf(`{"role":"%s"}`, role))
s.writeAudit(r, model.EventRoleRevoked, revokedBy, &acct.ID, audit.JSON("role", role))
w.WriteHeader(http.StatusNoContent)
}

10
internal/ui/handlers_auth.go
View File

@@ -1,9 +1,9 @@
package ui
import (
"fmt"
"net/http"
"git.wntrmute.dev/kyle/mcias/internal/audit"
"git.wntrmute.dev/kyle/mcias/internal/auth"
"git.wntrmute.dev/kyle/mcias/internal/crypto"
"git.wntrmute.dev/kyle/mcias/internal/model"
@@ -59,7 +59,7 @@ func (u *UIServer) handleLoginPost(w http.ResponseWriter, r *http.Request) {
// Security: always run dummy Argon2 to prevent timing-based user enumeration.
_, _ = auth.VerifyPassword("dummy", u.dummyHash())
u.writeAudit(r, model.EventLoginFail, nil, nil,
fmt.Sprintf(`{"username":%q,"reason":"unknown_user"}`, username))
audit.JSON("username", username, "reason", "unknown_user"))
u.render(w, "login", LoginData{Error: "invalid credentials"})
return
}
@@ -130,7 +130,7 @@ func (u *UIServer) handleTOTPStep(w http.ResponseWriter, r *http.Request) {
accountID, ok := u.consumeTOTPNonce(nonce)
if !ok {
u.writeAudit(r, model.EventLoginFail, nil, nil,
fmt.Sprintf(`{"username":%q,"reason":"invalid_totp_nonce"}`, username))
audit.JSON("username", username, "reason", "invalid_totp_nonce"))
u.render(w, "login", LoginData{Error: "session expired, please log in again"})
return
}
@@ -238,7 +238,7 @@ func (u *UIServer) finishLogin(w http.ResponseWriter, r *http.Request, acct *mod
u.writeAudit(r, model.EventLoginOK, &acct.ID, nil, "")
u.writeAudit(r, model.EventTokenIssued, &acct.ID, nil,
fmt.Sprintf(`{"jti":%q,"via":"ui"}`, claims.JTI))
audit.JSON("jti", claims.JTI, "via", "ui"))
// Redirect to dashboard.
if isHTMX(r) {
@@ -259,7 +259,7 @@ func (u *UIServer) handleLogout(w http.ResponseWriter, r *http.Request) {
u.logger.Warn("revoke token on UI logout", "error", revokeErr)
}
u.writeAudit(r, model.EventTokenRevoked, nil, nil,
fmt.Sprintf(`{"jti":%q,"reason":"ui_logout"}`, claims.JTI))
audit.JSON("jti", claims.JTI, "reason", "ui_logout"))
}
}
u.clearSessionCookie(w)