Contribution categorization by hdiniz · Pull Request #11456 · opencollective/opencollective-api

hdiniz · 2026-02-24T22:33:52Z

No description provided.

Betree · 2026-02-27T14:15:36Z

+        await ContributionAccountingCategoryRule.bulkCreate(rules, {
+          transaction,
+          updateOnDuplicate: ['order', 'enabled', 'name', 'predicates', 'AccountingCategoryId', 'updatedAt'],
+        });


🚫 This code would let users edit rules they are not allowed to delete. When building the rules list above, we simply idDecode the ID, without loading the entry to make sure it belongs to the account.

We use richDiffDBEntries in other places to prevent this issue.

Good catch, I've added a check and accompanying test case to validate this

Betree · 2026-02-27T14:24:50Z

+    await applyContributionAccountingCategoryRules(order);
+


Not a requested change, but out of curiosity - have you considered adding that as a afterCreate hook on Orders, to have it in a single place and make sure we don't forget any path?

There are only a couple of cases where we want to run this at the moment, I had a preference for doing it explicitly in such places to also take into consideration the roles and the semantics of the mutations where the host can perform remove/ignore a category on updating an order

Betree · 2026-02-27T14:32:35Z

Given that there is an order.update above, another order.update in applyContributionAccountingCategoryRules, both without transaction and without reloading the order, it sounds like there is a risk of concurrency issue.

I see what you mean, this is possible, I guess the particularities of the updates that happen mitigate this as there are other awaits inside the side effects.

I went ahead and refactored this section to execute the effects sequentially, I believe there should not be a need to reload the order explicitly as sequelize will update the internal order representation after the update.

…zation

Sign in to view

+          ...order.data,
+          valuesByRole: {
+            ...(order.data.valuesByRole || {}),


sentry · 2026-03-06T11:32:08Z

+              idDecode(rule.id, IDENTIFIER_TYPES.ACCOUNTING_CATEGORY_RULE),
+            );
+
+            if (existingRule && existingRule.type !== 'CONTRIBUTION') {
+              throw new ValidationFailed('This mutation can only update contribution accounting category rules');
+            }
+
+            if (existingRule && existingRule.CollectiveId !== account.id) {
+              throw new Forbidden('You are not authorized to update this rule');
+            }
+          }


Bug: The updateContributionAccountingCategoryRules mutation allows creating a new rule with a user-specified primary key if the provided id does not already exist.
_{Severity: MEDIUM}

Suggested Fix

Add validation to check if a rule exists when an id is provided. If the rule does not exist, throw an error to reject the request, similar to the validation logic in the editAccountingCategories mutation.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: server/graphql/v2/mutation/AccountingCategoriesMutations.ts#L176-L188 Potential issue: In the `updateContributionAccountingCategoryRules` mutation, if a rule `id` is provided that does not exist in the database, validation is skipped. The code then calls `bulkCreate` with the `updateOnDuplicate` option, which causes Sequelize to insert a new rule using the user-specified, non-existent ID as its primary key. This bypasses the intended upsert logic and allows users to create records with arbitrary primary keys, which could potentially interfere with the database's auto-increment sequence.

Betree

All blockers resolved on my end!

…zation

Sign in to view

+      const valuesByRole = {
+        ...(order.data.valuesByRole || {}),
+        ...(accountingCategory === null ? { hostAdmin: { accountingCategory: { code: UncategorizedValue } } } : {}),
+        ...(accountingCategory?.id ? { hostAdmin: { accountingCategory: accountingCategory?.publicInfo } } : {}),
+      };


Sign in to view

+        await AccountingCategoryRule.bulkCreate(rules, {
+          transaction,
+          updateOnDuplicate: ['order', 'enabled', 'name', 'predicates', 'AccountingCategoryId', 'updatedAt'],
+        });


Sign in to view

+      resolve(rule) {
+        return rule.id;
+      },


Sign in to view

+          lock: Transaction.LOCK.UPDATE,
+        });
+
+        const toDelete = existingRules.filter(rule => !rules.some(r => r.publicId === rule.publicId));


sentry · 2026-04-10T17:33:55Z

+    value: unknown,
+    order: Order,
+  ) => {
+    return typeof value === 'string' && value.length > 0 && order.description.includes(value as string);


Bug: The code calls order.description.includes() without checking if order.description is undefined, which will cause a TypeError.
_{Severity: MEDIUM}

Suggested Fix

Add an optional chaining operator to safely handle cases where order.description is undefined. Change order.description.includes(value as string) to order.description?.includes(value as string).

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: server/lib/accounting/categorization/types.ts#L169 Potential issue: The `Order.description` field is optional and can be `undefined`. However, when evaluating accounting category rules, the code at `server/lib/accounting/categorization/types.ts:169` attempts to call `order.description.includes(value)` without a null check. This will throw a `TypeError` if an order without a description is processed. While a `try/catch` block prevents a crash, the rule application will fail silently, preventing automatic categorization and creating Sentry error noise.

sentry · 2026-04-10T17:33:56Z

+      return;
+    }
+
+    const rules = await AccountingCategoryRule.getRulesForCollective(collective.HostCollectiveId, 'CONTRIBUTION');


Bug: The code incorrectly uses collective.HostCollectiveId to fetch rules, which can be null even when a host object exists, causing silent failure.
_{Severity: MEDIUM}

Suggested Fix

Use host.id instead of collective.HostCollectiveId when calling AccountingCategoryRule.getRulesForCollective. This ensures the correct host ID is used, as the host object is guaranteed to be available at that point.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: server/lib/accounting/categorization/contribution-rules.ts#L68 Potential issue: In `applyContributionAccountingCategoryRules`, the code fetches the host for a collective but then uses `collective.HostCollectiveId` to retrieve the accounting rules. The `getHostCollective()` method does not always populate `collective.HostCollectiveId`, even when it successfully returns a `host` object. This causes the subsequent rule lookup to query with a `null` ID, finding no rules and causing the categorization to fail silently.

hdiniz added 6 commits February 24, 2026 18:55

Add contribution accounting category table

63d69fe

Add feature

b64252b

Add graphql inputs

c0fcb86

Add graphql queries

5a6b015

Add test

99b0a4a

Update graphql

1936929

hdiniz self-assigned this Feb 24, 2026

hdiniz added 2 commits February 24, 2026 19:52

Remove wrong typping

8d990e7

fix allowed features test

c55922a

hdiniz mentioned this pull request Feb 25, 2026

Contribution categorization opencollective/opencollective-frontend#11929

Merged

hdiniz marked this pull request as ready for review February 25, 2026 17:18

hdiniz requested a review from Betree February 25, 2026 17:18

Betree requested changes Feb 27, 2026

View reviewed changes

hdiniz added 7 commits March 6, 2026 06:15

Merge remote-tracking branch 'origin/main' into contribution-categori…

490ea7f

…zation

Fix vunerability, add test

263a7e9

Generalize rules

9e76204

Use zod schema to validate contribution predicate

108561b

Validate predicates on model

4e48712

Update graphql

140c730

Fix index type

d8c4bc5

sentry bot reviewed Mar 6, 2026

View reviewed changes

Comment thread server/lib/accounting/categorization/types.ts Outdated

Execute promises sequencially

4aca3cc

sentry bot reviewed Mar 6, 2026

View reviewed changes

Comment thread server/lib/accounting/categorization/types.ts

FIx payment processor matcher

e93a82f

sentry bot reviewed Mar 6, 2026

View reviewed changes

Comment thread server/lib/accounting/categorization/types.ts

Comment thread server/graphql/v2/mutation/OrderMutations.js Outdated

Add null check

c4e28a1

sentry bot reviewed Mar 6, 2026

View reviewed changes

Betree approved these changes Mar 27, 2026

View reviewed changes

hdiniz added 2 commits April 10, 2026 12:36

Merge remote-tracking branch 'origin/main' into contribution-categori…

5e43229

…zation

rebase on publicId

ba84bb0

sentry bot reviewed Apr 10, 2026

View reviewed changes

Add permalink handler

6d420b5

sentry bot reviewed Apr 10, 2026

View reviewed changes

Comment thread server/graphql/v2/object/AccountingCategory.ts

Comment on lines +83 to +85

resolve(rule) {

return rule.id;

},

This comment was marked as outdated.

Sign in to view

Fix lint issue

b0e5ceb

sentry bot reviewed Apr 10, 2026

View reviewed changes

Comment thread server/graphql/v2/mutation/AccountingCategoriesMutations.ts

lock: Transaction.LOCK.UPDATE,

});

const toDelete = existingRules.filter(rule => !rules.some(r => r.publicId === rule.publicId));

This comment was marked as outdated.

Sign in to view

Include rule id in bulk create

135fdbc

sentry bot reviewed Apr 10, 2026

View reviewed changes

hdiniz merged commit 0e2d7d3 into main Apr 10, 2026
19 checks passed

hdiniz deleted the contribution-categorization branch April 10, 2026 21:17

Uh oh!

Conversation

hdiniz commented Feb 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Betree Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

hdiniz Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Betree Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

hdiniz Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Betree Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

hdiniz Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

sentry bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Betree left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

sentry bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

sentry bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants