<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [x] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> This PR fixes a bug in pseudo-variables related to "From" and "To" headers and changing the `Display Name` through `$tu/$fu/$fn/$tn`.
According to standard the URI must be enclosed in <> if a `Display Name` is given, but it may or may NOT if no display name is present.
So, if one tries to modify the `from`/`to` header with an initial empty display name and URI that is NOT enclosed in <>, it yields an invalid message.
This PR makes sure that if URI was not enclosed in <> and display name is modified, that will wrap URI in <>. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3935
-- Commit Summary --
* pv: Ensure URI enclosed in <> when changing Display Name
-- File Changes --
M src/modules/pv/pv_core.c (62)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3935.patch https://github.com/kamailio/kamailio/pull/3935.diff
@xkaraman pushed 1 commit.
2cec606da1dd576952b6dbbc42928e3901525e06 pv: Ensure URI enclosed in <> when changing Display Name
@xkaraman pushed 1 commit.
023fbf6f12c0b5d5285b70997691002895a3e370 pv: Ensure URI enclosed in <> when changing Display Name
This one needs more review and I can't do it immediately, initial version of commit had some issues.
Few comments now : declare variables at the beginning of the block. Also, I think that the macro for pkg error doesn't take parameter, it's another one for additional parameters.
@xkaraman pushed 1 commit.
8a8c9c2fe8dc591de9a36a017a33ccee7c695611 pv: Ensure URI enclosed in <> when changing Display Name
Yep, both of them fixed now!
After some review and search (https://github.com/kamailio/kamailio/issues/1719), it seems that there is "bug" when trying to apply multiple changes at once (at least regarding the display name and URI as well). Since all changes are done in respect to the original message, some consecutive changes mess up the message.
An example is when first changing the URI and then the Display name.
<details>
<summary> See example config (kamailio master branch) </summary>
``` # kamailio.cfg request_route { xlog("From: $fu and $fn\n"); xlog("To: $tu and $tn\n"); $fu = "sip:testing@test.com"; $fn = "Displayname"; dbg_sip_msg(); ... } ``` `dbg_sip_msg()` applies the changes and produces the following: `From: sip:testing@test.comDisplayname #012` debug line.
The same can be observed if we forward the message to server itself and see the log `ERROR: {1 248 REGISTER al@there.com} <script>: From: sip:testing@test.comDisplayname and <null>` </details>
If we `apply_msg_changes()` though just after URI change, and then continue as normally, it behaves as expected.
With https://github.com/kamailio/kamailio-wiki/pull/57, I added some docs in the PV cookbook intro, about this workaround, as it's currently only in `textopsx` docs.
The code introduced by this PR does not seem to deal properly with some valid cases. Detecting if the URI is enclosed in `<>` fails if there are header parameters. For example:
``` From: alice@x.com;tag=abc\r\n ```
The `tag` is a header parameter, is not a URI parameter. I think it should check the `uri` field from the struct tobody, not `body`.
Furthermore, even not conform with RFC, the parser was supposed cope with headers terminated only by `\n` to be able to test with messages forged in text files (e.g., during development of new features that are not supported by existing client UA).
The problem with checking the `uri` field is that it's always the URI **WITHOUT** the `<>` even if the message header includes them. That's why I had to check on the original unparsed body of the message to figure out if URI was enclosed or not in `< >`.
The check can be that if `uri` is inside `body`, then the before or after chars should be `<` and `>`. Maybe the SIP grammar should be checked to see if whitespaces are allowed after `<` or before `>`.
@xkaraman pushed 1 commit.
a39a5f58cf06e524cad380aa4f543133dd3a88f0 pv: Ensure URI enclosed in <> when changing Display Name
I just pushed an updated version.
I made some assumptions (also noted as comments): 1. When parsing the `FROM` body, all leading whitespace up to `<` are removed when there is no Display Name. 2. We are modifying a valid SIP message (otherwise parser has already failed beforehand). Therefore, we can check confidently, if there is `<` at the start of the `FROM` body, the URI is enclosed in `< >`.
If this is not enough, we can always check the `FROM` body, if it contains `<` followed by `>`.
Are there any more comments after the updated version?
@miconda commented on this pull request.
@@ -3715,6 +3721,29 @@ int pv_set_xto_attr(struct sip_msg *msg, pv_param_t *param, int op,
} buf.len = val->rs.len; memcpy(buf.s, val->rs.s, val->rs.len); + + /* Check if the URI is enclosed in angle brackets */ + is_enclosed = is_uri_enclosed(msg, tb); + /* If uri is not enclosed, we need to enclose it in < > + before adding display name */ + if(!is_enclosed) {
The function to discover if it is enclosed seems rather a simple condition, so maybe it is better to replace the condition directly like:
``` if(tb->display.len == 0 && tb->body.s[0] != '<') { ```
And remove the function.
@xkaraman commented on this pull request.
@@ -3715,6 +3721,29 @@ int pv_set_xto_attr(struct sip_msg *msg, pv_param_t *param, int op,
} buf.len = val->rs.len; memcpy(buf.s, val->rs.s, val->rs.len); + + /* Check if the URI is enclosed in angle brackets */ + is_enclosed = is_uri_enclosed(msg, tb); + /* If uri is not enclosed, we need to enclose it in < > + before adding display name */ + if(!is_enclosed) {
Indeed, it's a simple condition for now, but what if we want a more concrete way to identify the enclosure (assuming it fails in some strange case)? We can modify the function and be done; that's why I introduced the function.
Nevertheless, I can of course remove it completely.
This PR is stale because it has been open 6 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.
Merged #3935 into master.
Thanks! I will make the function inline for now.
-
Daniel-Constantin Mierla -
github-actions[bot]
-
Henning Westerholt
-
Xenofon Karamanos