Skip to content

Commit

Permalink
Fix system udf missing arg validators bug (#23749)
Browse files Browse the repository at this point in the history
The first change is to move `convex/server` to `convex/_system/server` so it gets covered by [this](https://github.com/get-convex/convex/blob/1486eb4f6429ed120a57c0ffd742e0cccd0fb202/crates/isolate/build.rs#L106), alternatively we could broaden that.

The second change is adding arg validators to a couple `mutationGeneric`s that were legitimately missing them. Confusingly, we were getting this error at import time so it looked like they were coming from other functions in the same file.

GitOrigin-RevId: 24bce4fbc0a8ed6207b310fed7ffb99830809ca0
  • Loading branch information
sshader authored and Convex, Inc. committed Mar 21, 2024
1 parent afe4b98 commit 0580822
Show file tree
Hide file tree
Showing 13 changed files with 154 additions and 32 deletions.
34 changes: 34 additions & 0 deletions npm-packages/system-udfs/convex/.eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,40 @@ module.exports = {
{ varsIgnorePattern: "_.*" },
],
"@typescript-eslint/no-floating-promises": "error",
// system UDF argument validation
"no-restricted-imports": [
"error",
{
patterns: [
{
group: ["**/_generated/server"],
importNames: [
"query",
"mutation",
"action",
"internalQuery",
"internalMutation",
"internalAction",
],
message:
"Use the query wrappers from convex/server.ts instead for system UDF argument validation.",
},
{
group: ["convex/server"],
importNames: [
"queryGeneric",
"mutationGeneric",
"actionGeneric",
"internalQueryGeneric",
"internalMutationGeneric",
"internalActionGeneric",
],
message:
"Use the query wrappers from convex/server.ts instead for system UDF argument validation.",
},
],
},
],
},
parserOptions: {
project: "./tsconfig.json",
Expand Down
2 changes: 2 additions & 0 deletions npm-packages/system-udfs/convex/_generated/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import type * as _system_frontend_tableSize from "../_system/frontend/tableSize.
import type * as _system_paginationLimits from "../_system/paginationLimits.js";
import type * as _system_repl_wrappers from "../_system/repl/wrappers.js";
import type * as _system_secretSystemTables from "../_system/secretSystemTables.js";
import type * as _system_server from "../_system/server.js";
import type * as deploymentAuditLogTable from "../deploymentAuditLogTable.js";
import type * as syscall from "../syscall.js";

Expand Down Expand Up @@ -100,6 +101,7 @@ declare const fullApi: ApiFromModules<{
"_system/paginationLimits": typeof _system_paginationLimits;
"_system/repl/wrappers": typeof _system_repl_wrappers;
"_system/secretSystemTables": typeof _system_secretSystemTables;
"_system/server": typeof _system_server;
deploymentAuditLogTable: typeof deploymentAuditLogTable;
syscall: typeof syscall;
}>;
Expand Down
2 changes: 1 addition & 1 deletion npm-packages/system-udfs/convex/_system/cli/queryTable.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { TableNames } from "../../_generated/dataModel";
import { queryGeneric } from "convex/server";
import { queryGeneric } from "../server";
import { v } from "convex/values";

// This query returns a new result every time
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { GenericDocument, mutationGeneric } from "convex/server";
import { GenericDocument } from "convex/server";
import { mutationGeneric } from "../server";

import { ConvexError, v } from "convex/values";

const MAX_IMPORT_COUNT = 4096; // TRANSACTION_MAX_NUM_USER_WRITES / 2
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Cursor, mutationGeneric } from "convex/server";
import { Cursor } from "convex/server";
import { mutationGeneric } from "../server";
import { v } from "convex/values";

export const MAX_CLEAR_ROWS = 4000;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { v } from "convex/values";
import { mutationGeneric } from "convex/server";
import { mutationGeneric } from "../server";

export default mutationGeneric({
args: { table: v.string() },
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { GenericId, v } from "convex/values";
import { mutationGeneric } from "convex/server";
import { mutationGeneric } from "../server";

const MAX_DOCUMENT_DELETIONS = 4096;

Expand Down
23 changes: 12 additions & 11 deletions npm-packages/system-udfs/convex/_system/frontend/fileStorageV2.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import {
PaginationResult,
SystemDataModel,
mutationGeneric,
} from "convex/server";
import { PaginationResult, SystemDataModel } from "convex/server";
import { mutationGeneric } from "../server";
import { Id } from "../../_generated/dataModel";
import { v } from "convex/values";
import { paginationOptsValidator } from "convex/server";
Expand Down Expand Up @@ -47,14 +44,17 @@ export const fileMetadata = queryGeneric({
},
});

export const deleteFile = mutationGeneric(
async (
export const deleteFile = mutationGeneric({
args: {
storageId: v.id("_storage"),
},
handler: async (
{ storage },
{ storageId }: { storageId: Id<"_storage"> },
): Promise<void> => {
return await storage.delete(storageId);
},
);
});

export const deleteFiles = mutationGeneric({
args: {
Expand All @@ -67,8 +67,9 @@ export const deleteFiles = mutationGeneric({
},
});

export const generateUploadUrl = mutationGeneric(
async ({ storage }): Promise<string> => {
export const generateUploadUrl = mutationGeneric({
args: {},
handler: async ({ storage }): Promise<string> => {
return await storage.generateUploadUrl();
},
);
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Value, v, GenericId, ConvexError } from "convex/values";
import { mutationGeneric } from "convex/server";
import { mutationGeneric } from "../server";

// Since clients cannot send `undefined` through a serialized Convex Value to a server function,
// we use a placeholder for it.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ConvexError, GenericId } from "convex/values";
import { v } from "convex/values";
import { mutationGeneric } from "convex/server";
import { mutationGeneric } from "../server";

export default mutationGeneric({
args: { id: v.string(), document: v.any() },
Expand Down
2 changes: 2 additions & 0 deletions npm-packages/system-udfs/convex/_system/repl/wrappers.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// Validation is not required for REPL functions
// eslint-disable-next-line no-restricted-imports
import { query, internalQuery } from "../../_generated/server";

export { query, internalQuery };
16 changes: 2 additions & 14 deletions npm-packages/system-udfs/convex/_system/secretSystemTables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@ import {
SystemDataModel,
DefaultFunctionArgs,
TableNamesInDataModel,
queryGeneric as baseQueryGeneric,
} from "convex/server";
import { Validator } from "convex/values";
import { query as baseQuery } from "../_generated/server";
import { query as baseQuery, queryGeneric as baseQueryGeneric } from "./server";
import { Id } from "../_generated/dataModel";
import { performOp } from "../syscall";

// This set must be kept up-to-date to prevent accidental access to secret
// system tables in system UDFs.
Expand Down Expand Up @@ -96,7 +94,6 @@ function maskPublicSystem<T extends GenericDataModel>(
type FunctionDefinition = {
args: Record<string, Validator<any, boolean>>;
handler: (ctx: any, args: DefaultFunctionArgs) => any;
exportArgs(): string;
};

/// `queryPrivateSystem` is for querying private system tables.
Expand All @@ -108,18 +105,9 @@ export const queryPrivateSystem = ((functionDefinition: FunctionDefinition) => {
if (!("args" in functionDefinition)) {
throw new Error("args validator required for system udf");
}
const query = baseQuery({
args: functionDefinition.args,
handler: () => {},
});
const argsValidatorJson = query.exportArgs();
return baseQuery({
args: functionDefinition.args,
handler: async (ctx: any, args: any) => {
const result = await performOp("validateArgs", argsValidatorJson, args);
if (!result.valid) {
throw new Error(result.message);
}
handler: (ctx: any, args: any) => {
return functionDefinition.handler(
{ ...ctx, db: maskPrivateSystem(ctx.db) },
args,
Expand Down
92 changes: 92 additions & 0 deletions npm-packages/system-udfs/convex/_system/server.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Argument-validated versions of wrappers for use in system UDFs necessary
// because system UDFs are not analyzed.

import { Validator } from "convex/values";
// This is where the alternatives are defined
import {
// eslint-disable-next-line no-restricted-imports
query as baseQuery,
// eslint-disable-next-line no-restricted-imports
mutation as baseMutation,
// eslint-disable-next-line no-restricted-imports
action as baseAction,
// eslint-disable-next-line no-restricted-imports
internalQuery as baseInternalQuery,
// eslint-disable-next-line no-restricted-imports
internalMutation as baseInternalMutation,
// eslint-disable-next-line no-restricted-imports
internalAction as baseInternalAction,
} from "../_generated/server";
import {
// eslint-disable-next-line no-restricted-imports
queryGeneric as baseQueryGeneric,
// eslint-disable-next-line no-restricted-imports
mutationGeneric as baseMutationGeneric,
// eslint-disable-next-line no-restricted-imports
actionGeneric as baseActionGeneric,
// eslint-disable-next-line no-restricted-imports
internalQueryGeneric as baseInternalQueryGeneric,
// eslint-disable-next-line no-restricted-imports
internalMutationGeneric as baseInternalMutationGeneric,
// eslint-disable-next-line no-restricted-imports
internalActionGeneric as baseInternalActionGeneric,
} from "convex/server";

import { DefaultFunctionArgs } from "convex/server";
import { performOp } from "../syscall";

type FunctionDefinition = {
args: Record<string, Validator<any, boolean>>;
handler: (ctx: any, args: DefaultFunctionArgs) => any;
};

type WrappedFunctionDefinition = {
args: Record<string, Validator<any, boolean>>;
handler: (ctx: any, args: DefaultFunctionArgs) => any;
exportArgs(): string;
};

type Wrapper = (def: FunctionDefinition) => WrappedFunctionDefinition;

function withArgsValidated<T>(wrapper: T): T {
return ((functionDefinition: FunctionDefinition) => {
if (!("args" in functionDefinition)) {
throw new Error("args validator required for system udf");
}
const wrap: Wrapper = wrapper as Wrapper;
const func = wrap({
args: functionDefinition.args,
handler: () => {},
});
const argsValidatorJson = func.exportArgs();
return wrap({
args: functionDefinition.args,
handler: async (ctx: any, args: any) => {
const result = await performOp("validateArgs", argsValidatorJson, args);
if (!result.valid) {
throw new Error(result.message);
}
return functionDefinition.handler(ctx, args);
},
});
}) as T;
}

export const queryGeneric = withArgsValidated(baseQueryGeneric);
export const mutationGeneric = withArgsValidated(baseMutationGeneric);
export const actionGeneric = withArgsValidated(baseActionGeneric);
export const internalQueryGeneric = withArgsValidated(baseInternalQueryGeneric);
export const internalMutationGeneric = withArgsValidated(
baseInternalMutationGeneric,
);
export const internalActionGeneric = withArgsValidated(
baseInternalActionGeneric,
);

// Specific to this schema.
export const query = withArgsValidated(baseQuery);
export const mutation = withArgsValidated(baseMutation);
export const action = withArgsValidated(baseAction);
export const internalQuery = withArgsValidated(baseInternalQuery);
export const internalMutation = withArgsValidated(baseInternalMutation);
export const internalAction = withArgsValidated(baseInternalAction);

0 comments on commit 0580822

Please sign in to comment.