Audit / Removal / Auth check of all dataSource related endpoint #9750

spolu · 2025-01-03T16:49:00Z

Description

Post Spaces work, we left unused endpoints behind. Goal of this PR to remove all unused legacy data sources endpoint, audit the ones that are still in use and audit the permissioning of all legacy and new endpoints.

Full audit log here explaining the PR:

## front/pages/api/w/[wId]/data_sources/[dsId]/managed/config/[key]/index.ts

- Used in useConnectorConfig which is in use in connector specific components, all admin views
- Adde canAdministrate on the dataSource

## front/pages/api/w/[wId]/data_sources/[dsId]/managed/permissions/index.ts

- Used in ConnectorPermissionModal (Connection Admin) + implementation used in poke as well
- Endpoint enforces canAdministrate on the dataSource

## front/pages/api/w/[wId]/data_sources/[dsId]/tables/[tId]/index.ts

- Fixed canWrite enforce on DELETE in spaces equivalent endpoint: pages/api/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/tables/[tableId]/index.ts
- Can't find any use of this one
- No calls to it in the past month
- Get for tables list view in folders go through content-nodes
- Get when editing a table go through spaces
- PATCH / DELETE go through spaces
  ==> Deleting endpoint

Audited spaces table endpoints.

create/patch/delete is on the data_sources
view before editing is on the data_source_views (a bit weird because same page but ok)
list is on data_source_views, used by the TablePicker

All endpoints authorizations have been audited

## front/pages/api/w/[wId]/data_sources/[dsId]/tables/index.ts

- Likely not used anymore
- 0 calls over the last month
- As per above not involved in table management
  ==> Deleting endpoint

## front/pages/api/w/[wId]/data_sources/[dsId]/tables/csv.ts

- Not used anymore in favor of the space/data source endpoint as per above
- 0 calls over the last month
  ==> Deleting endpoint

Renaming useCreateDataSourceViewTable into useCreateDataSourceTable

Weird that we create a file and yet pass the full CSV to the endpoint

## front/pages/api/w/[wId]/data_sources/[dsId]/managed/update.ts

- In use when updating connection
- Added a canAdministrate in endpoint implementation

## front/pages/api/w/[wId]/data_sources/[dsId]/configuration.ts

- Unused over the last month
- Seems replaced by pages/api/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/configuration.ts
- Confirmed by local tests

## front/pages/api/w/[wId]/data_sources/[dsId]/connector.ts

- In use
- Can be seen by non-admin based on code, so leaving the authorization open to all in workspace.
- Could potentially be restricted to canAdministrate with more audit but won't do it now.

## front/pages/api/w/[wId]/data_sources/request_access.ts

- In use
- Simple send email wrapper added a defense in depth auth.isUser() in the endpoint

## front/pages/api/w/[wId]/data_sources/[dsId]/search.ts

- Unused and dangerous.
- 0 calls over the last month
  ==> Deleting endpoint

## front/pages/api/w/[wId]/data_sources/[dsId]/usage.ts

- In use for folder deletion. Added canRead

## front/pages/api/w/[wId]/data_sources/[dsId]/index.ts

- GET unused removing
- DELETE unused removing
- POST is used to configure the Dust assistant the only thing updatable is the
  `assistantDefaultSelected` field. In that screen data source views are used to list. Will keep the
  auth to being isBuilder. Which is a small vector (someone can enable the flag on a data source
  they don't have access to (assuming they got their end on the ID) but this won't have any impact
  since we use the data source views of the global space in the Dust agent

## front/pages/api/w/[wId]/data_sources/managed.ts

- Unused
- Legacy connection and data source creation replaced by its space scoped version.
  ==> Deleting endpoint

## front/pages/api/w/[wId]/data_sources/index.ts

- Used useDataSources for name conflicts and plan limits
- Not great great as we leak spaces data source names across spaces
- Will create a specific task to refactor this: https://github.com/dust-tt/tasks/issues/1899

Risk

Low. All tested locally.

Deploy Plan

deploy front

spolu · 2025-01-03T16:53:08Z

PMRR (since it's all removing or making stricter so safe inherently)

spolu · 2025-01-04T11:41:53Z

@devloai review this PR

devloai · 2025-01-04T11:42:25Z

I encountered an error while processing the request. Please try again or contact support if the issue persists.

devloai · 2025-01-04T23:01:11Z

I encountered an error while processing the request. Please try again or contact support if the issue persists.

devloai

I've reviewed the PR and the security audit looks thorough. The removal of unused endpoints reduces attack surface, and the addition of permission checks improves security. Two suggestions to further enhance security:

Consider adding GET method permission checks in the tables endpoint for consistent auth across all operations
For the connector endpoint, consider filtering sensitive configuration data for non-admin users

Overall, good work on improving the authorization model and cleaning up unused endpoints.

devloai · 2025-01-04T23:08:01Z

front/pages/api/w/[wId]/spaces/[spaceId]/data_sources/[dsId]/tables/[tableId]/index.ts

@@ -123,6 +123,16 @@ async function handler(
      return;

    case "DELETE":
+      if (!dataSource.canWrite(auth)) {


Good addition of the canWrite permission check. However, consider also adding a similar check for the GET method to ensure consistent permission enforcement across all operations.

devloai · 2025-01-04T23:08:02Z

front/pages/api/w/[wId]/data_sources/[dsId]/connector.ts

@@ -29,8 +29,11 @@ async function handler(
    });
  }

+  // This endpoint can be access by non admin to get the connector chip status. Ensure that no


The comment about non-admin access is good, but consider adding a more explicit check to ensure no sensitive configuration data is returned in the response. You may want to filter the response object to only include connection state related fields.

devloai · 2025-01-04T23:08:02Z

front/pages/api/w/[wId]/data_sources/[dsId]/managed/update.ts

@@ -75,6 +64,17 @@ async function handler(
    });
  }

+  if (!dataSource.canAdministrate(auth) || !auth.isAdmin()) {


Good addition of canAdministrate check, but consider moving the auth check before fetching workspace and user data for better efficiency. This would prevent unnecessary database queries for unauthorized users.

No need for auth.isAdmin here.

flvndvd · 2025-01-06T08:07:34Z

front/pages/api/w/[wId]/data_sources/[dsId]/connector.ts

  const dataSource = await DataSourceResource.fetchById(auth, dsId);
-  if (!dataSource) {
+  if (!dataSource || !auth.isUser()) {


The auth.isUser() isn't required it's already covered by the withSessionAuthenticationForWorkspace.

We have decided to lean in duplicating security mitigations. Rationale is that every system can fail and we should accept duplication for authentication (which saved us from catastrophic situations in the recent past already)

flvndvd · 2025-01-06T08:09:28Z

front/pages/api/w/[wId]/data_sources/[dsId]/managed/config/[key]/index.ts

@@ -115,13 +115,14 @@ async function handler(
      return;

    case "POST":
-      if (!auth.isAdmin()) {
+      if (!auth.isAdmin() || !dataSource.canAdministrate(auth)) {


We can remove the auth.isAdmin here as well since it's covered by dataSource.canAdministrate

The two are slighly different. One tests the role directly the other tests the capability

The role is used in auth has well, see here:

dust/front/lib/auth.ts

Line 798 in 7104095

if (hasRolePermissions(resourcePermission)) {

flvndvd · 2025-01-06T08:09:43Z

front/pages/api/w/[wId]/data_sources/[dsId]/managed/update.ts

@@ -75,6 +64,17 @@ async function handler(
    });
  }

+  if (!dataSource.canAdministrate(auth) || !auth.isAdmin()) {


No need for auth.isAdmin here.

flvndvd · 2025-01-06T08:10:32Z

front/pages/api/w/[wId]/data_sources/request_access.ts

@@ -30,6 +30,16 @@ async function handler(
 ) {
  const user = auth.getNonNullableUser();

+  if (!auth.isUser()) {


Not needed, this is covered by withSessionAuthenticationForWorkspace.

spolu added 12 commits January 3, 2025 16:55

Add canAdministrate to connector config setter

46c7108

remove unused table post/delete

0c40a16

remove unused table endpoints

bba8383

add canAdministrate on managed/update

3121f3d

remove configuration endpoint

e3c6a01

auth audit on connector

925a593

add isUser to request_access

d76a6ba

remove unused search endpoint

6357f2d

canRead for usage endpoint

d91adde

restrict data_sources/dsId POST

26df506

remove legacy managed endpoint

1da165a

nit

b839041

spolu requested a review from flvndvd January 3, 2025 16:49

spolu merged commit 188bc09 into main Jan 3, 2025
3 checks passed

spolu deleted the spolu-audit branch January 3, 2025 16:53

devloai bot reviewed Jan 4, 2025

View reviewed changes

flvndvd reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit / Removal / Auth check of all dataSource related endpoint #9750

Audit / Removal / Auth check of all dataSource related endpoint #9750

spolu commented Jan 3, 2025

spolu commented Jan 3, 2025

spolu commented Jan 4, 2025

devloai bot commented Jan 4, 2025

devloai bot commented Jan 4, 2025

devloai bot left a comment

devloai bot Jan 4, 2025

devloai bot Jan 4, 2025

devloai bot Jan 4, 2025

flvndvd Jan 6, 2025

flvndvd Jan 6, 2025

spolu Jan 6, 2025

flvndvd Jan 6, 2025

spolu Jan 6, 2025

flvndvd Jan 6, 2025

flvndvd Jan 6, 2025

flvndvd Jan 6, 2025

Audit / Removal / Auth check of all dataSource related endpoint #9750

Audit / Removal / Auth check of all dataSource related endpoint #9750

Conversation

spolu commented Jan 3, 2025

Description

Risk

Deploy Plan

spolu commented Jan 3, 2025

spolu commented Jan 4, 2025

devloai bot commented Jan 4, 2025

devloai bot commented Jan 4, 2025

devloai bot left a comment

Choose a reason for hiding this comment

devloai bot Jan 4, 2025

Choose a reason for hiding this comment

devloai bot Jan 4, 2025

Choose a reason for hiding this comment

devloai bot Jan 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment