fix(security): escape repo-derived strings in Git sidebar rendering (XSS)

2026-08-23 23:10:17 +05:30 · 2026-08-23 19:31:33 +05:30
parent c43caf3902
commit 0604c65683
2 changed files with 169 additions and 11 deletions
@@ -1,3 +1,15 @@
+// Escape repo-derived strings (branch/file names, commit messages, git stderr) before
+// interpolating them into innerHTML. Quotes are included so attribute contexts
+// (data-file, data-branch) cannot be broken out of.
+function escapeHtml(str) {
+  return String(str)
+    .replace(/&/g, '&amp;')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;')
+    .replace(/"/g, '&quot;')
+    .replace(/'/g, '&#39;');
+}
+
 function renderGitPanel(
  container,
  { gitStatus, gitDiff, gitStage, gitCommit, gitLog, gitBranches, gitCheckout, gitPush, gitPull }
@@ -47,7 +59,7 @@ function renderGitPanel(
    if (!status || !changesEl) return;

    if (status.error) {
-      changesEl.innerHTML = `<p class="git-info">${status.error}</p>`;
+      changesEl.innerHTML = `<p class="git-info">${escapeHtml(status.error)}</p>`;
      return;
    }

@@ -65,11 +77,11 @@ function renderGitPanel(
      changesEl.innerHTML = files
        .map(
          (f) => `
-                <div class="git-file" data-file="${f.file}">
+                <div class="git-file" data-file="${escapeHtml(f.file)}">
                    <span class="git-file-status" style="color:${f.color}">${f.status}</span>
-                    <span class="git-file-name">${f.file}</span>
-                    <button class="git-diff-btn" data-file="${f.file}" title="View diff">diff</button>
-                    <button class="git-stage-btn" data-file="${f.file}" title="Stage file">+</button>
+                    <span class="git-file-name">${escapeHtml(f.file)}</span>
+                    <button class="git-diff-btn" data-file="${escapeHtml(f.file)}" title="View diff">diff</button>
+                    <button class="git-stage-btn" data-file="${escapeHtml(f.file)}" title="Stage file">+</button>
                </div>
            `
        )
@@ -101,8 +113,8 @@ function renderGitPanel(
          .map(
            (entry) => `
                <div class="git-log-entry">
-                    <div class="git-log-msg">${entry.message}</div>
-                    <div class="git-log-meta">${entry.date?.substring(0, 10) || ''} &middot; ${entry.author_name || ''}</div>
+                    <div class="git-log-msg">${escapeHtml(entry.message)}</div>
+                    <div class="git-log-meta">${entry.date?.substring(0, 10) || ''} &middot; ${escapeHtml(entry.author_name || '')}</div>
                </div>
            `
          )
@@ -127,7 +139,7 @@ function renderGitPanel(
    if (!result) return;

    if (result.error) {
-      branchesEl.innerHTML = `<p class="git-info">${result.error}</p>`;
+      branchesEl.innerHTML = `<p class="git-info">${escapeHtml(result.error)}</p>`;
      return;
    }

@@ -142,9 +154,9 @@ function renderGitPanel(
    branchesEl.innerHTML = names
      .map(
        (name) => `
-                <div class="git-branch-item${name === current ? ' git-branch-current' : ''}" data-branch="${name}">
-                    <span class="git-branch-name">${name === current ? '&#9679; ' : ''}${name}</span>
-                    ${name === current ? '' : `<button class="git-checkout-btn" data-branch="${name}" title="Checkout branch">checkout</button>`}
+                <div class="git-branch-item${name === current ? ' git-branch-current' : ''}" data-branch="${escapeHtml(name)}">
+                    <span class="git-branch-name">${name === current ? '&#9679; ' : ''}${escapeHtml(name)}</span>
+                    ${name === current ? '' : `<button class="git-checkout-btn" data-branch="${escapeHtml(name)}" title="Checkout branch">checkout</button>`}
                </div>
            `
      )
@@ -0,0 +1,146 @@
+/**
+ * Tests for the Git sidebar panel XSS hardening.
+ * renderGitPanel takes injected git operations (no electron mock needed); these tests
+ * verify that repository-derived strings (branch names, file names, commit messages,
+ * author names, error text) are HTML-escaped in both element and attribute contexts.
+ */
+
+const { renderGitPanel } = require('../src/sidebar/git-panel');
+
+const flush = () => new Promise((resolve) => setTimeout(resolve, 20));
+
+const EMPTY_STATUS = { modified: [], not_added: [], created: [], deleted: [], staged: [] };
+const EMPTY_BRANCHES = () => ({ all: [], current: '' });
+
+function makeOps(overrides = {}) {
+  return {
+    gitStatus: jest.fn().mockResolvedValue(EMPTY_STATUS),
+    gitDiff: jest.fn().mockResolvedValue(''),
+    gitStage: jest.fn().mockResolvedValue({}),
+    gitCommit: jest.fn().mockResolvedValue({}),
+    gitLog: jest.fn().mockResolvedValue({ all: [] }),
+    gitBranches: jest.fn().mockResolvedValue(EMPTY_BRANCHES()),
+    gitCheckout: jest.fn().mockResolvedValue({}),
+    gitPush: jest.fn().mockResolvedValue({}),
+    gitPull: jest.fn().mockResolvedValue({}),
+    ...overrides,
+  };
+}
+
+function mountPanel(ops) {
+  document.body.innerHTML = '';
+  const container = document.createElement('div');
+  document.body.appendChild(container);
+  renderGitPanel(container, ops);
+  return container;
+}
+
+describe('Git panel XSS escaping', () => {
+  it('renders a hostile branch name as literal text with no img element in the DOM', async () => {
+    const payload = '<img src=x onerror=alert(1)>';
+    mountPanel(
+      makeOps({
+        gitBranches: jest.fn().mockResolvedValue({ all: [payload, 'master'], current: 'master' }),
+      })
+    );
+    await flush();
+
+    const branchesEl = document.getElementById('git-branches');
+    expect(branchesEl.querySelectorAll('img')).toHaveLength(0);
+    expect(branchesEl.textContent).toContain(payload);
+    const item = branchesEl.querySelector('[data-branch]');
+    expect(item).not.toBeNull();
+    expect(item.dataset.branch).toBe(payload);
+  });
+
+  it('keeps a hostile branch name inert in attribute and text contexts', async () => {
+    const payload = '" onclick="alert(1)" data-x="';
+    mountPanel(
+      makeOps({
+        gitBranches: jest.fn().mockResolvedValue({ all: [payload], current: '' }),
+      })
+    );
+    await flush();
+
+    const item = document.querySelector('.git-branch-item');
+    expect(item.dataset.branch).toBe(payload);
+    expect(item.getAttribute('onclick')).toBeNull();
+    expect(item.textContent).toContain(payload);
+  });
+
+  it('renders a commit message with an event-handler payload as inert text', async () => {
+    const ops = makeOps({
+      gitLog: jest.fn().mockResolvedValue({
+        all: [
+          {
+            message: '<img src=x onerror=alert(1)> fix build',
+            author_name: 'Attacker <script>alert(2)</script>',
+            date: '2024-05-01T10:00:00',
+          },
+        ],
+      }),
+    });
+    mountPanel(ops);
+    await flush();
+
+    const logEl = document.getElementById('git-log');
+    expect(logEl.querySelectorAll('img, script')).toHaveLength(0);
+    const msg = logEl.querySelector('.git-log-msg');
+    expect(msg.textContent).toBe('<img src=x onerror=alert(1)> fix build');
+    const meta = logEl.querySelector('.git-log-meta');
+    expect(meta.textContent).toContain('Attacker <script>alert(2)</script>');
+  });
+
+  it('does not let a quoted file name break out of the data-file attribute', async () => {
+    const evilFile = 'notes" onmouseover="alert(1)" data-evil="x.md';
+    mountPanel(
+      makeOps({ gitStatus: jest.fn().mockResolvedValue({ ...EMPTY_STATUS, modified: [evilFile] }) })
+    );
+    await flush();
+
+    const fileRow = document.querySelector('.git-file');
+    expect(fileRow.getAttribute('data-file')).toBe(evilFile);
+    expect(fileRow.getAttribute('onmouseover')).toBeNull();
+    expect(fileRow.getAttribute('data-evil')).toBeNull();
+    expect(fileRow.textContent).toContain(evilFile);
+    const stageBtn = fileRow.querySelector('.git-stage-btn');
+    expect(stageBtn.dataset.file).toBe(evilFile);
+  });
+
+  it('escapes HTML in a git status error message', async () => {
+    const error = 'fatal: <b>not</b> a git repository <img src=x onerror=alert(1)>';
+    mountPanel(makeOps({ gitStatus: jest.fn().mockResolvedValue({ error }) }));
+    await flush();
+
+    const changesEl = document.getElementById('git-changes');
+    expect(changesEl.querySelectorAll('img')).toHaveLength(0);
+    expect(changesEl.querySelector('.git-info').textContent).toBe(error);
+  });
+
+  it('escapes HTML in a git branch listing error message', async () => {
+    const error = 'refs/heads/<script>alert(1)</script> is invalid';
+    mountPanel(makeOps({ gitBranches: jest.fn().mockResolvedValue({ error }) }));
+    await flush();
+
+    const branchesEl = document.getElementById('git-branches');
+    expect(branchesEl.querySelectorAll('script')).toHaveLength(0);
+    expect(branchesEl.querySelector('.git-info').textContent).toBe(error);
+  });
+
+  it('still renders benign branch names, files, and commits normally', async () => {
+    mountPanel(
+      makeOps({
+        gitStatus: jest.fn().mockResolvedValue({ ...EMPTY_STATUS, modified: ['README.md'] }),
+        gitLog: jest.fn().mockResolvedValue({
+          all: [{ message: 'initial commit', author_name: 'Dev', date: '2024-05-01T10:00:00' }],
+        }),
+        gitBranches: jest.fn().mockResolvedValue({ all: ['master'], current: 'master' }),
+      })
+    );
+    await flush();
+
+    expect(document.querySelector('.git-file-name').textContent).toBe('README.md');
+    expect(document.querySelector('.git-log-msg').textContent).toBe('initial commit');
+    expect(document.querySelector('.git-branch-name').textContent).toContain('master');
+  });
+});