diff --git a/src/bridge/imported-assets.test.ts b/src/bridge/imported-assets.test.ts new file mode 100644 index 0000000..9f03c43 --- /dev/null +++ b/src/bridge/imported-assets.test.ts @@ -0,0 +1,119 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' +import { assetUploadOptions } from './remote-client.ts' +import { + importedAssetFilename, + importedAssetMarkdown, + importedAssetRelPath +} from './imported-assets.ts' + +describe('importedAssetFilename', () => { + it('scrubs characters that would break or retarget a wikilink embed', () => { + assert.equal(importedAssetFilename('diagram [v2] #3.png'), 'diagram -v2- -3.png') + assert.equal(importedAssetFilename('diagram%5D^draft|wide.png'), 'diagram-5D-draft-wide.png') + }) + + it('drops path components and control characters before writing a file', () => { + assert.equal(importedAssetFilename('../folder/line\nbreak.png'), 'line-break.png') + assert.equal(importedAssetFilename('..'), 'file') + }) +}) + +describe('importedAssetRelPath', () => { + it('puts an attached file in assets/, never the vault root', () => { + // The bug: attach wrote to the vault root, scattering images among the + // notes, while paste and the cloud upload both used assets/. + assert.equal(importedAssetRelPath('shape_image 18.jpg'), 'assets/shape_image 18.jpg') + assert.equal(importedAssetRelPath('report.pdf'), 'assets/report.pdf') + }) +}) + +describe('importedAssetMarkdown', () => { + it('links an image the same way a pasted image is linked', () => { + const rel = importedAssetRelPath('diagram.png') + assert.equal(importedAssetMarkdown(rel, 'diagram.png', 'image'), '![[assets/diagram.png]]') + }) + + it('never emits a note-relative ../ path', () => { + const markdown = importedAssetMarkdown( + importedAssetRelPath('shape_image 18.jpg'), + 'shape_image 18.jpg', + 'image' + ) + assert.equal(markdown, '![[assets/shape_image 18.jpg]]') + assert.ok(!markdown.includes('../')) + }) + + it('angle-brackets a non-image link so a space cannot break it', () => { + const rel = importedAssetRelPath('year end.pdf') + assert.equal(importedAssetMarkdown(rel, 'year end.pdf', 'pdf'), '[year end.pdf]()') + }) + + it('escapes a closing angle bracket in the path', () => { + assert.equal( + importedAssetMarkdown('assets/we>ird.zip', 'we>ird.zip', 'file'), + '[we>ird.zip]()' + ) + }) +}) + +describe('assetUploadOptions', () => { + const options = assetUploadOptions({ + url: 'https://notes.example.test/api/assets/upload', + fileName: 'diagram.png', + base64Data: 'AQID', + targetDir: 'assets' + }) + + it('sends the file as a native base64File part, not as base64 text in the body', () => { + // The bug: the body was hand-built with `Content-Transfer-Encoding: base64`, + // which multipart/form-data parsers ignore (RFC 7578), so Go wrote the + // base64 TEXT to disk as the file's bytes and every attachment was corrupt. + assert.equal(options.dataType, 'formData') + assert.deepEqual(options.data, [ + { type: 'string', key: 'dir', value: 'assets' }, + { + type: 'base64File', + key: 'file', + fileName: 'diagram.png', + contentType: 'application/octet-stream', + value: 'AQID' + } + ]) + assert.equal(typeof options.data, 'object') + }) + + it('declares the boundary its body will be written with', () => { + const contentType = options.headers['Content-Type'] + assert.match(contentType, /^multipart\/form-data; boundary=----ZenNotesUpload[0-9a-f]+$/) + }) + + it('sanitizes the filename used in the native multipart header', () => { + const quoted = assetUploadOptions({ + url: 'https://notes.example.test/api/assets/upload', + fileName: 'we"ird".png', + base64Data: 'AQID' + }) + const file = quoted.data[1] as { fileName: string } + assert.equal(file.fileName, 'we-ird-.png') + }) + + it('strips newlines from the filename so it cannot inject multipart headers', () => { + const injected = assetUploadOptions({ + url: 'https://notes.example.test/api/assets/upload', + fileName: 'pic.png"\r\nX-Injected: yes', + base64Data: 'AQID' + }) + const file = injected.data[1] as { fileName: string } + assert.equal(file.fileName, 'pic.png---X-Injected- yes') + }) + + it('defaults the target directory to empty rather than sending undefined', () => { + const bare = assetUploadOptions({ + url: 'https://notes.example.test/api/assets/upload', + fileName: 'a.png', + base64Data: 'AQID' + }) + assert.deepEqual(bare.data[0], { type: 'string', key: 'dir', value: '' }) + }) +}) diff --git a/src/bridge/imported-assets.ts b/src/bridge/imported-assets.ts new file mode 100644 index 0000000..1195a1f --- /dev/null +++ b/src/bridge/imported-assets.ts @@ -0,0 +1,50 @@ +/** + * Where an imported file goes and how it is linked. + * + * A leaf module on purpose: `vault-core` reaches `@shared/*` through a Vite + * alias, which plain `node --test` cannot resolve, so nothing there is + * unit-testable. These rules are where the attachment bugs hid, so they live + * where a test can reach them. The vault core re-exports them so existing + * importers remain unaffected. + */ +import type { ImportedAssetKind } from '@bridge-contract/ipc' + +export const ASSETS_DIR = 'assets' + +/** Keep imported names valid as both filenames and wikilink targets. Pasted + * images already apply this rule: brackets, anchors, and pipes can terminate + * or retarget a wikilink, while path/control characters are unsafe on one or + * more supported filesystems. */ +export function importedAssetFilename(filename: string): string { + const segments = filename.split(/[\\/]/) + const leaf = segments[segments.length - 1] ?? '' + const safe = leaf + // eslint-disable-next-line no-control-regex + .replace(/[\\/:%\u0000-\u001f*?"<>|[\]#^]/g, '-') + .replace(/\s+/g, ' ') + .trim() + return safe && safe !== '.' && safe !== '..' ? safe : 'file' +} + +/** Every imported file lands in `assets/`, whichever way it arrived. Paste, + * attach and the cloud upload agree on this, so a link written by one is + * resolvable by the others. Attach used to write to the vault root, which + * scattered images among the notes (Discord, xenin); desktop had the same bug + * and fixed it in ZenNotes#377, and the port here was missed. */ +export function importedAssetRelPath(filename: string): string { + return `${ASSETS_DIR}/${filename}` +} + +/** The link written into the note, by vault-relative path. Images take the + * wikilink form, matching pasted images and the cloud vault; anything else + * gets an angle-bracketed markdown link so a space in the name cannot break + * it. Never a note-relative `../` path: the old attach path hand-built one + * from the note's depth, which pointed outside the vault from a nested note. */ +export function importedAssetMarkdown( + relPath: string, + filename: string, + kind: ImportedAssetKind +): string { + if (kind === 'image') return `![[${relPath}]]` + return `[${filename}](<${relPath.replace(/>/g, '%3E')}>)` +} diff --git a/src/bridge/remote-client.ts b/src/bridge/remote-client.ts index b7a7355..2894e5a 100644 --- a/src/bridge/remote-client.ts +++ b/src/bridge/remote-client.ts @@ -26,6 +26,7 @@ import type { VaultTextSearchMatch } from '@shared/ipc' import type { VaultTask } from '@shared/tasks' +import { importedAssetFilename } from './imported-assets.ts' export interface RemoteClientOptions { baseUrl: string @@ -40,6 +41,58 @@ const TIMEOUT_MS = 15000 * load-bearing for the absence-aware database reader in remote-vault.ts, * which must never read "the request never arrived" as "the file is absent". */ +export interface AssetUploadRequest { + url: string + fileName: string + base64Data: string + targetDir?: string +} + +/** + * The multipart upload for an attachment. + * + * This used to hand-build the body as one string, with the file part carrying + * `Content-Transfer-Encoding: base64` and the base64 text as its content. That + * header is a MIME construct: RFC 7578 dropped it from multipart/form-data, and + * Go's `mime/multipart` (what the server parses with) does not honour it. The + * server therefore wrote the base64 TEXT to disk as the file's bytes, so an + * attachment arrived in `assets/` with the right name, about a third larger + * than the original, and unreadable by anything (zennotesandroid#40). + * + * The bytes have to stay base64 to cross the native bridge at all, since a + * CapacitorHttp string body is written out as UTF-8 and would mangle anything + * above 0x7F. `dataType: 'formData'` is the supported way through: both native + * layers decode a `base64File` entry and write the raw bytes into the part + * (Android `CapacitorHttpUrlConnection.writeFormDataRequestBody`, iOS + * `CapacitorUrlRequest.getRequestDataFromFormData`), which is what the server + * expects. The sibling direct-object upload already used the same mechanism + * with `dataType: 'file'`. + */ +export function assetUploadOptions(request: AssetUploadRequest) { + // Both native layers reuse the boundary from the header when it carries one, + // so the body and the header cannot disagree. + const boundary = `----ZenNotesUpload${Date.now().toString(16)}` + return { + url: request.url, + headers: { 'Content-Type': `multipart/form-data; boundary=${boundary}` }, + dataType: 'formData' as const, + data: [ + { type: 'string', key: 'dir', value: request.targetDir ?? '' }, + { + type: 'base64File', + key: 'file', + // Capacitor inserts this value directly into Content-Disposition on + // both native platforms, so use the same safe name written to disk. + fileName: importedAssetFilename(request.fileName), + contentType: 'application/octet-stream', + value: request.base64Data + } + ], + connectTimeout: TIMEOUT_MS, + readTimeout: TIMEOUT_MS + } +} + export class RemoteRequestError extends Error { readonly status: number @@ -292,29 +345,18 @@ export class RemoteClient { return { base64: response.data, mimeType: contentType } } - /** Multipart upload — CapacitorHttp has no FormData bridge, so send the - * body prebuilt with an explicit boundary. */ async uploadAsset(fileName: string, base64Data: string, targetDir = ''): Promise { - const boundary = `----ZenNotesUpload${Date.now().toString(16)}` - const head = - `--${boundary}\r\n` + - `Content-Disposition: form-data; name="dir"\r\n\r\n${targetDir}\r\n` + - `--${boundary}\r\n` + - `Content-Disposition: form-data; name="file"; filename="${fileName.replace(/"/g, '')}"\r\n` + - `Content-Type: application/octet-stream\r\n` + - `Content-Transfer-Encoding: base64\r\n\r\n` - const tail = `\r\n--${boundary}--\r\n` + const options = assetUploadOptions({ + url: `${this.baseUrl}/api/assets/upload`, + fileName, + base64Data, + targetDir + }) let response: HttpResponse try { response = await CapacitorHttp.post({ - url: `${this.baseUrl}/api/assets/upload`, - headers: { - ...this.headers(), - 'Content-Type': `multipart/form-data; boundary=${boundary}` - }, - data: head + base64Data + tail, - connectTimeout: TIMEOUT_MS, - readTimeout: TIMEOUT_MS + ...options, + headers: { ...this.headers(), ...options.headers } }) } catch (error) { throw new Error(connectionErrorMessage(this.baseUrl, error)) diff --git a/src/bridge/remote-vault.ts b/src/bridge/remote-vault.ts index 3236208..ca9446a 100644 --- a/src/bridge/remote-vault.ts +++ b/src/bridge/remote-vault.ts @@ -34,6 +34,7 @@ import type { CustomTemplateFile, WriteTemplateInput } from '@bridge-contract/te import type { ImportedAsset } from '@shared/ipc' import { createAbsenceAwareReader } from '@shared/remote-absence' import { emitVaultChange } from './events' +import { importedAssetFilename } from './imported-assets' import { RemoteClient, RemoteRequestError } from './remote-client' function remoteOnly(what: string): never { @@ -347,7 +348,11 @@ export class RemoteVault { async importDroppedFile(_notePath: string, file: File): Promise { const bytes = new Uint8Array(await file.arrayBuffer()) - const meta = await this.client.uploadAsset(file.name, bytesToBase64(bytes), 'assets') + const meta = await this.client.uploadAsset( + importedAssetFilename(file.name), + bytesToBase64(bytes), + 'assets' + ) const isImage = /\.(png|jpe?g|gif|webp|svg|heic)$/i.test(meta.name) emitVaultChange({ kind: 'add', path: meta.path, folder: 'inbox', scope: 'content' }) return { diff --git a/src/bridge/vault-core.ts b/src/bridge/vault-core.ts index 04dcc2d..aad3b3c 100644 --- a/src/bridge/vault-core.ts +++ b/src/bridge/vault-core.ts @@ -12,10 +12,19 @@ import { systemFolderForDirName, type SystemFolderPaths } from '@shared/system-folder-paths' +import { ASSETS_DIR } from './imported-assets' + +// Re-exported so every existing importer keeps reaching them here; they live in +// a leaf module because this one cannot be loaded by `node --test`. +export { + ASSETS_DIR, + importedAssetFilename, + importedAssetMarkdown, + importedAssetRelPath +} from './imported-assets' export const FOLDERS: NoteFolder[] = ['inbox', 'quick', 'archive', 'trash'] export const SYSTEM_FOLDERS = new Set(FOLDERS) -export const ASSETS_DIR = 'assets' export const PRIMARY_ATTACHMENTS_DIR = 'attachements' export const LEGACY_ATTACHMENTS_DIRS = [PRIMARY_ATTACHMENTS_DIR, '_assets'] export const ATTACHMENTS_DIRS = [ASSETS_DIR, ...LEGACY_ATTACHMENTS_DIRS] diff --git a/src/bridge/vault-fs.ts b/src/bridge/vault-fs.ts index 261f99b..db6c34c 100644 --- a/src/bridge/vault-fs.ts +++ b/src/bridge/vault-fs.ts @@ -48,6 +48,9 @@ import { firstMatchColumn, folderForRelativePath, hiddenPrimaryRootNames, + importedAssetFilename, + importedAssetMarkdown, + importedAssetRelPath, isExcalidrawPath, isMarkdownPath, joinPath, @@ -1228,20 +1231,27 @@ export class MobileVault { return { name: filename, path: rel, markdown: `![[${rel}]]`, kind: 'image' } } - async importDroppedFile(notePath: string, file: File): Promise { - const filename = await this.uniqueFilename('', file.name) + /** + * An attached file lands in `assets/`, the same folder a pasted image goes to + * (`importPastedImage`) and the same one the cloud vault uploads to, and is + * linked by vault-relative path. + * + * It used to be written to the vault ROOT and linked with a hand-built + * `'../'.repeat(depth)` path, so attaching a file scattered images among the + * notes and produced `![name](<../name.jpg>)` (reported on Discord by xenin). + * Desktop had exactly this bug and fixed it in #377; the port was missed + * here. Images take the wikilink form so every surface resolves them the same + * way paste already does. + */ + async importDroppedFile(_notePath: string, file: File): Promise { const bytes = new Uint8Array(await file.arrayBuffer()) - await this.fs.writeBase64(filename, bytesToBase64(bytes)) + await this.fs.mkdir(ASSETS_DIR) + const filename = await this.uniqueFilename(ASSETS_DIR, importedAssetFilename(file.name)) + const rel = importedAssetRelPath(filename) + await this.fs.writeBase64(rel, bytesToBase64(bytes)) const kind = classifyImportedAsset(filename) - const noteDir = dirName(resolveSafeRel(notePath)) - const relFromNote = noteDir - ? `${'../'.repeat(noteDir.split('/').length)}${filename}` - : filename - const dest = `<${relFromNote.replace(/>/g, '%3E')}>` - const markdown = - kind === 'image' ? `![${stemName(filename)}](${dest})` : `[${filename}](${dest})` - emitVaultChange({ kind: 'add', path: filename, folder: 'inbox', scope: 'content' }) - return { name: filename, path: filename, markdown, kind } + emitVaultChange({ kind: 'add', path: rel, folder: 'inbox', scope: 'content' }) + return { name: filename, path: rel, markdown: importedAssetMarkdown(rel, filename, kind), kind } } async renameAsset(relPath: string, nextName: string): Promise {