Skip to content

fix: insecure-document-method-29 – prevent potential XSS vulnerability#26163

Closed
girichinna27 wants to merge 6 commits intojenkinsci:masterfrom
girichinna27:vulnerability-fix2
Closed

fix: insecure-document-method-29 – prevent potential XSS vulnerability#26163
girichinna27 wants to merge 6 commits intojenkinsci:masterfrom
girichinna27:vulnerability-fix2

Conversation

@girichinna27
Copy link

Hi Maintainers 👋,

This Pull Request addresses a Semgrep security finding related to the unsafe use of DOM manipulation methods that may lead to Cross-Site Scripting (XSS) vulnerabilities.

🔍 Issue Details

Rule ID: insecure-document-method

Severity: Medium

Rule Message:
User-controlled data passed to methods such as innerHTML, outerHTML, or document.write is considered an anti-pattern and can lead to XSS vulnerabilities.

📍 Affected Location

File Path:
/tools/scanResult/unzipped-1283725236/core/src/main/resources/hudson/PluginManager/_installed.js
Line: 29

✅ Fix Applied

Replaced the unsafe document method usage with a safer alternative that avoids direct HTML injection (for example, using textContent or properly sanitized DOM updates).

🎯 Impact

This change mitigates the risk of XSS attacks by ensuring that user-controlled data is not injected into the DOM using insecure document methods.

The issue was identified and remediated using AI-Guardian, a security analysis tool developed by my company OpsMx.

Thanks for your time and review 🙏

root and others added 4 commits January 22, 2026 17:28
…ctory-disallow-doctype-de-110-qXI6KU9kjQ

fix: semgrep-documentbuilderfactory-disallow-doctype-decl-missing
…-method-29-8L13xoquS0

fix: semgrep-insecure-document-method
@github-actions
Copy link
Contributor

Missing required label for changelog. Requires at least 1 of: bug, developer, dependencies, internal, localization, major-bug, major-rfe, rfe, regression-fix, removed, skip-changelog. Found: .

You can add the required label by adding a comment with the following text: /label <category>

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Jan 22, 2026

Please don't sabotage yourself as a new contributor. Be sure that you:

  • Duplicate the bug before trying to fix it
  • Read and follow the contributing guide
  • Test changes interactively
  • Create automated tests that show the problem is fixed
  • Test changes in the debugger so that you see them execute
  • Run automated tests locally before submitting a pull request
  • Use the pull request template and provide all the information it requests
  • Respond to review comments with changes to address the comment or a clear, concise explanation why you are not making the change
  • Wait at least 3 days before reminding that a review or update is needed

As far as I can tell, you skipped all of those items in your pull request. If you're not willing to do those things, we'll close the pull request. In the interim, I'm marking the pull request as draft so that others do not waste their time reviewing it.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses security hardening in the Jenkins core UI and SVG symbol handling, primarily to mitigate XSS and XML-based attacks.

Changes:

  • Replaces innerHTML with textContent when displaying server error responses in the plugin manager UI to avoid injecting unsanitized HTML into the DOM.
  • Configures the DocumentBuilderFactory used for loading SVG symbols to disallow XML DOCTYPE declarations, reducing exposure to XML entity expansion and related parser attacks.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
core/src/main/resources/hudson/PluginManager/_installed.js Stops writing arbitrary HTML into the #needRestart element by using textContent for error messages, addressing the Semgrep XSS finding.
core/src/main/java/org/jenkins/ui/symbol/Symbol.java Tightens XML parsing for SVG symbols by disallowing DOCTYPE declarations on the DocumentBuilderFactory used in loadSymbol.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MarkEWaite MarkEWaite marked this pull request as draft January 22, 2026 18:10
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request lacks motivation for the XXE change, and the XSS change does not look like it was tested.

if (!rsp.ok) {
rsp.text().then((responseText) => {
document.getElementById("needRestart").innerHTML = responseText;
document.getElementById("needRestart").textContent = responseText;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an adequate solution.

Screenshot 2026-01-22 at 21 38 00

DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
try {
dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniel-beck
Copy link
Member

If this addresses a real vulnerability, you just disclosed it to the world, contrary to our published reporting guidelines. Why?

If it doesn't, what does this PR accomplish?

@daniel-beck
Copy link
Member

The new commit does nothing to address previous feedback, instead it scope-creeps this PR with a change that will significantly negatively impact the plugin ecosystem and not actually improve security.

We're done here.

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.

4 participants