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

chore: Add spam checking for workflow bodies #18822

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

joeauyeung
Copy link
Contributor

What does this PR do?

Adds a background task to scan workflow bodies for spam and auto locking the user is detected

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Create an Akismet API key and add it to AKISMET_API_KEY .env variable
  • Create a workflow with a normal body
    • User shouldn't lock
  • Create a workflow with a spam body
    • The user should auto lock

@github-actions github-actions bot added the ❗️ .env changes contains changes to env variables label Jan 23, 2025
@keithwillcode keithwillcode added consumer core area: core, team members only labels Jan 23, 2025
Copy link

vercel bot commented Jan 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Jan 24, 2025 10:32pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Jan 24, 2025 10:32pm

import prisma from "@calcom/prisma";

export const scanWorkflowBodySchema = z.object({
userId: z.number(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need the userId to know which user to lock. This comes from the context of the workflow update tRPC endpoint.

const client = new AkismetClient({ key: process.env.AKISMET_API_KEY, blog: WEBAPP_URL });

const comment: Comment = {
user_ip: "127.0.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment needs an IP. From Akismet support we can just use this.

IP is used for scanning against know abuse IPs and adding a reputation score.

packages/features/tasker/tasks/scanWorkflowBody.ts Outdated Show resolved Hide resolved
@joeauyeung joeauyeung marked this pull request as ready for review January 23, 2025 05:09
@graphite-app graphite-app bot requested review from a team January 23, 2025 05:09
@dosubot dosubot bot added the workflows area: workflows, automations label Jan 23, 2025
Copy link

graphite-app bot commented Jan 23, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (01/23/25)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (01/23/25)

1 reviewer was added to this PR based on Keith Williams's automation.

@joeauyeung joeauyeung marked this pull request as draft January 23, 2025 14:16
@github-actions github-actions bot added the ❗️ migrations contains migration files label Jan 23, 2025
const isSpam = await client.checkSpam(comment);

if (isSpam) {
// We won't delete the workflow step incase it is flagged as a false positive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call on not deleting the step in the case of a false positive @Udit-takkar

-- AlterTable
ALTER TABLE "WorkflowStep" ADD COLUMN "safe" BOOLEAN NOT NULL DEFAULT false;

-- Update existing records to set safe to true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emrysal does this look correct?

@@ -22,6 +22,7 @@ export const workflowSelect = {
sender: true,
includeCalendarEvent: true,
numberRequired: true,
safe: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the safe property when fetching workflow steps

@@ -54,6 +54,8 @@ const processWorkflowStep = async (
seatReferenceUid,
}: ProcessWorkflowStepParams
) => {
if (!step.safe) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before processing the step, see if it is marked as safe

@joeauyeung joeauyeung marked this pull request as ready for review January 23, 2025 18:23

export const scanWorkflowBodySchema = z.object({
userId: z.number(),
workflowStepIds: z.array(z.number()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since scheduleWorkflowNotifications expects multiple steps we now accept multiple steps in the task

@@ -269,29 +270,29 @@ export const updateHandler = async ({ ctx, input }: UpdateOptions) => {
isOrg,
});

await scheduleWorkflowNotifications(
await scheduleWorkflowNotifications({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

scheduleWorkflowNotifications is refactored to accept an object for better readability.

@@ -337,7 +338,7 @@ export const updateHandler = async ({ ctx, input }: UpdateOptions) => {
id: oldStep.id,
},
});
} else if (isStepEdited(oldStep, newStep)) {
} else if (isStepEdited(oldStep, { ...newStep, safe: false })) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type fix, expects the safe property

Copy link
Member

Choose a reason for hiding this comment

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

Should this be safe: oldStep.safe? 🤔 Because otherwise if the oldStep is safe, isStepEdited would always be true because of the different value in the safe field

@@ -398,20 +399,24 @@ export const updateHandler = async ({ ctx, input }: UpdateOptions) => {
},
});

if (SCANNING_WORKFLOW_STEPS && newStep.reminderBody !== oldStep.reminderBody) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only send to scan the body if it's been updated

data: {
...step,
numberVerificationPending: false,
...(!SCANNING_WORKFLOW_STEPS ? { safe: true } : {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default to safe = true if a self-hoster isn't planning on scanning workflows

Copy link
Contributor

github-actions bot commented Jan 24, 2025

E2E results are ready!

@@ -337,7 +338,7 @@ export const updateHandler = async ({ ctx, input }: UpdateOptions) => {
id: oldStep.id,
},
});
} else if (isStepEdited(oldStep, newStep)) {
} else if (isStepEdited(oldStep, { ...newStep, safe: false })) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be safe: oldStep.safe? 🤔 Because otherwise if the oldStep is safe, isStepEdited would always be true because of the different value in the safe field

workflowSteps,
time: workflow?.time || null,
timeUnit: workflow?.timeUnit || null,
trigger: WorkflowTriggerEvents.AFTER_EVENT,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
trigger: WorkflowTriggerEvents.AFTER_EVENT,
trigger: workflow.trigger,

Should be like that I think

@@ -395,23 +398,28 @@ export const updateHandler = async ({ ctx, input }: UpdateOptions) => {
sender: newStep.sender,
numberVerificationPending: false,
includeCalendarEvent: newStep.includeCalendarEvent,
...(SCANNING_WORKFLOW_STEPS && didBodyChange ? { safe: true } : {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CarinaWolli thoughts here? Only if the body has changed we should remark the step as unsafe.

Copy link
Member

Choose a reason for hiding this comment

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

looks good

Copy link
Member

Choose a reason for hiding this comment

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

oh wait, why true here and not false?

Copy link

socket-security bot commented Jan 24, 2025

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@formkit/[email protected] None 0 35.4 kB justin-schroeder
npm/[email protected] None 0 221 kB dcode

🚮 Removed packages: npm/[email protected], npm/[email protected]

View full report↗︎

Copy link

socket-security bot commented Jan 24, 2025

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteCI
Possible typosquat attack npm/[email protected] ⚠︎

View full report↗︎

Next steps

What is a typosquat?

Package name is similar to other popular packages and may not be the package you want.

Use care when consuming similarly named packages and ensure that you did not intend to consume a different package. Malicious packages often publish using similar names as existing popular packages.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@zomars
Copy link
Member

zomars commented Jan 24, 2025

reviewing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consumer core area: core, team members only ❗️ .env changes contains changes to env variables ❗️ migrations contains migration files ready-for-e2e workflows area: workflows, automations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants