From 9c9a9a475d1d5482ddcf277bab9f17f7e6b46470 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 15 May 2026 08:27:42 +0200 Subject: [PATCH 1/5] refactor: flatten capture choice executor --- src/engine/CaptureChoiceEngine.ts | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index f23eb5d7..b007df25 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -20,10 +20,12 @@ import type { CaptureChoiceFormatter } from "../formatters/captureChoiceFormatte import { log } from "../logger/logManager"; import type QuickAdd from "../main"; import { FormatterFactory } from "../services/FormatterFactory"; +import { FrontmatterPropertyService } from "../services/FrontmatterPropertyService"; import { TemplateEvaluator, TemplateFileService, } from "../services/TemplateFileService"; +import { VaultFileService } from "../services/VaultFileService"; import type ICaptureChoice from "../types/choices/ICaptureChoice"; import { normalizeAppendLinkOptions, type AppendLinkOptions } from "../types/linkPlacement"; import { @@ -45,7 +47,6 @@ import { import { isCancellationError, reportError } from "../utils/errorUtils"; import { normalizeFileOpening } from "../utils/fileOpeningDefaults"; import { basenameWithoutMdOrCanvas } from "../utils/pathUtils"; -import { QuickAddChoiceEngine } from "./QuickAddChoiceEngine"; import { ChoiceAbortError } from "../errors/ChoiceAbortError"; import { MacroAbortError } from "../errors/MacroAbortError"; import { getCaptureAction, type CaptureAction } from "./captureAction"; @@ -61,10 +62,13 @@ import { handleMacroAbort } from "../utils/macroAbortHandler"; const DEFAULT_NOTICE_DURATION = 4000; -export class CaptureChoiceEngine extends QuickAddChoiceEngine { - choice: ICaptureChoice; +export class CaptureChoiceEngine { + public choice: ICaptureChoice; + public app: App; private formatter: CaptureChoiceFormatter; private readonly plugin: QuickAdd; + private readonly vaultFileService: VaultFileService; + private readonly frontmatterPropertyService: FrontmatterPropertyService; private readonly templateFileService: TemplateFileService; private templatePropertyVars?: Map; private capturePropertyVars: Map = new Map(); @@ -76,14 +80,20 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { private choiceExecutor: IChoiceExecutor, private readonly originLeaf: WorkspaceLeaf | null = null, ) { - super(app); + this.app = app; this.choice = choice; this.plugin = plugin; + this.vaultFileService = new VaultFileService(app); + this.frontmatterPropertyService = new FrontmatterPropertyService(app); this.formatter = new FormatterFactory( app, plugin, ).createCaptureChoiceFormatter(choiceExecutor); - this.templateFileService = new TemplateFileService(app); + this.templateFileService = new TemplateFileService( + app, + this.vaultFileService, + this.frontmatterPropertyService, + ); } private showSuccessNotice( From b228268014aabd260c112e42404964dc2e13a53c Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 15 May 2026 08:38:42 +0200 Subject: [PATCH 2/5] fix: correct capture merge guard --- .../CaptureChoiceEngine.selection.test.ts | 90 +++++++++++++++++++ src/engine/CaptureChoiceEngine.ts | 2 +- 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 66a58825..f9aa0301 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -1,5 +1,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { App } from "obsidian"; +import { Notice, TFile } from "obsidian"; +import merge from "three-way-merge"; import { CaptureChoiceEngine } from "./CaptureChoiceEngine"; import type ICaptureChoice from "../types/choices/ICaptureChoice"; import type { IChoiceExecutor } from "../IChoiceExecutor"; @@ -7,6 +9,12 @@ import { insertFileLinkToActiveView, isFolder, openFile } from "../utilityObsidi import { QA_INTERNAL_CAPTURE_TARGET_FILE_PATH } from "../constants"; import { ChoiceAbortError } from "../errors/ChoiceAbortError"; +type NoticeTestClass = typeof Notice & { + instances: Array<{ message: string; timeout?: number }>; +}; + +const noticeClass = Notice as unknown as NoticeTestClass; + const { setUseSelectionAsCaptureValueMock, setTitleMock } = vi.hoisted(() => ({ setUseSelectionAsCaptureValueMock: vi.fn(), setTitleMock: vi.fn(), @@ -136,6 +144,15 @@ const createExecutor = (): IChoiceExecutor => ({ variables: new Map(), }); +const createTFile = (path: string): TFile => { + const file = new TFile(); + file.path = path; + file.name = path.split("/").pop() ?? path; + file.extension = file.name.split(".").pop() ?? ""; + file.basename = file.name.replace(/\.[^.]+$/, ""); + return file; +}; + describe("CaptureChoiceEngine selection-as-value resolution", () => { beforeEach(() => { setUseSelectionAsCaptureValueMock.mockClear(); @@ -646,4 +663,77 @@ describe("CaptureChoiceEngine capture target resolution", () => { expect(onFileExistsMock).not.toHaveBeenCalled(); expect(app.vault.modify).not.toHaveBeenCalled(); }); + + it("writes joined three-way merge results when an existing file changes without conflicts", async () => { + const file = createTFile("Inbox.md"); + const app = createApp() as any; + app.vault.read = vi + .fn() + .mockResolvedValueOnce("original") + .mockResolvedValueOnce("concurrent"); + app.vault.modify = vi.fn(async () => {}); + app.vault.getAbstractFileByPath = vi.fn(() => file); + vi.mocked(merge).mockReturnValue({ + isSuccess: vi.fn(() => true), + joinedResults: vi.fn(() => "merged"), + } as any); + + const engine = new CaptureChoiceEngine( + app, + { settings: { useSelectionAsCaptureValue: false } } as any, + createChoice(), + createExecutor(), + ); + (engine as any).vaultFileService.fileExists = vi.fn(async () => true); + + await engine.run(); + + expect(merge).toHaveBeenCalledWith("concurrent", "original", ""); + expect(app.vault.modify).toHaveBeenCalledWith(file, "merged"); + }); + + it("aborts conflicting existing-file merges before write and follow-up side effects", async () => { + noticeClass.instances.length = 0; + vi.mocked(insertFileLinkToActiveView).mockClear(); + vi.mocked(openFile).mockClear(); + + const file = createTFile("Inbox.md"); + const app = createApp() as any; + app.vault.read = vi + .fn() + .mockResolvedValueOnce("original") + .mockResolvedValueOnce("concurrent"); + app.vault.modify = vi.fn(async () => {}); + app.vault.getAbstractFileByPath = vi.fn(() => file); + const processFrontMatter = vi.fn(); + app.fileManager.processFrontMatter = processFrontMatter; + vi.mocked(merge).mockReturnValue({ + isSuccess: vi.fn(() => false), + joinedResults: vi.fn(() => { + throw new Error("joinedResults should not be read on conflict"); + }), + } as any); + + const engine = new CaptureChoiceEngine( + app, + { + settings: { + useSelectionAsCaptureValue: false, + showCaptureNotification: true, + }, + } as any, + createChoice({ appendLink: true, openFile: true }), + createExecutor(), + ); + (engine as any).vaultFileService.fileExists = vi.fn(async () => true); + + await engine.run(); + + expect(merge).toHaveBeenCalledWith("concurrent", "original", ""); + expect(app.vault.modify).not.toHaveBeenCalled(); + expect(processFrontMatter).not.toHaveBeenCalled(); + expect(insertFileLinkToActiveView).not.toHaveBeenCalled(); + expect(openFile).not.toHaveBeenCalled(); + expect(noticeClass.instances).toEqual([]); + }); }); diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index b007df25..e9319539 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -689,7 +689,7 @@ export class CaptureChoiceEngine { formattedFileContent, ); invariant( - !res.isSuccess, + res.isSuccess(), () => `The file ${filePath} has been modified since the last read.\nQuickAdd could not merge the versions two without conflicts, and will not modify the file.\nThis is in order to prevent data loss.`, ); From 8fca3d01f88d898969ece3f70c725ee3e815238e Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 15 May 2026 08:42:27 +0200 Subject: [PATCH 3/5] fix: contain capture custom folder paths --- .../CaptureChoiceEngine.selection.test.ts | 107 +++++++++++++++++- src/engine/CaptureChoiceEngine.ts | 56 ++++++++- 2 files changed, 159 insertions(+), 4 deletions(-) diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index f9aa0301..2a08d53f 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -5,7 +5,14 @@ import merge from "three-way-merge"; import { CaptureChoiceEngine } from "./CaptureChoiceEngine"; import type ICaptureChoice from "../types/choices/ICaptureChoice"; import type { IChoiceExecutor } from "../IChoiceExecutor"; -import { insertFileLinkToActiveView, isFolder, openFile } from "../utilityObsidian"; +import { + getMarkdownFilesInFolder, + insertFileLinkToActiveView, + isFolder, + openFile, + overwriteTemplaterOnce, + templaterParseTemplate, +} from "../utilityObsidian"; import { QA_INTERNAL_CAPTURE_TARGET_FILE_PATH } from "../constants"; import { ChoiceAbortError } from "../errors/ChoiceAbortError"; @@ -15,7 +22,12 @@ type NoticeTestClass = typeof Notice & { const noticeClass = Notice as unknown as NoticeTestClass; -const { setUseSelectionAsCaptureValueMock, setTitleMock } = vi.hoisted(() => ({ +const { + inputSuggestMock, + setUseSelectionAsCaptureValueMock, + setTitleMock, +} = vi.hoisted(() => ({ + inputSuggestMock: vi.fn(), setUseSelectionAsCaptureValueMock: vi.fn(), setTitleMock: vi.fn(), })); @@ -73,7 +85,9 @@ vi.mock("three-way-merge", () => ({ })); vi.mock("src/gui/InputSuggester/inputSuggester", () => ({ - default: class {}, + default: class { + static Suggest = inputSuggestMock; + }, })); vi.mock("obsidian-dataview", () => ({ @@ -242,6 +256,9 @@ describe("CaptureChoiceEngine capture target resolution", () => { beforeEach(() => { vi.mocked(isFolder).mockReset(); vi.mocked(insertFileLinkToActiveView).mockReset(); + vi.mocked(getMarkdownFilesInFolder).mockReset(); + vi.mocked(getMarkdownFilesInFolder).mockReturnValue([]); + inputSuggestMock.mockReset(); setTitleMock.mockClear(); }); @@ -355,6 +372,90 @@ describe("CaptureChoiceEngine capture target resolution", () => { expect(result).toBe("Boards/Map.CANVAS"); }); + it.each([ + ["leading slash", "/Escape"], + ["backslash", "Sub\\Escape"], + ["absolute-looking drive path", "C:/Escape"], + ["duplicate separator", "Sub//Escape"], + ["parent-directory segment", "../Escape"], + ])( + "rejects folder-scoped custom capture paths with %s before side effects", + async (_caseName, unsafeInput) => { + noticeClass.instances.length = 0; + vi.mocked(insertFileLinkToActiveView).mockClear(); + vi.mocked(openFile).mockClear(); + vi.mocked(overwriteTemplaterOnce).mockClear(); + vi.mocked(templaterParseTemplate).mockClear(); + + const existingFile = createTFile("Folder/Existing.md"); + const app = createApp() as any; + app.vault.create = vi.fn(async () => createTFile("Folder/Created.md")); + app.vault.modify = vi.fn(async () => {}); + app.vault.read = vi.fn(async () => ""); + app.fileManager.processFrontMatter = vi.fn(); + vi.mocked(getMarkdownFilesInFolder).mockReturnValue([existingFile]); + inputSuggestMock.mockResolvedValue(unsafeInput); + + const engine = new CaptureChoiceEngine( + app, + { + settings: { + useSelectionAsCaptureValue: false, + showCaptureNotification: true, + }, + } as any, + createChoice({ + captureTo: "Folder/", + appendLink: true, + openFile: true, + createFileIfItDoesntExist: { + enabled: true, + createWithTemplate: false, + template: "", + }, + }), + createExecutor(), + ); + const fileExistsMock = vi.fn(async () => false); + (engine as any).vaultFileService.fileExists = fileExistsMock; + + await engine.run(); + + expect(fileExistsMock).not.toHaveBeenCalled(); + expect(app.vault.create).not.toHaveBeenCalled(); + expect(app.vault.modify).not.toHaveBeenCalled(); + expect(app.fileManager.processFrontMatter).not.toHaveBeenCalled(); + expect(templaterParseTemplate).not.toHaveBeenCalled(); + expect(overwriteTemplaterOnce).not.toHaveBeenCalled(); + expect(insertFileLinkToActiveView).not.toHaveBeenCalled(); + expect(openFile).not.toHaveBeenCalled(); + expect( + noticeClass.instances.some(({ message }) => + message.startsWith("Created and captured to") || + message.startsWith("Captured to"), + ), + ).toBe(false); + }, + ); + + it("normalizes valid folder-scoped custom capture paths under the selected folder", async () => { + const existingFile = createTFile("Folder/Existing.md"); + const app = createApp(); + vi.mocked(getMarkdownFilesInFolder).mockReturnValue([existingFile]); + inputSuggestMock.mockResolvedValue("Sub/New Note"); + + const engine = new CaptureChoiceEngine( + app, + { settings: { useSelectionAsCaptureValue: false } } as any, + createChoice({ captureTo: "Folder/" }), + createExecutor(), + ); + + const result = await (engine as any).getFormattedPathToCaptureTo(false); + + expect(result).toBe("Folder/Sub/New Note.md"); + }); + it("uses extensionless title for created .canvas capture files", async () => { const app = createApp() as any; app.vault.read = vi.fn(async () => ""); diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index e9319539..e667993d 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -606,13 +606,67 @@ export class CaptureChoiceEngine { // Ensure user has selected a file in target folder. InputSuggester allows user to write // their own file path, so we need to make sure it's in the target folder. + if (!captureAnywhereInVault && !filePaths.includes(targetFilePath)) { + return await this.formatCustomFolderCapturePath( + targetFilePath, + folderPath, + ); + } + const filePath = targetFilePath.startsWith(`${folderPathSlash}`) ? targetFilePath - : `${folderPathSlash}/${targetFilePath}`; + : `${folderPathSlash}${targetFilePath}`; return await this.formatFilePath(filePath); } + private async formatCustomFolderCapturePath( + customPath: string, + folderPath: string, + ): Promise { + const formattedCustomPath = await this.formatter.formatFileName( + customPath, + this.choice.name, + ); + const relativePath = formattedCustomPath.trim(); + + this.assertSafeFolderScopedCustomPath(relativePath); + + const fullPath = folderPath ? `${folderPath}/${relativePath}` : relativePath; + const normalizedPath = this.normalizeCaptureFilePath(fullPath); + const folderPrefix = folderPath ? `${folderPath}/` : ""; + + if (folderPrefix && !normalizedPath.startsWith(folderPrefix)) { + throw new ChoiceAbortError( + "Custom capture path must stay within the selected folder.", + ); + } + + return normalizedPath; + } + + private assertSafeFolderScopedCustomPath(path: string): void { + const segments = path.split("/"); + const hasParentSegment = segments.some((segment) => segment === ".."); + const hasEmptySegment = segments.some((segment) => segment === ""); + const isAbsoluteLooking = + path.startsWith("/") || + path.startsWith("~") || + /^[A-Za-z]:/.test(path); + + if ( + path.length === 0 || + path.includes("\\") || + isAbsoluteLooking || + hasEmptySegment || + hasParentSegment + ) { + throw new ChoiceAbortError( + "Custom capture path must stay within the selected folder.", + ); + } + } + private async selectFileWithTag(tag: string): Promise { const tagWithHash = tag.startsWith("#") ? tag : `#${tag}`; const filesWithTag = getMarkdownFilesWithTag(this.app, tagWithHash); From 6cf21a20fa2e6e1abfd687a2408d8379286c2232 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 15 May 2026 08:53:09 +0200 Subject: [PATCH 4/5] fix: validate raw capture custom paths --- src/engine/CaptureChoiceEngine.selection.test.ts | 12 ++++++++++-- src/engine/CaptureChoiceEngine.ts | 4 ++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 2a08d53f..6b32b472 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -24,10 +24,12 @@ const noticeClass = Notice as unknown as NoticeTestClass; const { inputSuggestMock, + formatFileNameMock, setUseSelectionAsCaptureValueMock, setTitleMock, } = vi.hoisted(() => ({ inputSuggestMock: vi.fn(), + formatFileNameMock: vi.fn(async (name: string) => name), setUseSelectionAsCaptureValueMock: vi.fn(), setTitleMock: vi.fn(), })); @@ -53,7 +55,7 @@ vi.mock("../formatters/captureChoiceFormatter", () => ({ return ""; } async formatFileName(name: string) { - return name; + return await formatFileNameMock(name); } getAndClearTemplatePropertyVars() { return new Map(); @@ -377,11 +379,13 @@ describe("CaptureChoiceEngine capture target resolution", () => { ["backslash", "Sub\\Escape"], ["absolute-looking drive path", "C:/Escape"], ["duplicate separator", "Sub//Escape"], + ["current-directory segment", "./Escape"], ["parent-directory segment", "../Escape"], ])( - "rejects folder-scoped custom capture paths with %s before side effects", + "rejects folder-scoped raw custom capture paths with %s before formatter or side effects", async (_caseName, unsafeInput) => { noticeClass.instances.length = 0; + formatFileNameMock.mockClear(); vi.mocked(insertFileLinkToActiveView).mockClear(); vi.mocked(openFile).mockClear(); vi.mocked(overwriteTemplaterOnce).mockClear(); @@ -421,6 +425,10 @@ describe("CaptureChoiceEngine capture target resolution", () => { await engine.run(); + expect(formatFileNameMock).not.toHaveBeenCalledWith( + unsafeInput, + "Capture", + ); expect(fileExistsMock).not.toHaveBeenCalled(); expect(app.vault.create).not.toHaveBeenCalled(); expect(app.vault.modify).not.toHaveBeenCalled(); diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index e667993d..7c4a8f18 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -624,6 +624,8 @@ export class CaptureChoiceEngine { customPath: string, folderPath: string, ): Promise { + this.assertSafeFolderScopedCustomPath(customPath.trim()); + const formattedCustomPath = await this.formatter.formatFileName( customPath, this.choice.name, @@ -648,6 +650,7 @@ export class CaptureChoiceEngine { private assertSafeFolderScopedCustomPath(path: string): void { const segments = path.split("/"); const hasParentSegment = segments.some((segment) => segment === ".."); + const hasCurrentSegment = segments.some((segment) => segment === "."); const hasEmptySegment = segments.some((segment) => segment === ""); const isAbsoluteLooking = path.startsWith("/") || @@ -659,6 +662,7 @@ export class CaptureChoiceEngine { path.includes("\\") || isAbsoluteLooking || hasEmptySegment || + hasCurrentSegment || hasParentSegment ) { throw new ChoiceAbortError( From fb2799c027c692ec213fc448ebc73e1e705a9180 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 15 May 2026 09:01:49 +0200 Subject: [PATCH 5/5] test: prove raw capture path validation skips formatting --- src/engine/CaptureChoiceEngine.selection.test.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 6b32b472..12779617 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -29,7 +29,7 @@ const { setTitleMock, } = vi.hoisted(() => ({ inputSuggestMock: vi.fn(), - formatFileNameMock: vi.fn(async (name: string) => name), + formatFileNameMock: vi.fn(async (name: string, _title?: string) => name), setUseSelectionAsCaptureValueMock: vi.fn(), setTitleMock: vi.fn(), })); @@ -54,8 +54,8 @@ vi.mock("../formatters/captureChoiceFormatter", () => ({ async formatContentWithFile() { return ""; } - async formatFileName(name: string) { - return await formatFileNameMock(name); + async formatFileName(name: string, title?: string) { + return await formatFileNameMock(name, title); } getAndClearTemplatePropertyVars() { return new Map(); @@ -425,9 +425,12 @@ describe("CaptureChoiceEngine capture target resolution", () => { await engine.run(); + expect(formatFileNameMock.mock.calls).toEqual([ + ["Folder/", "Capture Choice"], + ]); expect(formatFileNameMock).not.toHaveBeenCalledWith( unsafeInput, - "Capture", + "Capture Choice", ); expect(fileExistsMock).not.toHaveBeenCalled(); expect(app.vault.create).not.toHaveBeenCalled();