Skip to content

Fix attachment upload from custom widget with ACL #1512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fix attachment upload from custom widget with ACL #1512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc5c093
Fix attachment upload from custom widget with ACL
guillett Mar 14, 2025
8a9d9c9
lint
guillett Mar 14, 2025
872d599
Extract attachment check
guillett Mar 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/server/lib/ActiveDoc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1655,7 +1655,8 @@ export class ActiveDoc extends EventEmitter {

public async getAccessToken(docSession: OptDocSession, options: AccessTokenOptions): Promise<AccessTokenResult> {
const tokens = this._server.getAccessTokens();
const userId = getUserId(docSession);
const user = await this._granularAccess.getUser(docSession);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you walk me through what this fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It applied ACL related accessses instead of connected user's.

const userId = user.UserID;
const docId = this.docName;
const access = getDocSessionAccess(docSession);
// If we happen to be using a "readOnly" connection, max out at "readOnly"
Expand Down
6 changes: 4 additions & 2 deletions app/server/lib/Authorizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,11 @@ export async function addRequestUser(
if (auth) {
const tokens = options.gristServer.getAccessTokens();
const token = await tokens.verify(auth);
const userId = token.userId;
const user = await dbManager.getUser(userId);
mreq.accessToken = token;
// Once an accessToken is supplied, we don't consider anything else.
// User is treated as anonymous apart from having an accessToken.
mreq.user = user;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe? Could this potentially grant access to material beyond the specific document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that, this is safe as long as the party with access to the provided token is trusted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I understand the concern you raise with that question. It may be possible to access (or update) a another document from a custom widget linked to a document with read (and write) access.

I have the feeling that if this fix "only" reinforce that vulnerability but it does not create it.

I looked at the existing test suite, I remember some related to access limitations but I can't remember if any exist on cross document access limitation/validation.

Copy link
Contributor Author

@guillett guillett Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I tested this and I get a token misuse from app/server/lib/Authorizer.ts. Tested on DINUM sandbox environment and with the fix on my computer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repository with tests https://github.com/guillett/test-grist-access/blob/main/pages/index.js

(Sorry I did not added a test in grist-core test suite)

mreq.userId = userId;
authDone = true;
}

Expand Down
11 changes: 9 additions & 2 deletions app/server/lib/GranularAccess.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2376,6 +2376,14 @@ export class GranularAccess implements GranularAccessForBundle {
this._pruneColumns(action, permInfo, tableId, accessCheck);
}

private async _checkAttachmentAccess(docSession: OptDocSession, attId: number) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you talk me through what this fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes one specific situation. When a file is not attached to any cell yet. Without this fix, the access is denied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the lazy && does the same thing 😬

const list = await this._docStorage.findAttachmentReferences(attId);
if (list.length) {
return await this.findAttachmentCellForUser(docSession, attId);
}
return await this.isAttachmentUploadedByUser(docSession, attId);
}

/**
* Take a look at the DocAction and see if it might allow the user to
* introduce attachment ids into a cell. If so, make sure the user
Expand All @@ -2385,8 +2393,7 @@ export class GranularAccess implements GranularAccessForBundle {
const {docSession} = cursor;
const attIds = await this._gatherAttachmentChanges(cursor);
for (const attId of attIds) {
if (!await this.isAttachmentUploadedByUser(docSession, attId) &&
!await this.findAttachmentCellForUser(docSession, attId)) {
if (!await this._checkAttachmentAccess(docSession, attId)) {
throw new ErrorWithCode('ACL_DENY', 'Cannot access attachment', {
status: 403,
});
Expand Down
Loading