Skip to content

Renamed file's old path is not containment-checked: a rename into docs-folder can delete a target file outside it #307

Description

@mmcky

A renamed file's old path is never checked against docs-folder, and it is used unguarded to fetch and then delete a file in the target. A source PR that renames a root-level .md file into the docs folder will make the sync PR delete the target's file at that root path — outside the docs folder entirely.

Found at HEAD 818998a2 (v0.28.0) while investigating whether sync propagates .github/ config. It does not, and that containment is exactly what this defect breaks.

The path

1. Only the new name is filtered. src/sync-orchestrator.ts:277-281:

const renamedMarkdownFiles = files.filter(
  (file: any) =>
    file.filename.startsWith(prefix) && file.filename.endsWith('.md') && file.status === 'renamed'
);

prefix is docsFolder. file.previousFilename is never tested. A rename README.mdlectures/README.md passes: the new name is prefixed and ends .md.

2. The old path is used against the target without a containment check. src/index.ts:1018-1026:

if (previousFilename) {
  try {
    const result = await fetchFileContent(octokit, targetOwner, targetRepo, previousFilename);
    targetContent = result.content;
    oldFileSha = result.sha;

previousFilename here is the raw README.md. If the target has a root README.md, this succeeds and oldFileSha is set.

3. That becomes a deletion. src/sync-orchestrator.ts:652-658:

if (file.oldFileSha && file.previousFilename) {
  result.filesToDelete.push({
    path: file.previousFilename,
    sha: file.oldFileSha,
  });

src/pr-creator.ts then calls deleteFile on it. The sync PR deletes the target's root README.md.

The code already knows the old path may be unprefixed

src/index.ts:1176-1180 branches on exactly this case when computing the state-file path:

const oldDocsRelName =
  docsFolder && file.previousFilename.startsWith(docsFolder)
    ? file.previousFilename.slice(docsFolder.length)
    : file.previousFilename;

The : file.previousFilename arm is the unprefixed case. It is handled for state bookkeeping and not for the deletion.

A guard already exists — on the other code path

src/cli/forward-pr-creator.ts:189-206 has containedPath, with a comment stating the intent plainly:

// typo'd value ("-f ../../x.md") must fail cleanly here, not write outside
// the operator's clone. Containment is checked on the JOINED result —
// docsFolder "/" legitimately means the repo root and must keep working.

The Action's sync path has no equivalent.

Live exposure

The rename is realistic rather than contrived — moving a stray root document into the docs folder is ordinary tidying, and QuantEcon/lecture-python-programming has a root README.md today. Every sync target of it has a root README.md, and two also carry TRANSLATION-REPORT.md:

Target Root .md files at risk
lecture-python-programming.zh-cn README.md, TRANSLATION-REPORT.md
lecture-python-programming.fr README.md, TRANSLATION-REPORT.md
lecture-python-programming.fa README.md
lecture-intro.zh-cn README.md

A target's README.md and TRANSLATION-REPORT.md are target-authored, not synced, so the deletion would destroy content with no source-side copy to restore it from. It would land inside a translation-sync-* PR that otherwise looks routine.

Reproduction

  1. In a source repo with a root README.md and docs-folder: lectures, open a PR that renames README.mdlectures/README.md, and merge it.
  2. The path filter in the workflow (paths: ['lectures/**/*.md', ...]) matches the new name, so the sync job starts.
  3. In the resulting sync PR against a target that has its own root README.md, that file is deleted.

Suggested fix

Require both paths to be inside docs-folder before a rename is treated as a rename. When previousFilename falls outside, the correct reading is "a new file appeared in the docs folder" — translate it to the new path and delete nothing.

Worth applying the same containment check to filesToDelete generally, so no path outside docs-folder can ever reach deleteFile regardless of how it got there. Reusing the containedPath shape from forward-pr-creator.ts would keep the two paths consistent.

Two smaller notes from the same read

  • src/inputs.ts:48-50 maps docs-folder: . to an empty prefix, at which point every .md in the repo becomes eligible — .github/ISSUE_TEMPLATE/*.md included. Not live in any QuantEcon config (all four workflows set lectures), but it means the docs-folder filter is config-dependent and the .md suffix test is the only unconditional barrier.
  • README.md:5 says v0.26.0 while package.json is at 0.28.0.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinghigh-priorityAddress soon

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions