diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index 50545ecb1255..7ddfd8ebe1b2 100644 --- a/config/locales/js-en.yml +++ b/config/locales/js-en.yml @@ -1078,6 +1078,7 @@ en: error_could_not_resolve_user_name: "Couldn't resolve user name" error_attachment_upload: "File failed to upload: %{error}" error_attachment_upload_permission: "You don't have the permission to upload files on this resource." + error_attachment_type_not_allowed: "The file was rejected by an automatic filter. '%{value}' is not allowed for upload." units: workPackage: diff --git a/frontend/src/app/core/config/configuration.service.ts b/frontend/src/app/core/config/configuration.service.ts index ee186227657a..e0f0862197d2 100644 --- a/frontend/src/app/core/config/configuration.service.ts +++ b/frontend/src/app/core/config/configuration.service.ts @@ -100,6 +100,10 @@ export class ConfigurationService { return this.systemPreference('allowedLinkProtocols') || null; } + public get attachmentWhitelist():string[] { + return this.systemPreference('attachmentWhitelist') ?? []; + } + public dateFormatPresent():boolean { return !!this.systemPreference('dateFormat'); } diff --git a/frontend/src/app/features/hal/resources/configuration-resource.ts b/frontend/src/app/features/hal/resources/configuration-resource.ts index 39b8547c926e..5fd070799591 100644 --- a/frontend/src/app/features/hal/resources/configuration-resource.ts +++ b/frontend/src/app/features/hal/resources/configuration-resource.ts @@ -31,4 +31,5 @@ import { HalResource } from 'core-app/features/hal/resources/hal-resource'; export class ConfigurationResource extends HalResource { public perPageOptions:number[]; public allowedLinkProtocols?:string[]; + public attachmentWhitelist?:string[]; } diff --git a/frontend/src/react/components/OpBlockNoteEditor.tsx b/frontend/src/react/components/OpBlockNoteEditor.tsx index ac3148f355ce..35764a2f9e39 100644 --- a/frontend/src/react/components/OpBlockNoteEditor.tsx +++ b/frontend/src/react/components/OpBlockNoteEditor.tsx @@ -36,9 +36,9 @@ import { BlockNoteView } from '@blocknote/mantine'; import { getDefaultReactSlashMenuItems, SuggestionMenuController, useCreateBlockNote } from '@blocknote/react'; import { HocuspocusProvider } from '@hocuspocus/provider'; import { initializeOpBlockNoteExtensions, openProjectWorkPackageBlockSpec, openProjectWorkPackageSlashMenu } from 'op-blocknote-extensions'; -import { useCallback, useEffect, useMemo } from 'react'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; import * as Y from 'yjs'; -import { useBlockNoteAttachments } from '../hooks/useBlockNoteAttachments'; +import { useBlockNoteAttachments, BlockNoteEditorRef } from '../hooks/useBlockNoteAttachments'; import { useBlockNoteLocale } from '../hooks/useBlockNoteLocale'; import { useOpTheme } from '../hooks/useOpTheme'; @@ -79,7 +79,19 @@ export function OpBlockNoteEditor({ doc, }:OpBlockNoteEditorProps) { const { localeString, localeDictionary } = useBlockNoteLocale(window.I18n.locale); - const { enabled: attachmentsEnabled, uploadFile } = useBlockNoteAttachments(attachmentsCollectionKey, attachmentsUploadUrl); + + // useBlockNoteAttachments needs the editor instance to remove a stuck + // placeholder block on failed uploads, but the editor is created later + // in this function. We pass a lazy getter that reads from a ref assigned + // after useCreateBlockNote, breaking the would-be circular dependency. + const editorRef = useRef(null); + const getEditor = useCallback(() => editorRef.current, []); + + const { enabled: attachmentsEnabled, uploadFile } = useBlockNoteAttachments( + attachmentsCollectionKey, + attachmentsUploadUrl, + getEditor, + ); useEffect(() => { initializeOpBlockNoteExtensions({ baseUrl: openProjectUrl, locale: localeString }); @@ -113,6 +125,8 @@ export function OpBlockNoteEditor({ }, [hocuspocusProvider, doc, activeUser, localeDictionary, attachmentsEnabled, uploadFile, captureExternalLinks]); const editor = useCreateBlockNote(editorParams, [activeUser]); + editorRef.current = editor; + type EditorType = typeof editor; const theme = useOpTheme(); diff --git a/frontend/src/react/hooks/useAttachmentValidation.ts b/frontend/src/react/hooks/useAttachmentValidation.ts new file mode 100644 index 000000000000..9446288ac6df --- /dev/null +++ b/frontend/src/react/hooks/useAttachmentValidation.ts @@ -0,0 +1,70 @@ +/* + * -- copyright + * OpenProject is an open source project management software. + * Copyright (C) the OpenProject GmbH + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License version 3. + * + * OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: + * Copyright (C) 2006-2013 Jean-Philippe Lang + * Copyright (C) 2010-2013 the ChiliProject Team + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + * See COPYRIGHT and LICENSE files for more details. + * ++ + */ + +import { useCallback } from 'react'; + +export interface AttachmentValidationResult { + valid:boolean; + reason?:string; +} + +export function useAttachmentValidation() { + const validateFile = useCallback(async (file:File):Promise => { + const pluginContext = await window.OpenProject.getPluginContext(); + const whitelist = pluginContext.services.configurationService.attachmentWhitelist; + + if (!whitelist || whitelist.length === 0) { + return { valid: true }; + } + + // Empty file.type means the browser couldn't infer the MIME from the + // extension (e.g. .xyz). Defer to the backend in that case - it does + // real magic-byte detection that we can't replicate cheaply on the client. + if (!file.type) { + return { valid: true }; + } + + if (whitelist.includes(file.type)) { + return { valid: true }; + } + + return { + valid: false, + reason: window.I18n.t( + 'js.error_attachment_type_not_allowed', + { value: file.type }, + ), + }; + }, []); + + return { validateFile }; +} + +export default useAttachmentValidation; diff --git a/frontend/src/react/hooks/useBlockNoteAttachments.ts b/frontend/src/react/hooks/useBlockNoteAttachments.ts index 065da67dc6cb..41de30a34e7b 100644 --- a/frontend/src/react/hooks/useBlockNoteAttachments.ts +++ b/frontend/src/react/hooks/useBlockNoteAttachments.ts @@ -31,15 +31,21 @@ import { IUploadFile } from 'core-app/core/upload/upload.service'; import { useCallback } from 'react'; import { firstValueFrom } from 'rxjs'; +import { useAttachmentValidation } from './useAttachmentValidation'; export interface BlockNoteAttachmentsResult { enabled:boolean; - uploadFile?:(file:File) => Promise; + uploadFile?:(file:File, blockId?:string) => Promise; +} + +export interface BlockNoteEditorRef { + removeBlocks:(ids:string[]) => void; } export function useBlockNoteAttachments( attachmentsCollectionKey:string, attachmentsUploadUrl:string, + getEditor?:() => BlockNoteEditorRef | null, ):BlockNoteAttachmentsResult { const enabled = ( attachmentsCollectionKey !== undefined && @@ -48,25 +54,57 @@ export function useBlockNoteAttachments( attachmentsUploadUrl !== '' ); - const uploadFile = useCallback(async (file:File):Promise => { + const { validateFile } = useAttachmentValidation(); + + // BlockNote 0.44.x creates a "Loading..." placeholder block before awaiting + // uploadFile (blocknote.js Ie(), ~line 1130) and only calls updateBlock on + // the success path - it has no try/catch around the await. On rejection, + // the placeholder is left in the document forever. We remove it ourselves + // here. removeBlocks is synchronous and safe because BlockNote never + // touches the block again after the rejected await. + const removePlaceholder = useCallback((blockId?:string) => { + const editor = getEditor?.(); + if (!editor || !blockId) return; + try { + editor.removeBlocks([blockId]); + } catch { /* already removed by a collaborator via Yjs */ } + }, [getEditor]); + + const uploadFile = useCallback(async (file:File, blockId?:string):Promise => { const pluginContext = await window.OpenProject.getPluginContext(); + + const validation = await validateFile(file); + if (!validation.valid) { + pluginContext.services.notifications.addError(validation.reason ?? 'File not allowed'); + removePlaceholder(blockId); + return ''; + } + try { const service = pluginContext.services.attachmentsResourceService; const uploadFiles:IUploadFile[] = [{ file }]; const result = await firstValueFrom( - service.addAttachments(attachmentsCollectionKey, attachmentsUploadUrl, uploadFiles) + service.addAttachments(attachmentsCollectionKey, attachmentsUploadUrl, uploadFiles), ); - return result?.[0]._links.staticDownloadLocation.href ?? ''; + const href = result?.[0]?._links?.staticDownloadLocation?.href; + if (!href) { + throw new Error('Upload returned no download location'); + } + return href; // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (error:any) { - const toastService = pluginContext.services.notifications; // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - toastService.addError(error); + pluginContext.services.notifications.addError(error); + removePlaceholder(blockId); + // Return '' instead of rethrowing: BlockNote 0.44.x doesn't catch + // uploadFile rejections, so throwing would surface as Uncaught (in + // promise). The placeholder is already gone, so the success-path + // updateBlock that follows our return won't fire anyway. return ''; } - }, [attachmentsCollectionKey, attachmentsUploadUrl]); + }, [attachmentsCollectionKey, attachmentsUploadUrl, validateFile, removePlaceholder]); if (!enabled) { return { enabled }; diff --git a/lib/api/v3/configuration/configuration_representer.rb b/lib/api/v3/configuration/configuration_representer.rb index a708e87e3f4f..9dafe559c82d 100644 --- a/lib/api/v3/configuration/configuration_representer.rb +++ b/lib/api/v3/configuration/configuration_representer.rb @@ -116,6 +116,14 @@ class ConfigurationRepresenter < ::API::Decorators::Single property :allowed_link_protocols, getter: ->(*) { Setting::AllowedLinkProtocols.all } + property :attachment_whitelist, + getter: ->(*) { + raw = Setting.attachment_whitelist + Array(raw).flat_map { |entry| entry.to_s.split(/\r?\n/) } + .map(&:strip) + .compact_blank + } + property :user_preferences, embedded: true, exec_context: :decorator,