Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update LanguageHelper.php to handle translated text with embedded new lines. #42456

Closed
wants to merge 11 commits into from

Conversation

BrainforgeUK
Copy link
Contributor

Emulate Joomla v4.4.0 and earlier behaviour when multi-line text strings present.

Pull Request for Issue #42416 .

Summary of Changes

Allow embedded newlines in text (i.e. between quotes ").
Using them to structure very large areas of text makes maintenance easier.
In some cases using linewrap in IDE slightly helps, but one is left with trying to read / unravel a massive jumble of text.

Newlines in text worked up to v4.4.0
Security related change in v4.4.1 removed this functionality.

This change detects and removes the embedded newlines before using PHP ini parsing as before.
The security related change introduced in v4.4.1 is maintained.

Testing Instructions

Install this plugin.
plg_system_bflanguagetest.zip

Go to plugin admin page.

Actual result BEFORE applying this Pull Request

The plugin name and description not translated.
The language labels are displayed.

Expected result AFTER applying this Pull Request

Name and description translated.
As previous behavious in J4.1.0 and earlier.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Line breaks in characters string have worked for a very long time.
Mention made in another conversation that somewhere in the documentation it says they are not allowed.
Is so the location where that appears needs to be found and corrected.

Also mention was made that the JED needs to be changed to prevent the use of such line breaks.
Such a change will no longer be necessary - assume the developer wants such included.

Emulate Joomla v4.4.0 and earlier behaviour when multi-line text strings present.
Corrected comment.
@dryabov
Copy link
Contributor

dryabov commented Dec 4, 2023

Can anyone show me an example of a real attack vector that you are trying to fix with these patches? I know all the insides of the ini parser in PHP (and even contributed to it), I know the insides of Joomla of course. But I absolutely don't see how the ini files can be manipulated from the outside. Overriding ini files is limited to admins only. As for third-party extensions, just like using constants in ini files, they can simply print any constant or environment variable. Are you going to handle every php file in a similar way???

And as a result of this "fix" we have a big performance hit.

So, what are you still struggling with? You can answer me in Mattermost (@denis-ryabov).

@BrainforgeUK
Copy link
Contributor Author

This patch is not attempting to fix any attack vector.

An attack vector (real or imagained) was addressed in J4.1.1.
There were a few instances where safe .ini language files which previously worked now failed in J4.1.1
#42416

I produced a PR to address this which was rejected because of the performance raminifications. Later, after looking at further changes made in J5 I rewrote it without any performance impact (except in those few instances which failed anyway in J4.1.1).

Added lost function.
@dryabov
Copy link
Contributor

dryabov commented Dec 4, 2023

I thought since you're trying to fix it, you'd understand the exact reason for @bembelimen's patch (609cafc), and the problem is that I don't, and it looks like you're trying to fix the problem that shouldn't exist.

@BrainforgeUK
Copy link
Contributor Author

The problem did not exist until J4.1.1

The security fix in J4.1.1 meant that some previously working language .ini files no longer worked.

Without arguing about whether the security issue is real or imagined (in the language translation scenario) and reversing the change the quickest solution is to cater for those failing .ini files without impacting performance with the others... unless you argue that finding all the problematic .ini files (plus months of more come to light as people upgrade to J4.1.1+) is the correct solution - but then those problem .ini files I've encountered have been working for years without a problem.

@brianteeman
Copy link
Contributor

I will leave it to the JSST to respond specifically about the security issue but the explanation of why to use parse_ini_raw is well documented

@BrainforgeUK
Copy link
Contributor Author

No point waiting for JSST to respond, I am happy with what has been done for J5.0
My changes are based upon J5.0 as it is cleaner error handling.

@MacJoom
Copy link
Contributor

MacJoom commented Dec 23, 2023

@BrainforgeUK thanks for the PR - could you fix the phpcs error?

Improved comment.
Changed @SInCE to __DEPLOY_VERSION__
@BrainforgeUK
Copy link
Contributor Author

Anything else I need to do here?
This branch is out-of-date with the base branch
Do I have to do the Update branch with 4.4-dev? Joomla 5?

@MacJoom
Copy link
Contributor

MacJoom commented Dec 26, 2023

Anything else I need to do here? This branch is out-of-date with the base branch Do I have to do the Update branch with 4.4-dev? Joomla 5?

I did the Update branch (comes up if the current branch - 4.4- is updated with other PR's) - but there are still phpcs errors...

Changed bracket position {
@BrainforgeUK
Copy link
Contributor Author

Thanks.
However, I cannot see what the phpcs error is referring to.
https://ci.joomla.org/joomla/joomla-cms/72531/1/8

Corrections for phpcs errors.
Another phpcd error fix.
Further phpcs error correction.
Another phpcs error correction.
@bembelimen
Copy link
Contributor

Hello @BrainforgeUK we, the maintainer team decided, that the solution is not the way we want to go.

Thank you very much for your contribution, we really appreciate your work and hope, this does not discourage you and we're looking forward to further ideas from you.

@bembelimen bembelimen closed this Jan 3, 2024
@BrainforgeUK
Copy link
Contributor Author

Thanks, my concern is when will embedded newline capability be restored to .ini language files?
My solution would have at least provided a temporary resolution until a more acceptable longer term solution (and I assume pre v4.4.1 backward compatible)) implemented.
Starting work today on a new component and encountered how inconvenient this is going to be.
Wordwrap in the IDE partly helps but is not a long-term solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-4.4-dev RMDQ ReleaseManagerDecisionQueue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants