appexchange-review-patterns

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:

@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):

// 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:

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.