Skip to content

Commit aa1813d

Browse files
waleedlatif1waleedlatif1
committed
fix(bigquery): refuse a padded projectId instead of silently resolving it
Guarding these paths introduced a data-loss hazard that the guard itself hid. projectId was interpolated as encodeURIComponent(params.projectId) before this branch — never trimmed — so ' my-project ' became %20%20my-project%20%20, which names no GCP project and failed cleanly: before: /bigquery/v2/projects/%20%20my-project%20%20/datasets/prod_dataset after: /bigquery/v2/projects/my-project/datasets/prod_dataset safeUrlPathSegment trims, so on delete_dataset and delete_table that turns a request which did nothing into one that irreversibly destroys a real dataset or table, from a value the caller never wrote. The rule applied is narrow and testable: this change must not turn a failing request into a succeeding one. Every identifier it newly began trimming now refuses surrounding whitespace — projectId in all eleven tools, plus datasetId and tableId where those were previously untrimmed. Identifiers already trimmed before this branch keep safeUrlPathSegment, since trimming them is not a change made here and refusing them would break callers whose stored value works today. Rejection is not argued from consistency with the other guarded sites; that averages over very different blast radii. It stands on two facts specific to these values: no legitimate BigQuery identifier carries surrounding whitespace, so nothing real is refused, and the previous behaviour was already a clean failure, so refusing preserves it while naming the offending parameter. Pinned by a REJECTS-style set that upgrades the generic per-pair whitespace assertion to demand a throw, plus explicit delete-tool tests. Verified non-vacuous: reverting either guard to a plain trim fails both.
1 parent cfdba91 commit aa1813d

14 files changed

Lines changed: 244 additions & 26 deletions

‎apps/sim/tools/__tests__/path-safety.ts‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -414,12 +414,28 @@ export interface TraversalOptions {
414414
* ids, where `safeUrlPathSegment` trims and rejects.
415415
*/
416416
preservesWhitespace?: boolean
417+
/**
418+
* Parameter names that must **refuse** a padded value rather than trim it.
419+
*
420+
* Trimming is not neutral on a parameter that was not trimmed before: a
421+
* padded id previously named nothing and the request failed, so trimming
422+
* silently resolves it to a real resource. On an irreversible operation that
423+
* turns a no-op into a deletion. Naming those parameters here upgrades the
424+
* whitespace assertion from "same path or no path" to "must throw", so the
425+
* rejection cannot quietly regress into a trim.
426+
*/
427+
rejectsSurroundingWhitespace?: readonly string[]
417428
}
418429

419430
/** Asserts the traversal invariant for one (tool, parameter) pair. */
420431
export function itResistsTraversal(
421432
{ tool, paramName, context }: PathParam,
422-
{ origin, basePath, preservesWhitespace = false }: TraversalOptions
433+
{
434+
origin,
435+
basePath,
436+
preservesWhitespace = false,
437+
rejectsSurroundingWhitespace = [],
438+
}: TraversalOptions
423439
): void {
424440
const baselinePath = buildUrl(tool, paramName, PROBE_ID, context).pathname
425441
const baselineSegments = baselinePath.split('/')
@@ -505,6 +521,22 @@ export function itResistsTraversal(
505521
*/
506522
it('handles surrounding whitespace according to the parameter kind', () => {
507523
const padded = ` ${PROBE_ID} `
524+
525+
if (rejectsSurroundingWhitespace.includes(paramName)) {
526+
let message = ''
527+
try {
528+
buildUrl(tool, paramName, padded, context)
529+
} catch (error) {
530+
message = getErrorMessage(error, 'unknown error')
531+
}
532+
533+
expect(message, `${paramName} accepted a padded value instead of refusing it`).not.toBe('')
534+
expect(namesParam(message, paramName), `error did not name ${paramName}: ${message}`).toBe(
535+
true
536+
)
537+
return
538+
}
539+
508540
let url: URL
509541
try {
510542
url = buildUrl(tool, paramName, padded, context)

‎apps/sim/tools/google_bigquery/create_dataset.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@ import type {
22
GoogleBigQueryCreateDatasetParams,
33
GoogleBigQueryCreateDatasetResponse,
44
} from '@/tools/google_bigquery/types'
5-
import { canonicalBigQueryId } from '@/tools/google_bigquery/utils'
5+
import {
6+
canonicalBigQueryId,
7+
strictBigQueryPathSegment,
8+
strictCanonicalBigQueryId,
9+
} from '@/tools/google_bigquery/utils'
610
import type { ToolConfig } from '@/tools/types'
7-
import { safeUrlPathSegment } from '@/tools/url-path'
811

912
export const googleBigQueryCreateDatasetTool: ToolConfig<
1013
GoogleBigQueryCreateDatasetParams,
@@ -61,7 +64,7 @@ export const googleBigQueryCreateDatasetTool: ToolConfig<
6164

6265
request: {
6366
url: (params) =>
64-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets`,
67+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets`,
6568
method: 'POST',
6669
headers: (params) => ({
6770
Authorization: `Bearer ${params.accessToken}`,
@@ -70,7 +73,7 @@ export const googleBigQueryCreateDatasetTool: ToolConfig<
7073
body: (params) => {
7174
const body: Record<string, unknown> = {
7275
datasetReference: {
73-
projectId: canonicalBigQueryId(params.projectId, 'projectId'),
76+
projectId: strictCanonicalBigQueryId(params.projectId, 'projectId'),
7477
datasetId: canonicalBigQueryId(params.datasetId, 'datasetId'),
7578
},
7679
}

‎apps/sim/tools/google_bigquery/create_table.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@ import type {
22
GoogleBigQueryCreateTableParams,
33
GoogleBigQueryCreateTableResponse,
44
} from '@/tools/google_bigquery/types'
5-
import { canonicalBigQueryId } from '@/tools/google_bigquery/utils'
5+
import {
6+
canonicalBigQueryId,
7+
strictBigQueryPathSegment,
8+
strictCanonicalBigQueryId,
9+
} from '@/tools/google_bigquery/utils'
610
import type { ToolConfig } from '@/tools/types'
711
import { safeUrlPathSegment } from '@/tools/url-path'
812

@@ -68,7 +72,7 @@ export const googleBigQueryCreateTableTool: ToolConfig<
6872

6973
request: {
7074
url: (params) =>
71-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables`,
75+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables`,
7276
method: 'POST',
7377
headers: (params) => ({
7478
Authorization: `Bearer ${params.accessToken}`,
@@ -95,7 +99,7 @@ export const googleBigQueryCreateTableTool: ToolConfig<
9599

96100
const body: Record<string, unknown> = {
97101
tableReference: {
98-
projectId: canonicalBigQueryId(params.projectId, 'projectId'),
102+
projectId: strictCanonicalBigQueryId(params.projectId, 'projectId'),
99103
datasetId: canonicalBigQueryId(params.datasetId, 'datasetId'),
100104
tableId: canonicalBigQueryId(params.tableId, 'tableId'),
101105
},

‎apps/sim/tools/google_bigquery/delete_dataset.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type {
22
GoogleBigQueryDeleteDatasetParams,
33
GoogleBigQueryDeleteDatasetResponse,
44
} from '@/tools/google_bigquery/types'
5+
import { strictBigQueryPathSegment } from '@/tools/google_bigquery/utils'
56
import type { ToolConfig } from '@/tools/types'
67
import { safeUrlPathSegment } from '@/tools/url-path'
78

@@ -49,7 +50,7 @@ export const googleBigQueryDeleteDatasetTool: ToolConfig<
4950
request: {
5051
url: (params) => {
5152
const url = new URL(
52-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}`
53+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}`
5354
)
5455
if (params.deleteContents !== undefined) {
5556
url.searchParams.set('deleteContents', String(params.deleteContents))

‎apps/sim/tools/google_bigquery/delete_table.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type {
22
GoogleBigQueryDeleteTableParams,
33
GoogleBigQueryDeleteTableResponse,
44
} from '@/tools/google_bigquery/types'
5+
import { strictBigQueryPathSegment } from '@/tools/google_bigquery/utils'
56
import type { ToolConfig } from '@/tools/types'
67
import { safeUrlPathSegment } from '@/tools/url-path'
78

@@ -48,7 +49,7 @@ export const googleBigQueryDeleteTableTool: ToolConfig<
4849

4950
request: {
5051
url: (params) =>
51-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables/${safeUrlPathSegment(params.tableId, 'tableId')}`,
52+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables/${safeUrlPathSegment(params.tableId, 'tableId')}`,
5253
method: 'DELETE',
5354
headers: (params) => ({
5455
Authorization: `Bearer ${params.accessToken}`,

‎apps/sim/tools/google_bigquery/get_query_results.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type {
22
GoogleBigQueryGetQueryResultsParams,
33
GoogleBigQueryGetQueryResultsResponse,
44
} from '@/tools/google_bigquery/types'
5+
import { strictBigQueryPathSegment } from '@/tools/google_bigquery/utils'
56
import type { ToolConfig } from '@/tools/types'
67
import { safeUrlPathSegment } from '@/tools/url-path'
78

@@ -74,7 +75,7 @@ export const googleBigQueryGetQueryResultsTool: ToolConfig<
7475
request: {
7576
url: (params) => {
7677
const url = new URL(
77-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/queries/${safeUrlPathSegment(params.jobId, 'jobId')}`
78+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/queries/${safeUrlPathSegment(params.jobId, 'jobId')}`
7879
)
7980
if (params.pageToken) url.searchParams.set('pageToken', params.pageToken)
8081
if (params.maxResults !== undefined && params.maxResults !== null) {

‎apps/sim/tools/google_bigquery/get_table.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ import type {
22
GoogleBigQueryGetTableParams,
33
GoogleBigQueryGetTableResponse,
44
} from '@/tools/google_bigquery/types'
5+
import { strictBigQueryPathSegment } from '@/tools/google_bigquery/utils'
56
import type { ToolConfig } from '@/tools/types'
6-
import { safeUrlPathSegment } from '@/tools/url-path'
77

88
export const googleBigQueryGetTableTool: ToolConfig<
99
GoogleBigQueryGetTableParams,
@@ -48,7 +48,7 @@ export const googleBigQueryGetTableTool: ToolConfig<
4848

4949
request: {
5050
url: (params) =>
51-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables/${safeUrlPathSegment(params.tableId, 'tableId')}`,
51+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets/${strictBigQueryPathSegment(params.datasetId, 'datasetId')}/tables/${strictBigQueryPathSegment(params.tableId, 'tableId')}`,
5252
method: 'GET',
5353
headers: (params) => ({
5454
Authorization: `Bearer ${params.accessToken}`,

‎apps/sim/tools/google_bigquery/insert_rows.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ import type {
22
GoogleBigQueryInsertRowsParams,
33
GoogleBigQueryInsertRowsResponse,
44
} from '@/tools/google_bigquery/types'
5+
import { strictBigQueryPathSegment } from '@/tools/google_bigquery/utils'
56
import type { ToolConfig } from '@/tools/types'
6-
import { safeUrlPathSegment } from '@/tools/url-path'
77

88
export const googleBigQueryInsertRowsTool: ToolConfig<
99
GoogleBigQueryInsertRowsParams,
@@ -66,7 +66,7 @@ export const googleBigQueryInsertRowsTool: ToolConfig<
6666

6767
request: {
6868
url: (params) =>
69-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables/${safeUrlPathSegment(params.tableId, 'tableId')}/insertAll`,
69+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets/${strictBigQueryPathSegment(params.datasetId, 'datasetId')}/tables/${strictBigQueryPathSegment(params.tableId, 'tableId')}/insertAll`,
7070
method: 'POST',
7171
headers: (params) => ({
7272
Authorization: `Bearer ${params.accessToken}`,

‎apps/sim/tools/google_bigquery/list_datasets.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ import type {
22
GoogleBigQueryListDatasetsParams,
33
GoogleBigQueryListDatasetsResponse,
44
} from '@/tools/google_bigquery/types'
5+
import { strictBigQueryPathSegment } from '@/tools/google_bigquery/utils'
56
import type { ToolConfig } from '@/tools/types'
6-
import { safeUrlPathSegment } from '@/tools/url-path'
77

88
export const googleBigQueryListDatasetsTool: ToolConfig<
99
GoogleBigQueryListDatasetsParams,
@@ -49,7 +49,7 @@ export const googleBigQueryListDatasetsTool: ToolConfig<
4949
request: {
5050
url: (params) => {
5151
const url = new URL(
52-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets`
52+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets`
5353
)
5454
if (params.maxResults !== undefined && params.maxResults !== null) {
5555
const maxResults = Number(params.maxResults)

‎apps/sim/tools/google_bigquery/list_table_data.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type {
22
GoogleBigQueryListTableDataParams,
33
GoogleBigQueryListTableDataResponse,
44
} from '@/tools/google_bigquery/types'
5+
import { strictBigQueryPathSegment } from '@/tools/google_bigquery/utils'
56
import type { ToolConfig } from '@/tools/types'
67
import { safeUrlPathSegment } from '@/tools/url-path'
78

@@ -74,7 +75,7 @@ export const googleBigQueryListTableDataTool: ToolConfig<
7475
request: {
7576
url: (params) => {
7677
const url = new URL(
77-
`https://bigquery.googleapis.com/bigquery/v2/projects/${safeUrlPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables/${safeUrlPathSegment(params.tableId, 'tableId')}/data`
78+
`https://bigquery.googleapis.com/bigquery/v2/projects/${strictBigQueryPathSegment(params.projectId, 'projectId')}/datasets/${safeUrlPathSegment(params.datasetId, 'datasetId')}/tables/${safeUrlPathSegment(params.tableId, 'tableId')}/data`
7879
)
7980
if (params.maxResults !== undefined && params.maxResults !== null) {
8081
const maxResults = Number(params.maxResults)

0 commit comments

Comments
 (0)