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

Possible fix for JENKINS-9806, fix empty customWorkspace -if empry, repla #147

Closed
wants to merge 2 commits into from

Conversation

vjuranek
Copy link
Member

@vjuranek vjuranek commented Jun 2, 2011

Possible fix for JENKINS-9806, fix empty customWorkspace - if emprty, replace it by null (i.e. no custom workspace)

…eplace it by null (i.e. no custom workspace)
@kutzi
Copy link
Member

kutzi commented Jun 2, 2011

IMO this is a quick fix, only, but still better then nothing. A full fix should at least:

  • warn in UI that nothing was entered for the path resp. don't accept empty path
  • check permissions of user the access other job, if the path points to another jobs workspace

BTW: you should use Util.fixEmptyAndTrim() instead of writing your own method

@vjuranek
Copy link
Member Author

vjuranek commented Jun 3, 2011

thanks for comment, I'll improve the patch hopefully soon
PS. thanks for Util.fixEmptyAndTrim() reminder, I somehow overlooked it

@kohsuke
Copy link
Member

kohsuke commented Jun 5, 2011

Should we get this change in for the time being? Or would you like me to wait before you work on a revised version?

@vjuranek
Copy link
Member Author

vjuranek commented Jun 6, 2011

I added warning for users use standard fixEmpty(), so this can be hopefully merged. Not sure about checking permissions, taking into account, that that path to workspace can be different on different slave, there could be simlinks etc., determine if given path is not part of any other workspace could be quite complex. Do we need such complex implementation or some trivial check (e.g. check for names of other project in the given custom path) would be sufficient?

@kutzi
Copy link
Member

kutzi commented Jun 6, 2011

I think for a '100% correct' fix, permissions should be checked, but I also agree that it would be really complex to check them right. So from my sight, it's okay to integrate it, as it is.

@kohsuke
Copy link
Member

kohsuke commented Jun 7, 2011

Extending permission checks on file system access is really hard without OS-level user separation. Fixing that should be separate from this effort.

I'm merging this change now.

@kohsuke
Copy link
Member

kohsuke commented Jun 7, 2011

Merged toward 1.416.

@kohsuke kohsuke closed this Jun 7, 2011
@vjuranek
Copy link
Member Author

vjuranek commented Jun 7, 2011

I created a feature request for it: https://issues.jenkins-ci.org/browse/JENKINS-9899

oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this pull request Feb 17, 2017
https://github.com/jenkinsci/remoting/edit/master/CHANGELOG.md

Fixed issues:

* [JENKINS-40710](https://issues.jenkins-ci.org/browse/JENKINS-40710) - Match headers case-insensitively in `JnlpAgentEndpointResolver` in order to be compliant with HTTP2 lower-case headers. ([PR jenkinsci#139](jenkinsci/remoting#139), [PR jenkinsci#140](jenkinsci/remoting#140))
* [JENKINS-41513](https://issues.jenkins-ci.org/browse/JENKINS-41513) - Prevent `NullPointerException` in `JnlpAgentEndpointResolver` when receiving a header with `null` name. ([PR jenkinsci#140](jenkinsci/remoting#140))
* [JENKINS-41852](https://issues.jenkins-ci.org/browse/JENKINS-41852) - Fix exported object pinning logic to prevent release due to the integer overflow. ([PR jenkinsci#148](jenkinsci/remoting#148))

Improvements:

* [JENKINS-41730](https://issues.jenkins-ci.org/browse/JENKINS-41730) - Add the new `org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver.ignoreJenkinsAgentProtocolsHeader` property, which allows specifying a custom list of supported protocols instead of the one returned by the Jenkins master. ([PR jenkinsci#146](jenkinsci/remoting#146))
* Print the Filesystem Jar Cache directory location in the error message when this cache directory is not writable. ([PR jenkinsci#143](jenkinsci/remoting#143))
* Replace `MimicException` with the older `ProxyException` when serializing non-serializable exceptions thrown by the remote code. ([PR jenkinsci#141](jenkinsci/remoting#141))
* Use OID of the `ClassLoaderProxy` in error message when the proxy cannot be located in the export table. ([PR jenkinsci#147](jenkinsci/remoting#147))
olivergondza pushed a commit that referenced this pull request Mar 13, 2017
https://github.com/jenkinsci/remoting/edit/master/CHANGELOG.md

Fixed issues:

* [JENKINS-40710](https://issues.jenkins-ci.org/browse/JENKINS-40710) - Match headers case-insensitively in `JnlpAgentEndpointResolver` in order to be compliant with HTTP2 lower-case headers. ([PR #139](jenkinsci/remoting#139), [PR #140](jenkinsci/remoting#140))
* [JENKINS-41513](https://issues.jenkins-ci.org/browse/JENKINS-41513) - Prevent `NullPointerException` in `JnlpAgentEndpointResolver` when receiving a header with `null` name. ([PR #140](jenkinsci/remoting#140))
* [JENKINS-41852](https://issues.jenkins-ci.org/browse/JENKINS-41852) - Fix exported object pinning logic to prevent release due to the integer overflow. ([PR #148](jenkinsci/remoting#148))

Improvements:

* [JENKINS-41730](https://issues.jenkins-ci.org/browse/JENKINS-41730) - Add the new `org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver.ignoreJenkinsAgentProtocolsHeader` property, which allows specifying a custom list of supported protocols instead of the one returned by the Jenkins master. ([PR #146](jenkinsci/remoting#146))
* Print the Filesystem Jar Cache directory location in the error message when this cache directory is not writable. ([PR #143](jenkinsci/remoting#143))
* Replace `MimicException` with the older `ProxyException` when serializing non-serializable exceptions thrown by the remote code. ([PR #141](jenkinsci/remoting#141))
* Use OID of the `ClassLoaderProxy` in error message when the proxy cannot be located in the export table. ([PR #147](jenkinsci/remoting#147))

(cherry picked from commit 815da8a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants