fix(api): refuse file uploads unless a plugin declared the path - #7851
Open
ar2rsawseen wants to merge 7 commits into
Open
fix(api): refuse file uploads unless a plugin declared the path#7851ar2rsawseen wants to merge 7 commits into
ar2rsawseen wants to merge 7 commits into
Conversation
POST bodies are parsed with formidable before a request is routed or authorized. formidable writes multipart parts and raw application/octet-stream bodies to disk, so any POST carrying such a body leaves a temp file behind - including requests that never reach a handler, and requests rejected during validation. Nothing removed those files afterwards, so they accumulated in the OS temp directory for the lifetime of the process. - skip the file writing parsers for paths that cannot reach an upload handler. Countly routes API requests under /i and /o only, so bodies sent anywhere else can never be claimed. Both a filter and dropping the octetstream plugin are needed: formidable's octetstream plugin creates its file directly and never consults filter. - write upload temp files to a dedicated directory rather than straight into the OS temp directory, so they are attributable and separable from unrelated files. Configurable via api.uploadDir. - periodically remove unclaimed upload temp files from that directory. Removal is age based, so it cannot race a request that is still using its file, including handlers that keep reading after responding. Configurable via api.uploadTempMaxAge. Existing upload endpoints are unaffected: every path that consumes params.files lives under /i, and the installation subpath is stripped before matching so subdirectory installs keep working. Co-Authored-By: Claude <noreply@anthropic.com>
Replaces the periodic sweep with prevention at the API layer plus removal at the points where a request is rejected before dispatch. - multipart files are now only written under /i. Every endpoint that consumes params.files lives there; nothing under /o accepts an upload. - raw application/octet-stream bodies are now only written for the crash_symbols endpoints that actually read them, which is also why api/api.js already buffers those requests differently. Previously any octet-stream body anywhere created a file, since formidable's octetstream plugin creates it directly and never consults filter. - files parsed out of a request that is then rejected are removed at the rejection site: a missing app_key or device_id, a path no handler serves, and via /sdk/cancel a bad app key or failed checksum. Nothing has been dispatched at those points, so nothing can be mid-read. Removal stays away from response teardown, and only touches files directly inside the upload directory, because some handlers keep using their temp file after responding and crash_symbolication repoints params.files[x].path at files shipped with the plugin. api.uploadDir is kept so the files are attributable and an operator can target one directory. api.uploadTempMaxAge and the sweep are gone. Co-Authored-By: Claude <noreply@anthropic.com>
ar2rsawseen
changed the title
POST bodies are parsed with formidable before a request is routed or
authorized, so any multipart or application/octet-stream body was written
to disk regardless of whether an endpoint would ever read it. Enumerating
the places a request can be rejected does not cover this: rights.js alone
rejects in 63 places, with no shared exit point.
Uploads are now refused by default and allowed only where a plugin says
it reads params.files:
plugins.uploadPaths.push({path: "/i/license/upload"});
plugins.uploadPaths.push({path: "/i/crash_symbols/add_symbol", raw: true});
Paths match exactly, after the installation subpath is stripped, so
declaring the SDK write endpoint does not reopen the whole /i tree. `raw`
declares that the endpoint reads an application/octet-stream body; without
it the octetstream parser is left out, which is the only way to stop a raw
body reaching disk since that parser creates its file directly and never
consults `filter`.
Whatever does get written is removed when the response is done with, on
both 'finish' and 'close' - an upload aborted mid-stream leaves a partial
file that 'finish' never sees. Handlers that consume an upload still unlink
it themselves; this catches every request rejected before reaching one.
Only files directly inside the upload directory are removed, because
crash_symbolication repoints params.files[x].path at files shipped with
the plugin when serving populator data.
For this to be safe, apps.js now waits for iconUpload before responding:
it was not awaited, and jimp opened the temp file after the response.
Co-Authored-By: Claude <noreply@anthropic.com>
ar2rsawseen
changed the title
The dedicated upload directory existed only so cleanup could tell a file formidable created from a path a handler substituted, by comparing the directory. Recording the paths formidable produced, before any handler runs, does that more directly and needs no directory, no configuration and no startup mkdir. - drops api.uploadDir and its config.sample.js and configextender entries, both files are now untouched by this branch - trackUploads snapshots the paths at parse time, discardUploads removes exactly those, so a handler repointing params.files[x].path cannot redirect it at a file that must survive - upload-temp.js no longer needs os or path Operationally nothing is lost: /tmp has to be a writable mount anyway under readOnlyRootFilesystem, so emptyDir with medium Memory covers uploads along with report PDFs and browser profiles, without a second mount or a config knob to keep in sync. Co-Authored-By: Claude <noreply@anthropic.com>
The single fixture listed all sixteen upload paths under a comment saying they were what core and the plugins declare, which reads as if this repo registers /i. It does not: there is no users plugin here, so nothing in this repo reads an upload on /i, and the declaration lives in the users plugin in countly-enterprise-plugins. Splits the fixture into what this repo actually declares and what the enterprise plugins declare, and asserts the coupling both ways: the enterprise endpoints are refused with only this repo installed, and allowed once their declarations are present. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
Merge order update: the companion PR Countly/countly-enterprise-plugins#3324 now initialises That means the two no longer have to land in the same window — merge the enterprise PR first, then this one. Landing this one first would still leave the twelve enterprise upload endpoints undeclared and therefore refused, so the order matters even though the coupling is no longer simultaneous. For context on why: with no defensive init, |
The fixture restated the declarations, so it could not fail: deleting or mistyping a push in the source left every assertion passing, and listing the enterprise paths alongside this repo's read as if this repo declared them. Reads the declarations out of the source instead, and checks the things a restated list cannot: - the declared set matches an expected snapshot, so removing an endpoint is a visible diff rather than an endpoint quietly refusing uploads - each declaration is a bare path under /i or /o, with no trailing slash, query string or wildcard - each declaration is actually matched, so one the matcher can never match - a trailing slash, say - fails instead of silently refusing its own uploads - no declaration permits a deeper path The behavioural tests now use a synthetic registry, since the matcher does not care which paths happen to exist and realistic looking data was what made the old fixture misleading. Verified by breaking a declaration three ways - trailing slash, missing leading slash, deleted - and confirming each fails. Loading the plugins would be more faithful than a scan, but a plugin api.js cannot be required in isolation: it pulls in common.js, the config and the database drivers, and a stub would have to cover some 28 pluginManager members across 37 plugins. Noted in the file that only the integration suite can check that every endpoint reading params.files has a declaration. Co-Authored-By: Claude <noreply@anthropic.com>
This is the check that catches a declaration whose path shape is wrong. A declaration that matches no request refuses the upload it was meant to permit, and nothing else reveals it: the enterprise repo declared /i/surveys/create where clients request /i/surveys/nps/create, and only comparing the declarations against the URLs the tests really post to found it. Scans the plugin specs for a .post whose chain also has an .attach, bounding each request at the next .post so an upload in one test is not attributed to the next one, and ignoring commented out lines - plugins/push/tests.js has a disabled upload suite that otherwise looks real. This repo has no active upload test, so the check passes trivially today. /i/apps/* and /i/feedback/* have nothing exercising a real multipart POST, which is itself worth fixing; the check is here so it starts guarding as soon as one is added. Co-Authored-By: Claude <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.
Problem
api/api.jsparses every POST body with formidable before the request is routed or authorized, and formidable writes multipart parts and rawapplication/octet-streambodies to disk. So any POST carrying such a body left a temp file behind — including requests to paths no handler serves, and requests rejected during validation. Nothing removed them, so they accumulated in the OS temp directory.Cleaning up at each rejection site does not work:
rights.jsalone rejects in 63 places across seven validators with no shared exit point, so an unauthenticatedPOST /i/apps/updatewould still leak.Change
Uploads are refused by default. A plugin declares the paths where it actually reads
params.files:Mirrors the existing
plugins.ttlCollectionsregistry.init()requires every plugin'sapi.jswell before the server listens, so declarations are always in place. Core declares/i/apps/{create,update}; star-rating declares/i/feedback/{upload,logo}./i(the SDK write endpoint, which carries the app-user picture) does not reopen the whole/itree.POST /i/anythingwrites nothing.rawdeclares that an endpoint reads anapplication/octet-streambody. Without it theoctetstreamparser is left out, which is the only way to stop a raw body reaching disk — that parser calls_newFile()directly and never consultsfilter.params.qstringunchanged.Whatever does get written is removed when the response is done with, on both
finishandclose— an upload aborted mid-stream leaves a partial file thatfinishnever sees. Handlers that consume an upload still unlink it themselves, so in the normal case this is a no-op; what it catches is every request rejected before reaching a handler, with no rejection-site enumeration.Cleanup works off a snapshot of the paths formidable produced, taken at parse time before any handler runs — not off
params.files. That matters becausecrash_symbolicationrepointsparams.files.symbols.pathat files shipped with the plugin when serving populator data; those must never be deleted, and a snapshot makes that impossible by construction rather than by directory comparison.No new configuration:
config.sample.jsandconfigextender.jsare untouched by this branch.Ordering prerequisite
Cleanup at response time is only safe because handlers no longer use their temp file afterwards.
iconUploadinapi/parts/mgmt/apps.jswas not awaited beforecommon.returnOutput, andjimpopened the file after the response — it now waits, with a rejection handler sinceiconUploadcan reject on a bad MIME type. The two equivalents in the enterprise plugins (addAppUserPicture, and the cohorts long task) are handled in the companion PR.Compatibility
Note
init()requires every plugin directory present, including disabled ones, so a disabled plugin's path is still declared — harmless, since the endpoint will not route and the file is discarded at response time.No dependency change —
formidablestays at 2.1.3.Verification
test/unit-tests/api.utils.upload-temp.js— 21 unit tests: every declared endpoint allowed; undeclared paths under/irefused (/i/apps/delete,/i/bulk,/i/whatever); exact matching (/iallowed,/i/anythingrefused); empty and absent registries refuse everything;rawhonoured only where declared; subdirectory installs; near-misses like/iffy; and cleanup, including an explicit test that a path a handler substituted after parsing is left alone while the real upload is removed.Verified end-to-end against a real
formidable@2.1.3server mirroring the patched handler — 14 checks asserting both halves, that nothing is written for undeclared paths and that nothing survives the response even on declared paths whose handler never consumed the file: multipart and octet-stream to unrouted paths,/i/apps/delete,/i/garbageand/o/exportwrite nothing; octet-stream to/iand/i/apps/updatewrite nothing;/i,/i/apps/updateand/i/crash_symbols/*receive their file and it is gone afterwards; urlencoded fields still parse on a blocked path.🤖 Generated with Claude Code