Skip to content
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.

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

dont use url encoding for transport tag in path header #765

Closed
wants to merge 1 commit into from

Conversation

denzs
Copy link

@denzs denzs commented Sep 1, 2016

the transport tag was the only string which used urlencoding for ';' and '='
see line 90 for example

this has lead (on the wire) to headers like:
Path: sip:inbound_webrtc@10.3.66.231;lr;received=sip:127.0.0.1:41988%3Btransport%3Dws;ob

to me this does not look like intention?!

@miconda
Copy link
Member

miconda commented Sep 1, 2016

If the = and ; are not encoded, then the transport will be a parameter of the Path URI, not long seen as part of the value of the received parameter.

Were you having troubles with this? Because the module is rather old and used by many, I would expect this would have popped up as an issue if something was broken.

@denzs
Copy link
Author

denzs commented Sep 1, 2016

thank you daniel!
i am trying to build an edge proxy for a websocket client without using nathelper
and till now i dont get the whole picture of how the edge proxy determines the correct outgoing connection to the client for invite and notifies from the backend...

@denzs
Copy link
Author

denzs commented Sep 1, 2016

but that doesnt belong here ;)
so the pull request is obviously wrong!
thank you for your fast clarification!

@denzs denzs closed this Sep 1, 2016
@miconda
Copy link
Member

miconda commented Sep 1, 2016

One thing to keep in mind for the future: prefix the commit message with the module name, as suggested in contribution guidelines:

If the pull request is good, then it can be merged directly, otherwise the commit message needs to be adjusted.

You can come on sr-users mailing list with the description of what you want to do or what you don't understand properly and we will try to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants