fix(identity): 1 defect(s) (OC-0195)

Bound free-text profile fields by raw byte length before cleanText's
quadratic sanitizeToFixpoint pass runs, generalizing OC-0192's guard into
cleanTextBounded and applying it to HandlePresenceUpdate's custom_status,
SetCustomStatus, and group DM names.
This commit is contained in:
Claude
2026-08-20 17:10:05 +00:00
parent 4bab1b4b4b
commit bdbd5ac472
4 changed files with 86 additions and 14 deletions
+8 -4
View File
@@ -5,7 +5,6 @@ import (
"fmt"
"log/slog"
"time"
"unicode/utf8"
"github.com/owncord/server/auth"
"github.com/owncord/server/db"
@@ -179,9 +178,14 @@ func (s *ChannelService) HandlePresenceUpdate(ctx context.Context, userID int64,
var cleaned *string
if customStatus != nil {
text := cleanText(*customStatus)
if utf8.RuneCountInString(text) > MaxCustomStatusLen {
return nil, fmt.Errorf("%w: custom_status must be at most %d characters", ErrBadRequest, MaxCustomStatusLen)
// OC-0195: bound the raw bytes before cleanText (sanitizeToFixpoint)
// runs — see cleanTextBounded's doc comment (user.go). This path is
// reachable over the WS presence_update frame, whose read limit is
// config.MaxMessageBytes (1 MiB), far larger than any REST body that
// reaches the equivalent guard on SetCustomStatus/UpdateProfile.
text, err := cleanTextBounded(*customStatus, MaxCustomStatusLen, "custom_status")
if err != nil {
return nil, err
}
cleaned = nullable(text)
}
+10 -7
View File
@@ -5,7 +5,6 @@ import (
"fmt"
"log/slog"
"time"
"unicode/utf8"
"github.com/owncord/server/auth"
"github.com/owncord/server/db"
@@ -231,9 +230,11 @@ func (s *DMService) CreateGroupDM(ctx context.Context, userID int64, recipientID
return nil, fmt.Errorf("%w: a group DM holds at most %d users", ErrBadRequest, db.MaxGroupDMParticipants)
}
cleanName := cleanText(name)
if utf8.RuneCountInString(cleanName) > MaxGroupDMNameLen {
return nil, fmt.Errorf("%w: name must be at most %d characters", ErrBadRequest, MaxGroupDMNameLen)
// OC-0195 sibling: bound the raw bytes before cleanText (sanitizeToFixpoint)
// runs — see cleanTextBounded's doc comment (user.go).
cleanName, err := cleanTextBounded(name, MaxGroupDMNameLen, "name")
if err != nil {
return nil, err
}
for _, rid := range unique {
@@ -320,9 +321,11 @@ func (s *DMService) RenameGroupDM(ctx context.Context, userID, channelID int64,
return nil, fmt.Errorf("%w: only group DMs can be named", ErrBadRequest)
}
cleanName := cleanText(name)
if utf8.RuneCountInString(cleanName) > MaxGroupDMNameLen {
return nil, fmt.Errorf("%w: name must be at most %d characters", ErrBadRequest, MaxGroupDMNameLen)
// OC-0195 sibling: bound the raw bytes before cleanText (sanitizeToFixpoint)
// runs — see cleanTextBounded's doc comment (user.go).
cleanName, err := cleanTextBounded(name, MaxGroupDMNameLen, "name")
if err != nil {
return nil, err
}
if err := s.st.SetDMChannelName(ctx, channelID, cleanName); err != nil {
+38
View File
@@ -336,6 +336,44 @@ func TestHandlePresenceUpdate_AcceptsInvisibleAndCarriesCustomStatus(t *testing.
}
}
// OC-0195: same defect as OC-0192 (TestUpdateProfile_OversizedDisplayNameAndAboutRejectedBeforeSanitizing)
// but reached over presence_update instead of PATCH /users/me. HandlePresenceUpdate
// applied MaxCustomStatusLen to cleanText's *output*, so an adversarial
// nested-entity payload paid the full quadratic sanitizeToFixpoint cost before
// ever being measured. The WS read limit (config.MaxMessageBytes, 1 MiB) admits
// a payload here far larger than PATCH /users/me's body ever could, and this
// runs on the connection's own readPump goroutine.
func TestHandlePresenceUpdate_OversizedCustomStatusRejectedBeforeSanitizing(t *testing.T) {
database := newTestDB(t)
seedUser(t, database, &db.User{ID: 1, Username: "ada", PasswordHash: "h"})
svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database)))
ctx := context.Background()
// Adversarial nested-entity payload (16 KB) — see sanitizeToFixpoint's
// doc comment (message.go) for why this shape is quadratic to sanitize.
huge := "&" + strings.Repeat("amp;", 4000) + "lt;"
start := time.Now()
_, err := svc.HandlePresenceUpdate(ctx, 1, db.StatusOnline, &huge, nil)
elapsed := time.Since(start)
if !errors.Is(err, ErrBadRequest) {
t.Errorf("oversized custom_status err = %v, want ErrBadRequest", err)
}
// A guard that runs before sanitizing rejects in well under a
// millisecond; the pre-fix code spends well over 150ms in
// sanitizeToFixpoint on this payload before the rune-count check ever
// runs. 150ms gives generous margin over noise while staying far below
// the unguarded cost.
if elapsed > 150*time.Millisecond {
t.Errorf("oversized custom_status took %v, want well under 150ms (raw field must be bounded before sanitizing)", elapsed)
}
// The rejected call must not have committed the status either.
u, _ := database.GetUserByID(ctx, 1)
if u.Status == db.StatusOnline {
t.Error("a rejected presence_update must not commit the status")
}
}
func TestHandlePresenceUpdate_RejectsUnknownStatusAndOverlongText(t *testing.T) {
database := newTestDB(t)
seedUser(t, database, &db.User{ID: 1, Username: "ada", PasswordHash: "h"})
+30 -3
View File
@@ -114,6 +114,33 @@ func cleanText(v string) string {
return strings.TrimSpace(sanitizeToFixpoint(v))
}
// cleanTextBounded is cleanText plus the raw-byte guard OC-0192 established
// for UpdateProfile's DisplayName/About fields, generalized for every other
// free-text field that runs through cleanText: SetCustomStatus,
// HandlePresenceUpdate's custom_status, and group DM names (OC-0195).
//
// cleanText's sanitizeToFixpoint pass is quadratic in input length, so a
// bound applied only to its *output* (a plain rune-count check on the
// cleaned string) still lets an adversarial nested-entity payload pay the
// full sanitize cost first — it can even sanitize down to something well
// under maxRunes and be silently accepted, having spent seconds of CPU to
// get there. The byte-length pre-check runs before cleanText ever does, on
// the untouched input, so the cost of rejecting an oversized value is
// O(len(v)) instead of the sanitizer's cost. *4 is deliberately looser than
// maxRunes — it exists only to keep the sanitizer from ever seeing a
// pathological payload, not to duplicate the real (rune-count) bound, which
// still runs afterward on the cleaned, trimmed value.
func cleanTextBounded(v string, maxRunes int, fieldName string) (string, error) {
if len(v) > maxRunes*4 {
return "", fmt.Errorf("%w: %s must be at most %d characters", ErrBadRequest, fieldName, maxRunes)
}
cleaned := cleanText(v)
if utf8.RuneCountInString(cleaned) > maxRunes {
return "", fmt.Errorf("%w: %s must be at most %d characters", ErrBadRequest, fieldName, maxRunes)
}
return cleaned, nil
}
// resolveOptional picks the column value for one nullable text field: the
// sanitized patch when it was supplied, the existing row otherwise.
func resolveOptional(patch *string, existing *string) *string {
@@ -216,9 +243,9 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro
// value persists across reconnects and is cleared explicitly on logout, which
// is why it is stored rather than held on the connection.
func (s *UserService) SetCustomStatus(ctx context.Context, userID int64, text string) error {
cleaned := cleanText(text)
if utf8.RuneCountInString(cleaned) > MaxCustomStatusLen {
return fmt.Errorf("%w: custom_status must be at most %d characters", ErrBadRequest, MaxCustomStatusLen)
cleaned, err := cleanTextBounded(text, MaxCustomStatusLen, "custom_status")
if err != nil {
return err
}
if err := s.st.UpdateUserCustomStatus(ctx, userID, nullable(cleaned)); err != nil {
return fmt.Errorf("%w: failed to update custom status: %v", ErrInternal, err)