Skip to content
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

Honor table and view schema on query #14351

Merged
merged 30 commits into from
Aug 19, 2024

Conversation

adrinr
Copy link
Collaborator

@adrinr adrinr commented Aug 9, 2024

Description

Add the following security checks:

  1. For tables without specified columns, generate queries with the column names specified on the table instead of a global select *. Some columns might be hidden to budibase and we don't want to fetch it to ensure they are never exposed.
  2. For searches with specified columns (for views, for example) we are trimming any specified column that is not part of the actual view or table schema.
  3. For queries with filters, we are trimming any filter that is not present on the allowed table/view column schema. This will prevent using searches to infer some hidden column values.

Launchcontrol

Improve search row security resilience

Feature branch env

Feature Branch Link

Copy link

linear bot commented Aug 9, 2024

@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/m labels Aug 9, 2024
f => table.schema[f].visible !== false
)

if (options.fields) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this may be too high level for this - I think the SQL layers may ignore/override this in terms of query generation. If you look at server/src/api/controllers/row/ExternalRequest.ts on line 692 there is a function called buildSqlFieldList - this comes up with a list of fields which are going to be defined as part of the SELECT <fields> in the query. SQS does something similar with its function buildInternalFieldList - these today are independent of each other as how they generate their list of fields is quite different.

We could merge these into one path through the SDK which fetches a list of fields (and relationships) to add to the QueryJson which would take into account if it was performed through a view - we'd need to pass the list of visibleTableFields down to the search implementations via the RowSearchParams.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We would also need to handle the list of relationships, found top level on the QueryJson structure under the relationships key - this would need to have relationships removed from it if the relationship column is disabled through the view (this removes the JOIN). There are again two functions which do this today, for external DBs buildExternalRelationships and for SQS buildInternalRelationships.

Copy link
Collaborator

Choose a reason for hiding this comment

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

NOTE: none of this can be applicable to Lucene as it always returns the full row JSON, it cannot strip fields out at the database level - I wouldn't worry about this too much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After a reanalising the logic, I moved the trim to the outputProcessing. We still need to fetch all the columns for things like formulas.
This PR was meant to cover only the security around queries, so I will stick to it and I will tackle the actual SQL statement generation in a separated PR as talked on the meeting this morning

@github-actions github-actions bot added size/l and removed size/m labels Aug 14, 2024
@adrinr adrinr requested a review from mike12345567 August 14, 2024 10:33
@adrinr adrinr marked this pull request as ready for review August 14, 2024 10:35
@adrinr adrinr requested a review from a team as a code owner August 14, 2024 10:35
packages/server/src/sdk/app/rows/search.ts Outdated Show resolved Hide resolved
Comment on lines 149 to 153
if (handledTables.has(`${table._id}_${subSchema.tableId}`)) {
// avoid circular loops
continue
}
handledTables.add(`${subSchema.tableId}_${table._id}`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not obvious to me why the order is flipped here, a_b vs b_a. Is this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's to avoid circular loops. It's not so obvious, I am refactoring it and writing some tests for it

Copy link
Collaborator

@mike12345567 mike12345567 left a comment

Choose a reason for hiding this comment

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

LGTM - very nice work!

@adrinr adrinr added the feature-branch Release this PR code into a feature branch label Aug 16, 2024
@adrinr adrinr requested review from mike12345567 and samwho August 16, 2024 08:13
@adrinr adrinr force-pushed the BUDI-8547/honor-table-and-view-schema-on-query branch from febb92d to 07fe8c6 Compare August 19, 2024 14:00
@adrinr adrinr merged commit c0ad7d6 into master Aug 19, 2024
12 checks passed
@adrinr adrinr deleted the BUDI-8547/honor-table-and-view-schema-on-query branch August 19, 2024 19:39
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-branch Release this PR code into a feature branch firestorm Data/Infra/Revenue Team size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants