-
-
Notifications
You must be signed in to change notification settings - Fork 479
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @guillett. Some questions to help my understanding.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
this._pruneColumns(action, permInfo, tableId, accessCheck); | ||
} | ||
|
||
private async _checkAttachmentAccess(docSession: OptDocSession, attId: number) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😬
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
@fflorent here a quick/fast 😅 demo |
@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? |
Oh yes a missclick sorrryy |
@fflorent I confirm I have the issue when adding an editor (on another browser) and trying to sign and upload a file. |
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 |
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
Has this been tested?
Screenshots / Screencasts