Imported from tucario/Tucario-LWC-Toolkit (.claude/skills/appexchange-review-patterns/SKILL.md). Install upstream with npx skills add tucario/Tucario-LWC-Toolkit --skill appexchange-review-patterns. Copyright stays with the author.

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:

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.