Make attachments and app bindings more robust yet secure #8570
Conversation
larkox
added
2: Dev Review
larkox
requested review from
a team,
mgdelacroix and
hanzei
and removed request for
a team
|
@mgdelacroix @hanzei Friendly reminder on this |
| @@ -327,7 +330,7 @@ function AppsFormComponent({ | |||
|
|
|||
| const performLookup = useCallback(async (name: string, userInput: string): Promise<AppSelectOption[]> => { | |||
| const field = form.fields?.find((f) => f.name === name); | |||
| if (!field) { | |||
| if (!field || !field.name) { | |||
There was a problem hiding this comment.
nit:
| if (!field || !field.name) { | |
| if (!field?.name) { |
| if (!this.field.name) { | ||
| return this.asError(this.intl.formatMessage({ | ||
| id: 'apps.error.parser.missing_field_name', | ||
| defaultMessage: 'Field name is missing.', | ||
| })); | ||
| } |
There was a problem hiding this comment.
I'm surprised this is caught on the server side. Anyway, it doesn't hurt to validate also client side.
However, I wonder how much time we should spend on Apps. 1/5 that we should touch this code.
| @@ -23,15 +23,15 @@ type AppsState = { | |||
| }; | |||
|
|
|||
| type AppBinding = { | |||
| app_id: string; | |||
| location: string; | |||
| app_id?: string; | |||
There was a problem hiding this comment.
Yes and no. The thing is... many of these fields can be inherited. So... the "root binding" should have them, but the rest don't need to define it. The cleanBindings function will take care of that.
I thought about making the distinction between clean and raw bindings, but it complicated many things, so I preferred to go with just making them optional everywhere.
| app_id: string; | ||
| location: string; | ||
| app_id?: string; | ||
| location?: string; |
There was a problem hiding this comment.
Should we be more strict and require a location?
There was a problem hiding this comment.
Same as in the previous comment.
|
Regarding Apps, there has been some complaints about this breaking: https://community.mattermost.com/core/pl/4xqtw5taef88xk8wz71yq585je That is why I wanted to fix it. |
larkox
added
the
Build Apps for PR
|
@larkox the build app is failing. Can you help checking if its due to pr changes? |
|
/update-branch |
larkox
added
Build Apps for PR
Summary
When addressing some security issues, we started validating attachments and app bindings. This led to consider some missing fields as "invalid" attachments (or bindings).
This PR just consider every field as optional, and handle that in the code.
Will try to do something similar for webapp.
Ticket Link
Mobile part of https://mattermost.atlassian.net/browse/MM-62356
Release Note