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

Fix workspace path regressions #243

Merged

Conversation

@MarkEWaite
Copy link
Contributor

commented May 18, 2017

The security fixes in git client plugin 2.4.4 shifted from preferring the system temporary directory (with default permissions on the files) to preferring a temporary directory adjacent to the workspace directory. That change exposed the git client plugin credentials code (command line git, with all its complications) to the path name of the workspace, where previously the credentials code only used the path name to the system temporary directory.

This fixes the following bugs:

  • JENKINS-44041 - Windows authenticated git checkout fails if '(' or ')' in path to workspace
  • JENKINS-43931 - Windows authenticated git checkout fails if ' ' in path to workspace
  • JENKINS-44127 - Authenticated git checkout fails if '%' in path to workspace (Windows & Linux)
  • JENKINS-44301 - Authenticated git checkout fails if '%' in path to workspace (Windows & Linux)

This has completed initial testing in my automated tests, and in my interactive tests, and will run through another 1-2 days of interactive testing before I actually release it.

MarkEWaite added 2 commits May 18, 2017
JENKINS-44041 - Windows authenticated git checkout fails if '(' or ')' in path to workspace
JENKINS-43931 - Windows authenticated git checkout fails if ' ' in path to workspace
JENKINS-44127 - Authenticated git checkout fails if '%' in path to workspace (Windows & Linux)
JENKINS-44041 - Windows authenticated git checkout fails if '(' or ')' in path to workspace
JENKINS-43931 - Windows authenticated git checkout fails if ' ' in path to workspace
JENKINS-44127 - Authenticated git checkout fails if '%' in path to workspace (Windows & Linux)

Also safeguards against use for "`" (grave) in a workspace path.
Jenkins already guards against that, but the added safety check is low
cost and passes the credentials tests.
@MarkEWaite MarkEWaite requested a review from reviewbybees May 18, 2017
Directly tests that createTempFile is using the expected parent
directory.
Copy link
Member

left a comment

🐝 looks good

}

@Parameterized.Parameters(name = "{0}")
public static Collection workspaceDirNames() {

This comment has been minimized.

Copy link
@varyvol

varyvol May 19, 2017

This is not used anywhere?

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite May 19, 2017

Author Contributor

That is a parameterized JUnit test. The Parameterized annotation causes the JUnit executor to call this method to assemble the arguments to the constructor for this test class.

It allows me to readily construct many different cases which all use the same assertion. This particular case runs about 10 tests, one for each of the different workspace directory names in the workspaceDirNames method

This comment has been minimized.

Copy link
@varyvol

varyvol May 19, 2017

Didn't know. Thanks for the explanation!

@jglick
jglick approved these changes May 19, 2017
Copy link
Member

left a comment

Best we can do I suppose. As previously mentioned, I suspect we could get rid of SSH_ASKPASS altogether, though there could still be problems with unusual characters in the pathname for GIT_SSH, depending on what kind of process interprets that filename (I hope not a shell script).

@MarkEWaite

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

@jglick I fully intend to remove as much of the SSH_ASKPASS "mess" as I can in a future change.

@MarkEWaite MarkEWaite merged commit f4a6123 into jenkinsci:master May 19, 2017
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
@MarkEWaite MarkEWaite deleted the MarkEWaite:master-workspace-path-safety-checks branch May 19, 2017
@jmellor

This comment has been minimized.

Copy link

commented May 19, 2017

I can confirm that this git-client.hpi fixes JENKINS-44301 (2.4.5 bad regression: unable to handle previously-acceptable branch names)

@flavius

This comment has been minimized.

Copy link

commented May 23, 2017

Original reporter of JENKINS-44420 here, I confirm it fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.