uac_replace_xxx - Remove extra spaces and strip display name on "auto" restore #1742
Conversation
…lay name up to uri. This helps prevent any extra spaces from being present in the resulting header.
|
@rrb3942 , in regards to the second change (on deleting the display name in auto mode) - the same effect is to be achieved (if really wanted) by doing |
|
@bogdan-iancu Thanks for pointing that out. I hadn't thought of that scenario. I had gone back and forth on the second change, because even though I think it is probably a good default behaviour it can make certain scenarios harder. I think stripping the display name on sequential is rarely a bad thing to do and consistent. The current behaviour I would say is inconsistent (when changing the display name) and at worse leaks information (without additional work on the script writers part), which I would consider a bad thing. I would also consider the name leaking on sequential requests 'surprising' to script writers. Right now, for consistent and unsurprising behaviour (stripping or changing) on display name changes users have to resort to additional calls to the replace functions on sequential requests. What about either a parameter for auto mode, or a new restore mode (auto_strip, or auto_simple?) that causes the name to be stripped on sequential requests? And a doc update to explain the name replacement behaviour. That way users could pick auto with stripping if they don't want to worry about inconsistency and leaking, and if they have more advance scenarios they can choose auto and handle it in script on sequential requests. |
|
@rrb3942 , I see you point on (2) - please make this a separate PR. For (1) - what is the reason behind the cleanup ? the spaces at the end of the hdr are legal from RFC perspective ;). |
rrb3942
force-pushed
the
uac_replace_strip
branch
from
33732e8
to
2b66e88
Compare
|
Removed the name stripping on "auto" restore. Will make a new PR for that. Is 'It looks nicer' a good enough reason on stripping the extra spaces? :) RFC wise it is allowed, and I haven't run into any systems that have issue with it. Does nag at me every time I see it in a call trace though. A more 'valid' reason may just be we should be trying to match it as if we built the entire from header from scratch ourselves. And I think in that case we would not be including extra spaces. |
|
Any updates here? No progress has been made in the last 15 days, marking as stale. Will close this issue if no further updates are made in the next 30 days. |
| display_len = body->display.len; | ||
| } else { | ||
| //Removing, strip all characters up to the URI | ||
| display_len = ((body->uri.s-msg->buf) - start_offset) - 1; |
There was a problem hiding this comment.
I guess the -1 is for leaving a single space between the URI and the display, right ? If so, I'm not sure that the case of an enclosed URI is properly handled. If I'm not wrong, the < and > are not part of the uri field.
Also, the patch does not cover the situation of multiple spaces before the display, right?
There was a problem hiding this comment.
You are correct that <> are not part of the uri field.
The -1 actually stops us from trimming the < before the uri. I believe a header with a display name, but a non-enclosed uri is against spec (https://www.ietf.org/rfc/rfc2822.txt), so it shouldn't be a problem?
We remove everything from the start of the display name up to, but not including the <. What is left are the spaces from before the display name and the <> enclosed uri.
The patch does not cover multiple spaces before the display, but it could be modified to include those as well.
Example behaviour (verified against a test):
Original:
From: "Name" <sip:user@domain>;tag=as2ed502bc
Stripped:
From: <sip:user@domain>;tag=as2ed502bc
|
Any updates here? No progress has been made in the last 15 days, marking as stale. Will close this issue if no further updates are made in the next 30 days. |
This pull requests makes to changes to the uac_replace_xxx methods.
Previously if you stripped a display name (when was already present) you would end with an extra space in the resulting header. Something like this:
From: sip:[email protected];tag=9fxced76sl**
After the patch:
From: sip:[email protected];tag=9fxced76sl