the same issue can arise in other place, for example User Action Log export

Good point to check. User Action Log export already runs its fields through ActionlogsHelper::escapeCsvFormula (added back in 3.9.7), so it's covered - this PR just brings the banner tracks export in line with that same handling. I grepped the tree for other CSV exports and those two are the only endpoints that build a download, so nothing else is left unescaped.

shouldn't be better than to use the same method all over without duplicates?

Makes sense. Dropped the duplicate helper and made ActionlogsHelper::escapeCsvFormula public so the tracks export calls the same method. Pushed.

@arib06 Please conform that you have read the Generative AI policy and your contribution is either not created with the help of AI or is compatible with the policy and GNU/GPL 2 or later by checking the check box at the top of your PR description. Thanks in advance.

@richard67 Read the policy and ticked the box, it's GPL2-compatible. @joomdonation reverted the visibility change too and moved the escaping into BannersHelper so com_banners doesn't depend on the Action Logs helper.

Given that we’re ourselfs then using it identically in two places and it might be also helpful to third-party extension developers, could be an idea to have it as output filter.
https://github.com/joomla/joomla-cms/blob/5.4-dev/libraries/src/Filter/OutputFilter.php

What do you mean @joomdonation ?

Happy to do that. Moving escapeCsvFormula into OutputFilter as a shared method means com_actionlogs and the tracks export both call the same code, and it's exposed to third-party extensions too. It also sidesteps the earlier cross-component dependency concern, since OutputFilter lives in the core library rather than in one component. If that's the direction you both prefer, I'll add it there and update com_actionlogs to use it.

@arib06 i would assume that the issue only happens when you directly open the CSV with Excel by double click. This is not recommended anyway, the file should be imported into Excel from text. Can you confirm that the issue does not happen when you use the data import from text in Excel?

@richard67 Yes, that's right. The formula only evaluates when the file gets parsed automatically, which is what a direct double-click open does since .csv is associated with Excel by default. If you go through Data > From Text/Get Data and set the column type to Text, the value comes in verbatim and isn't evaluated, so that import path is safe.

The reason it's still worth escaping is that the double-click open is the common path for an admin grabbing a download, and LibreOffice Calc evaluates on open as well. This is the standard CSV formula injection case (CWE-1236), and the leading-space fix here is the same handling com_actionlogs already ships, so it's just bringing the tracks export in line rather than a new approach.

I don’t quite understand what role this plays. The fix was already implemented elsewhere some time ago, and the behaviour is now inconsistent in parts of the CMS.

@LadySolveig Agreed, the inconsistency was the real issue here. I've moved the escaping into OutputFilter::escapeCsvFormula in the core library and pointed both com_actionlogs and the banner tracks export at it. The duplicate helper I'd added to BannersHelper and the private copy in ActionlogsHelper are gone, so there's one implementation now and it's exposed to third-party extensions as well. Pushed.

@arib06 , your last commit was perhaps a bit rushed.
This is an architectural decision that needs to be made by the maintenance team, and was just a suggestion on my part to brainstorm.
Quite apart from that, I don’t think it could be included in a patch release of the 5.x series as a bug fix in that form, but would have to wait until version 6.2 at the earliest, if at all.

I think best would be to wait for further feedback here before you invest any more time and effort.

imho
as long as this pull request have no backward impact,
it reduce code duplication,
fix a potential bug,
so not sure it's a candidate for wait untill 6.x,
but maintainers can have different opinion

Addressed the review: instead of removing the old escapeCsvFormula method and its $characters property from ActionlogsHelper I've marked both @deprecated (5.4, to be removed in 7.0) with a pointer to OutputFilter, so nothing is dropped from the API and BC is preserved.

On the OutputFilter move itself, understood that the shared-method direction and the target version are the maintenance team's call. I'm happy to hold here and not touch it further until there's a decision on whether it goes in a 5.x patch or waits for a later release. If you'd rather I pare this back to just the banner tracks fix and leave com_actionlogs untouched, say the word and I'll do that.

I have tested this item ✅ successfully on 4cf3802

I have tested this item ✅ successfully on 4cf3802

On the OutputFilter move itself, understood that the shared-method direction and the target version are the maintenance team's call. I'm happy to hold here and not touch it further until there's a decision on whether it goes in a 5.x patch or waits for a later release. If you'd rather I pare this back to just the banner tracks fix and leave com_actionlogs untouched, say the word and I'll do that.

@arib06 Meanwhile we have discussed your PR in the maintainers team and came to the conclusion to do the fix in 2 steps:

1. A quick, fully backwards compatible (b/c) fix in 5.4-dev with this PR here (which will be merged up into 6.1-dev when it has been merged) so it can be fixed with a 5.4.x and 6.1.x patch version.
2. Later in a new minor version (currently 6.2-dev) the clean up of duplicate code by adding a new method to the OutputFilter, but not the OutputFilter from the CMS, it should be done in the OutputFilter of the framework because we will move the usage of the filter classes in the CMS from the CMS to the framework classes.

For 1. this means you should do the "pare this back to just the banner tracks fix and leave com_actionlogs untouched" thing as you supposed, but in the banner tracks it should be a private and not a public function for b/c (adding a private method does not break classes extended from that, while adding a public method would break that).

For 2. it means that you could create a PR for the framework here https://github.com/joomla-framework/filter/pulls for the 4.x-dev with the new method in file src/OutputFilter.php like you had added it here to file libraries/src/Filter/OutputFilter.php.

When 2. will be done and a new 4.x release of the filter framework has been made, it will then need a third step:
This will be a PR for the 6.2-dev branch here with the composer update for the filter framework and the following changes in additon:

• Change the function calls in banners and action logs to the new framework function.
• Remove Banner::escapeCsvFormula which was added with this PR here - this is fully b/c as only a private function is removed
• Deprecate Actionlog::escapeCsvFormula for removal in 8.0 - here we have to go this way because the function is public

Could you do the first 2 steps? That would be much appreciated.

@richard67 Both done.

1. Pared this PR back to the banner tracks fix only. escapeCsvFormula is now a private method on TracksModel, and the ActionlogsHelper and CMS OutputFilter changes are reverted, so the diff is a single file and fully b/c.

2. Framework PR for the shared method is up against 4.x-dev: joomla-framework/filter#88. It adds OutputFilter::escapeCsvFormula() plus unit tests in Tests/OutputFilterTest.php.

Happy to do step 3 here against 6.2-dev once that framework release is out.

I have tested this item ✅ successfully on b4eaaa8

I have tested this item ✅ successfully on b4eaaa8