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

SIP Header DoubleQuotes Param #110

Closed
jaimecasero opened this Issue Feb 1, 2016 · 22 comments

Comments

Projects
None yet
3 participants
@jaimecasero
Contributor

jaimecasero commented Feb 1, 2016

It would be nice if the implementaion could guess if the parameter needs to be doubleQuoted depending on parameter format (fe, if contains(comma) -> doubleQuote(paramValue) )

Log Time

@jaimecasero jaimecasero self-assigned this Feb 1, 2016

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented Feb 2, 2016

An example:

Address contact = msg.getAddressHeader(SipConstants.COMMON_HEADER.CONTACT);
contact.setParameter("subs-reuse",
"feature-status,dialog,message-summary");

in this case, it should be double quoted, because it contains commnas ( it is no Host or Token)

@deruelle deruelle assigned xhoaluu and unassigned jaimecasero May 19, 2016

@deruelle deruelle added this to the 4.0 milestone May 19, 2016

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented May 19, 2016

Hi all,

If I understand correctly, the requirement is if the sip-servlets tries to add new parameter to any SIP header and the value of parameter has comma inside then the api should double quote the value.Currently I dont know the scenario for testing this case, Could anyone help me to describe the testing scenario then I will test my code.

Implement proposal:
at father class ParameterableImpl which is inherited by the other header like Address class, TelURL class, URIImpl class. In the function public void setParameter(String name, String value), adding more code like this:

if(name.equalsIgnoreCase("branch") && isModifiable == ModifiableRule.Via) {
throw new IllegalStateException("it is forbidden to set the branch parameter on the Via Header");
}
if(value.contains(",")){
value = """ + value + """;
}

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented May 19, 2016

Hi Luu Huu Hoan,
The idea is to try to identify whether a param value must be DQUOTE enclosed or not. So, take "comma" as a sample scenario to be covered.

To implement this logic is key to take RFC3261 definitions into account. If we take a look to https://tools.ietf.org/html/rfc3261#section-25.1 and look for the "generic-param", we will find how the parameters are defined...
generic-param = token [ EQUAL gen-value ]
gen-value = token / host / quoted-string
quoted-string = SWS DQUOTE (qdtext / quoted-pair ) DQUOTE
qdtext = LWS / %x21 / %x23-5B / %x5D-7E
/ UTF8-NONASCII
token = 1
(alphanum / "-" / "." / "!" / "%" / "*"
/ "_" / "+" / "`" / "'" / "~" )

All of this must take into consideration how the underlying JAINSIP stack handles quoted params...
https://github.com/RestComm/jain-sip/blob/master/src/gov/nist/javax/sip/header/ParametersHeader.java#L181

Currently, just when the stack parses the String, the param is marked as quoted, so when the outgoing message is encoded, proper DoubleQuotes are printed...

So the condition should be a bit more complex, and the treatment is to engage the JAINSIP ParameterHEader to enable the quotedParam flag...

We may have a call if further clarifications are required....

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented May 21, 2016

Hi jaimecasero ,

I got your idea, that mean that if the value of parameters is match pattern (qdtext / quoted-pair ) we need to mark this value as need to be quoted.
I will need to investigate how to convert that pattern to reg ex and add the code to setParameter function of ParametersHeader class:
if (value.match(regex)){
{
nv.setQuotedValue();
}

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented May 22, 2016

Great, regex is just another way to implement this. Certainly you could use some regex to identify the incoming value is qdtext or not... Taking into account the replacing is to insert DQUOTE at the beginnign and end, I wouldnt spend much time on REGEX replacing...

Another thing worth mentioning is that we should also support apps which want to do the DQUOTE enclosing themselves, so the container dont do the enclosing one more time...

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented May 23, 2016

Hi jaimecasero ,

I followed you idea and found that the string is not quotable string if:

  • "" at the end of string. Example: "abscasc"
  • Current char in string is "", but next char in string is CR or LF. ex: "acvb\raasd" or "acsa\nsadas"
  • Currenet char is CR or LF, but next char is linear white space
  • DQUOTE is not escaped in string

That mean, a lot of cases that the value string of parameter can be quoted. For example: "abc", "abc!jkasdh", "asdasd,adsdas" ..etc So that, Does the app need to quote the string if it's quotable string?

If I was wrong please give me example for more easier to understand.....

Here is the code that I'm planing to check if the value string is quotable string or not:

      public boolean isQuotableString(String value){
    if (value.length() == 0){
        return true;
    }
    char c = value.charAt(0);
    for (int i = 0; i < value.length(); i++){
        c = value.charAt(i);
        if (c == '\\'){
            i++;
            if (i == value.length()){
                //Quotable String cannot be ended by an escape
                return false;
            }

            char c1 = value.charAt(i);
            if(c1 == 0x0d /* CR*/|| c1 == 0x0a /* LF*/){
                // CR, LF are not allowed to be escaped
                return false;
            }
        }
        else if (c == 0x0d /* CR*/ || c == 0x0a /* LF*/){
            i++;
            char c1 = value.charAt(i);
            if (c1 == 0x09 /* TAB*/||/***********************************/
                c1 == 0x0A /* LF*/||
                c1 == 0x0B /* VT*/||//  LINEAR WHITE SPACE
                c1 == 0x0C /* FF*/||
                c1 == 0x0D /* CR*/||
                c1 == 0x20)/* space*/{/************************************/
                return false;
            }
        }
        else if (c == '"'){
            // Double quotes must be escaped if they appear in a quotable
            // string
            return false;
        }
    }
    // the rest of case is quotable stirng
    return true;
}
@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented May 23, 2016

Rules (and statements) to take into account:
Unless otherwise stated, tokens are case-
insensitive. These special characters MUST be in a quoted string to
be used within a parameter value.

token = 1_(alphanum / "-" / "." / "!" / "%" / ""
/ "
" / "+" / "`" / "'" / "~" )

host = hostname / IPv4address / IPv6reference
hostname = _( domainlabel "." ) toplabel [ "." ]
domainlabel = alphanum
/ alphanum *( alphanum / "-" ) alphanum
toplabel = ALPHA / ALPHA *( alphanum / "-" ) alphanum
IPv4address = 1_3DIGIT "." 1_3DIGIT "." 1_3DIGIT "." 1_3DIGIT
IPv6reference = "[" IPv6address "]"
IPv6address = hexpart [ ":" IPv4address ]
hexpart = hexseq / hexseq "::" [ hexseq ] / "::" [ hexseq ]
hexseq = hex4 ( ":" hex4)
hex4 = 1_4HEXDIG
port = 1
DIGIT

quoted-string = SWS DQUOTE *(qdtext / quoted-pair ) DQUOTE
qdtext = LWS / %x21 / %x23-5B / %x5D-7E
/ UTF8-NONASCII
quoted-pair = "" (%x00-09 / %x0B-0C
/ %x0E-7F)

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented May 23, 2016

As we can see, Its quite complex to come out with a set of rules to identifiy whether quotes or not are required. Honestly, I would need some time to check your set of rules...

I can see this statement could be problematic:

Unless otherwise stated, tokens are case-
insensitive. These special characters MUST be in a quoted string to
be used within a parameter value.

This means, that there are generic rules, but then, when different headers are defined, particular header parameters may override these generic rules. This makes me think if a simple list of known parameters would be enough to do the trick. So, instead of inspecting the param value to check if quotes are required, the param name would be search in a Set. This could be potentially overriden by the app logic from the web descriptor, using predefined ContextParam name...

wdyt?

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented May 23, 2016

Regarding the code implemented, some generic comments on that.

  • Please avoid early return statement as possible. In this case the loop and nested if conditions are hard enough. I would prefer a local var to store a flag, and a return in the end...
  • Please avoid magic numbers: Although you have used proper comments next to the literals, I would prefer to define constants with MEANINGFULL names so, they are reused (your code uses CR literal three times)
  • I would try not to tamper with the loop counter ("i") inside the loop. This makes the code harder to read, and introduce extra risk of indexoutofbounds. To check next char i would use additional flags to track the state on next iteration (maybe overkill in this case), or simply increase "i" with not assigments ("if(value.length() >= i + 1) { //check condition}").

And some specific ones regarding the logic:

  • From the method name "isQuotableString" I understand "true" is returned if the param value must be DQUOTE enclosed. Apparently the flag logic is inverted.. Is this intentional?
  • Are you checking the DQUOTE condition is enabled when no previously escaped. It seems this condition is not enough "else if (c == '"'){" -> where are we checking the previous char is "escaped" or not?
  • As suggested in generic comments, the second branch is not checking the value length after increasing the loop counter, causing potential IndexOutOfBoundsException ... "i++;char c1 = value.charAt(i);"

Next time, please submit a PR, even if its not the final version, so I can use Github review tool to introduce meaningful comments over proper line...

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented May 23, 2016

Hi jaimecasero,

It's really great that you have supported and cared about my concerns and I would say thank you to all your valuable comments.

  • For the idea that use the configured list of parameters name for decide which parameter value must be quoted.
    --> I think it's good choice but I don't know how to use web desciptor to do it. Could you point out any link describe the way to do it. How to create or add new attribute on that tool?

Answer comments:
1/ Please avoid early return statement as possible. In this case the loop and nested if conditions are hard enough. I would prefer a local var to store a flag, and a return in the end...
--> Agree with this. will change it on next version.

2/ Please avoid magic numbers: Although you have used proper comments next to the literals, I would prefer to define constants with MEANINGFULL names so, they are reused (your code uses CR literal three times)
--> Agree, I will find out an existing class or maybe will create a new class for those constant values.

3/I would try not to tamper with the loop counter ("i") inside the loop. This makes the code harder to read, and introduce extra risk of indexoutofbounds. To check next char i would use additional flags to track the state on next iteration (maybe overkill in this case), or simply increase "i" with not assigments ("if(value.length() >= i + 1) { //check condition}").
--> Agree, I will change my mind to do not increase the loop counter inside the loop.

4/From the method name "isQuotableString" I understand "true" is returned if the param value must be DQUOTE enclosed. Apparently the flag logic is inverted.. Is this intentional?
--> no, it's not inverted if the function return false, that mean the parameter value cannot be quoted. If function return true, app can double quote the string.

5/Are you checking the DQUOTE condition is enabled when no previously escaped. It seems this condition is not enough "else if (c == '"'){" -> where are we checking the previous char is "escaped" or not?
--> In my implementation, I increased loop counter when checking for escape character if (c == ""){i++...}, then If DQUOTE is in next character right after backslash(), the DQUOTE will not be check in next of the loop because we have i++ in 'for' statement. And now, if the DQUOTE is checking inside the loop, that mean the DQUOTE is not right after any backslash. It's because my code is not clear to make it's easy to read. I'm sorry. Will improve it.
6/ As suggested in generic comments, the second branch is not checking the value length after increasing the loop counter, causing potential IndexOutOfBoundsException ... "i++;char c1 = value.charAt(i);"
--> Agree, Will improve the code.
7/ Next time, please submit a PR, even if its not the final version, so I can use Github review tool to introduce meaningful comments over proper line...
--> Yes, I understand now, this is first time I work with git website :)

Do you have any more comment, Please tell me, I will try to do it :)

@deruelle

This comment has been minimized.

Member

deruelle commented Jun 1, 2016

hi @xhoaluu, just checking if there was anything blocking so far ?

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented Jun 1, 2016

@xhoaluu find an exmaple on getting parameter from web descriptor at https://github.com/RestComm/sip-servlets/blob/master/sip-servlets-impl/src/main/java/org/mobicents/servlet/sip/message/SipServletMessageImpl.java#L1638

basically you need to find the reference to the ServletContext whenever you need to implement your logic. SipServletRequest should have reference to the servletContext

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented Jun 2, 2016

Hi Jean and Jaimecasero,

I were understanding wrong about web discriptor. I just though that the web descriptor is located in http:/localhost:8080/sip-servlets-management/. that mistake took me a lot of time on how to add new attribute into that web for user to config which parameter name that its value string need to be quoted.

Now it's more clearer for me that I need to add context-param in to web.xml file and get it back in sip-servlet implementation by your suggested code.

Question:
I don't see the param-name: "org.restcomm.servlets.sip.OVERRIDE_SYSTEM_HEADER_MODIFICATION" in any web.xml. Where is it defined?

I will try to implement and test my implementation. This is the first time I work with this framework. Could I take more time to investigate to get undestanding about the way you are implementing the application.

Could you add my skype: luuhuuhoan
I will have a lot of question and hope I can ask you directly on skype :)

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented Jun 2, 2016

Hi,
You dont see the param-name anywhere because its an optional param. We made
this feature for an specific customer with special needs, since in this
case this property will change the Spec rules, so its specially dangerous.
It should look like this:

override.system.header.modifiable
Modifiable

It maybe set on web.xml or sip.xml...

Take as much time as necessary, it is important you get knowledge on
sipservlet code, and how the app interacts with it...

I added your skype id, so feel free to contact me. Me may schedule a call
in the morning (GMT+1), so we may sync with Asia timezone (where are you
located?)

Regards

On Thu, Jun 2, 2016 at 8:58 AM, Luu Huu Hoan notifications@github.com
wrote:

Hi Jean and Jaimecasero,

I were understanding wrong about web discriptor. I just though that the
web descriptor is located in http:/localhost:8080/sip-servlets-management/.
that mistake took me a lot of time on how to add new attribute into that
web for user to config which parameter name that its value string need to
be quoted.

Now it's more clearer for me that I need to add context-param in to
web.xml file and get it back in sip-servlet implementation by your
suggested code.

Question:
I don't see the param-name:
"org.restcomm.servlets.sip.OVERRIDE_SYSTEM_HEADER_MODIFICATION" in any
web.xml. Where is it defined?

I will try to implement and test my implement. This is the first time I
work with this framework. Could I take more time to investigate to get
undestanding about the way you are implementing the application.

Could you add my skype: luuhuuhoan
I will have a lot of question and hope I can ask you directly on skype :)


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#110 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHe9STDHjuJFeRd6YAxL5rPc__A3Pvinks5qHn8rgaJpZM4HQfIa
.

@deruelle

This comment has been minimized.

Member

deruelle commented Jun 2, 2016

Let's use the contributor room https://gitter.im/RestComm/Restcomm-discuss or https://gitter.im/RestComm/sip-servlets instead of Skype so everyone can benefit

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented Jun 3, 2016

Hi,

I just finished prototype for this enhancement, am trying to test it by unit test. Currently, if I run unit test for sip-servlet, all testcases are failed or error. Will continue try on tomorrow .

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented Jun 6, 2016

Hi All,

Currently, I'm quite clear about the structure of the code, How sip-servlet connect to jain-sip, how to add new test case with new scenario for testing the application.

For the code to implement this feature, It's ready for testing. I Will create some test cases to test my code. Just want to report my status:
After editing sip.xml:
screen shot 2016-06-06 at 11 02 15 pm

Then in the app, I tried to add rport parameter with example value was xhoaluu. Then application would automatically quote the value of the parameter:
Code:
paramVia.setParameter("rport", "xhoaluu");
Output:
screen shot 2016-06-06 at 11 03 45 pm

Thank you for all your support.

@deruelle

This comment has been minimized.

Member

deruelle commented Jun 9, 2016

@xhoaluu Thanks for moving forward on this. Let us know when ready for a first review. Please do a pull request of your work when ready.

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented Jun 9, 2016

@xhoaluu take into consideration that parameter is meant to contain a list of names, rather than a single one. So somewhere that parsing needs to take place. You may separate names with Space, or Comma...

@jaimecasero jaimecasero modified the milestones: 4.1, 4.0 Jun 9, 2016

@jaimecasero jaimecasero added the ready label Jun 9, 2016

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented Jun 10, 2016

Hi @jaimecasero ,

Currently when I run the regression test on my machine, the result is:
Tests run: 414, Failures: 138, Errors: 65, Skipped: 0

I need to investigate why they are failed and error. Some test case is error because we are casting type is weird. for example: we cast SipServletRequestExt from SipServletRequest but SipServletRequestExt is larger than SipServletRequest. How can we know that the instance of SipServletRequest interface also implement extended function of SipServletRequestExt:
SipServletRequestExt req = (SipServletRequestExt)request;
this thing make testcase error and cannot run.

Another issue from test case is that `Failed to start component [StandardService[SIP-Servlet-Tomcat-Server]]. Do you know why?

I will try to investigate why the thing like that.the result on jenkin is good and not bad like I have:
https://mobicents.ci.cloudbees.com/job/Restcomm-SipServlets-Testsuite/489/#showFailuresLink
just 46 in 413 test cases failed.

@jaimecasero

This comment has been minimized.

Contributor

jaimecasero commented Jun 10, 2016

Hi Luu,

We recently fixed an ugly classloader issue affecting the testSuite, so I
kindly request you to update your branch. Please make sure this commit is
there
bb0ca1e

Then there are some known test which are faliing. Check github issues
https://github.com/RestComm/sip-servlets/issues you will find some issues
related to testsuite bugs.

Regards

On Fri, Jun 10, 2016 at 3:56 PM, Luu Huu Hoan notifications@github.com
wrote:

Hi Jaimecasero,

Currently when I run the regression test on my machine, the result is:
Tests run: 414, Failures: 138, Errors: 65, Skipped: 0

I need to investigate why they are failed and error. Some test case is
error because of we are casting type is weird. for example: we cast
SipServletRequestExt from SipServletRequest but SipServletRequestExt is
larger than SipServletRequest. How can we know that the instance of
SipServletRequest also implement extended function of SipServletRequestExt: SipServletRequestExt
req = (SipServletRequestExt)request;
this thing make testcase error and cannot run.

Another issue from test case is that `Failed to start component
[StandardService[SIP-Servlet-Tomcat-Server]]. Do you know why?

I will try to investigate why the thing like that.the result on jenkin is
good and not bad like I have:

https://mobicents.ci.cloudbees.com/job/Restcomm-SipServlets-Testsuite/489/#showFailuresLink
just 46 in 413 test cases failed.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#110 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHe9STiHiLyq2WXvWXSVIOymIdHOzZJNks5qKW0cgaJpZM4HQfIa
.

jaimecasero added a commit that referenced this issue Jul 15, 2016

Merge pull request #169 from xhoaluu/master
#110 sipservlet would have the feature to automatically quote parameter value if parameter name is in defined quoted parameters list

deruelle added a commit that referenced this issue Jul 16, 2016

@xhoaluu

This comment has been minimized.

Collaborator

xhoaluu commented Nov 14, 2016

Closed

@xhoaluu xhoaluu closed this Nov 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment