Skip to content

chore: Update wix to v3.14.1 (security update) #11912

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 17, 2024
Merged

chore: Update wix to v3.14.1 (security update) #11912

merged 1 commit into from
Sep 17, 2024

Conversation

pmiossec
Copy link
Member

No more need to ignore warning NU1903 raised by Wix package <= v3.14.0
(better fix for #11574 than ones done in #11575 and #11911 )

https://wixtoolset.org/news/2024/02/06/wix-v4.0.4-available/

Fixes #11574

Test methodology

  • Build successful in AppVeyor

Test environment(s)

  • GIT
  • Windows

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

@pmiossec
chore: Update wix to v3.14.1 (security update)
853f870 
No more need to ignore warning NU1903 raised by Wix package <= v3.14.0
(better fix for gitextensions#11574 than ones done in gitextensions#11575 and gitextensions#11911 )

https://wixtoolset.org/news/2024/02/06/wix-v4.0.4-available/
mstv
mstv approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, have not run the installer

gerhardol
gerhardol approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not run

RussKie
RussKie reviewed Sep 14, 2024
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test the installer with this change?
If - yes, and everything works - 👍 Feel free to merge.

@RussKie RussKie added this to the 5.0.1 milestone Sep 14, 2024
@pmiossec
Copy link
Member Author

Did you test the installer with this change? If - yes, and everything works - 👍 Feel free to merge.

Not extensively tested (as it is a security fix update that should normally not change the behavior) but I was about to generate the msi and install was successful.

@pmiossec
Copy link
Member Author

Side note: I don't really know how the released msi is built and the documentation on the subject doesn't help a lot ( https://github.com/gitextensions/gitextensions/wiki/How-To%3A-prepare-a-release ).

Create a pre-release and publish binaries

Where does the binaries come from? Are they build from AppVeyor or locally?

I had built my own package locally.

Maybe updating the doc with how pacakge/binaries could be generated could help.

@pmiossec
Copy link
Member Author

I just understood that the magic is in release branch having a different AppVeyor.yml file that generate the msi.
If it's the only hidden magic, I could update the documentation to make it clear.

@pmiossec pmiossec merged commit 2f44802 into gitextensions:master Sep 17, 2024
4 checks passed
@pmiossec pmiossec deleted the update-wix branch September 17, 2024 07:42
@RussKie
Copy link
Member

RussKie commented Sep 17, 2024

Side note: I don't really know how the released msi is built and the documentation on the subject doesn't help a lot ( Wiki: How To: prepare a release ).

Create a pre-release and publish binaries

Where does the binaries come from? Are they build from AppVeyor or locally?

CI builds both zip and msi, but we don't normally publish the msi (we can publish for PRs, but never for the master):

gitextensions/appveyor.yml

Lines 100 to 105 in 2f44802

after_test:
- ps: |
Write-Host "Preparing build artifacts..."
$cmd = "dotnet publish -c Release --no-build /bl:.\artifacts\log\publish.binlog $buildArgs"
Invoke-Expression $cmd
if ($LastExitCode -ne 0) { $host.SetShouldExit($LastExitCode) }

gitextensions/appveyor.yml

Lines 107 to 110 in 2f44802

if ($env:APPVEYOR_PULL_REQUEST_NUMBER -ne $null -and $env:ARTIFACT_PUBLISH_PULL_REQUEST_MSI -eq 'true') {
$msi = (Resolve-Path .\artifacts\Release\publish\GitExtensions-*.msi)[0].Path;
Get-ChildItem $msi | % { Push-AppveyorArtifact $_.FullName -FileName $_.Name }
}

I had built my own package locally.

Maybe updating the doc with how pacakge/binaries could be generated could help.

To generate an msi locally, I run the same commands as we do on CI.

I just understood that the magic is in release branch having a different AppVeyor.yml file that generate the msi. If it's the only hidden magic, I could update the documentation to make it clear.

No, the only major difference between the master and the release CI is that the release CI pushes the build artifacts to the SignPath:

gitextensions/appveyor.yml

Lines 140 to 148 in 2a3b78b

deploy:
- provider: Webhook
on:
ARTIFACT_SIGNING_ENABLED: true
url: https://app.signpath.io/API/v1/7c19b2cf-90f7-4d15-9b12-1b615f7c18c4/Integrations/AppVeyor?ProjectKey=GitExtensions_combined&SigningPolicyKey=release-2020
on_build_success: true
on_build_failure: false
on_build_status_changed: false
method: POST

Hope this help. Please let me know if you have more questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie left review comments

@gerhardol gerhardol gerhardol approved these changes

@mstv mstv mstv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

Vulnerability in WIX nuget package breaks AppVeyor build
4 participants
@pmiossec @RussKie @gerhardol @mstv