---
name: appexchange-review-patterns
description: Salesforce AppExchange Security Review patterns for 2GP managed packages — what manual reviewers accept/reject, anti-patterns to avoid, fix recipes. Derived from a real FlexibleTeamShare rejection (case study in body). TRIGGER when editing Apex (.cls/.trigger) or LWC (.js/.js-meta.xml/.html) in force-app/**, or when the user mentions "security review", "AppExchange", "FLS", "sharing violation", "CRUD", or reads appexchange-artifacts/**. Use to proactively steer code toward review-passing patterns and to flag the recurring anti-patterns that get packages rejected.
---

# AppExchange Security Review Patterns — FlexibleTeamShare

Reference knowledge for writing and reviewing Apex/LWC in this repo so that code does not repeat the patterns that caused prior Salesforce manual-review rejections.

## Hard-won lessons (2026 submissions)

Two patterns were explicitly rejected by Salesforce manual reviewers across Checkmarx (1attempt) and the Product Security team's manual review (2attempt). **Both had been dismissed as false positives in our own documentation.** The FP doc's claims were wrong — do not repeat.

### Lesson 1: CRUD alone is not enough. FLS must be enforced per field.

Reviewers will not accept `Schema.sObjectType.X.isCreateable()` / `isUpdateable()` / `isDeletable()` object-level guards as FLS enforcement on DML. They explicitly asked for one of:

- `insert x as user;` / `update x as user;` / `delete x as user;` — Apex `USER_MODE` (requires at-least Spring '23 API and compatible context)
- `Database.insert(records, AccessLevel.USER_MODE)` — equivalent for bulk DML
- `Security.stripInaccessible(AccessType.CREATABLE, records)` followed by plain DML
- Explicit per-field loop via `Schema.sObjectType.X.fields.getMap()` checking each `.getDescribe().isCreateable()/isUpdateable()` on every field being written

**This rule applies inside `without sharing` classes too.** Reviewers no longer accept the legacy argument "`without sharing` + CRUD check is fine because sharing is intentionally bypassed." Use `as user` (which enforces FLS+CRUD without re-enabling sharing) or the per-field loop. The 2attempt report Findings 1–5 were all about exactly this.

### Lesson 2: A comment is not an authorization gate.

The rejected code had inner `without sharing` classes (`TeamMemberSelector`, `ElevatedDmlOperations`) with comments like:

> "users can only see team members for records they already have access to (recordId comes from the UI context)"

This is a **claim about callers**, not a control. `@AuraEnabled` methods are callable directly from any authenticated user with Apex execute permission and the argument (`recordId`, `memberId`, …) is fully controlled by the client. The reviewer demonstrated a sharing bypass by calling the endpoint with IDs the calling user did not have access to.

**Server-side authorization must exist in code, not in comments.** Acceptable patterns are listed in the Authorization Gate section below. See 2attempt Sharing Violation Findings 1–2.

### Lesson 3: False-positive doc claims must be verifiable.

`SECURITY-SCAN-FALSE-POSITIVES.md` contained the claim "All read queries use `WITH USER_MODE`" — which was materially false for `TeamMemberSelector` and `ElevatedDmlOperations`. The reviewer verified by grep and rejected the justifications.

When writing a false-positive entry, the claim must be grep-provable. "Uses `WITH USER_MODE`" is grep-provable. "Users only reach this via UI" is not.

## The FLS/CRUD decision tree

When writing a SOQL query:

```
Is the result returned to the client (LWC / API response)?
├─ Yes → MUST enforce FLS on read:
│        WITH USER_MODE  (preferred — enforces sharing + FLS)
│        or WITH SECURITY_ENFORCED (legacy equivalent)
│        or per-field isAccessible() before building the result
└─ No (internal auth/logic only, result never leaves Apex context) →
         WITH SYSTEM_MODE is acceptable *if* the class is without sharing
         and the result is used only for authorization decisions
```

When writing DML:

```
Is the object a *Share / *__Share (platform share table)?
├─ Yes → plain DML is fine; FLS not applicable to share objects
└─ No →
         Are you in an InstallHandler inserting PermissionSetAssignment?
         ├─ Yes → plain DML acceptable (install-time, by design system context)
         └─ No → MUST enforce FLS:
                  `insert record as user;`  (Apex USER_MODE on DML)
                  or `Database.insert(records, AccessLevel.USER_MODE)`
                  or `Security.stripInaccessible(...)` before plain DML
                  or explicit per-field loop
```

## The `@AuraEnabled` attack-surface checklist

For every `@AuraEnabled` method (including `@AuraEnabled(cacheable=true)`):

1. **Inputs are attacker-controlled.** `recordId`, `memberId`, `userId`, `accessLevel`, `role` — all of these come from the client and may be forged. Validate format and semantics; never trust that they correspond to records the caller is allowed to access.

2. **Reachability into `without sharing` code must be gated.** If this method ultimately causes a SOQL/DML to execute in `without sharing` context (class-level or inner-selector), there must be a server-side authorization check at this method's entry, before the without-sharing call. See Authorization Gate below.

3. **Mutating methods (add/update/remove/create/save/delete) MUST have an authorization check server-side.** UI-side gates like hiding a button are not enough — `@AuraEnabled` is a public endpoint from the attacker's perspective.

4. **Outputs must respect FLS.** Either the SOQL enforces FLS (see decision tree above), or the wrapper only exposes fields the caller is allowed to see, or `Security.stripInaccessible(AccessType.READABLE, records)` is applied before building the response.

## Authorization gate — acceptable patterns

Use one of these patterns at the entry of any `@AuraEnabled` method whose work depends on record-level authorization:

**Pattern A — explicit boolean check with throw:**
```apex
@AuraEnabled
public static TeamMemberWrapper updateTeamMember(String memberId, ...) {
  ObjectTeamMember__c m = [SELECT Record_Id__c FROM ObjectTeamMember__c
                           WHERE Id = :memberId WITH USER_MODE LIMIT 1];
  if (!isCurrentUserManager(m.Record_Id__c)) {
    throw new AuraHandledException(Label.FTS_Error_NotAuthorized);
  }
  // ... proceed with without-sharing mutation
}
```

**Pattern B — `with sharing` re-query of the parent record (returns 0 rows if caller lacks access):**
```apex
// Re-query parent in with-sharing context; if no rows, caller has no access.
List<Account> accessibleParent =
  new ParentAccessor().fetch(parentId); // class declared with sharing
if (accessibleParent.isEmpty()) {
  throw new AuraHandledException(Label.FTS_Error_NotAuthorized);
}
```

**Pattern C — profile check for admin-only operations:**
```apex
Profile p = [SELECT Name FROM Profile WHERE Id = :UserInfo.getProfileId()
             WITH SYSTEM_MODE LIMIT 1];
if (p.Name != 'System Administrator') {
  throw new AuraHandledException(Label.FTS_Error_AdminRequired);
}
```

**NOT acceptable as a gate:**
- A comment claiming "UI prevents this"
- Object-level CRUD check alone (`isUpdateable()`) — this only says "user has field access," not "user is allowed to affect this specific record"
- `if (!Test.isRunningTest()) { … }` or any condition that is not a per-record authorization decision
- Relying on `with sharing` at the controller class level when the actual SOQL/DML is delegated to a `without sharing` inner class

## `without sharing` — when it is OK

Acceptable uses:
- **InstallHandler** (`PostInstallHandler`) — runs system-context at install time by design. Document in FP doc.
- **Flow-invocable actions** (`@InvocableMethod`) that must operate across all records regardless of caller — document the business reason. Example in this package: `SyncOwnerInvocable` (Salesforce reviewer accepted this justification in 2attempt).
- **Queueable/Batchable/Schedulable** system jobs, when they must operate on records the caller does not own. Document.
- **Private inner selector classes** invoked from `with sharing` outer classes, where the outer class has an authorization gate per Pattern A/B/C. The auth gate is what makes this safe; `without sharing` alone is not.

Always:
- Keep the class `private` (inner) or tightly scoped.
- Add justification in `SECURITY-SCAN-FALSE-POSITIVES.md` with a grep-provable claim.
- Keep CRUD checks where applicable and FLS enforcement on DML/SOQL per the decision tree.

## Anti-patterns that previously failed review

From 2attempt findings — do not reintroduce:

| Anti-pattern | Why it failed | Fix |
|---|---|---|
| `SELECT … FROM ObjectTeamMember__c WHERE …` in `without sharing` inner class, returned via `@AuraEnabled` | No FLS, no server-side auth. Any user with read on `ObjectTeamMember__c` could enumerate team members for records they couldn't see. | Add `WITH USER_MODE` to the SOQL and add Pattern A auth gate at the `@AuraEnabled` entry. |
| `ElevatedDmlOperations.updateMember(member)` → plain `update member;` in `without sharing`, only object-level CRUD checked at caller | No FLS, no per-record auth. Any user with update on `ObjectTeamMember__c` could mutate any member org-wide. | `update member as user;` plus Pattern A auth gate verifying caller manages `member.Record_Id__c`. |
| `@SuppressWarnings('PMD.ApexCRUDViolation')` on class with no FLS | Silences the static analyzer while the violation remains. Review team verifies manually, not via PMD output. | Remove the suppression, add real FLS per decision tree. Only keep suppression after a grep-provable FP-doc justification. |
| Comment-based "security note" as justification | Claim, not a control. | Add a concrete auth check in code per Authorization Gate. |
| FP doc claim "uses `WITH USER_MODE`" not matching actual SOQL | Reviewer greps; claim false → immediate rejection. | Make the claim true (edit the SOQL) before writing it in the doc. |

## What the Code Analyzer catches (and doesn't)

`sf code-analyzer run --rule-selector AppExchange` reliably flags:
- `ApexFlsViolation` — FLS missing on SOQL/DML (matches Lesson 1)
- `DatabaseOperationsMustUseWithSharing` — class-level sharing issue (partial match for Lesson 2)

It does **not** reliably flag:
- Missing server-side authorization for client-controlled IDs (Lesson 2's real core — it's a logic issue, not a syntactic one)
- Stale false-positive doc entries (Lesson 3)
- Weak-justification patterns in comments

The `/appexchange-audit` command in this repo adds checks for these gaps. Run it before submission.

## Related files in this repo

- **Command**: `.claude/commands/appexchange-audit.md` — full pre-submission audit
- **Command**: `.claude/commands/quality-warden.md` — file-level review during dev
- **Command**: `.claude/commands/audit.md` — project-level readiness rollup
- **Skill**: `.claude/skills/fp-doc-audit/SKILL.md` — verifies false-positive doc claims against code
- **Skill**: `.claude/skills/locker-lwr-compat/SKILL.md` — runtime compatibility patterns
- **FP doc** (default location): `docs/false-positives.md` — create when you have your first finding to document. Move to `docs/reference/false-positive-template.docx` (official Salesforce format) before submission.
- **AppExchange artifacts** (optional, present if you've started a real submission process): `appexchange-artifacts/` — scan results, prior rejection reports, scanner config. Not shipped by this toolkit; user populates as their submission progresses.

This skill was originally written from the FlexibleTeamShare rejection case (Tucario, 2026). The lessons in this file are the durable patterns; the path references for scan-result CSVs etc. are conventional, not required. If your repo uses a different layout, the substance of the patterns still applies.