mirror of
https://github.com/Azure/k8s-deploy.git
synced 2026-06-10 03:02:17 +08:00
ef735e2cde
* docs: add design for manifest path traversal fix * docs: add implementation plan for manifest path traversal fix * fix: confine manifest paths to workspace moveFileToTmpDir previously used path.join(tempDirectory, originalFilepath), which normalizes ../ sequences. A manifests input containing a traversal sequence caused the action to read .yaml/.yml files from outside the workspace and write copies outside RUNNER_TEMP. Directory inputs made this stronger because recurisveManifestGetter enumerated YAML files under the traversed directory. Add assertPathWithinWorkspace, which resolves symlinks via realpathSync and rejects any path not contained in GITHUB_WORKSPACE. Apply it in getFilesFromDirectoriesAndURLs before lstat / readdir / file inclusion. Rewrite moveFileToTmpDir to use a basename-only destination under RUNNER_TEMP with a getCurrentTime() uniquifier to avoid collisions, matching the safer pattern already used by getNewTempManifestFileName. * fix: handle errors in writeYamlFromURLToFile The https.get callback was marked async without any await, which caused thrown errors to be silently swallowed as floating promise rejections. There were no error listeners on the response stream or the file writer, so socket or disk errors hung the promise instead of rejecting it. On HTTP status >= 400 the function called reject but then fell through and opened a write stream anyway. Drop the misleading async, return after rejecting HTTP errors, drain the response, and add error listeners on both streams. Wrap the string verification error in new Error so stack traces are preserved. * fix: harden verifyYaml and warn on unset GITHUB_WORKSPACE Two follow-ups from review of the path-traversal series: verifyYaml is called inside writeYamlFromURLToFile's finish listener. If the body parsed to a null YAML document (e.g. "---" or a multi-doc file with a trailing separator), the loop dereferenced obj.kind on null and threw. Because the throw happened inside an EventEmitter listener attached to a WriteStream rather than the Promise executor, it was not routed to reject, so the promise hung. Wrap the finish body in try/catch and add a null guard inside verifyYaml. assertPathWithinWorkspace previously returned silently when GITHUB_WORKSPACE was unset. In a real Action run the runner always sets it, so unset is a signal that something is wrong with the environment, not "skip the security check". Emit core.warning so a misconfigured self-hosted runner does not lose the containment protection without notice. * test: realpath both sides of dirname comparison On macOS, RUNNER_TEMP under /var/folders/... resolves through a /private symlink. moveFileToTmpDir builds its destination from the raw RUNNER_TEMP, so comparing path.dirname(out) directly with realpathSync(tmpDir) would fail on macOS. Normalize both sides. * fix: resolve relative manifest paths against workspace and clean up URL temp files Address two Copilot review comments on PR #528: - assertPathWithinWorkspace now resolves relative inputPath values against the realpathed GITHUB_WORKSPACE instead of process.cwd(). Previously a step that changed CWD could cause unexpected rejections (or false acceptances) for relative manifests inputs. Absolute paths are still passed through and validated unchanged. - writeYamlFromURLToFile now unlinks the partial temp file on any rejection that occurs after the write stream is created (writer error, response error, verification failure, sync throw in verify). The success path still resolves without unlinking. Pre-stream request errors leave nothing to clean up. Tests added: a workspace-relative resolution test that deliberately chdirs elsewhere, plus two cleanup-assertion tests covering verification-failure and mid-stream response error.