From 5453c4d1ff59279a5478680745b497899f1b82bb Mon Sep 17 00:00:00 2001 From: Alex Fish Date: Tue, 14 Jul 2026 13:02:22 -0700 Subject: [PATCH 1/3] TEL-730: Update validation methods to return soft errors. --- .changeset/nine-deer-smell.md | 6 + livekit/sip.go | 254 +++++++++++++++++++++------------ livekit/sip_validation.go | 87 +++++++++-- livekit/sip_validation_test.go | 90 ++++++++++++ package.json | 2 +- pnpm-lock.yaml | 2 +- rpc/sip.go | 22 ++- 7 files changed, 353 insertions(+), 110 deletions(-) create mode 100644 .changeset/nine-deer-smell.md diff --git a/.changeset/nine-deer-smell.md b/.changeset/nine-deer-smell.md new file mode 100644 index 000000000..261d8c306 --- /dev/null +++ b/.changeset/nine-deer-smell.md @@ -0,0 +1,6 @@ +--- +"github.com/livekit/protocol": minor +"@livekit/protocol": minor +--- + +Add validation method variants that include information about soft errors. diff --git a/livekit/sip.go b/livekit/sip.go index 454bf00ae..012a608f8 100644 --- a/livekit/sip.go +++ b/livekit/sip.go @@ -307,17 +307,16 @@ func (p *SIPOutboundTrunkInfo) AsTrunkInfo() *SIPTrunkInfo { } } -// validateHeaders makes sure header names/keys and values are per SIP specifications -func validateHeaders(headers map[string]string) error { +func validateHeaders(headers map[string]string) ValidationResult { for headerName, headerValue := range headers { if err := ValidateHeaderName(headerName, true); err != nil { - return fmt.Errorf("invalid header name: %w", err) + return ValidationFailure(fmt.Errorf("invalid header name: %w", err)) } if err := ValidateHeaderValue(headerName, headerValue); err != nil { - return fmt.Errorf("invalid header value for %s: %w", headerName, err) + return ValidationFailure(fmt.Errorf("invalid header value for %s: %w", headerName, err)) } } - return nil + return ValidationResult{} } // validateHeaderNames Makes sure the values of the given map correspond to valid SIP header names @@ -361,107 +360,124 @@ func (p *SIPTrunkInfo) Validate() error { } func (p *CreateSIPOutboundTrunkRequest) Validate() error { + return p.ValidateResult().Error() +} + +func (p *CreateSIPOutboundTrunkRequest) ValidateResult() ValidationResult { if p.Trunk == nil { - return errors.New("missing trunk") + return ValidationFailure(errors.New("missing trunk")) } if p.Trunk.SipTrunkId != "" { - return errors.New("trunk id must not be set") - } - if err := p.Trunk.Validate(); err != nil { - return err + return ValidationFailure(errors.New("trunk id must not be set")) } - return nil + return p.Trunk.ValidateResult() } func (p *CreateSIPInboundTrunkRequest) Validate() error { + return p.ValidateResult().Error() +} + +func (p *CreateSIPInboundTrunkRequest) ValidateResult() ValidationResult { if p.Trunk == nil { - return errors.New("missing trunk") + return ValidationFailure(errors.New("missing trunk")) } if p.Trunk.SipTrunkId != "" { - return errors.New("trunk id must not be set") + return ValidationFailure(errors.New("trunk id must not be set")) } - if err := p.Trunk.Validate(); err != nil { - return err - } - return nil + return p.ValidateResult() } func (p *UpdateSIPOutboundTrunkRequest) Validate() error { + return p.ValidateResult().Error() +} + +func (p *UpdateSIPOutboundTrunkRequest) ValidateResult() ValidationResult { if p.SipTrunkId == "" { - return errors.New("trunk id must be set") + return ValidationFailure(errors.New("trunk id must be set")) } if p.Action == nil { - return errors.New("missing or unsupported update action") + return ValidationFailure(errors.New("missing or unsupported update action")) } switch a := p.Action.(type) { default: - return nil + return ValidationResult{} case *UpdateSIPOutboundTrunkRequest_Replace: info := a.Replace if info == nil { - return errors.New("missing trunk") + return ValidationFailure(errors.New("missing trunk")) } if info.SipTrunkId != "" && info.SipTrunkId != p.SipTrunkId { - return errors.New("trunk id in the info must be empty or match the id in the update") + return ValidationFailure(errors.New("trunk id in the info must be empty or match the id in the update")) } - return info.Validate() + return info.ValidateResult() case *UpdateSIPOutboundTrunkRequest_Update: diff := a.Update if diff == nil { - return errors.New("missing trunk update") + return ValidationFailure(errors.New("missing trunk update")) } - return diff.Validate() + return ValidationResult{diff.Validate(), nil} } } func (p *UpdateSIPInboundTrunkRequest) Validate() error { + return p.ValidateResult().Error() +} + +func (p *UpdateSIPInboundTrunkRequest) ValidateResult() ValidationResult { if p.SipTrunkId == "" { - return errors.New("trunk id must be set") + return ValidationFailure(errors.New("trunk id must be set")) } if p.Action == nil { - return errors.New("missing or unsupported update action") + return ValidationFailure(errors.New("missing or unsupported update action")) } switch a := p.Action.(type) { default: - return nil + return ValidationResult{} case *UpdateSIPInboundTrunkRequest_Replace: info := a.Replace if info == nil { - return errors.New("missing trunk") + return ValidationFailure(errors.New("missing trunk")) } if info.SipTrunkId != "" && info.SipTrunkId != p.SipTrunkId { - return errors.New("trunk id in the info must be empty or match the id in the update") + return ValidationFailure(errors.New("trunk id in the info must be empty or match the id in the update")) } - return info.Validate() + return info.ValidateResult() case *UpdateSIPInboundTrunkRequest_Update: diff := a.Update if diff == nil { - return errors.New("missing trunk update") + return ValidationFailure(errors.New("missing trunk update")) } - return diff.Validate() + return ValidationResult{diff.Validate(), nil} } } func (p *SIPInboundTrunkInfo) Validate() error { + return p.ValidateResult().Error() +} + +func (p *SIPInboundTrunkInfo) ValidateResult() ValidationResult { hasAuth := p.AuthUsername != "" || p.AuthPassword != "" hasCIDR := len(p.AllowedAddresses) != 0 hasNumbers := len(p.Numbers) != 0 // TODO: remove this condition, it doesn't really help with security if !hasAuth && !hasCIDR && !hasNumbers { - return psrpc.NewErrorf(psrpc.InvalidArgument, "for security, one of the fields must be set: AuthUsername+AuthPassword, AllowedAddresses or Numbers") + return ValidationFailure(psrpc.NewErrorf(psrpc.InvalidArgument, "for security, one of the fields must be set: AuthUsername+AuthPassword, AllowedAddresses or Numbers")) } if err := p.Media.Validate(); err != nil { - return err + return ValidationFailure(err) } - if err := validateHeaders(p.Headers); err != nil { - return err + + result := validateHeaders(p.Headers) + if !result.OK() { + return result } + if err := validateAttributesToHeaders(p.AttributesToHeaders); err != nil { - return err + return result.WithError(err) } if err := validateHeaderToAttributes(p.HeadersToAttributes); err != nil { - return err + return result.WithError(err) } - return nil + return result } func (p *SIPInboundTrunkUpdate) Validate() error { @@ -481,8 +497,12 @@ func (p *SIPInboundTrunkUpdate) Validate() error { } func (p *SIPInboundTrunkUpdate) Apply(info *SIPInboundTrunkInfo) error { + return p.ApplyResult(info).Error() +} + +func (p *SIPInboundTrunkUpdate) ApplyResult(info *SIPInboundTrunkInfo) ValidationResult { if err := p.Validate(); err != nil { - return err + return ValidationFailure(err) } applyListUpdate(&info.Numbers, p.Numbers) applyListUpdate(&info.AllowedAddresses, p.AllowedAddresses) @@ -494,12 +514,13 @@ func (p *SIPInboundTrunkUpdate) Apply(info *SIPInboundTrunkInfo) error { applyUpdate(&info.Metadata, p.Metadata) updateMediaConfig(info, &info.Media, &info.MediaEncryption, p.Media, p.MediaEncryption) info.Upgrade() - return info.Validate() + return info.ValidateResult() } type UpdateSIPOutboundTrunkRequestAction interface { isUpdateSIPOutboundTrunkRequest_Action Apply(info *SIPOutboundTrunkInfo) (*SIPOutboundTrunkInfo, error) + ApplyResult(info *SIPOutboundTrunkInfo) (*SIPOutboundTrunkInfo, ValidationResult) } var ( @@ -508,57 +529,82 @@ var ( ) func (p *UpdateSIPOutboundTrunkRequest_Replace) Apply(info *SIPOutboundTrunkInfo) (*SIPOutboundTrunkInfo, error) { + val, result := p.ApplyResult(info) + if !result.OK() { + return nil, result.Error() + } + return val, nil +} + +func (p *UpdateSIPOutboundTrunkRequest_Replace) ApplyResult(info *SIPOutboundTrunkInfo) (*SIPOutboundTrunkInfo, ValidationResult) { val := proto.CloneOf(p.Replace) if val == nil { - return nil, errors.New("missing trunk") + return nil, ValidationFailure(errors.New("missing trunk")) } if info.SipTrunkId != "" { val.SipTrunkId = info.SipTrunkId } - if err := val.Validate(); err != nil { - return nil, err + result := val.ValidateResult() + if !result.OK() { + return nil, result } - return val, nil + return val, result } func (p *UpdateSIPOutboundTrunkRequest_Update) Apply(info *SIPOutboundTrunkInfo) (*SIPOutboundTrunkInfo, error) { + val, result := p.ApplyResult(info) + if !result.OK() { + return nil, result.Error() + } + return val, nil +} + +func (p *UpdateSIPOutboundTrunkRequest_Update) ApplyResult(info *SIPOutboundTrunkInfo) (*SIPOutboundTrunkInfo, ValidationResult) { diff := p.Update if diff == nil { - return nil, errors.New("missing trunk update") + return nil, ValidationFailure(errors.New("missing trunk update")) } val := proto.CloneOf(info) - if err := diff.Apply(val); err != nil { - return nil, err + result := diff.ApplyResult(val) + if !result.OK() { + return nil, result } - return val, nil + return val, ValidationResult{} } func (p *SIPOutboundTrunkInfo) Validate() error { + return p.ValidateResult().Error() +} + +func (p *SIPOutboundTrunkInfo) ValidateResult() ValidationResult { if len(p.Numbers) == 0 { - return errors.New("no trunk numbers specified") + return ValidationFailure(errors.New("no trunk numbers specified")) } if p.Address == "" { - return errors.New("no outbound address specified") + return ValidationFailure(errors.New("no outbound address specified")) } if err := validateHostnameFormat(p.Address, "trunk address"); err != nil { - return err + return ValidationFailure(err) } if err := validateHostnameFormat(p.FromHost, "from_host"); err != nil { - return err + return ValidationFailure(err) } - if err := validateHeaders(p.Headers); err != nil { - return err + + result := validateHeaders(p.Headers) + if !result.OK() { + return result } + if err := validateAttributesToHeaders(p.AttributesToHeaders); err != nil { - return err + return result.WithError(err) } if err := validateHeaderToAttributes(p.HeadersToAttributes); err != nil { - return err + return result.WithError(err) } if err := p.Media.Validate(); err != nil { - return err + return result.WithError(err) } - return nil + return result } func (p *SIPOutboundConfig) Validate() error { @@ -591,8 +637,12 @@ func (p *SIPOutboundTrunkUpdate) Validate() error { } func (p *SIPOutboundTrunkUpdate) Apply(info *SIPOutboundTrunkInfo) error { + return p.ApplyResult(info).Error() +} + +func (p *SIPOutboundTrunkUpdate) ApplyResult(info *SIPOutboundTrunkInfo) ValidationResult { if err := p.Validate(); err != nil { - return err + return ValidationFailure(err) } applyUpdate(&info.Address, p.Address) applyUpdate(&info.Transport, p.Transport) @@ -605,12 +655,13 @@ func (p *SIPOutboundTrunkUpdate) Apply(info *SIPOutboundTrunkInfo) error { applyUpdate(&info.FromHost, p.FromHost) updateMediaConfig(info, &info.Media, &info.MediaEncryption, p.Media, p.MediaEncryption) info.Upgrade() - return info.Validate() + return info.ValidateResult() } type UpdateSIPInboundTrunkRequestAction interface { isUpdateSIPInboundTrunkRequest_Action Apply(info *SIPInboundTrunkInfo) (*SIPInboundTrunkInfo, error) + ApplyResult(info *SIPInboundTrunkInfo) (*SIPInboundTrunkInfo, ValidationResult) } var ( @@ -619,29 +670,45 @@ var ( ) func (p *UpdateSIPInboundTrunkRequest_Replace) Apply(info *SIPInboundTrunkInfo) (*SIPInboundTrunkInfo, error) { + val, result := p.ApplyResult(info) + if !result.OK() { + return nil, result.Error() + } + return val, nil +} + +func (p *UpdateSIPInboundTrunkRequest_Replace) ApplyResult(info *SIPInboundTrunkInfo) (*SIPInboundTrunkInfo, ValidationResult) { val := proto.CloneOf(p.Replace) if val == nil { - return nil, errors.New("missing trunk") + return nil, ValidationFailure(errors.New("missing trunk")) } if info.SipTrunkId != "" { val.SipTrunkId = info.SipTrunkId } - if err := val.Validate(); err != nil { - return nil, err + if result := val.ValidateResult(); !result.OK() { + return nil, result } - return val, nil + return val, ValidationResult{} } func (p *UpdateSIPInboundTrunkRequest_Update) Apply(info *SIPInboundTrunkInfo) (*SIPInboundTrunkInfo, error) { + val, result := p.ApplyResult(info) + if !result.OK() { + return nil, result.Error() + } + return val, nil +} + +func (p *UpdateSIPInboundTrunkRequest_Update) ApplyResult(info *SIPInboundTrunkInfo) (*SIPInboundTrunkInfo, ValidationResult) { diff := p.Update if diff == nil { - return nil, errors.New("missing trunk update") + return nil, ValidationFailure(errors.New("missing trunk update")) } val := proto.CloneOf(info) - if err := diff.Apply(val); err != nil { - return nil, err + if result := diff.ApplyResult(val); !result.OK() { + return nil, result } - return val, nil + return val, ValidationResult{} } func (p *CreateSIPDispatchRuleRequest) DispatchRuleInfo() *SIPDispatchRuleInfo { @@ -774,43 +841,48 @@ func (p *UpdateSIPDispatchRuleRequest_Update) Apply(info *SIPDispatchRuleInfo) ( } func (p *CreateSIPParticipantRequest) Validate() error { + return p.ValidateResult().Error() +} + +func (p *CreateSIPParticipantRequest) ValidateResult() ValidationResult { if p.SipTrunkId == "" && p.Trunk == nil && p.SipNumber == "" { - return errors.New("missing sip trunk id and sip number") + return ValidationFailure(errors.New("missing sip trunk id and sip number")) } if p.Trunk != nil { if err := p.Trunk.Validate(); err != nil { - return err + return ValidationFailure(err) } } if p.SipCallTo == "" { - return errors.New("missing sip callee number") + return ValidationFailure(errors.New("missing sip callee number")) } else if strings.Contains(p.SipCallTo, "@") { - return errors.New("SipCallTo should be a phone number or SIP user, not a full SIP URI") + return ValidationFailure(errors.New("SipCallTo should be a phone number or SIP user, not a full SIP URI")) } if p.RoomName == "" { - return errors.New("missing room name") + return ValidationFailure(errors.New("missing room name")) } // Validate display_name if provided if p.DisplayName != nil { if len(*p.DisplayName) > 128 { - return errors.New("DisplayName too long (max 128 characters)") + return ValidationFailure(errors.New("DisplayName too long (max 128 characters)")) } if _, err := strconv.Unquote("\"" + *p.DisplayName + "\""); err != nil { - return fmt.Errorf("DisplayName must be a valid quoted string: %w", err) + return ValidationFailure(fmt.Errorf("DisplayName must be a valid quoted string: %w", err)) } } // Validate destination if provided if err := p.Destination.Validate(); err != nil { - return err + return ValidationFailure(err) } - if err := validateHeaders(p.Headers); err != nil { - return err + result := validateHeaders(p.Headers) + if !result.OK() { + return result } if err := p.Media.Validate(); err != nil { - return err + return result.WithError(err) } - return nil + return result } func (d *Destination) Validate() error { @@ -854,14 +926,18 @@ func (d *Destination) Validate() error { } func (p *TransferSIPParticipantRequest) Validate() error { + return p.ValidateResult().Error() +} + +func (p *TransferSIPParticipantRequest) ValidateResult() ValidationResult { if p.RoomName == "" { - return errors.New("missing room name") + return ValidationFailure(errors.New("missing room name")) } if p.ParticipantIdentity == "" { - return errors.New("missing participant identity") + return ValidationFailure(errors.New("missing participant identity")) } if p.TransferTo == "" { - return errors.New("missing transfer to") + return ValidationFailure(errors.New("missing transfer to")) } // Validate TransferTo URI format and ensure RFC compliance @@ -876,7 +952,7 @@ func (p *TransferSIPParticipantRequest) Validate() error { if !strings.HasPrefix(innerURI, "sip:") && !strings.HasPrefix(innerURI, "sips:") && !strings.HasPrefix(innerURI, "tel:") { // In theory the Refer-To header can receive the full name-addr. // This can make this check inaccurate, but we want to limit to just SIP and TEL URIs. - return errors.New("transfer_to must be a valid SIP(s) or TEL URI (sip:, sips: or tel:)") + return ValidationFailure(errors.New("transfer_to must be a valid SIP(s) or TEL URI (sip:, sips: or tel:)")) } if strings.HasPrefix(innerURI, "sip:") || strings.HasPrefix(innerURI, "sips:") { @@ -890,11 +966,7 @@ func (p *TransferSIPParticipantRequest) Validate() error { p.TransferTo = innerURI } - if err := validateHeaders(p.Headers); err != nil { - return err - } - - return nil + return validateHeaders(p.Headers) } func filterSlice[T any](arr []T, fnc func(v T) bool) []T { diff --git a/livekit/sip_validation.go b/livekit/sip_validation.go index a30df1c92..8319e657b 100644 --- a/livekit/sip_validation.go +++ b/livekit/sip_validation.go @@ -19,6 +19,8 @@ import ( fmt "fmt" "strconv" "strings" + + "github.com/livekit/protocol/logger" ) // RFC 3261 compliant validation functions for SIP headers and messages @@ -212,10 +214,68 @@ var nameAddrHeaders = map[string]bool{ "referred-by": true, // RFC 3892 Section 3 } -var softFailureReporter func(err error) +// Deprecated: has no effect. Use ValidationResult method variants instead. +func SetSoftFailureReporter(reporter func(err error)) {} + +// ValidationResult holds information about something that's been validated. +type ValidationResult struct { + err error + softErrs []error +} + +// OK returns whether or not an error was seen. +func (vr ValidationResult) OK() bool { + return vr.err == nil +} + +// Error returns the error associated with the result. +func (vr ValidationResult) Error() error { + return vr.err +} + +// SoftErrors returns errors that can probably be ignored. +func (vr ValidationResult) SoftErrors() []error { + return vr.softErrs +} -func SetSoftFailureReporter(reporter func(err error)) { - softFailureReporter = reporter +// Combine returns a new validation result that is comprised of this result and +// the other result. +func (vr ValidationResult) Combine(other ValidationResult) ValidationResult { + var err error + if vr.err != nil { + err = vr.err + } else { + err = other.err + } + softErrs := append(vr.softErrs, other.softErrs...) + return ValidationResult{err, softErrs} +} + +// WithError returns a new ValidationResult with the given error. +func (vr ValidationResult) WithError(err error) ValidationResult { + return ValidationResult{err, vr.softErrs} +} + +// LogSoftErrors logs warnings for each of the result's soft errors and +// returns the result's associated error, if any. +func (vr ValidationResult) LogSoftErrors(l logger.Logger) error { + for _, err := range vr.softErrs { + l.Warnw("soft validation failure: %w", err) + } + return vr.err +} + +// LogUnlikelySoftErrors is a variant of LogSoftErrors. +func (vr ValidationResult) LogUnlikelySoftErrors(l logger.UnlikelyLogger) error { + for _, err := range vr.softErrs { + l.Warnw("soft validation failure: %w", err) + } + return vr.err +} + +// ValidationFailure returns a new ValidationResult. +func ValidationFailure(err error) ValidationResult { + return ValidationResult{err, nil} } // ValidateHeaderName validates a SIP header name per RFC 3261 Section 25.1 @@ -245,32 +305,33 @@ func ValidateHeaderName(name string, restrictNames bool) error { // ValidateHeaderValue validates a SIP header value per RFC 3261 Section 25.1 func ValidateHeaderValue(name, value string) error { + return ValidateHeaderValueResult(name, value).Error() +} + +// ValidateHeaderValueResult +func ValidateHeaderValueResult(name, value string) ValidationResult { if value == "" { - return nil + return ValidationResult{} } if len(value) > 1024 { - return fmt.Errorf("header %s: value too long (max 1024 characters)", name) + return ValidationFailure(fmt.Errorf("header %s: value too long (max 1024 characters)", name)) } // Basic character validation - printable ASCII. We're stricter than the spec here - no UTF-8 for now if err := headerValuesCharacters.Validate(value); err != nil { - return fmt.Errorf("header %s: value: %w", name, err) + return ValidationFailure(fmt.Errorf("header %s: value: %w", name, err)) } // Convert to lowercase for case-insensitive comparison + var softErrs []error lowerName := strings.ToLower(name) if _, exists := nameAddrHeaders[lowerName]; exists { if err := validateNameAddrHeader(value); err != nil { - err = fmt.Errorf("header %s: value: %w", name, err) - // For now do not actually error out on header values, just note failure. - if cb := softFailureReporter; cb != nil { - cb(err) - } + softErrs = append(softErrs, fmt.Errorf("header %s: value: %w", name, err)) } } - - return nil + return ValidationResult{nil, softErrs} } // findAngleBrackets efficiently finds angle brackets in a single scan diff --git a/livekit/sip_validation_test.go b/livekit/sip_validation_test.go index 266b5409f..f54baf5a2 100644 --- a/livekit/sip_validation_test.go +++ b/livekit/sip_validation_test.go @@ -15,9 +15,12 @@ package livekit import ( + "errors" "fmt" "strings" "testing" + + "github.com/stretchr/testify/require" ) // Valid Header Test Cases @@ -223,6 +226,13 @@ func TestValidateHeaderValue_ValidValues(t *testing.T) { if err != nil { t.Errorf("ValidateHeaderValue(%q) = %v, want nil", headerValue, err) } + + // Test the result-flavored method, too. + err = ValidateHeaderValueResult("Test-Header", headerValue).Error() + if err != nil { + t.Errorf("ValidateHeaderValueResult(%q).Error() = %v, want nil", headerValue, err) + } + }) } } @@ -236,6 +246,12 @@ func TestValidateHeaderValue_InvalidValues(t *testing.T) { if err == nil { t.Errorf("ValidateHeaderValue(%q) = nil, want error", headerValue) } + + // Test the result-flavored method, too. + err = ValidateHeaderValueResult("Test-Header", headerValue).Error() + if err == nil { + t.Errorf("ValidateHeaderValueResult(%q).Error() = nil, want error", headerValue) + } }) } } @@ -264,6 +280,21 @@ func TestValidateNameAddr_InvalidHeaders(t *testing.T) { } } +// Ensure that we are collecting soft failures correctly. +func TestValidateHeaderValueResult(t *testing.T) { + for i, nameAddr := range InvalidNameAddrHeaders { + t.Run(testCaseName(nameAddr, 32, i), func(t *testing.T) { + result := ValidateHeaderValueResult("from", nameAddr) + if err := result.Error(); err != nil { + t.Errorf("ValidateHeaderValueResult.Error()(%q) = %v, want nil", nameAddr, err) + } + if softErrs := result.SoftErrors(); len(softErrs) == 0 { + t.Errorf("ValidateHeaderValueResult.SoftErrors()(%q) is empty, want non-empty slice", nameAddr) + } + }) + } +} + func TestFrobiddenSipHeaderNames(t *testing.T) { i := 0 for name := range FrobiddenSipHeaderNames { @@ -276,3 +307,62 @@ func TestFrobiddenSipHeaderNames(t *testing.T) { }) } } + +// TODO(alexfish): Finish this +func TestValidationResultCombine(t *testing.T) { + type testCase struct { + name string + source ValidationResult + other ValidationResult + expected ValidationResult + } + + sourceErr := errors.New("source error") + otherErr := errors.New("other error") + + testCases := []testCase{ + { + name: "empty source and other", + source: ValidationResult{}, + other: ValidationResult{}, + expected: ValidationResult{}, + }, + { + name: "empty source", + source: ValidationResult{nil, nil}, + other: ValidationResult{otherErr, nil}, + expected: ValidationResult{otherErr, nil}, + }, + { + name: "empty other", + source: ValidationResult{sourceErr, nil}, + other: ValidationResult{}, + expected: ValidationResult{sourceErr, nil}, + }, + { + name: "both non-empty", + source: ValidationResult{sourceErr, nil}, + other: ValidationResult{otherErr, nil}, + expected: ValidationResult{sourceErr, nil}, + }, + { + name: "soft errors", + source: ValidationResult{nil, []error{errors.New("soft error 1"), errors.New("soft error 2")}}, + other: ValidationResult{nil, []error{errors.New("soft error 3"), errors.New("soft error 4")}}, + expected: ValidationResult{nil, []error{ + errors.New("soft error 1"), + errors.New("soft error 2"), + errors.New("soft error 3"), + errors.New("soft error 4"), + }}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + result := testCase.source.Combine(testCase.other) + require.Equal(t, result.Error(), testCase.expected.Error()) + require.Equal(t, result.SoftErrors(), testCase.expected.SoftErrors()) + }) + } +} diff --git a/package.json b/package.json index b1ca131cd..6fc43db29 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ }, "devDependencies": { "@babel/core": "^7.27.1", - "@changesets/cli": "^2.29.4", + "@changesets/cli": "^2.29.7", "@livekit/changesets-changelog-github": "^0.0.4", "esbuild": "^0.25.4" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2739f05b3..688483e65 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -16,7 +16,7 @@ importers: specifier: ^7.27.1 version: 7.29.6 '@changesets/cli': - specifier: ^2.29.4 + specifier: ^2.29.7 version: 2.29.7 '@livekit/changesets-changelog-github': specifier: ^0.0.4 diff --git a/rpc/sip.go b/rpc/sip.go index a3276628d..1d3f0505c 100644 --- a/rpc/sip.go +++ b/rpc/sip.go @@ -73,9 +73,23 @@ func NewCreateSIPParticipantRequest( req *livekit.CreateSIPParticipantRequest, trunk *livekit.SIPOutboundTrunkInfo, ) (*InternalCreateSIPParticipantRequest, error) { + val, result := NewCreateSIPParticipantRequestResult(projectID, callID, fromHostname, wsUrl, token, req, trunk) + if !result.OK() { + return nil, result.Error() + } + return val, nil +} + +// NewCreateSIPParticipantRequestResult is a variant of the above that returns a livekit.ValidationResult. +func NewCreateSIPParticipantRequestResult( + projectID, callID, fromHostname, wsUrl, token string, + req *livekit.CreateSIPParticipantRequest, + trunk *livekit.SIPOutboundTrunkInfo, +) (*InternalCreateSIPParticipantRequest, livekit.ValidationResult) { req.Upgrade() - if err := req.Validate(); err != nil { - return nil, err + result := req.ValidateResult() + if !result.OK() { + return nil, result } var ( hostname string @@ -116,7 +130,7 @@ func NewCreateSIPParticipantRequest( outboundNumber := req.SipNumber if outboundNumber == "" { if trunk == nil || len(trunk.Numbers) == 0 { - return nil, psrpc.NewErrorf(psrpc.FailedPrecondition, "no numbers on outbound trunk") + return nil, result.WithError(psrpc.NewErrorf(psrpc.FailedPrecondition, "no numbers on outbound trunk")) } outboundNumber = trunk.Numbers[rand.IntN(len(trunk.Numbers))] } @@ -205,7 +219,7 @@ func NewCreateSIPParticipantRequest( WaitUntilAnswered: req.WaitUntilAnswered, DisplayName: req.DisplayName, Destination: req.Destination, - }, nil + }, result } // NewTransferSIPParticipantRequest fills InternalTransferSIPParticipantRequest from From c6999245b714a7f47c568912e425fe75bd20c971 Mon Sep 17 00:00:00 2001 From: Alex Fish Date: Tue, 14 Jul 2026 17:18:05 -0700 Subject: [PATCH 2/3] Move ValidationResult to sip.go (and its tests to sip_test.go); update Combine implementation. --- livekit/sip.go | 57 ++++++++++++++++++++++++++++++ livekit/sip_test.go | 59 +++++++++++++++++++++++++++++++ livekit/sip_validation.go | 63 ---------------------------------- livekit/sip_validation_test.go | 62 --------------------------------- 4 files changed, 116 insertions(+), 125 deletions(-) diff --git a/livekit/sip.go b/livekit/sip.go index 012a608f8..b461feac4 100644 --- a/livekit/sip.go +++ b/livekit/sip.go @@ -14,6 +14,7 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" + "github.com/livekit/protocol/logger" "github.com/livekit/protocol/utils/xtwirp" "github.com/livekit/psrpc" ) @@ -307,6 +308,62 @@ func (p *SIPOutboundTrunkInfo) AsTrunkInfo() *SIPTrunkInfo { } } +// ValidationResult holds information about something that's been validated. +type ValidationResult struct { + err error + softErrs []error +} + +// OK returns whether or not an error was seen. +func (vr ValidationResult) OK() bool { + return vr.err == nil +} + +// Error returns the error associated with the result. +func (vr ValidationResult) Error() error { + return vr.err +} + +// SoftErrors returns errors that can probably be ignored. +func (vr ValidationResult) SoftErrors() []error { + return vr.softErrs +} + +// Combine returns a new validation result that is comprised of this result and +// the other result. +func (vr ValidationResult) Combine(other ValidationResult) ValidationResult { + err := errors.Join(vr.err, other.err) + softErrs := append(vr.softErrs, other.softErrs...) + return ValidationResult{err, softErrs} +} + +// WithError returns a new ValidationResult with the given error. +func (vr ValidationResult) WithError(err error) ValidationResult { + return ValidationResult{err, vr.softErrs} +} + +// LogSoftErrors logs warnings for each of the result's soft errors and +// returns the result's associated error, if any. +func (vr ValidationResult) LogSoftErrors(l logger.Logger) error { + for _, err := range vr.softErrs { + l.Warnw("soft validation failure: %w", err) + } + return vr.err +} + +// LogUnlikelySoftErrors is a variant of LogSoftErrors. +func (vr ValidationResult) LogUnlikelySoftErrors(l logger.UnlikelyLogger) error { + for _, err := range vr.softErrs { + l.Warnw("soft validation failure: %w", err) + } + return vr.err +} + +// ValidationFailure returns a new ValidationResult. +func ValidationFailure(err error) ValidationResult { + return ValidationResult{err, nil} +} + func validateHeaders(headers map[string]string) ValidationResult { for headerName, headerValue := range headers { if err := ValidateHeaderName(headerName, true); err != nil { diff --git a/livekit/sip_test.go b/livekit/sip_test.go index 59f0b96dc..f672bf313 100644 --- a/livekit/sip_test.go +++ b/livekit/sip_test.go @@ -1,6 +1,7 @@ package livekit import ( + errors "errors" "slices" "testing" "time" @@ -1068,3 +1069,61 @@ func TestDestinationValidation(t *testing.T) { }) } } + +func TestValidationResultCombine(t *testing.T) { + type testCase struct { + name string + source ValidationResult + other ValidationResult + expected ValidationResult + } + + sourceErr := errors.New("source error") + otherErr := errors.New("other error") + + testCases := []testCase{ + { + name: "empty source and other", + source: ValidationResult{}, + other: ValidationResult{}, + expected: ValidationResult{}, + }, + { + name: "empty source", + source: ValidationResult{nil, nil}, + other: ValidationResult{otherErr, nil}, + expected: ValidationResult{otherErr, nil}, + }, + { + name: "empty other", + source: ValidationResult{sourceErr, nil}, + other: ValidationResult{}, + expected: ValidationResult{sourceErr, nil}, + }, + { + name: "both non-empty", + source: ValidationResult{sourceErr, nil}, + other: ValidationResult{otherErr, nil}, + expected: ValidationResult{errors.Join(errors.Join(sourceErr, otherErr)), nil}, + }, + { + name: "soft errors", + source: ValidationResult{nil, []error{errors.New("soft error 1"), errors.New("soft error 2")}}, + other: ValidationResult{nil, []error{errors.New("soft error 3"), errors.New("soft error 4")}}, + expected: ValidationResult{nil, []error{ + errors.New("soft error 1"), + errors.New("soft error 2"), + errors.New("soft error 3"), + errors.New("soft error 4"), + }}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + result := testCase.source.Combine(testCase.other) + require.Equal(t, result.Error(), testCase.expected.Error()) + require.Equal(t, result.SoftErrors(), testCase.expected.SoftErrors()) + }) + } +} diff --git a/livekit/sip_validation.go b/livekit/sip_validation.go index 8319e657b..4b80df171 100644 --- a/livekit/sip_validation.go +++ b/livekit/sip_validation.go @@ -19,8 +19,6 @@ import ( fmt "fmt" "strconv" "strings" - - "github.com/livekit/protocol/logger" ) // RFC 3261 compliant validation functions for SIP headers and messages @@ -217,67 +215,6 @@ var nameAddrHeaders = map[string]bool{ // Deprecated: has no effect. Use ValidationResult method variants instead. func SetSoftFailureReporter(reporter func(err error)) {} -// ValidationResult holds information about something that's been validated. -type ValidationResult struct { - err error - softErrs []error -} - -// OK returns whether or not an error was seen. -func (vr ValidationResult) OK() bool { - return vr.err == nil -} - -// Error returns the error associated with the result. -func (vr ValidationResult) Error() error { - return vr.err -} - -// SoftErrors returns errors that can probably be ignored. -func (vr ValidationResult) SoftErrors() []error { - return vr.softErrs -} - -// Combine returns a new validation result that is comprised of this result and -// the other result. -func (vr ValidationResult) Combine(other ValidationResult) ValidationResult { - var err error - if vr.err != nil { - err = vr.err - } else { - err = other.err - } - softErrs := append(vr.softErrs, other.softErrs...) - return ValidationResult{err, softErrs} -} - -// WithError returns a new ValidationResult with the given error. -func (vr ValidationResult) WithError(err error) ValidationResult { - return ValidationResult{err, vr.softErrs} -} - -// LogSoftErrors logs warnings for each of the result's soft errors and -// returns the result's associated error, if any. -func (vr ValidationResult) LogSoftErrors(l logger.Logger) error { - for _, err := range vr.softErrs { - l.Warnw("soft validation failure: %w", err) - } - return vr.err -} - -// LogUnlikelySoftErrors is a variant of LogSoftErrors. -func (vr ValidationResult) LogUnlikelySoftErrors(l logger.UnlikelyLogger) error { - for _, err := range vr.softErrs { - l.Warnw("soft validation failure: %w", err) - } - return vr.err -} - -// ValidationFailure returns a new ValidationResult. -func ValidationFailure(err error) ValidationResult { - return ValidationResult{err, nil} -} - // ValidateHeaderName validates a SIP header name per RFC 3261 Section 25.1 func ValidateHeaderName(name string, restrictNames bool) error { if name == "" { diff --git a/livekit/sip_validation_test.go b/livekit/sip_validation_test.go index f54baf5a2..424faba7d 100644 --- a/livekit/sip_validation_test.go +++ b/livekit/sip_validation_test.go @@ -15,12 +15,9 @@ package livekit import ( - "errors" "fmt" "strings" "testing" - - "github.com/stretchr/testify/require" ) // Valid Header Test Cases @@ -307,62 +304,3 @@ func TestFrobiddenSipHeaderNames(t *testing.T) { }) } } - -// TODO(alexfish): Finish this -func TestValidationResultCombine(t *testing.T) { - type testCase struct { - name string - source ValidationResult - other ValidationResult - expected ValidationResult - } - - sourceErr := errors.New("source error") - otherErr := errors.New("other error") - - testCases := []testCase{ - { - name: "empty source and other", - source: ValidationResult{}, - other: ValidationResult{}, - expected: ValidationResult{}, - }, - { - name: "empty source", - source: ValidationResult{nil, nil}, - other: ValidationResult{otherErr, nil}, - expected: ValidationResult{otherErr, nil}, - }, - { - name: "empty other", - source: ValidationResult{sourceErr, nil}, - other: ValidationResult{}, - expected: ValidationResult{sourceErr, nil}, - }, - { - name: "both non-empty", - source: ValidationResult{sourceErr, nil}, - other: ValidationResult{otherErr, nil}, - expected: ValidationResult{sourceErr, nil}, - }, - { - name: "soft errors", - source: ValidationResult{nil, []error{errors.New("soft error 1"), errors.New("soft error 2")}}, - other: ValidationResult{nil, []error{errors.New("soft error 3"), errors.New("soft error 4")}}, - expected: ValidationResult{nil, []error{ - errors.New("soft error 1"), - errors.New("soft error 2"), - errors.New("soft error 3"), - errors.New("soft error 4"), - }}, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - result := testCase.source.Combine(testCase.other) - require.Equal(t, result.Error(), testCase.expected.Error()) - require.Equal(t, result.SoftErrors(), testCase.expected.SoftErrors()) - }) - } -} From 5c429035a262b4c812ea1a2b404dafbdbecd5763 Mon Sep 17 00:00:00 2001 From: Alex Fish Date: Tue, 14 Jul 2026 17:26:06 -0700 Subject: [PATCH 3/3] Fix ValidationResult test. --- livekit/sip_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/livekit/sip_test.go b/livekit/sip_test.go index f672bf313..20838ffdb 100644 --- a/livekit/sip_test.go +++ b/livekit/sip_test.go @@ -1122,7 +1122,12 @@ func TestValidationResultCombine(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { result := testCase.source.Combine(testCase.other) - require.Equal(t, result.Error(), testCase.expected.Error()) + if testCase.source.Error() != nil { + require.ErrorContains(t, result.Error(), testCase.source.Error().Error()) + } + if testCase.other.Error() != nil { + require.ErrorContains(t, result.Error(), testCase.other.Error().Error()) + } require.Equal(t, result.SoftErrors(), testCase.expected.SoftErrors()) }) }