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

fix: Restrict write access to flows.data column via GraphQL API #2488

Merged
merged 5 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 61 additions & 32 deletions api.planx.uk/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { gql } from "graphql-request";
import { capitalize } from "lodash";
import { Flow, Node } from "./types";
import { ComponentType, FlowGraph } from "@opensystemslab/planx-core/types";
import { $public, getClient } from "./client";
import { $api, $public, getClient } from "./client";

// Get a flow's data (unflattened, without external portal nodes)
const getFlowData = async (id: string): Promise<Flow> => {
Expand All @@ -23,6 +23,11 @@ const getFlowData = async (id: string): Promise<Flow> => {
return flow;
};

interface InsertFlow {
flow: {
id: string;
};
}
// Insert a new flow into the `flows` table
const insertFlow = async (
teamId: number,
Expand All @@ -32,40 +37,64 @@ const insertFlow = async (
copiedFrom?: Flow["id"],
) => {
const { client: $client } = getClient();
const data = await $client.request<{ flow: { id: string } }>(
gql`
mutation InsertFlow(
$team_id: Int!
$slug: String!
$data: jsonb = {}
$creator_id: Int
$copied_from: uuid
) {
flow: insert_flows_one(
object: {
team_id: $team_id
slug: $slug
data: $data
version: 1
creator_id: $creator_id
copied_from: $copied_from
}
try {
const {
flow: { id },
} = await $client.request<InsertFlow>(
gql`
mutation InsertFlow(
$team_id: Int!
$slug: String!
$creator_id: Int
$copied_from: uuid
) {
id
flow: insert_flows_one(
object: {
team_id: $team_id
slug: $slug
version: 1
creator_id: $creator_id
copied_from: $copied_from
}
) {
id
}
}
}
`,
{
team_id: teamId,
slug: slug,
data: flowData,
creator_id: creatorId,
copied_from: copiedFrom,
},
);
`,
{
team_id: teamId,
slug: slug,
creator_id: creatorId,
copied_from: copiedFrom,
},
);

// Populate flow data using API role now that we know user has permission to insert flow
// Access to flow.data column is restricted to limit unsafe content that could be inserted
await $api.client.request<InsertFlow>(
gql`
mutation UpdateFlowData($data: jsonb = {}, $id: uuid!) {
flow: update_flows_by_pk(
pk_columns: { id: $id }
_set: { data: $data }
) {
id
}
}
`,
{
id: id,
data: flowData,
},
);

if (data) await createAssociatedOperation(data?.flow?.id);
return data?.flow;
await createAssociatedOperation(id);
return { id };
} catch (error) {
throw Error(
`User ${creatorId} failed to insert flow to teamId ${teamId}. Please check permissions.`,
);
}
};

// Add a row to `operations` for an inserted flow, otherwise ShareDB throws a silent error when opening the flow in the UI
Expand Down
4 changes: 2 additions & 2 deletions api.planx.uk/modules/flows/copyFlow/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export type CopyFlowController = ValidatedRequestHandler<
>;

export const copyFlowController: CopyFlowController = async (
req,
_req,
res,
next,
) => {
Expand All @@ -48,7 +48,7 @@ export const copyFlowController: CopyFlowController = async (
});
} catch (error) {
return next(
new ServerError({ message: "Failed to copy flow", cause: error }),
new ServerError({ message: `Failed to copy flow. Error: ${error}` }),
);
}
};
57 changes: 57 additions & 0 deletions api.planx.uk/modules/flows/copyFlow/copyFlow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@ beforeEach(() => {
},
});

queryMock.mockQuery({
name: "UpdateFlowData",
matchOnVariables: false,
data: {
flow: {
id: 2,
},
},
});

queryMock.mockQuery({
name: "InsertOperation",
matchOnVariables: false,
Expand Down Expand Up @@ -85,6 +95,53 @@ it("returns an error if required replacement characters are not provided in the
});
});

it("returns an error if the operation to insert a new flow fails", async () => {
queryMock.reset();

const body = {
insert: true,
replaceValue: "T3ST1",
};

queryMock.mockQuery({
name: "GetFlowData",
matchOnVariables: false,
data: {
flow: {
data: mockFlowData,
},
},
});

queryMock.mockQuery({
name: "InsertFlow",
matchOnVariables: false,
data: {
flow: {
id: 2,
},
},
graphqlErrors: [
{
message: "Something went wrong",
},
],
variables: {
id: "3",
},
});

await supertest(app)
.post("/flows/3/copy")
.send(body)
.set(auth)
.expect(500)
.then((res) => {
expect(res.body.error).toMatch(/failed to insert flow/);
expect(res.body.error).toMatch(/Please check permissions/);
});
});

it("returns copied unique flow data without inserting a new record", async () => {
const body = {
insert: false,
Expand Down
11 changes: 10 additions & 1 deletion api.planx.uk/modules/flows/findReplace/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ import { z } from "zod";
import { ServerError } from "../../../errors";
import { findAndReplaceInFlow } from "./service";
import { FlowGraph } from "@opensystemslab/planx-core/types";
import { JSDOM } from "jsdom";
import createDOMPurify from "dompurify";

// Setup JSDOM and DOMPurify
const window = new JSDOM("").window;
const DOMPurify = createDOMPurify(window);

interface FindAndReplaceResponse {
message: string;
Expand All @@ -17,7 +23,10 @@ export const findAndReplaceSchema = z.object({
}),
query: z.object({
find: z.string(),
replace: z.string().optional(),
replace: z
.string()
.optional()
.transform((val) => val && DOMPurify.sanitize(val)),
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching this sanitation step too 👍

}),
});

Expand Down
Loading