Skip to content

Commit a582830

Browse files
committed
fix(self-update): pin installer ref and harden dashboard path checks
1 parent de5b06b commit a582830

4 files changed

Lines changed: 109 additions & 8 deletions

File tree

packages/apm-contributor-dashboard/.apm/extensions/issue-monitor/server-handler.mjs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
// const server = createServer(handler);
88

99
import { readFileSync } from "node:fs";
10-
import { join, resolve, normalize } from "node:path";
10+
import { isAbsolute, join, normalize, relative, resolve } from "node:path";
1111
import { randomBytes } from "node:crypto";
1212
import { parsePanelReview, extractFollowUpItems } from "./logic.mjs";
1313

@@ -456,8 +456,10 @@ export function createHandler(deps) {
456456
res.end("Not found");
457457
}
458458
} else if (urlPath.startsWith("/assets/")) {
459-
const resolved = resolve(distDir, normalize(urlPath.slice(1)));
460-
if (!resolved.startsWith(resolve(distDir))) {
459+
const root = resolve(distDir);
460+
const resolved = resolve(root, normalize(urlPath.slice(1)));
461+
const rel = relative(root, resolved);
462+
if (rel.startsWith("..") || isAbsolute(rel)) {
461463
res.writeHead(403); res.end("Forbidden"); return;
462464
}
463465
serveStatic(res, resolved);

packages/apm-contributor-dashboard/tests/server.test.mjs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { describe, it, before, after, beforeEach } from "node:test";
66
import assert from "node:assert/strict";
77
import { createServer } from "node:http";
88
import { createHandler } from "../.apm/extensions/issue-monitor/server-handler.mjs";
9-
import { join, dirname } from "node:path";
9+
import { basename, join, dirname } from "node:path";
1010
import { fileURLToPath } from "node:url";
1111

1212
const __dir = dirname(fileURLToPath(import.meta.url));
@@ -617,4 +617,21 @@ describe("Path traversal protection", () => {
617617
const res = await fetch(`${baseUrl}/assets/nonexistent.js`);
618618
assert.equal(res.status, 404);
619619
});
620+
621+
it("blocks sibling prefix traversal to dist-evil paths", async () => {
622+
const evilPath = `/assets/../../${basename(DIST_DIR)}-evil/secret.txt`;
623+
const url = new URL(`${baseUrl}${evilPath}`);
624+
const http = await import("node:http");
625+
const res = await new Promise((resolve) => {
626+
const req = http.request({
627+
hostname: url.hostname,
628+
port: url.port,
629+
path: evilPath,
630+
method: "GET",
631+
}, resolve);
632+
req.end();
633+
});
634+
635+
assert.equal(res.statusCode, 403);
636+
});
620637
});

src/apm_cli/commands/self_update.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ def _is_windows_platform() -> bool:
4040
return sys.platform == "win32"
4141

4242

43-
def _get_update_installer_url() -> str:
43+
def _get_update_installer_url(script_ref: str | None = None) -> str:
4444
"""Return the installer URL for the current platform, respecting mirror env vars.
4545
4646
``APM_INSTALLER_BASE_URL`` wins when configured, using ``install.sh`` on
@@ -65,7 +65,8 @@ def _get_update_installer_url() -> str:
6565
if github_url == _DEFAULT_GITHUB_URL:
6666
return "https://aka.ms/apm-windows" if _is_windows_platform() else "https://aka.ms/apm-unix"
6767

68-
return f"{github_url}/{apm_repo}/raw/{_INSTALL_SCRIPT_REF}/{script_name}"
68+
resolved_ref = script_ref or _INSTALL_SCRIPT_REF
69+
return f"{github_url}/{apm_repo}/raw/{resolved_ref}/{script_name}"
6970

7071

7172
def _get_update_installer_suffix() -> str:
@@ -155,7 +156,7 @@ def _build_self_update_installer_env(latest_version: str) -> dict[str, str]:
155156
channel = _get_effective_self_update_channel()
156157
if _ENV_SELF_UPDATE_CHANNEL not in env:
157158
env[_ENV_SELF_UPDATE_CHANNEL] = channel
158-
if channel == "prerelease" and _ENV_VERSION not in env:
159+
if _ENV_VERSION not in env:
159160
env[_ENV_VERSION] = f"v{latest_version}"
160161
return env
161162

@@ -272,7 +273,8 @@ def self_update(check):
272273
import requests
273274

274275
try:
275-
install_script_url = _get_update_installer_url()
276+
installer_ref = f"v{latest_version}"
277+
install_script_url = _get_update_installer_url(installer_ref)
276278
except RuntimeError as e:
277279
logger.error(str(e))
278280
logger.info(

tests/unit/commands/test_self_update_air_gapped.py

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,24 @@ def test_custom_github_url_unix_uses_sh(self) -> None:
122122
parsed = urlparse(url)
123123
assert parsed.path.endswith("install.sh")
124124

125+
def test_custom_github_url_uses_supplied_script_ref(self) -> None:
126+
"""Supplying a script ref points installer downloads to that tag path."""
127+
from urllib.parse import urlparse
128+
129+
env = self._clean_env()
130+
env["GITHUB_URL"] = "https://gh.corp.com"
131+
env["APM_REPO"] = "corp/apm"
132+
with patch.dict("os.environ", env, clear=True):
133+
with patch(
134+
"apm_cli.commands.self_update._is_windows_platform",
135+
return_value=False,
136+
):
137+
from apm_cli.commands.self_update import _get_update_installer_url
138+
139+
url = _get_update_installer_url("v1.2.3")
140+
parsed = urlparse(url)
141+
assert parsed.path == "/corp/apm/raw/v1.2.3/install.sh"
142+
125143
def test_github_url_with_trailing_slash_is_normalised(self) -> None:
126144
"""GITHUB_URL with a trailing slash must not produce double-slash in the URL."""
127145
env = self._clean_env()
@@ -357,3 +375,65 @@ def fake_get(url: str, **_kwargs: object) -> Mock:
357375
"/apm/installers/install.sh",
358376
]
359377
mock_run.assert_called_once()
378+
379+
def test_self_update_pins_installer_ref_to_latest_version(self) -> None:
380+
"""Self-update downloads installer from the exact latest release ref."""
381+
env = self._clean_env()
382+
env.update(
383+
{
384+
"APM_NO_DIRECT_FALLBACK": "1",
385+
"APM_RELEASE_METADATA_URL": "https://mirror.corp.example/apm/latest.json",
386+
"GITHUB_URL": "https://gh.corp.com",
387+
"APM_TEMP_DIR": str(self.scratch),
388+
"APM_REPO": "corp/apm",
389+
"APM_E2E_TESTS": "1",
390+
}
391+
)
392+
requested_urls: list[str] = []
393+
target_version = "1.1.0"
394+
expected_installer_path = f"/corp/apm/raw/v{target_version}/install.sh"
395+
396+
def fake_get(url: str, **_kwargs: object) -> Mock:
397+
requested_urls.append(url)
398+
parsed = urlparse(url)
399+
if parsed.hostname == "mirror.corp.example":
400+
if parsed.path == "/apm/latest.json":
401+
response = Mock()
402+
response.status_code = 200
403+
response.raise_for_status.return_value = None
404+
response.json.return_value = {"tag_name": f"v{target_version}"}
405+
return response
406+
raise AssertionError(f"unexpected mirror request: {url}")
407+
408+
if parsed.hostname == "gh.corp.com":
409+
if parsed.path == expected_installer_path:
410+
response = Mock()
411+
response.status_code = 200
412+
response.raise_for_status.return_value = None
413+
response.text = "echo install"
414+
return response
415+
raise AssertionError(f"unexpected github request: {url}")
416+
417+
raise AssertionError(f"unexpected request: {url}")
418+
419+
with (
420+
patch.dict("os.environ", env, clear=True),
421+
patch("requests.get", side_effect=fake_get),
422+
patch("subprocess.run", return_value=Mock(returncode=0)) as mock_run,
423+
patch("apm_cli.commands.self_update.get_version", return_value="1.0.0"),
424+
patch("apm_cli.commands.self_update.os.chmod"),
425+
patch.object(update_module.sys, "platform", "linux"),
426+
patch("apm_cli.commands.self_update.os.path.exists", return_value=True),
427+
):
428+
result = self.runner.invoke(cli, ["self-update"])
429+
430+
assert result.exit_code == 0
431+
assert "Successfully updated to version 1.1.0" in result.output
432+
parsed_paths = [urlparse(url).path for url in requested_urls]
433+
assert parsed_paths == [
434+
"/apm/latest.json",
435+
expected_installer_path,
436+
]
437+
assert "/main/" not in "".join(parsed_paths)
438+
assert f"/raw/v{target_version}/" in "".join(parsed_paths)
439+
mock_run.assert_called_once()

0 commit comments

Comments
 (0)