Added more scrutinizing SIP header validation warnings #1238
Conversation
🦋 Changeset detectedLatest commit: 09b7506 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
| logger.Warnw("Header validation failed for Headers field", err) | ||
| // No error, just a warning for SIP RFC validation for now | ||
| } | ||
| // Don't bother with HeadersToAttributes. If they're invalid, we just won't match |
There was a problem hiding this comment.
That's also an issue from the user's point of view. For example, adding an extra space may ignore the header mapping silently.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| return err | ||
| } | ||
| if err := validateHeaders(p.Headers); err != nil { | ||
| logger.Warnw("Header validation failed for Headers field", err) |
There was a problem hiding this comment.
We should pass the logger here then. This way we could print it in the context of the request, not globally.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // validateURI validates URIs that can appear in name-addr format | ||
| func validateURI(uri string) error { |
There was a problem hiding this comment.
Maybe we could reuse the parser from sipgo for validation?
There was a problem hiding this comment.
Tempting!
The reason I didn't is I was under the impression we don't want more dependencies in protocol.
Additionally, do we want livekit/sipgo, /emiago/[email protected] (the same verion we have our own depend on), or emiago/sipgo@latest. That last one feels like a problem waiting to happen. Of the first two, I'd probably go with livekit/sipgo just to make sure we don't have to coordinate versions between those.
On the other hand, the only livekit/ module protocol depends on is mageutil, and this feels like a big change.
There was a problem hiding this comment.
Yeah, you are right, adding this dependency to the protocol is a bit too much. Maybe we could do basic validation here and push the rest to the actual SIP server? It can validate these properly with a parser.
There was a problem hiding this comment.
that we can do no problem.
2 participants
I still need to test this, but this is the rough spirit here.
The few qualms that I still have with this:
protocol/livekit. I'm not sure I like this.