Skip to content

Added more scrutinizing SIP header validation warnings #1238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Oct 23, 2025
Merged

Added more scrutinizing SIP header validation warnings #1238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
86e7c85
Validation is there
alexlivekit Oct 4, 2025
7fbabf2
:Validation and forbidden stuff in place
alexlivekit Oct 4, 2025
19f8615
Initial WIP
alexlivekit Oct 4, 2025
65b9fcf
Initial plumbing complete
alexlivekit Oct 4, 2025
2b12ec4
Adding chageset
alexlivekit Oct 4, 2025
31bb1f0
Tests correctly failing
alexlivekit Oct 4, 2025
249610c
Fixed not using proper uquote and bare display name per spec
alexlivekit Oct 6, 2025
9212f24
PR comments, using Unquote, removing heavy name-addr processing and j…
alexlivekit Oct 9, 2025
1c50813
Dropping now-unused regex
alexlivekit Oct 9, 2025
b6bd036
Added todo
alexlivekit Oct 14, 2025
b4e3651
pr suggestion
alexlivekit Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fair-beds-take.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"github.com/livekit/protocol": patch
---

Added warning prints to SIP headers
59 changes: 58 additions & 1 deletion livekit/sip.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/livekit/protocol/logger"
"github.com/livekit/protocol/utils/xtwirp"
"golang.org/x/text/language"
)
Expand Down Expand Up @@ -263,6 +264,29 @@ func validateHeaderValues(headers map[string]string) error {
return nil
}

// validateHeaders makes sure header names/keys and values are per SIP specifications
func validateHeaders(headers map[string]string) error {
for headerName, headerValue := range headers {
if err := ValidateHeaderName(headerName); err != nil {
return 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 nil
}

// validateHeaderNames Makes sure the values of the given map correspond to valid SIP header names
func validateHeaderNames(attributesToHeaders map[string]string) error {
for _, headerName := range attributesToHeaders {
if err := ValidateHeaderName(headerName); err != nil {
return fmt.Errorf("invalid header name: %w", err)
}
}
return nil
}

func (p *SIPTrunkInfo) Validate() error {
if len(p.InboundNumbersRegex) != 0 {
return fmt.Errorf("trunks with InboundNumbersRegex are deprecated")
Expand Down Expand Up @@ -368,6 +392,15 @@ func (p *SIPInboundTrunkInfo) Validate() error {
if err := validateHeaderValues(p.AttributesToHeaders); err != nil {
return err
}
if err := validateHeaders(p.Headers); err != nil {
logger.Warnw("Header validation failed for Headers field", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pass the logger here then. This way we could print it in the context of the request, not globally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the thing, this is protocol/livekit. I don't even know how to pass loggers in here.

The only thing I see here is logger.SetLogger, which we would need to explicitly call to make these messages available.

Plus, the point of these errors is to see what and how many failures we get, not necessarily attribute it to a specific client or project.

Got any ideas on how to pass a logger here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrasing Denys: Since the validation is something we added on top of PB stuff, and that's getting called explicitly (either via an interface or directly), and the idea was to pass a logger to that.

However, given the scope of that change (either break existing api or add another ValidateWithLogger()) and start using that, it seemed like a saner choice to just global-log and later turn into an error.

// TODO: Once we're happy with the validation, we want this to error out
}
// Don't bother with HeadersToAttributes. If they're invalid, we just won't match
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also an issue from the user's point of view. For example, adding an extra space may ignore the header mapping silently.

Copy link
Contributor Author

@alexlivekit alexlivekit Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much validation do we want here in protocol? I could add this, but if you have a sip headers named X-my-custom-header and you specify HeadersToAttributes["X-my-custom -header"] = "my.attribute", I don't know that we need to protect users from themselves on this, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we should just guard against silly mistakes like adding spaces, punctuation, etc. This validation is reused for the CLI as well, so it's convenient when it detects these issues early without sending anything to the server.

Checking against protected headers is a good idea too. Although if we want to change the list, it will require a pretty long update process if it's defined in the protocol.

if err := validateHeaderNames(p.AttributesToHeaders); err != nil {
logger.Warnw("Header validation failed for AttributesToHeaders field", err)
// TODO: Once we're happy with the validation, we want this to error out
}
return nil
}

Expand Down Expand Up @@ -455,6 +488,15 @@ func (p *SIPOutboundTrunkInfo) Validate() error {
if err := validateHeaderValues(p.AttributesToHeaders); err != nil {
return err
}
if err := validateHeaders(p.Headers); err != nil {
logger.Warnw("Header validation failed for Headers field", err)
// TODO: Once we're happy with the validation, we want this to error out
}
// Don't bother with HeadersToAttributes. If they're invalid, we just won't match
if err := validateHeaderNames(p.AttributesToHeaders); err != nil {
logger.Warnw("Header validation failed for AttributesToHeaders field", err)
// TODO: Once we're happy with the validation, we want this to error out
}
return nil
}

Expand All @@ -472,6 +514,11 @@ func (p *SIPOutboundConfig) Validate() error {
if err := validateHeaderValues(p.AttributesToHeaders); err != nil {
return err
}
// Don't bother with HeadersToAttributes. If they're invalid, we just won't match
if err := validateHeaderNames(p.AttributesToHeaders); err != nil {
logger.Warnw("Header validation failed for AttributesToHeaders field", err)
// No error, just a warning for SIP RFC validation for now
}
return nil
}

Expand Down Expand Up @@ -677,13 +724,18 @@ func (p *CreateSIPParticipantRequest) Validate() error {
return err
}

if err := validateHeaders(p.Headers); err != nil {
logger.Warnw("Header validation failed for Headers field", err)
// TODO: Once we're happy with the validation, we want this to error out
}

// Validate display_name if provided
if p.DisplayName != nil {
if len(*p.DisplayName) > 128 {
return errors.New("display_name too long (max 128 characters)")
}

// TODO: Validate display name doesn't contain invalid characters
// TODO: Once we're happy with the validation, we want this to error out
}

// Validate destination if provided
Expand Down Expand Up @@ -775,6 +827,11 @@ func (p *TransferSIPParticipantRequest) Validate() error {
return err
}

if err := validateHeaders(p.Headers); err != nil {
logger.Warnw("Header validation failed for Headers field", err)
// TODO: Once we're happy with the validation, we want this to error out
}

return nil
}

Expand Down
Loading
Loading