[JEP-222] Add experimental WebSocket support + Jetty update in Winstone + Remoting 4.0 - #1
[JEP-222] Add experimental WebSocket support + Jetty update in Winstone + Remoting 4.0#1Chi-Song-Owen wants to merge 298 commits into
Conversation
[JENKINS-59631] Fix styling issue in build history table
* Try to execute flyweight tasks on the master * Fix offline behavior, add tests * Remove obsolete TODOs, also check #isAcceptingTasks
[JENKINS-59793] Avoid hanging jobs with faulty SubTasks
The jenkins-slave.xml file has been copied to the windows-slave-installer-module, but a unused (old) copy is still present in the jenkins core.
…pane of the Plugin Manager (#4298) * refs #JENKINS-59665 Fixed sorting for uninstall column of installed in plugin manager * refs #JENKINS-59665 removed test attribute
* Hide password form fields by default * Trying to bypass enforced autocompletion by just having a test field at first Something like onfocus didn't work, you'd tab through form elements and unless you filled in the user name, changing the form field to password would cause it to autocomplete. It looks like, at least in Mac/Firefox, going from plain text to password in the 'oninput' event handler works. The plain text is revealed neither with typing nor pasting. * Update core/src/main/resources/lib/form/password.jelly Co-Authored-By: daniel-beck <daniel-beck@users.noreply.github.com> * Use previously defined value * Make new password form fields opt-out * Add support for redacting form secrets in new password field * Have a password value pre-set in Jelly * Fix method * Fix test by clicking on the button to change the password * Forgot period separator between class and property name
[JENKINS-59508] Sidebar links show full label on hover
Switch from maven-jenkins-dev-plugin to upstream jetty-maven-plugin
Add StandardOpenOption.CREATE flag to create FileChannelWriter to avoid full fs flush and 5sec log operation on creating empty file with CephFS as a storage
* JENKINS-6722 Show slave error status on Dashboard * Update executors.properties
…unds on the plugin side (#3935) * JENKINS-56553 Make proxy config compatible with JCASC * Use DataBoundSetters as other fields are optional * Fix Jelly for ProxyConfiguration * Use default field name now that field matches * Fix validate proxy * Change field type * After review * Update core/src/main/java/jenkins/model/Jenkins.java Co-Authored-By: timja <t.jacomb@kainos.com> * Update core/src/main/java/jenkins/model/Jenkins.java Co-Authored-By: timja <t.jacomb@kainos.com> * Update core/src/main/java/hudson/ProxyConfiguration.java Co-Authored-By: timja <t.jacomb@kainos.com> * Update core/src/main/java/jenkins/model/Jenkins.java Co-Authored-By: timja <t.jacomb@kainos.com> * Update core/src/main/java/jenkins/model/Jenkins.java Co-Authored-By: timja <t.jacomb@kainos.com> * Update core/src/main/java/hudson/ProxyConfiguration.java Co-Authored-By: Oleg Nenashev <o.v.nenashev@gmail.com> * Tweak javadoc * Align fields * Update core/src/main/java/hudson/ProxyConfiguration.java Co-Authored-By: Adrien Lecharpentier <adrien.lecharpentier@gmail.com>
Document workaround for Stapler data binding bug in repeatableProperty
…ling events (#4345) * Rejuvenate log levels * clean up * Reverting the plugin download Co-Authored-By: Oleg Nenashev <o.v.nenashev@gmail.com>
… Fix JavaScript error in Plugin Manager when optional dependency metadata cannot be retrieved (#3904) * [JENKINS-56173] Uncaught TypeError: Cannot read property 'jenkinsPluginMetadata' of undefined * [JENKINS-56173] missing dependencies covers both not installed and not enabled * [JENKINS-56152] i18n: DirectlyDependentPlugins.title / IndirectlyDependentPlugins.title * [JENKINS-56152] replace `version X` with `(X)` * [JENKINS-56152] simplify messages; replace `version X` with `(X)`; use higher instead of later * [JENKINS-56173] Uncaught Exception: TypeError: Cannot read property 'get' of undefined at _createResponder * Update PluginWrapperTest * Switch to descriptive class for plugin version
[JENKINS-42658] - Fix NPE in Agent API when the agent is offline (e.g. retrieving agent version or OS description)
Inlining test-pom into test and removing test-jdk8
…IZABLE, ES_COMPARING_STRINGS_WITH_EQ) (#4379) * fixed Spotbugs issues (SE_COMPARATOR_SHOULD_BE_SERIALIZABLE, ES_COMPARING_STRINGS_WITH_EQ) * replaced wildcard import with specific imports * added missing import * fixed spotbugs issues * removed accidently added import
…stages (#4207) * [JENKINS-59412] Clarify that build history does not include pipeline stages * Clarify that pipelines are merely an example of tasks that may not be listed
Simplify CoreUpdateMonitor/message.properties: changelog.url → jenkins-version.properties
* Add the maintainer checklist to the pull request template * Add checklist items for LTS * Add "Proposed changelog entries" to the pull request template * Pull request template: `Internal: ` prefix is now added automatically by the Core Changelog generator jenkinsci/core-changelog-generator#6 * Apply suggestions from review by @MarkEWaite Co-Authored-By: Mark Waite <mark.earl.waite@gmail.com> Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
Upload draft Changelog YAMLs to GitHub Release assets
…or-yaml Revert "Upload draft Changelog YAMLs to GitHub Release assets"
…g YAMLs to Release Drafter
… release metadata
[JENKINS-60740] - Rely on GitHub Actions to generate the release draft YAMLs
Release Drafter Fix: Properly inherit the parent config
[JENKINS-60716] Retain causes of a LogRotator failure for diagnosis
* Spotbugs: Fixed comparing strings with equals instead of == * Spotbugs: Do not return null in @nonnull marked method, instead return empty string. * Added Suppress for Spotbugs * Apply suggestions from code review Co-Authored-By: Oleg Nenashev <o.v.nenashev@gmail.com> * fixed javadoc Co-authored-by: Oleg Nenashev <o.v.nenashev@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| this.sec10 = new TimeSeries(initialValue, decay, 6 * (int) TimeUnit.HOURS.toMinutes(6)); | ||
| this.min = new TimeSeries(initialValue, decay, (int) TimeUnit.DAYS.toMinutes(2)); | ||
| this.hour = new TimeSeries(initialValue, decay, (int) TimeUnit.DAYS.toHours(56)); |
There was a problem hiding this comment.
🔴 Load-statistics graphs keep three to six times more data than intended, tripling memory use
The three history windows for load statistics are sized with the wrong multipliers (new TimeSeries(...) at core/src/main/java/hudson/model/MultiStageTimeSeries.java:100-102) instead of the previous 1 hour / 1 day / 4 weeks, so every statistic retains far more samples and each one uses about three times more memory than before.
Impact: Memory consumption for load statistics grows roughly threefold on instances with many labels/nodes, and the trend data no longer matches the documented retention periods.
Arithmetic of the refactored constants
Old values: 6*60 = 360 (10-second samples for 1 hour), 60*24 = 1440 (1-minute samples for 1 day), 28*24 = 672 (hourly samples for 4 weeks).
New values: 6 * TimeUnit.HOURS.toMinutes(6) = 2160, TimeUnit.DAYS.toMinutes(2) = 2880, TimeUnit.DAYS.toHours(56) = 1344 — i.e. 6×, 2× and 2× too large. hudson.model.TimeSeries.update grows float[] history up to historySize, so the arrays really do grow to these sizes. The field javadoc at core/src/main/java/hudson/model/MultiStageTimeSeries.java:77-90 still documents 1 hour / 1 day / 4 weeks, and the class javadoc line stating "one instance uses about 8KB" was removed rather than corrected.
| this.sec10 = new TimeSeries(initialValue, decay, 6 * (int) TimeUnit.HOURS.toMinutes(6)); | |
| this.min = new TimeSeries(initialValue, decay, (int) TimeUnit.DAYS.toMinutes(2)); | |
| this.hour = new TimeSeries(initialValue, decay, (int) TimeUnit.DAYS.toHours(56)); | |
| this.sec10 = new TimeSeries(initialValue, decay, 6 * (int) TimeUnit.HOURS.toMinutes(1)); | |
| this.min = new TimeSeries(initialValue, decay, (int) TimeUnit.DAYS.toMinutes(1)); | |
| this.hour = new TimeSeries(initialValue, decay, (int) TimeUnit.DAYS.toHours(28)); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| String param = params.getParameter(0); | ||
| Result v = fromString(param.replace('-', '_')); | ||
| if (v==null) | ||
| if (v== FAILURE) { |
There was a problem hiding this comment.
🟡 Command line option that takes a build status now rejects the valid value "FAILURE"
A build status typed on the command line is rejected as unknown whenever it resolves to failure (if (v== FAILURE) at core/src/main/java/hudson/model/Result.java:198), so users can no longer pass the perfectly valid value FAILURE.
Impact: Any CLI command taking a build status fails with "No such status 'FAILURE'" when the user legitimately asks for FAILURE.
Why the new comparison is wrong
Result.fromString (core/src/main/java/hudson/model/Result.java:150-155) never returns null; it returns FAILURE as its fallback for unrecognized input. The previous if (v == null) check was therefore dead code, but replacing it with v == FAILURE makes the handler reject the legitimate input "FAILURE"/"failure" while still accepting all other unknown strings (which silently become FAILURE). The check must distinguish the fallback from a genuine match, e.g. by comparing the normalized parameter text against the name.
| if (v== FAILURE) { | |
| if (v == FAILURE && !"FAILURE".equalsIgnoreCase(param.replace('-', '_'))) { |
Was this helpful? React with 👍 or 👎 to provide feedback.
| \u041d\u043e\u0432\u0430\u0442\u0430 \u0432\u0435\u0440\u0441\u0438\u044f \u043d\u0430 Jenkins ({0}) \u0435 \u043d\u0430\u043b\u0438\u0447\u043d\u0430 \u0437\u0430 <a href="{1}">\u0438\u0437\u0442\u0435\u0433\u043b\u044f\u043d\u0435</a>\ | ||
| (<a href="${changelog.url}">\u0441\u043f\u0438\u0441\u044a\u043a \u0441 \u043f\u0440\u043e\u043c\u0435\u043d\u0438\u0442\u0435</a>). |
There was a problem hiding this comment.
🟡 Changelog link in the "new version available" notice is broken in every translated language
The translated update notices were moved out of the build-filtered resource folder (rename to core/src/main/resources/hudson/model/UpdateCenter/CoreUpdateMonitor/message_bg.properties:31-32 and the equivalent files for all other locales) while still containing the build-time placeholder ${changelog.url}, so the placeholder is no longer replaced.
Impact: Non-English users see a broken "changelog" link (its target is the literal text ${changelog.url}) in the core-update notification.
How the substitution was lost
Previously these files lived in core/src/filter/resources/... where Maven resource filtering replaced ${changelog.url} (defined in pom.xml:99). This PR renamed them into core/src/main/resources/..., which is not filtered, and changed the English message.properties to use the {2} message argument supplied by message.jelly (${app.CHANGELOG_URL}). The translations were not converted to {2}; roughly two dozen message_*.properties files still contain <a href="${changelog.url}">.
Prompt for agents
All localized message_*.properties files under core/src/main/resources/hudson/model/UpdateCenter/CoreUpdateMonitor/ still use the Maven filter placeholder ${changelog.url} for the changelog hyperlink. These files used to live under core/src/filter/resources (which is resource-filtered) and were moved to core/src/main/resources in this PR, so the placeholder is now emitted verbatim into the HTML. The English message.properties was updated to take the URL as message argument {2} (message.jelly passes app.CHANGELOG_URL). Update every translated NewVersionAvailable entry to use {2} instead of ${changelog.url}.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (lbl == null && h.canTake(p) == null && masterComputer.isOnline() && masterComputer.isAcceptingTasks()) { | ||
| // The flyweight task is not tied to a specific label, so execute on master if possible. | ||
| // This will ensure that actual agent disconnects do not impact flyweight tasks randomly assigned to them. | ||
| return createFlyWeightTaskRunnable(p, masterComputer); | ||
| } | ||
|
|
||
| // Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it. | ||
| hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)); | ||
| Map<Node, Integer> hashSource = new HashMap<>(h.getNodes().size()); | ||
|
|
||
| for (Node n : h.getNodes()) { | ||
| hashSource.put(n, n.getNumExecutors() * 100); |
There was a problem hiding this comment.
🔴 Lightweight tasks pinned to a label that only the built-in node has can never start
The built-in node was dropped from the list of candidate machines for lightweight tasks (removal of the entry that always added it, now only agents are added at core/src/main/java/hudson/model/Queue.java:1749-1753), so such a task that requires a label held only by that node is never assigned anywhere.
Impact: Builds of lightweight tasks restricted to a custom label that only the master node matches stay queued forever.
Control flow
makeFlyWeightTaskBuildable handles two cases explicitly: the label equals the master self-label (line 1734) and no label at all (line 1743). Any other label falls through to the consistent-hash selection, whose hashSource is now built solely from h.getNodes() — which never contains the master node. Previously hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)) guaranteed the master was a candidate here (with the explicit comment "Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it"), and the loop at lines 1759-1772 then filtered by lbl.contains(n). With the master removed, a flyweight task whose assigned label expression matches only the master (e.g. a user-defined label on the master, or an expression such as master&&foo) finds no candidate and returns null, so it keeps waiting for node provisioning.
Note also that masterComputer (line 1733) is dereferenced unconditionally at line 1743 without a null check, unlike the old code path which went through n.toComputer() null checks.
(Refers to lines 1743-1753)
Prompt for agents
In Queue.makeFlyWeightTaskBuildable the line that unconditionally added the Jenkins master to hashSource (hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1))) was removed while adding the new 'prefer master when no label' shortcut. As a result, flyweight tasks with an assigned label that is satisfied only by the master (but is not the master self-label) can no longer be scheduled at all, because the consistent-hash candidate set only contains h.getNodes(). Keep the master in hashSource for the labeled path (the subsequent loop already filters with lbl.contains(n) and canTake), while retaining the new behaviour of preferring the master for unlabeled flyweight tasks. Also consider null-checking masterComputer before calling isOnline()/isAcceptingTasks().
Was this helpful? React with 👍 or 👎 to provide feedback.
| </td> | ||
| <td class="center pane uninstall"> | ||
| <j:choose> | ||
| <j:when test="${p.hasMandatoryDependents() or p.hasMandatoryDependents()}"> |
There was a problem hiding this comment.
🟡 Plugin list marks uninstall availability using the same check twice, ignoring dependencies
The flag telling the plugin table whether uninstalling is possible is computed from the same condition twice (p.hasMandatoryDependents() or p.hasMandatoryDependents() at core/src/main/resources/hudson/PluginManager/installed.jelly:119), so the second, presumably dependency-related, condition is never evaluated.
Impact: The uninstall column of the installed-plugins page is annotated with an incorrect value for plugins whose dependencies (rather than dependents) matter.
Detail
The surrounding markup at lines 132-145 distinguishes p.hasMandatoryDependents() (dependent-list) from p.hasMandatoryDependencies() (dependency-list), which strongly suggests the second operand of the or was meant to be p.hasMandatoryDependencies(). As written the expression is a copy/paste duplicate and the data attribute consumed by _table.js is therefore wrong for plugins that only have mandatory dependencies.
Prompt for agents
installed.jelly computes uninstallPossible with the duplicated expression `p.hasMandatoryDependents() or p.hasMandatoryDependents()`. Determine the intended second condition (most likely p.hasMandatoryDependencies(), as used a few lines below) and also verify the polarity of the variable name: having mandatory dependents normally means uninstall is NOT possible, so the flag may need to be inverted for the consuming JavaScript in hudson/PluginManager/_table.js.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try (ClientSideImpl connection = new ClientSideImpl(out)) { | ||
| session.addMessageHandler(InputStream.class, is -> { | ||
| try { | ||
| connection.handle(new DataInputStream(is)); | ||
| } catch (IOException x) { | ||
| LOGGER.log(Level.WARNING, null, x); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📝 Info: WebSocket CLI registers its message handler after the connection is established
client.connectToServer(...) returns an already-open session; the addMessageHandler call happens afterwards. Any frame the server pushes between the upgrade and handler registration would be delivered with no handler installed (Tyrus logs/throws rather than buffering). In practice the server only writes after receiving the start frame, but registering the handler from CLIEndpoint.onOpen would remove the race entirely.
Was this helpful? React with 👍 or 👎 to provide feedback.
| break; | ||
| case NO_SUCH_PLUGIN: | ||
| returnCode = RETURN_CODE_NO_SUCH_PLUGIN; | ||
| } | ||
|
|
||
| if (returnCode == 0) { | ||
| for (PluginWrapper.PluginDisableResult oneDependentResult : result.getDependentsDisableStatus()) { | ||
| returnCode = getResultCode(oneDependentResult); | ||
| if (returnCode != 0) { | ||
| break; | ||
| break; | ||
| default: | ||
| for (PluginWrapper.PluginDisableResult oneDependentResult : result.getDependentsDisableStatus()) { | ||
| returnCode = getResultCode(oneDependentResult); | ||
| if (returnCode != 0) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
📝 Info: Refactored disable-plugin result code switch is behaviour-preserving
Replacing the post-switch if (returnCode == 0) block with a default: branch is equivalent here: the only cases that set a non-zero code (NOT_DISABLED_DEPENDANTS, NO_SUCH_PLUGIN) now break, and every remaining status (DISABLED, ALREADY_DISABLED, ERROR_DISABLING) previously left returnCode == 0 and therefore also ran the dependents loop.
(Refers to lines 217-232)
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -110,7 +123,7 @@ THE SOFTWARE. | |||
| </st:include> | |||
| <td class="pane" align="center" valign="middle"> | |||
| <j:if test="${e.hasStopPermission()}"> | |||
| <l:stopButton href="${rootURL}/${c.url}${url}/stop" confirm="${%confirm(exe.fullDisplayName)}" alt="${%terminate this build}" /> | |||
| <l:stopButton href="${rootURL}/${c.url}${url}/stopBuild?runExtId=${h.urlEncode(exe.externalizableId)}" confirm="${%confirm(exe.fullDisplayName)}" alt="${%terminate this build}" /> | |||
There was a problem hiding this comment.
📝 Info: Stop button now targets a build-specific endpoint; relies on lenient expression evaluation
exe.externalizableId is evaluated for every executable, including ones that are not Run instances (e.g. Pipeline flyweight executables). JEXL resolves the unknown property to null, which is why Functions.urlEncode was made null-tolerant in this PR; the resulting empty runExtId is normalised back to null by @QueryParameter(fixEmpty = true) so doStopBuild falls back to unconditional interruption. Worth confirming that the missing-property path does not log noisy warnings for such executables.
Was this helpful? React with 👍 or 👎 to provide feedback.
| public HttpResponse doWs() { | ||
| if (!WebSockets.isSupported()) { | ||
| return HttpResponses.notFound(); | ||
| } | ||
| Authentication authentication = Jenkins.getAuthentication(); | ||
| return WebSockets.upgrade(new WebSocketSession() { |
There was a problem hiding this comment.
🟥 New WebSocket CLI endpoint performs no origin/CSRF check, allowing cross-site command execution
CLIAction.doWs upgrades any request to a WebSocket and then executes CLI commands under Jenkins.getAuthentication() (i.e. the session identity of the browser making the request). WebSocket upgrade requests are not subject to the same-origin policy or CORS, and browsers attach Jenkins session cookies to them, so a page on an attacker-controlled site can open ws(s)://jenkins/cli/ws in the victim's browser and drive arbitrary CLI commands (create/delete jobs, run groovy for admins, etc.). The classic cli?remoting=false full-duplex endpoint is protected by the crumb/CSRF filter for POSTs; the WebSocket upgrade path bypasses that entirely (WebSockets.upgrade in core/src/main/java/jenkins/websocket/WebSockets.java:56-75 performs no Origin validation).
Was this helpful? React with 👍 or 👎 to provide feedback.
| <td class="pane details"> | ||
| <div> | ||
| <a href="${p.url}" class="display-name"> | ||
| <a href="${p.url}" target="_blank" class="display-name"> |
There was a problem hiding this comment.
🟨 External plugin links opened in a new tab without noopener protection
Plugin URLs coming from update-center metadata (p.url, p.wiki) are now rendered with target="_blank" but without rel="noopener noreferrer" (core/src/main/resources/hudson/PluginManager/installed.jelly:82, core/src/main/resources/hudson/PluginManager/table.jelly:99, core/src/main/resources/lib/hudson/thirdPartyLicenses.jelly:42,54). The opened page receives a window.opener reference and can navigate the original Jenkins tab to a phishing page (reverse tabnabbing), and the Referer header leaks the Jenkins URL.
Was this helpful? React with 👍 or 👎 to provide feedback.
20 participants
See JEP-222. Downstream of
jenkinsci/winstone#79+jenkinsci/remoting#357+jenkinsci/jenkins-test-harness#183.Proposed Changelog Entries