Prevents documents being copied with external attachments #1433
Conversation
|
This may seem like an odd place to add this check to (the This felt like an okay approach for now, but I'm open to refactors if desired, such as renaming the function or creating a new |
| @@ -2803,6 +2809,23 @@ function testDocApi(settings: { | |||
| locationSummary: "internal", | |||
| }); | |||
| }); | |||
|
|
|||
| it("POST /docs/{did}/copy fails when the document has external attachments", async function () { | |||
There was a problem hiding this comment.
Forking works correctly under the external attachments model - forks use their parent's document pool, so that doesn't need blocking.
I did test with forks and the copy block, and "Save to copy" is also correctly blocked in a document fork.
app/server/lib/uploads.ts
Outdated
| } | ||
| const attachmentsLocation: DocAttachmentsLocation = (await transferStatusResponse.json()).locationSummary; | ||
| if (attachmentsLocation !== 'internal' && attachmentsLocation !== 'none') { | ||
| throw new ApiError("Cannot copy a document with external attachments", 400); |
There was a problem hiding this comment.
In the options object you have template parameter. When it is passed here as true it means that user wants to copy the structure, not the data. So in this case it should be allowed.
There was a problem hiding this comment.
This is a really good catch, thanks :)
There was a problem hiding this comment.
This should be sorted now :)
Spoffy
force-pushed
the
spoffy/external-attachments-copy-block
branch
from
1f7fdee to
fccdb25
Compare
| @@ -534,7 +534,7 @@ export class DocWorkerApi { | |||
| })); | |||
|
|
|||
| // Returns the status of any current / pending attachment transfers | |||
| this._app.get('/api/docs/:docId/attachments/transferStatus', isOwner, withDoc(async (activeDoc, req, res) => { | |||
| this._app.get('/api/docs/:docId/attachments/transferStatus', canView, withDoc(async (activeDoc, req, res) => { | |||
There was a problem hiding this comment.
Should /attachments/store also be canView for consistency?
There was a problem hiding this comment.
That's a little leakier, as it reveals (in the future) user label and maybe store type to unauthenticated users.
Happy with either, I'm not sure if that's a security concern or not.
There was a problem hiding this comment.
That said - all that info is in the downloaded Grist doc, thinking about it, so it's not private info. So yes, we could make it canView.
| { name: 'hello.doc', contents: 'foobar' }, | ||
| { name: 'world.jpg', contents: '123456' }, | ||
| { name: 'hello2.doc', contents: 'foobar' } | ||
| ], chimpy); |
There was a problem hiding this comment.
You removed the last assert here.
There was a problem hiding this comment.
I moved it into the addAttachmentsToDoc function - although it's now a length check rather than an exact ID check.
I'll re-add the exact ID check to be safe.
Context
When external attachments are uploaded, they are placed in a folder in the external storage called a 'document pool', which is specific to the document it was uploaded.
When a document is copied, the new copy has a different document pool. This means any external attachments in the document will be broken, and show as unavailable.
Proposed solution
This blocks the copying of documents with external attachments, until copy behaviour can be implemented in the future.
Related issues
#1021
Has this been tested?