Add/Edit DISA with Outbound CID gives SQL error

hmm - can you give some more system specifics like what version of sql you are running? I just tested and it seems to be fine, the '"' are being escaped?

This is the same problem I have reported in bug #2684 (closed as invalid because cannot be reproduced). Bug #2638 seems to be the same also.

I've just tested on 2.5rc1 (everything updated), the problem is still there

test <5555555555> => ok "test" <5555555555> => gives fatal error

using mysql 4.1.20 and php 5.1.6 (on a centos 4 based ditro).

the testing I did was 4.1.20 on a CentOS 4 based system, and it works fine? And looking at the output, the ' " ' are escaped - does another set of eyes see something I'm not seeing?

the quote count is all screwd up because it is escaping a quote, then a quote appears then it escapes another quote... if I run it thu manually in SQL as {{ INSERT INTO disa (displayname,pin,cid,context,resptimeout,digittimeout,needconf,hangup) values ("test","","\"dave\"<0414849849>","from-internal", "10", "5", "", ""); }}

Then is goes thru

so the following should fix this

--- functions.inc.php   2008-08-20 07:43:07.000000000 +1000
+++ functions.inc.php.orig      2008-08-20 07:25:32.000000000 +1000
@@ -178,7 +178,7 @@
                if(empty($displayname)) {
                        $displayname = "unnamed";
                }
-               $results = sql("INSERT INTO disa (displayname,pin,cid,context,resptimeout,digittimeout,needconf,hangup) values (\"".str_replace("\"", "\"\"",$displayname)."\",\"".$pin."\",\"".$cid."\",\"".$context."\", \"$resptimeout\", \"$digittimeout\", \"$needconf\", \"$hangup\")");
+               $results = sql("INSERT INTO disa (displayname,pin,cid,context,resptimeout,digittimeout,needconf,hangup) values (\"".str_replace("\"", "\"\"",$displayname)."\",\"".$pin."\",\"".str_replace("\"", "\"\"", $cid)."\",\"".$context."\", \"$resptimeout\", \"$digittimeout\", \"$needconf\", \"$hangup\")");
 }

 function disa_del($id) {
@@ -197,6 +197,6 @@
        if(empty($displayname)) {
                $displayname = "unnamed";
        }
-       $results = sql("UPDATE disa  set displayname = \"".str_replace("\"", "\"\"",$displayname)."\", pin = \"$pin\", cid = \"".$cid."\", context = \"$context\", resptimeout = \"$resptimeout\", digittimeout = \"$digittimeout\", needconf = \"$needconf\", hangup = \"$hangup\" where disa_id = \"$id\"");
+       $results = sql("UPDATE disa  set displayname = \"".str_replace("\"", "\"\"",$displayname)."\", pin = \"$pin\", cid = \"".str_replace("\"", "\"\"",$cid)."\", context = \"$context\", resptimeout = \"$resptimeout\", digittimeout = \"$digittimeout\", needconf = \"$needconf\", hangup = \"$hangup\" where disa_id = \"$id\"");
 }
 ?>

wasn't paying attention..diffed back to front.

--- functions.inc.php.orig      2008-08-20 07:25:32.000000000 +1000
+++ functions.inc.php   2008-08-20 07:43:07.000000000 +1000
@@ -178,7 +178,7 @@
                if(empty($displayname)) {
                        $displayname = "unnamed";
                }
-               $results = sql("INSERT INTO disa (displayname,pin,cid,context,resptimeout,digittimeout,needconf,hangup) values (\"".str_replace("\"", "\"\"",$displayname)."\",\"".$pin."\",\"".str_replace("\"", "\"\"", $cid)."\",\"".$context."\", \"$resptimeout\", \"$digittimeout\", \"$needconf\", \"$hangup\")");
+               $results = sql("INSERT INTO disa (displayname,pin,cid,context,resptimeout,digittimeout,needconf,hangup) values (\"".str_replace("\"", "\"\"",$displayname)."\",\"".$pin."\",\"".$cid."\",\"".$context."\", \"$resptimeout\", \"$digittimeout\", \"$needconf\", \"$hangup\")");
 }

 function disa_del($id) {
@@ -197,6 +197,6 @@
        if(empty($displayname)) {
                $displayname = "unnamed";
        }
-       $results = sql("UPDATE disa  set displayname = \"".str_replace("\"", "\"\"",$displayname)."\", pin = \"$pin\", cid = \"".str_replace("\"", "\"\"",$cid)."\", context = \"$context\", resptimeout = \"$resptimeout\", digittimeout = \"$digittimeout\", needconf = \"$needconf\", hangup = \"$hangup\" where disa_id = \"$id\"");
+       $results = sql("UPDATE disa  set displayname = \"".str_replace("\"", "\"\"",$displayname)."\", pin = \"$pin\", cid = \"".$cid."\", context = \"$context\", resptimeout = \"$resptimeout\", digittimeout = \"$digittimeout\", needconf = \"$needconf\", hangup = \"$hangup\" where disa_id = \"$id\"");
 }
 ?>

I still can't reproduce the issue, and I don't see where you are getting your version that does not escape $cid since checking both 2.4 and 2.5 they both do. However, I just checked in a new version of functions.inc.php, see r6428, where I replaced str_replace() with addslashes() which is more proper anyhow. I also added it to additional parameters (in case js validation lets garbage through). Please pull the latest functions.inc.php and test it. This is only 2.5 at this point.

That looks fine to me..no further errors..

(In [6429]) fixes #3090 replace str_replace with addslashes to better protect allinput in sql

r6431 fixes this in 2.4, you can manually get the modules and use upload modules from Module Admin from here:

http://mirror.freepbx.org/modules/release/2.4/disa-2.4.0.4.tgz

or you can apply the patch and test. I need conformation that the 2.4 version works properly before publishing it to the general online system as I don't have a 2.4 system to test it on and we can't publish an un-tested module fix.

dani - can you please test the 2.4 version since you reported this on 2.4 so we can publish the module and make it available and then provide feedback here?

Hi. I've tested the fix with both 2.4 and 2.5 release, but there's still a problem. Now I can enter a callerID with this format:

"name" <number>

But, as soon as I save the changes, backslash are added, and the field becomes

\"name\" <number>

Then, if I edit this disa, I need to manually remove those backslash (if I don't I've got an error message saying I need to enter a valid callerID or leave the field blank). Why this field cannot work as other CallerID (at extension or trunk level) where we can enter

"name" <number>

Without backslash automatically added?

sounds like you have something odd going on with your system. It's being escaped with standard addslashes() in php like other places in the code is. You may want to strike up a forum topic to see if other users are seeing anything similar. The testing and feedback from elsewhere indicates that it is working fine.

I don't think it's a problem with my system (I've tested several new install), but a problem with magic_quotes_gpc parameter. I've checked how outbound CID was treated at users level, and saw that addSlashes is invoked only if get_magic_quotes_gpc returns false:

//escape quotes and any other bad chars:
        if(!get_magic_quotes_gpc()) {
                $outboundcid = addslashes($outboundcid);
                $name = addslashes($name);
        }

On my system, it's on, so I think the problem comes from here. (If I set magic_quote_gpc to Off in my php.ini, everything is ok).

I think the solution is to change disa_add and disa_edit to invoke addSlashes only if magic_quote_gpc is Off

you should not have that on, I suspect there are probably other places in the code that have the same issue and having that on is bad (it has even been removed in php 6)

magic_quotes_gpc should be off, but even as of php 5.2.6, it's on by default in the config file. I'm so used to it being off, I didn't even think to check it. I can confirm, though, that with it off, I don't see the SQL problem.

well a reasonable feature request would be to test for magic_quotes_gpc being on and if it is, put an error in the notification panel. So if someone wants to submit that ticket (and preferably the code to generate the warning), we can look at adding that.