Skip to content

Modernization of CDR#53

Open
danardf wants to merge 4 commits into
FreePBX:release/16.0from
danardf:modernization-cdr
Open

Modernization of CDR#53
danardf wants to merge 4 commits into
FreePBX:release/16.0from
danardf:modernization-cdr

Conversation

@danardf

@danardf danardf commented Sep 9, 2025

Copy link
Copy Markdown

I modernized this old module which was ugly for a long time ago. Now it looks better.

@chrsmj

chrsmj commented Sep 9, 2025

Copy link
Copy Markdown
Member

This appears to be AI-generated and contains multiple potential SQL injection errors.

@chrsmj chrsmj self-assigned this Sep 9, 2025
@danardf

danardf commented Sep 9, 2025

Copy link
Copy Markdown
Author

@chrsmj
Hi.
Yes, I played with Claude 4 for fun. I don't hide this fact. I wanted to test it and see if that was correct to use it. ;)

Can you give me an example just to check?

It's just to see if claude is able to fixe it.
I saw some issue with a direct injection too.

@chrsmj

chrsmj commented Sep 9, 2025

Copy link
Copy Markdown
Member

No I am not going to debug your AI generated code.

@danardf

danardf commented Sep 9, 2025

Copy link
Copy Markdown
Author

Well. As you like.
"I know there is a bug, but I don't want to help you."
That's your mindset. I see you are nice, indeed.
Whatever. I saw some. Don't worry. ;)

@chrsmj

chrsmj commented Sep 9, 2025

Copy link
Copy Markdown
Member

Are there any unit tests being added ?

@mrpbueno

mrpbueno commented Sep 9, 2025

Copy link
Copy Markdown

Hey @danardf , just a quick pointer on the SQL injection topic. Check out how $_REQUEST['sort'] is used to build the ORDER BY clause in the new getCdrData function. Directly using request parameters in that part of a query is a common risk. A whitelist for allowed column names is usually the safest approach there. Hope that's a helpful starting point!

@danardf

danardf commented Sep 10, 2025

Copy link
Copy Markdown
Author

@mrpbueno hi
Yes I know. I saw that. indeed.
I did not review the code after generating by I.A, That's it.
I just trust on it. But I should not indeed.

@blazestudios97

blazestudios97 commented Sep 10, 2025

Copy link
Copy Markdown

This appears to be AI-generated and contains multiple potential SQL injection errors.

Interestingly, the current Cdr.class.php has the same SQL injection issues, AI didn't seem to create those Franck just used what already existed. So kudos you just discovered existing SQL injection issues in the CDR module.

Fixe SQL injections
Add unit test
Fixe SQL injections

Fixe SQL injections
Add unit test
@danardf

danardf commented Sep 11, 2025

Copy link
Copy Markdown
Author

I think it's fixed now
I hope I've fixed everything out.

@danardf

danardf commented Sep 16, 2025

Copy link
Copy Markdown
Author

Hi
Feel free to review guys.

@danardf

danardf commented Oct 3, 2025

Copy link
Copy Markdown
Author

@kguptasangoma Hi
No way to be tested?

@kapilgupta01

Copy link
Copy Markdown
Contributor

Hi @danardf did not get chance to look into this one. Needs to kick off the QA also.

Thanks
Kapil

@danardf

danardf commented Oct 3, 2025

Copy link
Copy Markdown
Author

@kguptasangoma Ok update me asap. please.

@kapilgupta01

Copy link
Copy Markdown
Contributor

May be we can ask community help also to test the PR.

@danardf

danardf commented Oct 3, 2025

Copy link
Copy Markdown
Author

As you want.
Lorne tested it quickly, and looks good for him. Just need to test further

@hannes427

Copy link
Copy Markdown
Contributor

Hi @danardf,
I tested your code and found a few issues:

  • In page.cdr.php, the link to the JS file seems to be incorrect (a 404 error appears in the browser console). It should be modules/cdr/assets/js/cdr.js.
    However, this line could probably be removed entirely, since the function framework_include_js automatically includes JavaScript files when certain conditions are met — for example, when a file modules/$moduleName/$moduleName.js exists (like in your PR)
  • When I open the CDR Reports page and use the “Quick Date Range Picker” to select a custom range (e.g. 2025-09-01 to 2025-10-17) — without opening or changing anything under “Advanced Search Options” — not all records are returned.
    Only after manually setting the “From Date” in the Advanced Search Options to 2025-09-01 are all results shown. To debug this, I captured the requests in Wireshark (see below). It seems this happens because there are two variables for both the start and end dates: startdate and from_day, as well as enddate and to_day. In the failed request, startdate is correctly set to 2025-09-01, but from_day remains at 17. In the successful request (after changing the "From Date" in the Advanced Search Options), from_day is correctly updated to 01.
    Are both variable pairs (startdate/from_day and enddate/to_day) required? If so, a possible fix might be to automatically update the “Start Date” and “To Date” fields in the Advanced Search Options via JavaScript whenever the user changes the corresponding fields in the Quick Date Range Picker.

From the failed request:

Request URI Query [truncated]: module=cdr&command=getJSON&search=&sort=calldate&order=desc&offset=0&limit=50&startdate=2025-09-01%2000%3A00%3A00&enddate=2025-10-17%2023%3A59%3A59&from_day=17&from_month=09&from_year=2025&from_hour=&to_day=1
    Request URI Query Parameter: module=cdr
    Request URI Query Parameter: command=getJSON
    Request URI Query Parameter: search=
    Request URI Query Parameter: sort=calldate
    Request URI Query Parameter: order=desc
    Request URI Query Parameter: offset=0
    Request URI Query Parameter: limit=50
    Request URI Query Parameter: startdate=2025-09-01%2000%3A00%3A00
    Request URI Query Parameter: enddate=2025-10-17%2023%3A59%3A59
    Request URI Query Parameter: from_day=17
    Request URI Query Parameter: from_month=09
    Request URI Query Parameter: from_year=2025
    Request URI Query Parameter: from_hour=
    Request URI Query Parameter: to_day=17
    Request URI Query Parameter: to_month=10
    Request URI Query Parameter: to_year=2025
    Request URI Query Parameter: to_hour=
    Request URI Query Parameter: report_type=inbound%2Coutbound%2Cinternal
    Request URI Query Parameter: result_limit=50
    Request URI Query Parameter: _=1760711367571

From the successful request:

Request URI Query [truncated]: module=cdr&command=getJSON&search=&sort=calldate&order=desc&offset=0&limit=50&startdate=2025-09-01%2000%3A00%3A00&enddate=2025-10-17%2023%3A59%3A59&from_day=01&from_month=09&from_year=2025&from_hour=&to_day=1
    Request URI Query Parameter: module=cdr
    Request URI Query Parameter: command=getJSON
    Request URI Query Parameter: search=
    Request URI Query Parameter: sort=calldate
    Request URI Query Parameter: order=desc
    Request URI Query Parameter: offset=0
    Request URI Query Parameter: limit=50
    Request URI Query Parameter: startdate=2025-09-01%2000%3A00%3A00
    Request URI Query Parameter: enddate=2025-10-17%2023%3A59%3A59
    Request URI Query Parameter: from_day=01
    Request URI Query Parameter: from_month=09
    Request URI Query Parameter: from_year=2025
    Request URI Query Parameter: from_hour=
    Request URI Query Parameter: to_day=17
    Request URI Query Parameter: to_month=10
    Request URI Query Parameter: to_year=2025
    Request URI Query Parameter: to_hour=
    Request URI Query Parameter: report_type=inbound%2Coutbound%2Cinternal
    Request URI Query Parameter: result_limit=50
    Request URI Query Parameter: _=1760711367572

One more usability note:
When selecting a custom range in the Quick Date Range Picker with a large gap between start and end dates (e.g. 2021-01-01 to 2025-10-17), it’s quite cumbersome.
You have to scroll through all months to reach the start date, and then scroll all the way forward again to pick the end date.
Choosing the end date first and then going back to pick the start date doesn’t work.
Would it be possible to have separate navigation arrows for the start and end calendars — so that users can browse months independently for each date?
Alternatively, perhaps a “Jump to today” link below the calendar could reset the view to the current and previous month, highlighting the current day.

@danardf

danardf commented Oct 18, 2025

Copy link
Copy Markdown
Author

@hannes427 Thank you so much fro your feedback.

I will try to fixe these issues asap. just need to be free a while. ;)

If there is only this kind of issues, That fine....
I know nobody wanted to put their hands on it. A real bag of screws. :D

@hannes427

Copy link
Copy Markdown
Contributor

@danardf You're welcome! I'm not sure if this is already obvious, but the issue where startdate and from_day/from_month/from_year (and respectively enddate and to_day/to_month/to_year) contain different values also occurs the other way around.

If you change the date only under “Advanced Search Options” and not in the “Quick Date Range Picker”, different date values are sent in the request, and not all entries are found. For example, if you set the start date under the Advanced search options to 2021-01-01 and the end date to 2025-10-18 without changing anything under the “Quick Date Range Picker”, the following values are sent via the HTTP request (truncated):

startdate=2025-09-20%2002%3A23%3A05
enddate=2025-10-19%2002%3A23%3A05
from_day=01
from_month=01
from_year=2021
from_hour=
to_day=19
to_month=10
to_year=2025
to_hour=

You obviously have to enter the desired dates in both fields — that is, under the “Advanced Search Options” and in the “Quick Date Range Picker.”

@jacotec

jacotec commented Apr 12, 2026

Copy link
Copy Markdown

@danardf Just wondering what happened to the state of this module? IMHO a huge improvement compared to what we have now ...

@danardf

danardf commented Apr 16, 2026

Copy link
Copy Markdown
Author

@jacotec Hi.

Yes indeed, I stopped to worked on it for a while. I was busy on somthing else.
I will try to working on it soon.

@jacotec

jacotec commented Apr 17, 2026

Copy link
Copy Markdown

No problem, just want to ask because I like the approach. Thanks for the update!

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.

7 participants