FreePBX
  1. FreePBX
  2. FREEPBX-3467

Use of builtin CID lookup can trash valid incoming CID info

    Details

    • Type: Bugs Bugs
    • Status: Closed (View Workflow)
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: None
    • Component/s: CallerID Lookup
    • Labels:
      None
    • Backend Engine:
      All

      Description

      When a CID source is specific in Inbound Call Control => CallerID Lookup Sources, it generates a call to whatever (in my case the astdb phonebook) to return CID info. If the number passed in is not in the specified DB, a null string is returned, which freepbx writes on top of the (possibly valid) CID info passed in by the telco/itsp. I don't believe it used to do that, although I am unclear on when this changed. IMO, the string returned should only be assigned to the asterisk CID variables if the returned string is non-null.

        Activity

        Hide
        mickecarlsson added a comment -

        Please specify what version of FreePBX you are using. All I can say right now is that it works for me.

        Show
        mickecarlsson added a comment - Please specify what version of FreePBX you are using. All I can say right now is that it works for me.
        Hide
        danswartz added a comment -

        Freepbx 2.5.1.1 on top of asterisk 1.6.

        Here is the problematic code:

        [cidlookup]
        include => cidlookup-custom
        exten => cidlookup_5,1,Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})})
        exten => cidlookup_5,n,Return()
        exten => cidlookup_return,1,Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})})
        exten => cidlookup_return,n,Return()
        

        Rereading my description, I may not have been clear. My complaint is that it overwrites the CID name, not the CID info in general.

        Show
        danswartz added a comment - Freepbx 2.5.1.1 on top of asterisk 1.6. Here is the problematic code: [cidlookup] include => cidlookup-custom exten => cidlookup_5,1,Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})}) exten => cidlookup_5,n,Return() exten => cidlookup_return,1,Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})}) exten => cidlookup_return,n,Return() Rereading my description, I may not have been clear. My complaint is that it overwrites the CID name, not the CID info in general.
        Hide
        danswartz added a comment -

        Okay, I have root-caused this as being broken in 1.6. Prior to that, LookupCIDName() called. That is documented as deprecated and we should do "${DB(cidname/${CALLERID(num)})}" instead (per asterisk source code comments). What the comments do not make clear enough is that the routine does NOT set the CIDNAME if the lookup failed from the ASTDB, whereas the naive assignment recommended will in fact trash an existing CIDNAME with a null string in the same case.

        Show
        danswartz added a comment - Okay, I have root-caused this as being broken in 1.6. Prior to that, LookupCIDName() called. That is documented as deprecated and we should do "${DB(cidname/${CALLERID(num)})}" instead (per asterisk source code comments). What the comments do not make clear enough is that the routine does NOT set the CIDNAME if the lookup failed from the ASTDB, whereas the naive assignment recommended will in fact trash an existing CIDNAME with a null string in the same case.
        Hide
        Philippe Lindheimer added a comment -

        danswartz,

        can you have a look at the attached patch. I don't have a 1.6 system but I believe this is the correct syntax to address your needs and you are right that we should not set it if it returns null. If it checks out I can check it in.

        Index: extensions.class.php
        ===================================================================
        --- extensions.class.php        (revision 7407)
        +++ extensions.class.php        (working copy)
        @@ -985,6 +985,7 @@
         
                        if (version_compare($version, "1.6", "ge")) {
                                $outstr=addslashes('Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})})');
        +                       $outstr='ExecIf($["${DB(cidname/${CALLERID(num)}" != ""]?Set,CALLERID(name)=${DB(cidname/${CALLERID(num)})})';
                                return $outstr;
                        } else { 
                                return "LookupCIDName";
        
        Show
        Philippe Lindheimer added a comment - danswartz, can you have a look at the attached patch. I don't have a 1.6 system but I believe this is the correct syntax to address your needs and you are right that we should not set it if it returns null. If it checks out I can check it in. Index: extensions.class.php =================================================================== --- extensions.class.php (revision 7407) +++ extensions.class.php (working copy) @@ -985,6 +985,7 @@ if (version_compare($version, "1.6", "ge")) { $outstr=addslashes('Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})})'); + $outstr='ExecIf($["${DB(cidname/${CALLERID(num)}" != ""]?Set,CALLERID(name)=${DB(cidname/${CALLERID(num)})})'; return $outstr; } else { return "LookupCIDName";
        Hide
        danswartz added a comment -

        A couple of syntax issues kept this from working. Here is what it should be (I believe). I made the below changes and it now works:

        $outstr='ExecIf($[\!= ""|"$\{DB(cidname/$\{CALLERID(num)})"]?Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})}))';

        e.g. a ',' after Set instead of '(' [that legal?|is] and a missing ')' for the first call to DB?

        Show
        danswartz added a comment - A couple of syntax issues kept this from working. Here is what it should be (I believe). I made the below changes and it now works: $outstr='ExecIf($ [\!= ""|"$\{DB(cidname/$\{CALLERID(num)})"] ?Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})}))'; e.g. a ',' after Set instead of '(' [that legal?|is] and a missing ')' for the first call to DB?
        Hide
        Philippe Lindheimer added a comment -

        Ok I may have been a little quick to plug that in, thanks for trying it out. I forgot that ExecIf changes from ? to , AND from the other , to ().

        Anyhow, I'm not sure I got it right for what you pasted in above because of the way trac likes to turn the execif into the ExceIf wiki format, so let me paste another patch from what I think is correct and if it looks good, let me know and I'll check it in. Otherwise, paste your code between a starting {} and an ending {} so we can see what it is.

        Index: extensions.class.php
        ===================================================================
        --- extensions.class.php        (revision 7407)
        +++ extensions.class.php        (working copy)
        @@ -984,8 +984,7 @@
                        global $version;
         
                        if (version_compare($version, "1.6", "ge")) {
        -                       $outstr=addslashes('Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})})');
        -                       return $outstr;
        +                       return 'ExecIf($["${DB(cidname/${CALLERID(num)})}" != ""]?Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})}))';
                        } else { 
                                return "LookupCIDName";
                        }
        
        Show
        Philippe Lindheimer added a comment - Ok I may have been a little quick to plug that in, thanks for trying it out. I forgot that ExecIf changes from ? to , AND from the other , to (). Anyhow, I'm not sure I got it right for what you pasted in above because of the way trac likes to turn the execif into the ExceIf wiki format, so let me paste another patch from what I think is correct and if it looks good, let me know and I'll check it in. Otherwise, paste your code between a starting { } and an ending { } so we can see what it is. Index: extensions.class.php =================================================================== --- extensions.class.php (revision 7407) +++ extensions.class.php (working copy) @@ -984,8 +984,7 @@ global $version; if (version_compare($version, "1.6", "ge")) { - $outstr=addslashes('Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})})'); - return $outstr; + return 'ExecIf($["${DB(cidname/${CALLERID(num)})}" != ""]?Set(CALLERID(name)=${DB(cidname/${CALLERID(num)})}))'; } else { return "LookupCIDName"; }
        Hide
        danswartz added a comment -

        That did it, thanks!

        Show
        danswartz added a comment - That did it, thanks!
        Hide
        Philippe Lindheimer added a comment -

        (In http://www.freepbx.org/trac/changeset/7482) fixes FREEPBX-3467
        - don't set CNAM when calling LookupCIDName and null is returned, Asterisk 1.6+ only

        Show
        Philippe Lindheimer added a comment - (In http://www.freepbx.org/trac/changeset/7482 ) fixes FREEPBX-3467 - don't set CNAM when calling LookupCIDName and null is returned, Asterisk 1.6+ only
        Hide
        Philippe Lindheimer added a comment -

        (In http://www.freepbx.org/trac/changeset/7545) Merged revisions 7461-7519,7521,7524-7544 via svnmerge from
        http://svn.freepbx.org/freepbx/branches/2.5

        ........

        (http://www.freepbx.org/trac/changeset/7461) | mickecarlsson | 2009-02-16 12:48:05 -0800 (Mon, 16 Feb 2009) | 1 line

        Closes FREEPBX-3538 adds missing eclosures for localization in ari. Thank you chocho

        ........

        (http://www.freepbx.org/trac/changeset/7461) | p_lindheimer | 2009-03-12 06:18:05 -0700 (Thu, 12 Mar 2009) | 1 line

        fixes FREEPBX-3467
        - don't set CNAM when calling LookupCIDName and null is returned, Asterisk 1.6+ only

        ........

        (http://www.freepbx.org/trac/changeset/7461) | p_lindheimer | 2009-03-15 08:38:55 -0700 (Sun, 15 Mar 2009) | 1 line

        fixes FREEPBX-3586
        - always move fw_langpack to be the last module to process

        ........

        (http://www.freepbx.org/trac/changeset/7461) | p_lindheimer | 2009-03-21 08:49:07 -0700 (Sat, 21 Mar 2009) | 1 line

        fixes FREEPBX-3590 clear output arrays to exec before calling since exec appends data

        ........

        Show
        Philippe Lindheimer added a comment - (In http://www.freepbx.org/trac/changeset/7545 ) Merged revisions 7461-7519,7521,7524-7544 via svnmerge from http://svn.freepbx.org/freepbx/branches/2.5 ........ ( http://www.freepbx.org/trac/changeset/7461 ) | mickecarlsson | 2009-02-16 12:48:05 -0800 (Mon, 16 Feb 2009) | 1 line Closes FREEPBX-3538 adds missing eclosures for localization in ari. Thank you chocho ........ ( http://www.freepbx.org/trac/changeset/7461 ) | p_lindheimer | 2009-03-12 06:18:05 -0700 (Thu, 12 Mar 2009) | 1 line fixes FREEPBX-3467 - don't set CNAM when calling LookupCIDName and null is returned, Asterisk 1.6+ only ........ ( http://www.freepbx.org/trac/changeset/7461 ) | p_lindheimer | 2009-03-15 08:38:55 -0700 (Sun, 15 Mar 2009) | 1 line fixes FREEPBX-3586 - always move fw_langpack to be the last module to process ........ ( http://www.freepbx.org/trac/changeset/7461 ) | p_lindheimer | 2009-03-21 08:49:07 -0700 (Sat, 21 Mar 2009) | 1 line fixes FREEPBX-3590 clear output arrays to exec before calling since exec appends data ........

          People

          • Assignee:
            Unassigned
            Reporter:
            danswartz
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development