[FIX JENKINS-39433] Make URI encoding check into admin monitor #2661
Conversation
daniel-beck
added
the
needs-more-reviews
| @@ -4454,20 +4434,21 @@ public void doResources(StaplerRequest req, StaplerResponse rsp) throws IOExcept | |||
| * Checks if container uses UTF-8 to decode URLs. See | |||
| * http://wiki.jenkins-ci.org/display/JENKINS/Tomcat#Tomcat-i18n | |||
| */ | |||
| @Restricted(NoExternalUse.class) | |||
| @RestrictedSince("since TODO") | |||
| @Deprecated | |||
There was a problem hiding this comment.
I would also add the deprecated Javadoc comment with usage guidelines
There was a problem hiding this comment.
There is no legitimate usage. The new admin monitor is now also @Restricted.
| import hudson.FilePath; | ||
| import hudson.Functions; | ||
| import hudson.Launcher; | ||
| import hudson.*; |
There was a problem hiding this comment.
hmm.... If you use IDEA, you can disable it
| if (!expected.equals(value)) | ||
| return FormValidation.warningWithMarkup(Messages.Hudson_NotUsesUTF8ToDecodeURL()); | ||
| return FormValidation.ok(); | ||
| return ExtensionList.lookup(URICheckEncodingMonitor.class).get(0).doCheckURIEncoding(request); |
There was a problem hiding this comment.
I would add the syntax sugar to the URICheckEncodingMonitor class (as a public static method?). lookup.get(0) is formally incorrect, hence that method may at least do the check inside and throw IllegalState or log&return warnings
There was a problem hiding this comment.
lookup.get(0) is formally incorrect
Could you explain? Is there an alternative I missed (other than moving the code into the monitor)?
There was a problem hiding this comment.
So technically it is possible to remove the AdministrativeMonitor from the extension list. E.g.
...
- if there is a bug there, as an admin I can remove it from the extension list using the System Groovy script.
- Then
ExtensionList.lookup(URICheckEncodingMonitor.class)will start returning the empty list .get(0).doCheckURIEncoding(request)will fail with RuntimeException in this case
It would be more relevant to return FormValidation error in such case than to blow u the UI
There was a problem hiding this comment.
It would be more relevant to return FormValidation error in such case than to blow u the UI
These deprecated methods aren't used anymore. I tried to be nice here instead of just returning false or something similar…
| <?jelly escape-by-default='true'?> | ||
| <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"> | ||
| <j:if test="${it.checkEnabled}"> | ||
| <script> |
There was a problem hiding this comment.
I am not sure you really need JavaScript and AJAX here. It will unlikely work properly since the administrative monitor needs to be calculated before the message is shown. Or not?
There was a problem hiding this comment.
This admin monitor is always active, similar to ReverseProxyMonitor, and only shows a message upon AJAX failure. I just moved the existing code over, it works exactly the same otherwise.
|
I agree with merge after the discussion |
|
@jenkinsci/code-reviewers |
| package jenkins.diagnostics; | ||
|
|
||
| import hudson.Extension; | ||
| import hudson.model.*; |
There was a problem hiding this comment.
I have the threshold for a * import at 15. This seems reasonable.
|
@jenkinsci/code-reviewers anybody else? |
oleg-nenashev
added
ready-for-merge
Basically move the code into an administrative monitor.
Tested manually that sending a different string than expected results in a warning, and that the deprecated code still works as expected despite being obsolete.
The original request in JENKINS-39433 was to be able to disable the check, but the implementation was a hack anyway. Now it's an admin monitor, and can be disabled in Configure System as a side effect.