mirror of
https://github.com/hanzoai/team.git
synced 2026-09-30 01:57:35 +00:00
f573fc6601
* Bound document content extraction to prevent upload DoS (MM-69098) Extraction of uploaded Office documents ran with no time or size bound and acquired a shared worker-pool slot on the request goroutine, so a low-privilege user could keep cheap-to-upload but expensive-to-extract documents in flight and degrade file uploads for every user on the server. - Enforce a configurable per-extraction timeout (FileSettings.ExtractContentTimeout, default 10s) and honor the previously-ignored size argument in the document and PDF extractors to bound the work performed. - Run extraction on a dedicated, bounded worker pool with non-blocking submit (GoExtraction) so saturating it can never block the request goroutines that dispatch uploads; overflow is skipped and backfilled by the ExtractContent job. - Route the file and resumable-upload extraction dispatch sites through the new pool instead of the shared GoBuffered/Go pools. Co-authored-by: Julien Tant <[email protected]> * Use require.Fail instead of t.Fatal in extraction pool test The mattermost-govet tFatal analyzer (run by check-style) forbids t.Fatal in tests in favor of testify assertions. Co-authored-by: Julien Tant <[email protected]> * ci: retrigger workflows (transient container init failure) Co-authored-by: Julien Tant <[email protected]> * Use sync.WaitGroup.Go for extraction workers Satisfies the golangci-lint waitgroupgo/modernize analyzer flagged by check-style at goroutines.go:58. Co-authored-by: Julien Tant <[email protected]> * Fix file close race with detached extraction goroutine On timeout, extractWithTimeout returns while the converter may still be reading the input on a detached goroutine. ExtractContentFromFileInfo previously closed the file via defer as soon as Extract returned, racing with (and closing the file out from under) that goroutine. Transfer close ownership to docextractor via ExtractSettings.ReaderCloser: the reader is now closed only after extraction actually finishes - in the detached goroutine on the timeout path, or after Extract returns on the synchronous path. ExtractContentFromFileInfo no longer closes the file itself. Co-authored-by: Julien Tant <[email protected]> * Document that extraction timeout bounds wait time, not CPU Clarify in ExtractSettings.Timeout and extractWithTimeout that on timeout the docconv converter keeps running on a detached goroutine (docconv is not context-aware), so the timeout bounds how long an extraction occupies a worker slot, not the CPU it consumes. MaxFileSize is the primary bound on per-extraction work. Note load-shedding as a possible future improvement. Co-authored-by: Julien Tant <[email protected]> * Add tests for PDF size cap and ExtractContentTimeout validation - TestPdfMaxFileSize exercises the LimitedReaderWithError branch in pdf.go: a tight MaxFileSize errors out before extraction, while zero and generous limits extract normally. - TestFileSettingsExtractContentTimeout asserts ExtractContentTimeout=-1 fails IsValid while 0 (disabled) and 10 (default) pass. Co-authored-by: Julien Tant <[email protected]> * Harden extraction shutdown, panic safety, and timeout input validation - stopExtractionWorkers now drains the queue after signaling stop so a worker cannot dequeue and run buffered tasks during shutdown. - The detached extraction goroutine recovers panics and converts them to errors, so a panic in an extractor cannot crash the server. - Admin console enforces a client-side minimum of 0 for ExtractContentTimeout (validators.minValue). - Test hardening: bounded wait for the detached extractor to start, plus a test that a panic is surfaced as an error. Co-authored-by: Julien Tant <[email protected]> * ci: retrigger workflows (transient Docker Hub image pull timeout) Co-authored-by: Julien Tant <[email protected]> * Correct misleading extraction-skip logs and comments The queue-full warning logs and GoExtraction doc comments claimed skipped content would be backfilled by a 'periodic ExtractContent job'. That job is registered with a nil scheduler, so it never runs automatically -- it only runs when an admin manually triggers a content extraction job (mmctl extract). Reword the logs and comments to state that skipped files stay unsearchable until an admin runs a content extraction job, so we don't imply automatic recovery that doesn't exist. --------- Co-authored-by: Julien Tant <[email protected]> Co-authored-by: Hanzo Dev <[email protected]>
69 lines
1.8 KiB
Go
69 lines
1.8 KiB
Go
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
|
|
// See LICENSE.txt for license information.
|
|
|
|
package docextractor
|
|
|
|
import (
|
|
"errors"
|
|
"io"
|
|
"path"
|
|
"strings"
|
|
|
|
"code.sajari.com/docconv/v2"
|
|
|
|
"github.com/mattermost/mattermost/server/v8/channels/utils"
|
|
)
|
|
|
|
type documentExtractor struct{}
|
|
|
|
var doconvConverterByExtensions = map[string]func(io.Reader) (string, map[string]string, error){
|
|
"doc": docconv.ConvertDoc,
|
|
"docx": docconv.ConvertDocx,
|
|
"pptx": docconv.ConvertPptx,
|
|
"odt": docconv.ConvertODT,
|
|
"html": func(r io.Reader) (string, map[string]string, error) { return docconv.ConvertHTML(r, true) },
|
|
// Temporarily disabled to avoid crashes on malicious .pages files
|
|
// "pages": docconv.ConvertPages,
|
|
"rtf": docconv.ConvertRTF,
|
|
"pdf": docconv.ConvertPDF,
|
|
}
|
|
|
|
func (de *documentExtractor) Name() string {
|
|
return "documentExtractor"
|
|
}
|
|
|
|
func (de *documentExtractor) Match(filename string) bool {
|
|
extension := strings.TrimPrefix(path.Ext(filename), ".")
|
|
_, ok := doconvConverterByExtensions[extension]
|
|
return ok
|
|
}
|
|
|
|
func (de *documentExtractor) Extract(filename string, r io.ReadSeeker, maxFileSize int64) (out string, outErr error) {
|
|
defer func() {
|
|
if r := recover(); r != nil {
|
|
out = ""
|
|
outErr = errors.New("error extracting document text")
|
|
}
|
|
}()
|
|
|
|
extension := strings.TrimPrefix(path.Ext(filename), ".")
|
|
converter, ok := doconvConverterByExtensions[extension]
|
|
if !ok {
|
|
return "", errors.New("unknown converter")
|
|
}
|
|
|
|
// Bound how much data the converter is allowed to read so a small upload
|
|
// cannot expand into an unbounded amount of in-memory work.
|
|
var reader io.Reader = r
|
|
if maxFileSize > 0 {
|
|
reader = utils.NewLimitedReaderWithError(r, maxFileSize)
|
|
}
|
|
|
|
text, _, err := converter(reader)
|
|
if err != nil {
|
|
return "", err
|
|
}
|
|
|
|
return text, nil
|
|
}
|