fix: reject copy/move destinations in the reserved .da-versions folder#303
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The generic /copy and /move routes read their destination from the request
body. The finding-16 router guard runs on daCtx.key. That key is the copy/move
source, which has no .da-versions segment, so the guard passes. The destination
is not checked against the reserved namespace. A writer can plant objects under
{repo}/.da-versions/{fileId}/... and forge a document's version history and
audit log.
These tests drive a .da-versions destination through copyHandler, moveRoute,
and copyFile. Each expects a 400 with no write. They fail against current code:
the copy proceeds and returns 200. Companion tests assert a segment that only
contains 'da-versions' (my-da-versions-notes) still dispatches, pinning
segment-exact matching.
Version bodies and audit logs live at the reserved key
{org}/{repo}/.da-versions/{fileId}/.... The generic /copy and /move routes read
their destination from the request body. They did not check it against that
namespace. A writer could copy or move an object into {repo}/.da-versions/....
That plants attacker-controlled bodies where version and audit storage lives.
It forges a document's version history and audit log.
#301 rejects a different-org destination. It does not reject a same-org
.da-versions destination, so the vector was open. The .da-versions router guard
added in #302 inspects daCtx.key, which is the copy/move source, not the
destination. So it did not cover this either.
Add hasReservedSegment to the version paths module. It is true when any segment
of a path is .da-versions. copyHelper and moveHelper reject a destination in
that folder, next to the existing cross-org check. copyFile applies the same
check to each derived Key, so a folder copy cannot slip a reserved key through.
The check does not rely on hasPermission, which grants .da-versions writes in
no-config and broad-grant orgs. The ACL-aware version routes build the physical
key internally. Their daCtx.key does not carry a .da-versions segment, so they
are unaffected. A segment that only contains 'da-versions' stays allowed.
Adds an integration assertion after the cross-org tests. A copy and a move
whose destination is inside {repo}/.da-versions/... both return 400 from the
destination guard. The blocked move leaves its source in place. Against
pre-fix code the copy returns 204 and plants the object in version storage.
A repo-level move lists the repo's own version storage under
{repo}/.da-versions/.... copyFile must carry those keys to the new repo. The
broad reserved-Key guard rejects them with 400, so moveObject skips the delete
and strands the history at the old repo name. This test drives copyFile with a
source already under .da-versions and expects the copy to proceed. It fails
against the broad guard: it returns 400 and sends nothing.
The copyFile reserved-key guard rejected every derived key under .da-versions. A repo-level move lists the repo's own version storage, so the guard 400-skipped those keys and moveObject left them at the old repo name, stranding the version history. Reject only when the destination introduces the segment: a source that is already under .da-versions carries along. The forge is still blocked, because its source is ordinary content and the destination is what adds .da-versions.
hasReservedSegment is the shared reserved-name check for the copy/move guard. It had no direct unit test, so a substring form would pass the suite. Lock the contract: it matches .da-versions as a whole path segment, ignores a segment that only contains the name (my-da-versions-notes, .da-versions-backup), and is safe for non-string input.
…mption The refined guard skipped the check when the source was already under .da-versions, so a repo-level copy or move could relocate a .da-versions object into another .da-versions location. Chained with a separate write hole that plants crafted content in some .da-versions, that reopens the forge. This test drives copyFile with a source under .da-versions and expects 400 with no send. It fails against the exemption: it returns 200 and sends the copy.
An earlier revision exempted a source already under .da-versions. That let a repo-level rename carry its version storage along. The exemption is a hole. An attacker who plants crafted content in any .da-versions object could relocate it into a victim's version history through a repo-level copy or move. Drop the exemption: reject every derived key under .da-versions. Documented copy and move act on a path below the repo. That path does not list the repo-root .da-versions folder, so this changes nothing for them. Only an undocumented repo-root move or copy reaches version storage, and it no longer carries or duplicates it.
benpeter
force-pushed
the
da-versions-copy-move-destination-guard
branch
from
July 17, 2026 08:52
684ae97 to
8bfca4d
Compare
benpeter
marked this pull request as ready for review
July 17, 2026 08:53
kptdobe
reviewed
Jul 17, 2026
kptdobe
requested changes
Jul 17, 2026
kptdobe
left a comment
kptdobe
left a comment
Contributor
There was a problem hiding this comment.
Thanks. Just a minor feedback.
Co-authored-by: Alexandre Capt <acapt@adobe.com>
Co-authored-by: Alexandre Capt <acapt@adobe.com>
kptdobe
approved these changes
Jul 20, 2026
adobe-bot
pushed a commit
that referenced
this pull request
Jul 20, 2026
Collaborator
|
🎉 This PR is included in version 1.13.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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
3 participants
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.
The generic
/copyand/moveroutes read their destination from the request body. They did not check it against the reserved.da-versionsnamespace. Version bodies and audit logs live at{org}/{repo}/.da-versions/{fileId}/.... A writer could set the destination to{repo}/.da-versions/...and plant attacker-controlled objects in version and audit storage. That forges a document's version history and audit log, with chosen authors and timestamps.The router guard from #302 does not cover this. It inspects
daCtx.key, which for a copy or move is the source, not the destination. The cross-org guard from #301 does not cover it either. It rejects a different-org destination. A same-org.da-versionsdestination still passed.hasPermissionis not a gate here, because it grants.da-versionswrites in no-config and broad-grant orgs.The fix adds
hasReservedSegmentto the version paths module. It is true when any segment of a path is.da-versions.copyHelperandmoveHelperreject a destination in that folder with400 {"error":"Destination cannot be inside the reserved .da-versions folder."}, next to the existing cross-org check. A copy or move can no longer target the reserved namespace.copyFileapplies the same check to every key it derives. A folder copy cannot slip a reserved key through, whatever the source or the caller's grants. The ACL-aware/versionsourceand/versionlistroutes build the physical key in the storage layer. TheirdaCtx.keyhas no.da-versionssegment, so they are unaffected. A segment that only containsda-versions(for examplemy-da-versions-notes) stays allowed.Tests: route-level rejection for copy and move, and acceptance of a lookalike segment. A storage test asserts
copyFilereturns 400 and sends no copy, including when the source is itself under.da-versions. Unit tests lock the segment-exact matching. Integration coverage shows the copy and move into.da-versionsreturn 400. A live check shows the forge before the fix (copy returned 204) and the block after (400).Relates to #301, #302.