[JENKINS-70240] fix for non http based UpdateCenter URLs cause exception in plugin manager #7520
Conversation
- Local update centers use local json files which are valid
- I have added the value.replace("file:/", ""); as when using a war, it adds 'file:/' to the path
- This checks if the file is a valid file and if true returns FormValidation.ok
Update error message for when invalid URL message appears.
julieheard
changed the title
NotMyFault
added
the
regression-fix
| @@ -1966,6 +1966,13 @@ public HttpResponse doUploadPlugin(StaplerRequest req) throws IOException, Servl | |||
| return FormValidation.error(Messages.PluginManager_emptyUpdateSiteUrl()); | |||
| } | |||
|
|
|||
| //Check to see if the value is a file as some update centers are local json files | |||
There was a problem hiding this comment.
will involve a file lookup for something like http://foo.example.com/bar
e.g.
new File("http://foo.example.com/bar").exists()
Rather than check for something that is obviously bogus perhaps first the URI should be created.
Then if it is a file based one do X, if it is http/https do Y (create another URI from the parsed URI appending the query parameter for the version)
if it is anything else return an FormValidation.warning("unable to check validity of update center due to protocol -> the protocol")
| if (file.isFile()) { | ||
| return FormValidation.ok(); | ||
| } | ||
|
|
There was a problem hiding this comment.
if the url is file://foo but the file does not exist we keep on trucking as if it is HTTP which is doomed to failure.
There was a problem hiding this comment.
Since this functionality is rarely used, I think it is at a high risk of regression during future code changes. For this reason, I think it would be desirable to add a test to ensure this fix does not regress in the future. I think it should be straightforward to ensure that files that exist pass form validation and files that do not exist fail form validation. Such a test could even likely be written without JenkinsRule. An example of a recent change that added a similar form validation test is #7508, and git grep doCheck | grep src.test turns up several more examples.
Updated wording on invalidURL
|
Closing in favor of #7524. |
See JENKINS-70240.
Testing done
Steps:
Before:
After:
This issue was caused by #6886 which did not have any tests written for the method. I have not written any tests for the method as a whole yet.
Proposed changelog entries
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge:upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered (see query).