[JENKINS-46914] [JENKINS-47885] [JENKINS-54568] JavaScript improvements #60
Conversation
daniel-beck
force-pushed
the
JENKINS-46914
branch
from
15dc6a1
to
dc705c4
Compare
| @@ -139,13 +139,12 @@ THE SOFTWARE. | |||
| <f:helpArea /> | |||
| </table> | |||
| <script> | |||
| (function() { | |||
| <!-- place master outside the DOM tree so that it won't creep into the submitted form --> | |||
There was a problem hiding this comment.
This is not actually necessary. An extra row in the submission doesn't really hurt, and since principals with no permissions assigned are actually ignored server-side, not doing this will not change the result.
daniel-beck
changed the title
| @@ -187,7 +187,7 @@ public AuthorizationMatrixNodeProperty newInstance(StaplerRequest req, @Nonnull | |||
|
|
|||
| @Override | |||
| public boolean isApplicable(Class<? extends Node> node) { | |||
| return Node.class.isAssignableFrom(node) && isApplicable(); | |||
| return isApplicable(); | |||
There was a problem hiding this comment.
This only existed as a hack to prevent Kubernetes Plugin from showing the node property, as it passes a wrong class argument.
| } | ||
| if (!e.hasAttribute('data-checked')) { | ||
| FormChecker.delayedCheck("${descriptorPath}/checkName?value="+encodeURIComponent(e.getAttribute("name")),"GET",e.firstChild); | ||
| e.setAttribute('data-checked', 'true'); |
There was a problem hiding this comment.
Since we add calls to Behavior#applySubtree on the <table>, not doing this attribute check would result in all rows being checked again through unrelated operations on the table. #applySubtree on the row doesn't work (and would still result in 1 unnecessary check each time), so do this instead.
src/main/resources/hudson/security/GlobalMatrixAuthorizationStrategy/config.jelly
Outdated
Show resolved
Hide resolved
src/main/resources/hudson/security/GlobalMatrixAuthorizationStrategy/config.jelly
Outdated
Show resolved
Hide resolved
| } | ||
| return true; | ||
| }; | ||
| e = null; <!-- avoid memory leak --> |
There was a problem hiding this comment.
I am really curious about this trick, do you know where it comes from?
From my PoV it seems like premature optimization coming from nowhere ^^ as JS engines have equivalent of Java garbage collector and when a variable is unused, it's cleaned. The e is scoped to the function block and thus is not a global variable.
There was a problem hiding this comment.
No idea. It's been here from the beginning in 2007, jenkinsci/jenkins@d1ff750#diff-3048bbea3ad809584b44aaf822148017R102. Perhaps related to browser bugs from a decade ago?
src/main/resources/hudson/security/GlobalMatrixAuthorizationStrategy/config.jelly
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
| if (!e.hasAttribute('data-checked')) { | ||
| FormChecker.delayedCheck("${descriptorPath}/checkName?value="+encodeURIComponent(e.getAttribute("name")),"GET",e.firstChild); |
There was a problem hiding this comment.
delayedCheck('${descriptorPath with single quote instead of double, it would have been XSS vulnerable.
There was a problem hiding this comment.
Any way we can make this safer? encodeURIComponent this part, too?
There was a problem hiding this comment.
The current version is safe thx to the Jelly escape. Nothing to change, that was just an "informational warning".
EncodeURIComponent will not solve the injection as the malicious code is applied during "jelly compilation" and the encode is called during JavaScript execution. If the malicious code put like ') + alert(1, the encode will be just completely blind :)
daniel-beck
force-pushed
the
JENKINS-46914
branch
from
fed3eb3
to
016e813
Compare
authorizationMatrixwork in declarative snippet generator