Make address option for kyc verification, add check to payee

[hdiniz]

Related opencollective/opencollective#8341

Resolves opencollective/opencollective#8369

[kewitz]

Please double-check relying on expense.HostCollectiveId for security checks on pending expenses.

[kewitz]

As a suggestion, Object.assign accepts false arguments:

data: Object.assign({ legalName: providerRequest.legalName }, providerRequest.legalAddress && { legalAddress: providerRequest.legalAddress })

In this particular situation pick(providerRequest, ['legalName', 'legalAddress']) would also have the same effect.

[kewitz]

I'm not sure how the code stands now, but please review exactly in which conditions expense.HostCollectiveId is populated. It used to be that it was populated only after the expense was paid, to cover for cases where the collective would change hosts with existing pending expenses.

[sentry]

Bug: hostCollectiveId can be undefined if an expense has no host. This causes the KYC verification loader to receive undefined and silently fail, bypassing the check.
_{Severity: CRITICAL | Confidence: High}

🔍 Detailed Analysis

The hostCollectiveId variable can become undefined if both expense.HostCollectiveId and expense.collective?.HostCollectiveId are null. This undefined value is then passed to req.loaders.KYCVerification.verifiedStatusByProvider. The underlying SQL query will use NULL for the RequestedByCollectiveId, which will not match any records because NULL = NULL is not true in SQL. As a result, the kycVerifications array will contain only null values, and the subsequent loop will silently skip all KYC checks, potentially allowing payments to unverified individuals.

💡 Suggested Fix

Add a check to ensure hostCollectiveId is a valid number before calling req.loaders.KYCVerification.verifiedStatusByProvider. If hostCollectiveId is not available, either throw an error or handle the case explicitly to prevent the silent failure of the KYC check.

🤖 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/security/expense.ts#L506-L514

Potential issue: The `hostCollectiveId` variable can become `undefined` if both
`expense.HostCollectiveId` and `expense.collective?.HostCollectiveId` are null. This
`undefined` value is then passed to
`req.loaders.KYCVerification.verifiedStatusByProvider`. The underlying SQL query will
use `NULL` for the `RequestedByCollectiveId`, which will not match any records because
`NULL = NULL` is not true in SQL. As a result, the `kycVerifications` array will contain
only `null` values, and the subsequent loop will silently skip all KYC checks,
potentially allowing payments to unverified individuals.

_{Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8181271}