Fix attachment upload from custom widget with ACL #1512

guillett · 2025-03-14T14:11:34Z

Context

I am building a custom widget to sign PDF (not electronically, yet?) and I had issues when access rules apply.

Proposed solution

Fixing auth related bugs. 😬

Related issues

First of all, it was hard to get a minimal failing test.
To get a working bogus example

Create a new doc
Update column A to be of attachment type
Add an editor
Add an ACL rule to prevent editors from editing
Add a table specific ACL rule to allow editors edition
Add a custom widget to your view (for instance https://betagouv.github.io/grist-previsionnel/signer-pdf)
- (link it to Table1 to select)
Create a new row adding a PDF in column A
It should display the PDF document in the custom widget
Click on « create and add updated PDF »
Nothing should happen
In the browser console, the « Error: Cannot access attachment » error should appeared.

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help I don't really know where to look at to add a test (or multiple)

Screenshots / Screencasts

guillett · 2025-04-07T10:10:33Z

Oupsy https://betagouv.github.io/grist-previsionnel/signer-pdf not ~~https://betagouv.github.io/grist-previsionnel/sign-pdf~~

paulfitz

Thanks @guillett. Some questions to help my understanding.

paulfitz · 2025-04-07T14:20:22Z

app/server/lib/Authorizer.ts

    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;


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

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

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.

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.

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)

paulfitz · 2025-04-07T14:22:15Z

app/server/lib/GranularAccess.ts

    this._pruneColumns(action, permInfo, tableId, accessCheck);
  }

+  private async _checkAttachmentAccess(docSession: OptDocSession, attId: number) {


Can you talk me through what this fixes?

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

Actually the lazy && does the same thing 😬

paulfitz · 2025-04-07T14:22:33Z

app/server/lib/ActiveDoc.ts

  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);


Can you walk me through what this fixes?

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

fflorent · 2025-04-07T14:39:59Z

Also @guillett, I have just tried your reproduction scenario without success. I wondered if there exist steps that I should follow as the editor instead of the owner.

guillett · 2025-04-07T15:25:30Z

@fflorent here a quick/fast 😅 demo
Capture vidéo du 2025-04-07 17-21-11-s.webm

fflorent · 2025-04-07T15:57:16Z

@guillett thanks. I wonder if that's not due to the "view as" feature, simply. I'll try to reproduce that on my side.

Also I see that you closed the issue, is it a missclick?

guillett · 2025-04-07T16:05:32Z

Oh yes a missclick sorrryy

guillett · 2025-04-07T16:07:49Z

@fflorent I confirm I have the issue when adding an editor (on another browser) and trying to sign and upload a file.

fflorent · 2025-04-08T15:58:28Z

Ah, I have made a mistake in my access rules. I know reproduce your issue @guillett!

Also I could locate the code of your widget: https://github.com/betagouv/grist-previsionnel/blob/main/pages/signer-pdf.js

fflorent · 2025-04-08T16:13:17Z

Interestingly, to reproduce the issue, the editor MUST be removed the READ access to all the tables by default, which is the most important part:

If they are granted this access, the error does not occur anymore, even when they are forbidden update, creation and deletion rights to records by default (overridden by the specific rules for Table1):

vviers · 2025-04-25T13:29:00Z

@paulfitz we took a step back with @fflorent while reviewing this PR and came across this piece of code

grist-core/app/server/lib/Authorizer.ts

Lines 162 to 173 in b90a730

    
           // Support providing an access token via an `auth` query parameter. 
        
           // This is useful for letting the browser load assets like image 
        
           // attachments. 
        
           const auth = optStringParam(mreq.query.auth, 'auth'); 
        
           if (auth) { 
        
             const tokens = options.gristServer.getAccessTokens(); 
        
             const token = await tokens.verify(auth); 
        
             mreq.accessToken = token; 
        
             // Once an accessToken is supplied, we don't consider anything else. 
        
             // User is treated as anonymous apart from having an accessToken. 
        
             authDone = true; 
        
           }

Reading the comment, it looks like the fact that providing an AccessToken does not identify the user is actually on purpose, we assume for security reasons ? Could you explain what motivates this ? Would it be an issue if a custom widget with an auth token could fully impersonate a user in a document through the REST API (@guillett has shown in a comment above that access is limited to the document) ?

Trying to assert whether this limitation is by design or not, before looking at the best way to implement this 🙂

vviers · 2025-04-25T13:36:42Z

fwiw, @fflorent point out that the view-as helper defined here

grist-core/app/common/DocListAPI.ts

Lines 85 to 88 in b90a730

    
           export interface UserOverride { 
        
             user: FullUser|null; 
        
             access: Role|null; 
        
           }

could be another way to implement a solution

Also, thinking out loud here, and probably not really a solution : wondering how soon-to-be-implemented ServiceAccounts could pave the way for a clever long term solution here 🤔 (I'm thinking, define a service account dedicated to a widget's access to the document ?) this doesn't feel right, or safe

vviers · 2025-04-25T14:07:25Z

@guillett we think there's another workaround to your issue.

Looking at your code (https://github.com/betagouv/grist-previsionnel/blob/80d573bbea25c8c27c6a672485da5bc81ea56d4a/pages/signer-pdf.js#L78-L98), you perform the attachment upload by the widget in 2 steps:

step 1, upload the attachment using the Grist REST API and an AccessToken from the user (L78-86). This, as shown above in the PR, is technically performed by an anonymous user.
step 2, add the newly uploaded attachment ids to the Table using a method of the grist-plugin-api (window.grist.getTable().update) (L90-98). This performs the action as if done by the connected user.

The fundamental issue is that, under "complicated access rules" context, Grist calls the isAttachmentUploadedByUser method in step 2 and concludes that no, the attachment you're trying to add to the table in step 2 was not uploaded by you in step 1.

We think there are two ways to solve this :

make sure that calls to the RestAPI performed by the widget using an AccessToken are authenticated (what you propose in this PR)
or you could modify your custom widget so that step 2 (adding attachments to the table) is done through the RestAPI (https://support.getgrist.com/api/#tag/records/operation/addRecords) as well instead of the grist-plugin-api, so that Grist considers that both steps were performed by the same (anonymous) user. This looks like this would be a quick fix that fits your need while we take the time to work on this PR (which I think would be the more satisfying solution). WDYT ?

paulfitz · 2025-04-25T16:08:49Z

Reading the comment, it looks like the fact that providing an AccessToken does not identify the user is actually on purpose, we assume for security reasons ? Could you explain what motivates this ?

It could be reasonable for the AccessToken to identify the user. A naive implementation in that area of the code, though, would result in making the request have the full power and credentials of the user. That could have unintended consequences, for example, allowing other unrelated documents that the user has access to to be enumerated and read and modified for example. But the token is supposed to be narrower than that.

Many ways to make this happen, some of them easy! The AccessToken work was a minimal piece of work needed to implement viewing attachments in a custom widget that happened to be somewhat more general purpose. But there wasn't time in the project to elaborate on it before moving on. I'd have loved to have built it out more. Sorry it is rough and ready!

vviers · 2025-04-30T14:26:59Z

@paulfitz what's you take on this PR ? As far as we're concerned we're OK with the current implementation. Should we spend more time on this ?

vviers · 2025-04-30T14:27:26Z

@guillett did you get a chance to try the workaround we suggested ?

guillett · 2025-04-30T14:46:31Z

I tried but it failed, I got a cannot access attachment error. betagouv/grist-previsionnel@fd42e71

I ended up adding a backend proxy with some level of check 😬
betagouv/grist-budget-agriculture@c9038ef

betagouv/grist-budget-agriculture@e37589f

It is an ok temporary solution

guillett · 2025-04-30T14:46:44Z

(and many many thanks for your time on this)

paulfitz · 2025-04-30T17:50:40Z

@paulfitz what's you take on this PR ? As far as we're concerned we're OK with the current implementation. Should we spend more time on this ?

@guillett @vviers I'd like to see this checked with non-doc endpoints such as /api/profile/apiKey. Doc endpoints are aware of these access tokens, and check for a doc match in the token. Non-doc endpoints are simply unaware of access tokens. So if you go ahead and authenticate as the user based on this access token, I think there is no further protection.

guillett · 2025-05-13T11:58:46Z

Many thanks @paulfitz for your insights. You are right with the current implementation it drop existing protection. I will dig a little to suggest another implementation.

guillett · 2025-05-14T11:41:52Z

This is superseeded by #1594 and #1614.

fflorent self-requested a review March 25, 2025 21:16

paulfitz reviewed Apr 7, 2025

View reviewed changes

guillett closed this Apr 7, 2025

guillett reopened this Apr 7, 2025

fflorent moved this to Needs feedback in French administration Board Apr 9, 2025

fflorent added this to French administration Board Apr 9, 2025

fflorent moved this from Needs feedback to In Progress in French administration Board Apr 16, 2025

vviers self-requested a review April 23, 2025 14:25

guillett added 3 commits April 25, 2025 10:03

Fix attachment upload from custom widget with ACL

cc5c093

lint

8a9d9c9

Extract attachment check

872d599

fflorent force-pushed the fix-attachment-upload branch from 494b5c5 to 872d599 Compare April 25, 2025 08:04

vviers moved this from In Progress to Needs feedback in French administration Board Apr 25, 2025

guillett requested a review from paulfitz April 29, 2025 15:17

guillett mentioned this pull request May 5, 2025

Fix accessToken generated in custom widgets using the "view as another user" feature #1594

Open

4 tasks

guillett closed this May 14, 2025

github-project-automation bot moved this from Needs feedback to Done in French administration Board May 14, 2025

hexaltation removed this from French administration Board Jun 11, 2025

paulfitz mentioned this pull request Jul 24, 2025

Attach user from access token after checks #1614

Open

1 task

maluhialoha mentioned this pull request Sep 24, 2025

Allow to download attachment from custom widget link when ACL are applied #1853

Open

Uh oh!

Fix attachment upload from custom widget with ACL #1512

Fix attachment upload from custom widget with ACL #1512

Uh oh!

Conversation

guillett commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Proposed solution

Related issues

Has this been tested?

Screenshots / Screencasts

Uh oh!

guillett commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulfitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guillett Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fflorent commented Apr 7, 2025

Uh oh!

guillett commented Apr 7, 2025

Uh oh!

fflorent commented Apr 7, 2025

Uh oh!

guillett commented Apr 7, 2025

Uh oh!

guillett commented Apr 7, 2025

Uh oh!

fflorent commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fflorent commented Apr 8, 2025

Uh oh!

vviers commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vviers commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vviers commented Apr 25, 2025

Uh oh!

paulfitz commented Apr 25, 2025

Uh oh!

vviers commented Apr 30, 2025

Uh oh!

vviers commented Apr 30, 2025

Uh oh!

guillett commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guillett commented Apr 30, 2025

Uh oh!

paulfitz commented Apr 30, 2025

Uh oh!

guillett commented May 13, 2025

Uh oh!

guillett commented May 14, 2025

guillett commented Mar 14, 2025 •

edited

Loading

guillett commented Apr 7, 2025 •

edited

Loading

guillett Apr 16, 2025 •

edited

Loading

fflorent commented Apr 8, 2025 •

edited

Loading

vviers commented Apr 25, 2025 •

edited

Loading

vviers commented Apr 25, 2025 •

edited

Loading

guillett commented Apr 30, 2025 •

edited

Loading