mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
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:
@@ -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
@@ -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 {
|
||||
|
||||
@@ -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
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user