From b0f6b9a33db00e2f02511d05eeb8d3a2cced30f0 Mon Sep 17 00:00:00 2001 From: Zef Hemel Date: Wed, 19 Nov 2025 14:57:10 +0100 Subject: [PATCH] Add permission check before write/delete operations This avoids accidentally writing to a read-only file and only finding out about it after a sync. Also removes redundant space primitive implementations that weren't used --- client/client.ts | 27 +---- client/spaces/checked_space_primitives.ts | 33 ++++-- client/spaces/plug_space_primitives.ts | 134 ---------------------- client/spaces/ro_space_primitives.ts | 44 ------- 4 files changed, 30 insertions(+), 208 deletions(-) delete mode 100644 client/spaces/plug_space_primitives.ts delete mode 100644 client/spaces/ro_space_primitives.ts diff --git a/client/client.ts b/client/client.ts index b503dc94..218ffe6d 100644 --- a/client/client.ts +++ b/client/client.ts @@ -37,7 +37,6 @@ import type { } from "@silverbulletmd/silverbullet/type/event"; import type { StyleObject } from "../plugs/index/style.ts"; import { jitter, throttle } from "@silverbulletmd/silverbullet/lib/async"; -import { PlugSpacePrimitives } from "./spaces/plug_space_primitives.ts"; import { EventedSpacePrimitives } from "./spaces/evented_space_primitives.ts"; import { HttpSpacePrimitives } from "./spaces/http_space_primitives.ts"; import { @@ -54,12 +53,10 @@ import { import { ClientSystem } from "./client_system.ts"; import { createEditorState, isValidEditor } from "./codemirror/editor_state.ts"; import { MainUI } from "./editor_ui.tsx"; -import type { SpacePrimitives } from "./spaces/space_primitives.ts"; import { DataStore } from "./data/datastore.ts"; import { IndexedDBKvPrimitives } from "./data/indexeddb_kv_primitives.ts"; import { DataStoreMQ } from "./data/mq.datastore.ts"; -import { ReadOnlySpacePrimitives } from "./spaces/ro_space_primitives.ts"; import { LimitedMap } from "@silverbulletmd/silverbullet/lib/limited_map"; import { fsEndpoint } from "./spaces/constants.ts"; import { diffAndPrepareChanges } from "./codemirror/cm_util.ts"; @@ -72,7 +69,7 @@ import type { PageMeta, } from "@silverbulletmd/silverbullet/type/index"; import { parseMarkdown } from "./markdown_parser/parser.ts"; -import { CheckPathSpacePrimitives } from "./spaces/checked_space_primitives.ts"; +import { CheckedSpacePrimitives } from "./spaces/checked_space_primitives.ts"; import { notFoundError, offlineError, @@ -109,7 +106,6 @@ export class Client { space!: Space; clientSystem!: ClientSystem; - plugSpaceRemotePrimitives!: PlugSpacePrimitives; eventedSpacePrimitives!: EventedSpacePrimitives; httpSpacePrimitives!: HttpSpacePrimitives; @@ -315,24 +311,11 @@ export class Client { }, ); - let remoteSpacePrimitives: SpacePrimitives = new CheckPathSpacePrimitives( - this.httpSpacePrimitives, - ); - - if (this.bootConfig.readOnly) { - remoteSpacePrimitives = new ReadOnlySpacePrimitives( - this.httpSpacePrimitives, - ); - } - - this.plugSpaceRemotePrimitives = new PlugSpacePrimitives( - remoteSpacePrimitives, - this.clientSystem.namespaceHook, - this.bootConfig.readOnly ? undefined : "client", - ); - this.eventedSpacePrimitives = new EventedSpacePrimitives( - this.httpSpacePrimitives, + new CheckedSpacePrimitives( + this.httpSpacePrimitives, + this.bootConfig.readOnly, + ), this.eventHook, this.ds, ); diff --git a/client/spaces/checked_space_primitives.ts b/client/spaces/checked_space_primitives.ts index 37128f2d..6e3a019e 100644 --- a/client/spaces/checked_space_primitives.ts +++ b/client/spaces/checked_space_primitives.ts @@ -2,9 +2,15 @@ import type { FileMeta } from "@silverbulletmd/silverbullet/type/index"; import type { SpacePrimitives } from "./space_primitives.ts"; import { isValidPath } from "@silverbulletmd/silverbullet/lib/ref"; -export class CheckPathSpacePrimitives implements SpacePrimitives { +/** + * Adds checks for two things: + * 1. Allowed path names + * 2. Permissions + */ +export class CheckedSpacePrimitives implements SpacePrimitives { constructor( private wrapped: SpacePrimitives, + private readOnly: boolean, ) { } @@ -28,23 +34,23 @@ export class CheckPathSpacePrimitives implements SpacePrimitives { return this.wrapped.getFileMeta(path, observing); } - writeFile( + async writeFile( path: string, data: Uint8Array, meta?: FileMeta, ): Promise { - if (!this.isWritable(path)) { - throw new Error("Couldn't write file, path is invalid"); + if (!await this.isWritable(path)) { + throw new Error("Couldn't write file, path is not writable"); } return this.wrapped.writeFile(path, data, meta); } - deleteFile(path: string): Promise { + async deleteFile(path: string): Promise { // We allow deletion of paths we can't write to. This is for the case when // the user has an invalidly named file in their space and they need to // remove it/rename it - if (!this.isReadable(path)) { - throw new Error("Couldn't delete file, path isn't writable"); + if (!await this.isWritable(path)) { + throw new Error("Couldn't delete file, path is not writable"); } return this.wrapped.deleteFile(path); } @@ -53,7 +59,18 @@ export class CheckPathSpacePrimitives implements SpacePrimitives { return !path.startsWith("."); } - private isWritable(path: string): boolean { + private async isWritable(path: string): Promise { + if (this.readOnly) { + return false; + } + try { + const fileMeta = await this.getFileMeta(path); + if (fileMeta.perm === "ro") { + return false; + } + } catch { + // Assumption, not found, that's ok + } return this.isReadable(path) && isValidPath(path); } } diff --git a/client/spaces/plug_space_primitives.ts b/client/spaces/plug_space_primitives.ts deleted file mode 100644 index ec8ca434..00000000 --- a/client/spaces/plug_space_primitives.ts +++ /dev/null @@ -1,134 +0,0 @@ -import type { SpacePrimitives } from "./space_primitives.ts"; -import type { NamespaceOperation } from "@silverbulletmd/silverbullet/type/namespace"; -import type { PlugNamespaceHook } from "../plugos/hooks/plug_namespace.ts"; -import type { FileMeta } from "@silverbulletmd/silverbullet/type/index"; - -export class PlugSpacePrimitives implements SpacePrimitives { - constructor( - private wrapped: SpacePrimitives, - private hook: PlugNamespaceHook, - private env?: string, - ) { - } - - // Used e.g. by the sync engine to see if it should sync a certain path (likely not the case when we have a plug space override) - public isLikelyHandled(path: string): boolean { - for ( - const { pattern } of this.hook.spaceFunctions - ) { - if (path.match(pattern)) { - return true; - } - } - return false; - } - - performOperation( - type: NamespaceOperation, - path: string, - ...args: any[] - ): Promise | false { - for ( - const { operation, pattern, plug, name, env } of this.hook.spaceFunctions - ) { - // console.log( - // "Going to match agains pattern", - // operation, - // pattern, - // path, - // this.env, - // env, - // ); - if ( - operation === type && path.match(pattern) && - // Both envs are set, and they don't match - (!this.env || !env || env === this.env) - ) { - return plug.invoke(name, [path, ...args]); - } - } - return false; - } - - async fetchFileList(): Promise { - const allFiles: FileMeta[] = []; - const alreadySeenFiles = new Set(); - for (const { plug, name, operation } of this.hook.spaceFunctions) { - if (operation === "listFiles") { - try { - for (const pm of await plug.invoke(name, [])) { - allFiles.push(pm); - // console.log("Adding file from plug space", pm.name); - alreadySeenFiles.add(pm.name); - } - } catch (e: any) { - if (!e.message.includes("not available")) { - // Don't report "not available in" environments errors - console.error("Error listing files", e); - } - } - } - } - const files = await this.wrapped.fetchFileList(); - for (const pm of files) { - // We'll use the files coming from the wrapped space only as a fallback - if (alreadySeenFiles.has(pm.name)) { - continue; - } - allFiles.push(pm); - } - return allFiles; - } - - async readFile( - path: string, - ): Promise<{ data: Uint8Array; meta: FileMeta }> { - const result: { data: Uint8Array; meta: FileMeta } | false = await this - .performOperation( - "readFile", - path, - ); - if (result) { - return result; - } - return this.wrapped.readFile(path); - } - - getFileMeta(path: string, observing?: boolean): Promise { - const result = this.performOperation("getFileMeta", path, observing); - if (result) { - return result; - } - return this.wrapped.getFileMeta(path, observing); - } - - writeFile( - path: string, - data: Uint8Array, - meta?: FileMeta, - ): Promise { - const result = this.performOperation( - "writeFile", - path, - data, - meta, - ); - if (result) { - return result; - } - - return this.wrapped.writeFile( - path, - data, - meta, - ); - } - - deleteFile(path: string): Promise { - const result = this.performOperation("deleteFile", path); - if (result) { - return result; - } - return this.wrapped.deleteFile(path); - } -} diff --git a/client/spaces/ro_space_primitives.ts b/client/spaces/ro_space_primitives.ts deleted file mode 100644 index 7c05da9e..00000000 --- a/client/spaces/ro_space_primitives.ts +++ /dev/null @@ -1,44 +0,0 @@ -import type { SpacePrimitives } from "./space_primitives.ts"; -import type { FileMeta } from "../../plug-api/types/index.ts"; - -export class ReadOnlySpacePrimitives implements SpacePrimitives { - wrapped: SpacePrimitives; - - constructor(wrapped: SpacePrimitives) { - this.wrapped = wrapped; - } - - async fetchFileList(): Promise { - return (await this.wrapped.fetchFileList()).map((f: FileMeta) => ({ - ...f, - perm: "ro", - })); - } - - async readFile(path: string): Promise<{ meta: FileMeta; data: Uint8Array }> { - const { meta, data } = await this.wrapped.readFile(path); - return { - meta: { - ...meta, - perm: "ro", - }, - data, - }; - } - - async getFileMeta(path: string, observing?: boolean): Promise { - const meta = await this.wrapped.getFileMeta(path, observing); - return { - ...meta, - perm: "ro", - }; - } - - writeFile(): Promise { - throw new Error("Read only space, not allowed to write"); - } - - deleteFile(): Promise { - throw new Error("Read only space, not allowed to delete"); - } -}