Skip to content

Conversation

@danardf
Copy link

@danardf danardf commented Sep 9, 2025

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

@chrsmj
Copy link
Member

chrsmj commented Sep 9, 2025

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

@chrsmj chrsmj self-assigned this Sep 9, 2025
@danardf
Copy link
Author

danardf commented Sep 9, 2025

@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
Copy link
Member

chrsmj commented Sep 9, 2025

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

@danardf
Copy link
Author

danardf commented Sep 9, 2025

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
Copy link
Member

chrsmj commented Sep 9, 2025

Are there any unit tests being added ?

@mrpbueno
Copy link

mrpbueno commented Sep 9, 2025

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
Copy link
Author

danardf commented Sep 10, 2025

@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
Copy link

blazestudios97 commented Sep 10, 2025

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
Copy link
Author

danardf commented Sep 11, 2025

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

@danardf
Copy link
Author

danardf commented Sep 16, 2025

Hi
Feel free to review guys.

@danardf
Copy link
Author

danardf commented Oct 3, 2025

@kguptasangoma Hi
No way to be tested?

@kguptasangoma
Copy link
Member

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

Thanks
Kapil

@danardf
Copy link
Author

danardf commented Oct 3, 2025

@kguptasangoma Ok update me asap. please.

@kguptasangoma
Copy link
Member

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

@danardf
Copy link
Author

danardf commented Oct 3, 2025

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

@hannes427
Copy link
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
Copy link
Author

danardf commented Oct 18, 2025

@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
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.”

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.

6 participants