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

Allow phpoffice/phpspreadsheet 2.0+ also #4164

Open
wants to merge 2 commits into
base: 3.1
Choose a base branch
from

Conversation

mwikberg-virta
Copy link

1️⃣ Why should it be added? What are the benefits of this change?
Newer versions of phpoffice/phpspreadsheet (2.x) exist and someone might need to use those

2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No.

3️⃣ Does it include tests, if possible?
Some tests were modified as needed for different behavior on 1.x and 2.x

4️⃣ Any drawbacks? Possible breaking changes?
None detected nor expected.

5️⃣ Mark the following tasks as done:

  • Checked the codebase to ensure that your feature doesn't already exist.
  • Take note of the contributing guidelines.
  • Checked the pull requests to ensure that another person hasn't already submitted a fix.
  • Added tests to ensure against regression.

6️⃣ Thanks for contributing! 🙌

@saulens22
Copy link

Yes, there are no major differences between 1.x and 2.x.
I've been using this workaround in composer.json for months now:

"require": {
    "phpoffice/phpspreadsheet": "^2.1.0",
},
"provide": {
    "phpoffice/phpspreadsheet": "1.29"
},

Will test this pull request too, but so far it seems good!

@underdpt
Copy link

underdpt commented Aug 6, 2024

I think I hit one compatibility issue:

Declaration of Maatwebsite\Excel\DefaultValueBinder::bindValue(PhpOffice\PhpSpreadsheet\Cell\Cell $cell, $value) must be compatible with PhpOffice\PhpSpreadsheet\Cell\DefaultValueBinder::bindValue(PhpOffice\PhpSpreadsheet\Cell\Cell $cell, mixed $value): bool 

@JamesFreeman
Copy link

Is there any update on this @patrickbrouwers?

@patrickbrouwers
Copy link
Member

No update, as I mentioned in previous PR attempts and questions. This will be done in the 4.0 release which has no eta

@JamesFreeman
Copy link

Understood, thanks for the update. :)

@emargareten
Copy link

GHSA-ghg6-32f9-2jp7
GHSA-wgmf-q9vr-vww6

@roc26002w
Copy link

I have same compatibility issue:

Declaration of Maatwebsite\Excel\DefaultValueBinder::bindValue(PhpOffice\PhpSpreadsheet\Cell\Cell $cell, $value) must be compatible with PhpOffice\PhpSpreadsheet\Cell\DefaultValueBinder::bindValue(PhpOffice\PhpSpreadsheet\Cell\Cell $cell, mixed $value): bool 

@geneowak
Copy link

No update, as I mentioned in previous PR attempts and questions. This will be done in the 4.0 release which has no eta

@patrickbrouwers given that there are two reported vulnerabilities with the version being used currently, is it possible to just put in a patch for this package while the rest wait for v4?

The cross-site scripting vulnerability is considered moderate (5.4/10) but the bug where bypassing the filter allows an XXE-attack is classified with a high severity (8.8/10) and is only patched in v2.2.1 and above...

Is there some work required to get this fix in? we can help out with that.

@patrickbrouwers
Copy link
Member

We'll first see if the backport PHPOffice/PhpSpreadsheet#4154 gets merged

@prialgauskaslt
Copy link

prialgauskaslt commented Aug 30, 2024

Solution if your pipelines depend on this to be resolved. It seems it will not be done so soon. So just use proper composer.config.audit.ignore like this in your composer.json:

    "config": {
        "audit": {
            "ignore": {
                "CVE-2024-45048": "https://github.com/SpartnerNL/Laravel-Excel/pull/4164",
                "CVE-2024-45046": "https://github.com/SpartnerNL/Laravel-Excel/pull/4164"
            }
        }
    },

@kieranbarlow
Copy link

Do we have any progress on this?

@ultrono
Copy link

ultrono commented Sep 1, 2024

Kinda crazy this is ongoing IMO. I had a pretty important app. using this package with two exports. Decided to migrate them to a simple .CSV file. Unfortunately I couldn't get the above composer.json solutions working.

@prialgauskaslt
Copy link

Kinda crazy this is ongoing IMO. I had a pretty important app. using this package with two exports. Decided to migrate them to a simple .CSV file. Unfortunately I couldn't get the above composer.json solutions working.

Our CI/CD was stuck on composer audit —locked as it failed due to CVE. The solution I provided should be good for most checks during pipelines. Maybe if you’d share how your release/deployment is breaking due to active CVE, we can find a solution for that too. Sorry that I can’t provide more help without a context.

@ykchan97
Copy link

ykchan97 commented Sep 2, 2024

Ignoring the CVEs does not work for me because I have roave/security-advisories in require-dev (it will cause conflicts), I ended up using OP's forked repo temporarily with

"repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/mwikberg-virta/Laravel-Excel.git"
        }
    ]

So far it is working fine, will only switch back to main repo after the CVEs fixed (hope that we do not need to wait for v4 :)

@nuernbergerA
Copy link

@ykchan97
Copy link

ykchan97 commented Sep 3, 2024

PHPOffice/PhpSpreadsheet#4154 (comment)

Yeah I tried this before, it conflicts with roave/security-advisories unfortunately
image

@patrickbrouwers
Copy link
Member

patrickbrouwers commented Sep 3, 2024

Because v2 contains breaking changes (https://github.com/PHPOffice/PhpSpreadsheet/blob/master/CHANGELOG.md#breaking-change) that I want to check. Just don't have the time for it right now.
The security fix will be backported to v1, that means all people using it including people on older php versions (v2 is >8.1) will get the fix.

@quaddy
Copy link

quaddy commented Sep 3, 2024

Yeah I tried this before, it conflicts with roave/security-advisories unfortunately image

Same problem here. Would be nice when this can be handled prioritized.

@patrickbrouwers
Copy link
Member

It's being handled by phpspreadsheet: PHPOffice/PhpSpreadsheet#4154 (comment) Please be patient, it's open source, nobody is paying PhpSpreadsheet to handle this with priority or even having to do the backport.

@nuernbergerA
Copy link

PHPOffice/PhpSpreadsheet#4154 (comment)

Yeah I tried this before, it conflicts with roave/security-advisories unfortunately image

you need to disable/remove roave/security-advisories till its updated but at least you have a secure version installed

@patrickbrouwers
Copy link
Member

Laravel Excel now requires 1.29.1 which has the security fix

@PanchoDP
Copy link

PanchoDP commented Sep 4, 2024

@patrickbrouwers Thank you!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.