Fix open_basedir restriction issue in selectFont - #3415
Conversation
|
@bsweeney please We need to fix this issue asap . |
|
This change is targeting the symptom of the issue but not the cause. Dompdf shouldn't be generating a "/.ufm" font file path. I was under the impression that we had sufficiently covered invalid font files when the Dompdf configuration is workable (i.e., paths are valid). Which version of Dompdf are you using? |
Very well for me worked!!! |
version = 2.0.4 the issue it's when loading fronts the last load get empty
i KNOW IT'S symptom of the issue BUT this Pr it covering and double validation make the package more robust . as you know the selectFont function used on several places and so hard to debug it . |
|
Please, @bsweeney consider this PR. We're also experiencing the issue and it'd be nice if this PR promotes to master |
|
I am considering this PR, but I'd also like to know what triggers the issue since I have been unable to reproduce. A sample of the questions that come to mind are: Is there a bug in the font installation logic creating an invalid entry in the installed-fonts.json file? Is it a configuration issue that needs to be addressed by the end user and/or that we can work around? Is it caused due to parsing issues with a particular document structure or styling? Ignoring the cause and hiding a logic error could lead to unexpected issues or results in the future. I'm not asking anyone to debug the issue if they can at least provide information that I can use to reproduce the issue. |
|
Regardless of the issue, there's an additional loop present. Issue come from PHP open_basedir is a PHP security feature that lets you define the directories PHP scripts can access. when open_basedir restrictions are enabled, a problem occurs. The Idea was that we must covering all issues conditions. |
|
Can you explain the "additional loop" comment? I'm aware of where the error occurs, which is Dompdf sending an empty font path to Cpdf which is then triggering an error due to open_basedir configuration. As I previously mentioned, Dompdf itself should not be using an empty path so what I'm trying to figure out is why that's happening. Have you tried the upcoming 3.0.0 release to see if the issue is still present? Significant improvements were made to font handling. |
|
i use pdf/Dompdf under package barryvdh/laravel-dompdf i cant use 3.0.0 release . |
|
@NacerKH you could try download 3.0.0 release and replace it on vendor manually |
|
Or alias dev-master in your composer configuration. But the 3.0.0 release is ready I'm just putting together the release notes so you can also just wait a few days. I'll move this to the 3.0.1 milestone for further review. |
yeah I know this way 😀. It not safe way always we need to add other packages what we will do then ? |
Im aware that we must know the cause of issue but |
|
@bsweeney what about this PR , we still need this patch please . |
|
3.0 is out, is this still an issue with that release?
Which is why we check for an empty value a few lines above your change. Admittedly the logic here is a bit wonky since we then strip the extension if it's either AFM or UFM, and the issue appears to be that the font path is just
Agreed, and I'm fine adding the extra validation. Though I do prefer early returns for readability. For example, making the start of the method something like: With his logic I don't have to read to the bottom of the method to see what the logic does with an "empty" value. Lastly, I'd really appreciate it if somebody could provide more info so I can fully analyze the issue. A stack trace? A minimal reproducible example? Yes, the code can be made more robust with additional validation, but you don't make good code by treating only the symptom and ignoring the source of the issue. So far, I have not gotten sufficient information to help determine the cause of this issue. Possibly relevant are #2909 and #3384. |
|
@NacerKH I understand you're disappointed this wasn't included in 3.0, but I do plan to include a change in the next release. I had two items in my last comment that can be answered directly:
|
- Moved validation for an empty or null to the top of the function to enable an early return. - Improves code readability and prevents unnecessary processing when the font name is invalid.
Title:
Fix open_basedir restriction issue in selectFont
Description:
This pull request addresses an issue related to the
open_basedirrestriction in theselectFontfunction. The problem arises when attempting to load a font with an empty name ($fontName === ''), resulting in an error. The solution involves adding a simple condition to check if the font name is not empty before proceeding with font loading.Changes Made:
$fontNameis not empty before attempting to load the font in theselectFontfunction.Testing:
open_basedirrestriction issue is resolved.Additional Context:
Related Issues:
Contributor Agreement:
I confirm that I have read and agreed to the terms of the dompdf opensource project .