fix(security): validate uploaded file types by content with Apache Tika in the AI plugin#42005
Draft
wyattwalter wants to merge 5 commits into
Draft
fix(security): validate uploaded file types by content with Apache Tika in the AI plugin#42005wyattwalter wants to merge 5 commits into
wyattwalter wants to merge 5 commits into
Conversation
… Tika The Appsmith AI plugin forwarded uploaded files to the AI server without any server-side content inspection, trusting the client-supplied filename and Content-Type. The allowed types (application/pdf, text/plain, text/markdown) existed only as a client-side "allowedFileTypes" hint in form.json, so a crafted request could upload a disallowed type such as a malicious SVG. Detect each uploaded file's true type from its content bytes with Apache Tika (tika-core; MimeTypes magic detection composed with TextDetector, instantiated directly so it stays robust once the plugin is shaded into an uber-jar) and reject any file whose real content type is not in a server-side allow-list. A spoofed extension or Content-Type no longer matters because magic detection wins. Files are buffered (BufferedFilePart) so they can still be forwarded upstream after inspection. No bespoke size/count caps or markup scanning are added; those belong in framework/proxy config. APP-15345 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5-66516 Addresses the independent re-review of the AI-plugin upload validation. Finding 1 (bypass): Tika's text fallback reports SVG/HTML as text/plain when the markup does not begin at byte 0 (leading whitespace or a short text prefix), so a malicious SVG slipped through the allow-listed text/plain path. Add a narrow anti-smuggling check on the text/* branch only: after skipping a BOM and leading whitespace, scan the leading window for XML/HTML/SVG/script root markers (<svg, <?xml, <html, <!doctype html, <!doctype svg, <script) and reject if any is present. This is targeted, not the broad markup scan we deliberately avoided. Added regression tests for whitespace-prefixed SVG, SVG after a text prefix, markdown/text containing <script>/<html>, and whitespace-prefixed HTML - all rejected; genuine plain text and plain markdown still pass. Finding 2 (CVE): bump org.apache.tika:tika-core 3.2.1 -> 3.2.2, since 1.13-3.2.1 are affected by CVE-2025-66516 (XXE via crafted XFA in PDFs). Finding 3 (comment only, per captain): note that the 150MB multipart body limit is the size control for the buffering; no bespoke per-file/count cap. APP-15345 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…byte-scan Follow-up to the second re-review, which showed the byte-scan was the wrong control: UTF-16/UTF-32 SVG and markup padded past the 8KB scan window still slipped through as text/plain. Fix the root cause in detection instead. tika-core's bundled magic only recognises XML/SVG roots as ASCII bytes at offset 0, so an encoded or prolog-padded SVG fell through to the allow-listed text/plain. FileValidationUtils now does a two-pass, content-only detection: when the first pass is ambiguous (text/plain or octet-stream) it decodes the head from its real charset (BOM or null-pattern heuristic for UTF-16LE/BE and UTF-32), strips the BOM and XML-prolog whitespace, re-encodes as UTF-8 and re-detects. An encoded/padded SVG then surfaces as image/svg+xml and is rejected by the existing allow-list; a genuinely wide-encoded text file is accepted as text/plain; opaque binary stays rejected. This stays tika-core-only (its transitive commons-io is the only extra runtime jar - both are shaded in; verified), so no tika-parsers / ServiceLoader fragility. The containsSmuggledMarkup byte-scan and its markup markers/error message are removed entirely - the type allow-list now does the work. Empirically verified (probe + tests): UTF-8, UTF-16LE/BE (with and without BOM), UTF-32, namespace-prefixed and leading-whitespace SVG, and leading-whitespace HTML are all detected as image/svg+xml / text/html and REJECTED; genuine plain text, markdown, PDF and wide-encoded plain text are ACCEPTED. Keeps the tika-core 3.2.2 bump (CVE-2025-66516). APP-15345 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Follow-up to PR re-review: validateFileType joined the whole file into heap with no maxByteCount, bounded only by the global 150MB spring.webflux.multipart.max-in-memory-size (shared across every upload path, not scoped here, and not per-request). Pass a per-file byte cap to DataBufferUtils.join so the framework fails fast as the file is read instead of after it is fully buffered. The cap is 20 MiB, mirroring form.json's maxFileSizeInBytes (the limit the client already enforces) and well below the global ceiling - aligning server with client, the same way the type validation aligns with allowedFileTypes. join throws DataBufferLimitException on exceed; it is translated to an AppsmithPluginException with a clear message in the FILE_TYPE_NOT_SUPPORTED style, not a raw framework error. Added a test for the oversized-file rejection path. APP-15345 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to review: the previous oversized-file test was vacuous - a zero-filled MAX+1 buffer was rejected as octet-stream regardless of the cap (mutation-reverting join's maxByteCount still passed), and a single pre-allocated buffer never exercised join's mid-stream fail-fast. Replace it with three targeted tests (all in FileValidationUtilsTest): - oneByteOverSizeCap: 'a'-filled (valid text/plain) MAX+1 content, so only the size cap can reject it, and assert the exact FILE_TOO_LARGE message rather than any AppsmithPluginException. - withChunkedStreamExceedingCap: feed the content as many 1 MiB chunks from a Flux able to emit 3x the cap, assert size rejection AND that join cancelled the source near the cap (emitted <= capChunks + 2) - pinning real mid-stream fail-fast, not full buffering. - atExactlySizeCap: a file of exactly MAX is accepted, guarding the > vs >= boundary against an off-by-one. Verified by mutation: reverting join(content, MAX) to join(content) now fails 2 of these 3 tests. APP-15345 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The AI plugin's upload path validated files by the client-supplied filename and Content-Type — both spoofable. The allowed types already existed as a client-side
allowedFileTypeshint inform.json(application/pdf,text/plain,text/markdown), but the server never enforced them. This brings server-side validation in line with that hint — closing a client/server mismatch, not adding a new restriction — by detecting the true type from the file's content with Apache Tika (MIME + magic bytes) and rejecting anything outside the allow-list (e.g. a malicious SVG).Scope / severity: defense-in-depth, not an XSS-serving fix. Uploaded files are never served back to the user from the app origin — they're forwarded to the AI service and stored in S3, reachable only via short-lived presigned URLs on a different origin. Independently re-reviewed (SHIP-WITH-RISKS).
How
FileValidationUtilsdetects the type via tika-core detectors constructed directly (noServiceLoader, so it survives the plugin uber-jar shading; notika-parsers).image/svg+xmland is rejected. Genuine wide-encoded text still passes.DataBufferUtils.join's fail-fastmaxByteCount(20 MiB, mirroringform.json'smaxFileSizeInBytes), so an oversized upload is rejected as it is read rather than buffered into heap — scoped to this endpoint, unlike the shared 150MB global multipart ceiling. The resultingDataBufferLimitExceptionis translated to a clear rejection message.BufferedFilePartbuffers bytes so the file is inspected and still forwarded.tika-core3.2.2 (1.13–3.2.1 are affected by CVE-2025-66516).Residual (low): benign non-markup padding followed by a markup root stays
text/plain— correct, since it isn't well-formed XML and no parser renders it as SVG.Tests cover the encoding matrix (all rejected), oversized-file rejection, and allowed types incl. wide-encoded text (all accepted); module suite green.
Linear: https://linear.app/appsmith/issue/APP-15345
Warning
Tests have not run on the HEAD 6be7672 yet
Fri, 17 Jul 2026 13:16:03 UTC