Skip to content

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented May 17, 2022

JENKINS-68562 Fix Git checkouts for controllers running on Windows

The security fix in b295606 broke all Git checkouts that use remotes with an explicit protocol on Jenkins controllers running on Windows. This PR fixes that issue by catching InvalidPathException in the relevant location.

I have not tested the PR (I do not have access to a Windows machine right now). I am not sure if we easily can create automated tests with non-file remotes that would exercise this behavior. We should be able to test it at surface level using mocks or maybe via a complex WireMock harness for a more thorough test.

Just filing a draft PR for now since I have not tested anything.

CC @Pldi23

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Online help has been added and reviewed for any new or modified fields
  • I have interactively tested my changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

@dwnusbaum dwnusbaum changed the base branch from v4.11.x to master May 17, 2022 18:56
@github-actions github-actions bot added the test label May 17, 2022
@MarkEWaite
Copy link
Contributor

I've confirmed with interactive testing on Windows that the change resolves the issue as reported in JENKINS-68562

@MarkEWaite
Copy link
Contributor

I think we want the destination branch to be the v4.11.x branch rather than the master branch so that we can release the change with as few differences as possible. The changes on the master branch seem fine, but being more conservative in this case seems worth the effort.

@dwnusbaum
Copy link
Member Author

@MarkEWaite Ok, up to you as far as basing this against v4.11.x vs master. Daniel and I also discussed it, and the changes between those versions seemed pretty harmless so we figured it probably wouldn't matter either way.

@MarkEWaite
Copy link
Contributor

@MarkEWaite Ok, up to you as far as basing this against v4.11.x vs master. Daniel and I also discussed it, and the changes between those versions seemed pretty harmless so we figured it probably wouldn't matter either way.

That sounds great to me. Targeting the master branch is actually easier for me.

I've run the unit test on Windows with the change and confirmed that the test passes.

I've run the unit test on Windows without the change and confirmed that the test fails.

Thanks for writing the test!

@dwnusbaum
Copy link
Member Author

Ok, given the new unit test and that @MarkEWaite has very generously tested this interactively, I think it's ready for review.

@dwnusbaum dwnusbaum marked this pull request as ready for review May 17, 2022 19:47
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

The change looks right to me. I've tested interactively and confirmed that it is well-behaved in cases that were failing with the version in release 4.11.2.

@MarkEWaite
Copy link
Contributor

I've run the automated tests on my RHEL-8, Windows, and other operating systems. Working well.

I plan to release a new version today

@MarkEWaite MarkEWaite added bugfix Fixes a bug - used by Release Drafter and removed test labels May 18, 2022
@MarkEWaite MarkEWaite merged commit 170f24f into jenkinsci:master May 18, 2022
@MarkEWaite
Copy link
Contributor

MarkEWaite commented May 18, 2022

Release build has started for 4.11.3. Releases from other branches will need a backport of the fix.

@dwnusbaum dwnusbaum deleted the JENKINS-68562 branch May 18, 2022 02:42
@MarkEWaite
Copy link
Contributor

Git plugin 4.11.3 and 4.9.2 have been released with this fix. Thanks @dwnusbaum !

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

Labels

bugfix Fixes a bug - used by Release Drafter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants