[fix][star-rating] keep the uploaded logo name inside the images directory - #7930
Open
ar2rsawseen wants to merge 2 commits into
Open
[fix][star-rating] keep the uploaded logo name inside the images directory#7930ar2rsawseen wants to merge 2 commits into
ar2rsawseen wants to merge 2 commits into
Conversation
…ctory
uploadFile() took the identifier straight from the request and concatenated it into the
upload path:
var pp = path.resolve(__dirname, './../images/' + id + "." + detectedExt);
so separators or leading dots in the identifier chose where the file was written rather
than only what it was called. With fileStorage set to "fs" the destination reaches
fs.writeFile unchanged, so the write could land in any directory that already exists.
Validate the identifier before it is used, next to the existing parseFeedbackLogoName in
image-utils.js, and use the validated value for the path, for the stored file id and for
the name returned to the caller. The dashboard sends Date.now() as the identifier, so a
plain name is all that ever needs to be accepted.
The sibling /i/feedback/upload already validates its target name this way, and app icon
uploads in api/parts/mgmt/apps.js sanitize theirs, so this brings the last of the three
into line.
test/unit-tests/star-rating.image-utils.js covers the traversal, separator, absolute path
and truncation shapes, plus the identifiers the dashboard actually sends. Reverting the
validator to the previous pass-through fails four of them.
The logo upload is authorized with validateCreate against the caller's own app_id, but the file it writes is named only by the request's identifier, and every app's logos live in one shared directory. A widget's logo field holds just that file name, so the identifier alone decided whose logo was replaced. The name is not private either: it comes back with the widget from the sdk facing by-id lookups. Refuse a name that a widget belonging to a different app already points at. The sibling /i/feedback/upload route decodes its target app out of the file name for the same reason, with a comment saying an admin of one app must not be able to plant a logo for another, so this brings the older route into line. Matching on the full name including the extension is deliberate: a different extension is a different file and overwrites nothing. Re-uploading your own app's logo is unaffected, since the check only looks at widgets whose app differs from the caller's.
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.
Two problems with the same request parameter on
/i/feedback/logo: where the file lands, and whose file it replaces.1. The identifier chose the write location
uploadFile()concatenated the identifier into the upload path:idisparams.qstring.identifier, so separators or leading dots in it selected a directory rather than only a file name. WithfileStorageset to"fs"the destination reachesfs.writeFileunchanged, so the write could land in any directory that already exists. Under the shipped default of"gridfs"the same call keeps only the basename, so the path never reaches disk.Fix: validate the identifier in
image-utils.js, next to the existingparseFeedbackLogoName, and use the validated value for the path, the stored id and the returned name. The dashboard sendsDate.now(), so accepting a plain name covers every real upload.2. The identifier also chose whose logo was replaced
The route is authorized with
validateCreateagainst the caller's ownapp_id, but every app's logos share one directory and a widget'slogofield holds just the file name. So the identifier alone decided which app's logo was overwritten, and an account with create rights on its own app could replace the logo shown in another app's end-user feedback popup. The name is not private: it comes back with the widget from the sdk facing by-id lookups.Fix: refuse a name that a widget belonging to a different app already points at.
The sibling
/i/feedback/uploadroute already does this, decoding its target app out of the file name with a comment saying an admin of one app must not be able to plant a logo for another. This brings the older route into line.Two details worth stating:
Scope
Every place a request supplied value reaches a file write, with a verdict:
plugins/star-rating/api/api.jsuploadFile(/i/feedback/logo)plugins/star-rating/api/api.jsuploadFeedbackFile(/i/feedback/upload)api/parts/mgmt/apps.jsiconUploadcommon.sanitizeFilenameapi/utils/render.jsscreenshot pathcrash_symbolicationsymbol_idpassescommon.db.ObjectID()and afindOnematch before the writeuploadFilehas exactly one caller, so both checks cover the whole reachable surface.Verification
test/unit-tests/star-rating.image-utils.js, 29 cases: traversal, separators in both slash directions, absolute and drive style paths, NUL and tab truncation, plus the numeric identifiers the dashboard sends and plain names with dashes and underscores. Reverting the validator to the previous pass through fails four of them.feedback_widgetsand is not covered by an automated case: the plugin suite has one application, so a second one would have to be created first. Verified by reading the query and its two branches instead, and noted here rather than implied.node --checkclean on the changed files.