diff --git a/.claude/skills/generate-site-preferences-impex/SKILL.md b/.claude/skills/generate-site-preferences-impex/SKILL.md index 95f7b58..e383421 100644 --- a/.claude/skills/generate-site-preferences-impex/SKILL.md +++ b/.claude/skills/generate-site-preferences-impex/SKILL.md @@ -19,7 +19,7 @@ Use proactively whenever: - Creating a new commerce app (always needs preferences) - User mentions "configuration", "settings", or "preferences" - App needs merchant-configurable options -- Adding API credentials, feature toggles, or environment selection +- Adding feature toggles, environment selection, or other non-secret merchant settings - Any scenario where merchants need to configure app behavior ## Step 1: Collect preference information @@ -39,14 +39,14 @@ Use proactively whenever: Read `references/attribute-types.md` for complete type reference with examples: - **boolean** - Enable/disable flags -- **string** - API keys, URLs, short text +- **string** - URLs, public identifiers, short text - **text** - Long descriptions, JSON configs - **integer** - Counts, limits - **decimal** - Percentages, ratings - **enum-of-string** - Dropdown selections - **set-of-string** - Multiple selections - **email** - Email addresses -- **password** - Encrypted secrets +- **password** - Legacy masked values only; do not use Site preferences for secret values ## Step 3: Use app-specific patterns @@ -80,24 +80,6 @@ Create the attribute definitions file: false - - - {displayName} API Key - string - false - false - 0 - - - - - {displayName} API Secret - password - false - false - 0 - - {displayName} Environment @@ -133,8 +115,6 @@ Create the attribute definitions file: {displayName} - - @@ -157,7 +137,6 @@ Create default preference values: sandbox false - ``` @@ -177,10 +156,11 @@ For international merchants, add translations. See `references/attribute-types.m ### Defaults - Safe defaults: Start with features disabled - Sensible values: Choose defaults that work for most cases -- Empty credentials: Don't include placeholder API keys +- Secret values: Do not configure them as Site preferences ### Security -- Use `password` type for API secrets +- Never generate Site preferences for keys, tokens, passwords, credentials, or other secret values +- Use ecom service credentials and `LocalServiceRegistry` for authentication secrets - Don't expose secrets in XML - Document sensitive fields clearly @@ -197,7 +177,7 @@ For international merchants, add translations. See `references/attribute-types.m - [ ] Descriptions provide helpful guidance - [ ] Appropriate data types - [ ] Safe default values -- [ ] Password type for secrets +- [ ] No secret values stored in Site preferences - [ ] All attributes in group definition - [ ] XML well-formed - [ ] SITEID placeholder used @@ -225,7 +205,7 @@ xmllint --noout impex/install/sites/SITEID/preferences.xml | No default values | Set sensible defaults | | Everything mandatory | Only require essentials | | Generic descriptions | Provide clear guidance | -| Hardcoded credentials | Leave empty for merchants | +| Secret values in Site preferences | Remove them; use ecom service credentials for authentication secrets | | Not in group | Add all attributes to group | ## Quick reference diff --git a/.claude/skills/generate-site-preferences-impex/evals/evals.json b/.claude/skills/generate-site-preferences-impex/evals/evals.json index 9477162..0dd5f34 100644 --- a/.claude/skills/generate-site-preferences-impex/evals/evals.json +++ b/.claude/skills/generate-site-preferences-impex/evals/evals.json @@ -1,9 +1,9 @@ { "evals": [ { - "prompt": "Merchants need to configure API keys and select sandbox vs production. Can you create the site preferences?", - "context": "User needs configurable merchant settings", - "expected_behavior": "Should trigger generate-site-preferences-impex skill, create attribute definitions with API key (string), secret (password), environment (enum)" + "prompt": "Merchants need to select sandbox vs production and enable the integration. Can you create the site preferences?", + "context": "User needs non-secret configurable merchant settings", + "expected_behavior": "Should trigger generate-site-preferences-impex skill and create environment (enum) and enabled (boolean) attributes; no secret values are stored in Site preferences, and authentication secrets remain in ecom service credentials" }, { "prompt": "I need to add some settings that merchants can configure in Business Manager for my tax app", diff --git a/.claude/skills/generate-site-preferences-impex/references/app-patterns.md b/.claude/skills/generate-site-preferences-impex/references/app-patterns.md index b8c26aa..aa00fd7 100644 --- a/.claude/skills/generate-site-preferences-impex/references/app-patterns.md +++ b/.claude/skills/generate-site-preferences-impex/references/app-patterns.md @@ -45,6 +45,11 @@ This reference contains pre-built site preference patterns for common commerce a ## Payment App Preferences +Secret values are intentionally absent from this pattern. Do not store keys, tokens, +passwords, or credentials in Site preferences. Configure authentication secrets as ecom service +credentials and read them through `LocalServiceRegistry`. Public identifiers such as a +publishable key may remain Site preferences. + ```xml @@ -65,12 +70,6 @@ This reference contains pre-built site preference patterns for common commerce a 0 - - Secret API Key - password - 0 - - Test Mode boolean diff --git a/.claude/skills/generate-site-preferences-impex/references/attribute-types.md b/.claude/skills/generate-site-preferences-impex/references/attribute-types.md index 738a346..d77f13d 100644 --- a/.claude/skills/generate-site-preferences-impex/references/attribute-types.md +++ b/.claude/skills/generate-site-preferences-impex/references/attribute-types.md @@ -18,8 +18,8 @@ Complete reference for all supported SFCC site preference attribute types with e ## String ```xml - - API Key + + Public Identifier string false false @@ -127,9 +127,14 @@ Complete reference for all supported SFCC site preference attribute types with e ## Password (encrypted) +This type is documented for legacy schema compatibility, but masking does not make Site +preferences an approved secret store. Do not use it for keys, tokens, passwords, credentials, +or other secret values. Authentication secrets belong in ecom service credentials and +must be read through `LocalServiceRegistry`. + ```xml - - API Secret + + Restricted Note password false false diff --git a/.claude/skills/scaffold-app/SKILL.md b/.claude/skills/scaffold-app/SKILL.md index 30af96a..a2b9f92 100644 --- a/.claude/skills/scaffold-app/SKILL.md +++ b/.claude/skills/scaffold-app/SKILL.md @@ -136,8 +136,8 @@ Generate merchant-facing tasks that guide post-installation setup and verificati [ { "name": "Configure API Credentials", - "description": "Add your [vendor] API key in Business Manager > Merchant Tools > Custom Site Preferences.", - "link": "/on/demandware.store/Sites-Site/default/ViewApplication-BM?SelectedMenuItem=site-prefs_custom_prefs", + "description": "Add your [vendor] API credentials in Business Manager > Administration > Operations > Services > Credentials.", + "link": "/on/demandware.store/Sites-Site/default/ViewApplication-BM?SelectedMenuItem=service_credentials", "taskNumber": "1" }, { @@ -149,7 +149,7 @@ Generate merchant-facing tasks that guide post-installation setup and verificati ``` Tailor tasks to domain for merchant post-installation: -- **Tax/Shipping/Payment:** Add API credentials, configure service settings, test checkout transactions +- **Tax/Shipping/Payment:** Add API credentials to ecom service credentials, configure service settings, test checkout transactions - **Loyalty/Gift Cards:** Configure points rules, test balance lookups, verify redemption - **UI-only apps:** Verify components appear on storefront, check responsive behavior - **Fullstack:** Both UI verification and backend configuration steps diff --git a/.claude/skills/shared/security-rules.md b/.claude/skills/shared/security-rules.md index 060ebe9..ef549ea 100644 --- a/.claude/skills/shared/security-rules.md +++ b/.claude/skills/shared/security-rules.md @@ -2,7 +2,7 @@ Canonical list of all checks performed by `.github/scripts/security-scan.sh`. -## Blocking findings (21 checks — exit code 1) +## Blocking findings (22 checks — exit code 1) Must fix before packaging or submission: @@ -19,6 +19,7 @@ Must fix before packaging or submission: - S13: setTimeout/setInterval in hook scripts — blocking calls - S14: Unbounded loops (while(true)/for(;;)) without break/return - S15: Service profiles missing rate-limit-enabled AND circuit-breaker-enabled +- S20: Secret values stored in Site preferences (any attribute type) or read through `getCustomPreferenceValue(...)`. Secret IDs include API keys, secrets, passwords, credentials, and IDs ending in `Token`. CI flags secret-like `SitePreferences` attribute IDs regardless of declared type and same-line literal secret-ID reads, and fails closed on malformed `SitePreferences` XML. Multiline, computed, or semantically hidden IDs require manual review. **Suggested fix:** remove secret values from Site preferences; store authentication secrets in ecom service credentials and access them through `LocalServiceRegistry` (`svc.configuration.credential.getUser()` / `getPassword()`). - P1: Service profile XML missing timeout-millis - Q1: Hook scripts referenced in hooks.json that don't exist - Q2: Hook scripts missing expected function exports diff --git a/.github/scripts/security-scan.sh b/.github/scripts/security-scan.sh index 5f1246f..f53452e 100755 --- a/.github/scripts/security-scan.sh +++ b/.github/scripts/security-scan.sh @@ -50,6 +50,36 @@ strip_comments() { grep -vE '^[0-9]+:\s*//' | grep -vE '^[0-9]+:\s*\*' | grep -vE '^[0-9]+:\s*/\*' } +# Remove full-line and multiline JavaScript comments while preserving line count. +# This intentionally stays line-oriented; semantic review handles complex cases. +sanitize_code() { + awk ' + BEGIN { block = 0 } + { + if (block) { + if ($0 ~ /\*\//) block = 0 + print "" + next + } + if ($0 ~ /^[[:space:]]*\/\*/) { + if ($0 !~ /\*\//) block = 1 + print "" + next + } + if ($0 ~ /^[[:space:]]*\/\//) { + print "" + next + } + print + } + ' "$1" +} + +if ! command -v xmllint >/dev/null 2>&1; then + echo "::error::xmllint is required for Site preference security checks" + exit 1 +fi + # Collect JS/DS files (cartridge server-side scripts + storefront-next TS) # Using while-read for macOS bash 3 compatibility (no mapfile) JS_FILES=() @@ -169,7 +199,18 @@ for f in ${JS_FILES[@]+"${JS_FILES[@]}"}; do [[ -z "$f" ]] && continue while IFS= read -r line; do block "$f" "Inline Authorization header — use service framework instead: $line" - done < <(grep -nE 'setRequestHeader\s*\(\s*['\''"]Authorization' "$f" 2>/dev/null | head -3) + done < <(sanitize_code "$f" | awk ' + { + if (statement == "") start = NR + statement = statement " " $0 + if (statement ~ /setRequestHeader[[:space:]]*\([[:space:]]*["\047]Authorization["\047]/) { + print start ":" statement + statement = "" + } else if ($0 ~ /;/) { + statement = "" + } + } + ' | head -3) done # S8: Additional DOM sinks — outerHTML, document.write, insertAdjacentHTML (BLOCK) @@ -321,6 +362,57 @@ for f in ${ALL_CODE_FILES[@]+"${ALL_CODE_FILES[@]}"}; do done < <(grep -nE '\bencodeURI\s*\([^)]*(\+|\$\{)' "$f" 2>/dev/null | strip_comments | head -5) done +# S20: Secret values stored in or read from Site preferences (BLOCK) +S20_MESSAGE="Secret value stored in or read from Site preferences — remove it; use ecom service credentials/LocalServiceRegistry for authentication secrets" + +s20_secret_site_preference_ids() { + # Match secret-like attribute IDs on SitePreferences regardless of . + # Password-typed prefs are the obvious case, but string-typed secrets (the + # shape the old site-preferences skill generated, e.g. vendorApiKey) are + # just as dangerous and BM won't even mask them — so the ID heuristic, not + # the declared type, is the gate here. + xmllint --xpath ' + //*[local-name()="type-extension" and @type-id="SitePreferences"] + //*[local-name()="attribute-definition"] + /@attribute-id + ' "$1" 2>/dev/null | + grep -oE 'attribute-id="[^"]+"' | + cut -d'"' -f2 | + awk '{ + lower = tolower($0) + if (lower ~ /(api[_-]?key|secret|password|passwd|credential)/ || lower ~ /token$/) print + }' +} + +# Inspect secret-like attribute IDs within the SitePreferences type extension. +# A malformed SitePreferences XML must fail closed: xmllint cannot parse it, so +# the xpath above would silently return nothing and a file carrying secret prefs +# would slip through. Block any SitePreferences-declaring XML that is not +# well-formed rather than skipping it. +for f in ${XML_FILES[@]+"${XML_FILES[@]}"}; do + [[ -z "$f" ]] && continue + if grep -q 'SitePreferences' "$f" 2>/dev/null && ! xmllint --noout "$f" 2>/dev/null; then + block "$f" "$S20_MESSAGE: malformed SitePreferences XML could not be parsed — failing closed" + continue + fi + while IFS= read -r preference_id; do + block "$f" "$S20_MESSAGE: $preference_id" + done < <(s20_secret_site_preference_ids "$f" | head -5) +done + +# Detect literal reads of secret Site preference IDs. +s20_secret_site_preference_reads() { + sanitize_code "$1" | + grep -niE "getCustomPreferenceValue[[:space:]]*\\([[:space:]]*['\"]([^'\"]*(api[_-]?key|secret|password|passwd|credential)[^'\"]*|[^'\"]*token)['\"]" +} + +for f in ${JS_FILES[@]+"${JS_FILES[@]}"}; do + [[ -z "$f" ]] && continue + while IFS= read -r line; do + block "$f" "$S20_MESSAGE: $line" + done < <(s20_secret_site_preference_reads "$f" | head -5) +done + echo "" # ============================ PERFORMANCE =================================== diff --git a/.github/scripts/test-security-scan.sh b/.github/scripts/test-security-scan.sh index 3bb9fb4..eb8e296 100755 --- a/.github/scripts/test-security-scan.sh +++ b/.github/scripts/test-security-scan.sh @@ -36,6 +36,18 @@ assert_blocks() { fi } +assert_blocks_with() { + local desc="$1" needle="$2"; shift 2 + run_scan "$@" + if [[ "$LAST_RC" -eq 1 ]] && echo "$LAST_OUTPUT" | grep -qF "$needle"; then + echo " PASS: $desc" + PASS=$((PASS + 1)) + else + echo " FAIL: $desc (expected exit 1 + finding containing '$needle', got exit $LAST_RC)" + FAIL=$((FAIL + 1)) + fi +} + assert_passes() { local desc="$1"; shift run_scan "$@" @@ -233,6 +245,12 @@ echo "--- S7: Inline Authorization ---" cap="$(mkcap)"; printf "req.setRequestHeader('Authorization', token);\n" > "$cap/app.js" assert_blocks "setRequestHeader Authorization blocks" "$cap" +cap="$(mkcap)"; printf "// req.setRequestHeader('Authorization', token);\n" > "$cap/app.js" +assert_passes "commented Authorization header is ignored" "$cap" + +cap="$(mkcap)"; printf "/*\nreq.setRequestHeader('Authorization', token);\n*/\n" > "$cap/app.js" +assert_passes "multiline-commented Authorization header is ignored" "$cap" + echo "" # --------------------------------------------------------------------------- @@ -681,6 +699,197 @@ assert_no_warning "commented encodeURI is ignored" "encodeURI()" "$cap" echo "" +# --------------------------------------------------------------------------- +# S20: Site preference credentials used for authentication (BLOCK) +# --------------------------------------------------------------------------- +echo "--- S20: Site preference credentials ---" + +S20_MSG="Secret value stored in or read from Site preferences — remove it; use ecom service credentials/LocalServiceRegistry for authentication secrets" + +cap="$(mkcap)" +cat > "$cap/preferences.xml" <<'EOF' + + + + + password + + + + +EOF +cat > "$cap/service.js" <<'EOF' +var apiKey = Site.current.getCustomPreferenceValue('zenkraftMasterAPIKey'); +svc.addHeader('zkkey', apiKey); +EOF +assert_blocks_with "Zenkraft-shaped password preference and zkkey use blocks" "$S20_MSG" "$cap" + +cap="$(mkcap)" +cat > "$cap/service.ds" <<'EOF' +var token = Site.current.getCustomPreferenceValue("vendorAuthToken"); +EOF +assert_blocks_with "literal secret Site preference read blocks" "$S20_MSG" "$cap" + +cap="$(mkcap)"; printf "var mode = Site.current.getCustomPreferenceValue('vendorTokenMode');\n" > "$cap/service.js" +assert_passes "Site preference with token in the middle does not block" "$cap" + +cap="$(mkcap)" +cat > "$cap/service.js" <<'EOF' +svc.setHeader('Authorization', Site.current.getCustomPreferenceValue('vendorMode')); +EOF +assert_passes "ordinary preference used in Authorization does not match S20" "$cap" + +cap="$(mkcap)" +cat > "$cap/service.js" <<'EOF' +var value = Site.current.getCustomPreferenceValue('vendorMode'); +svc.addHeader('X-API-Key', value); +EOF +assert_passes "ordinary preference used in API key header does not match S20" "$cap" + +for preference_id in \ + vendorSecret vendorToken vendorPassword vendorPasswd vendorCredential \ + vendorApiKey vendorAPISecret vendorClientSecret; do + cap="$(mkcap)" + printf 'password\n' "$preference_id" > "$cap/preferences.xml" + assert_blocks_with "password SitePreferences attribute '$preference_id' blocks" "$S20_MSG" "$cap" +done + +cap="$(mkcap)" +cat > "$cap/preferences.xml" <<'EOF' + + + + Signing secret + + password + + + + +EOF +assert_blocks_with "multiline reordered single-quoted SitePreferences XML blocks" "$S20_MSG" "$cap" + +cap="$(mkcap)" +cat > "$cap/preferences.xml" <<'EOF' + + + password + + + boolean + + +EOF +assert_passes "password attribute outside SitePreferences does not block" "$cap" + +cap="$(mkcap)" +cat > "$cap/preferences.xml" <<'EOF' + + + +EOF +assert_passes "credential preference in XML comment is ignored" "$cap" + +for preference_id in vendorPublicKey vendorKey vendorTokenMode vendorTokenizationId vendorTokenizationEnabled vendorMonkey vendorKeynote; do + cap="$(mkcap)" + printf 'password\n' "$preference_id" > "$cap/preferences.xml" + assert_passes "non-secret password preference '$preference_id' does not block" "$cap" +done + +cap="$(mkcap)" +cat > "$cap/service.js" <<'EOF' +// var key = Site.current.getCustomPreferenceValue('vendorApiKey'); +// svc.addHeader('zkkey', key); +/* +var token = Site.current.getCustomPreferenceValue('vendorAuthToken'); +svc.setHeader('Authorization', token); +*/ +EOF +assert_passes "commented getter and authentication sinks are ignored" "$cap" + +cap="$(mkcap)" +cat > "$cap/service.js" <<'EOF' +var color = Site.current.getCustomPreferenceValue('vendorThemeColor'); +svc.addHeader('X-Theme', color); +EOF +assert_passes "ordinary preference in ordinary header does not block" "$cap" + +cap="$(mkcap)" +cat > "$cap/service.js" <<'EOF' +var key = Site.current.getCustomPreferenceValue('vendorApiKey'); +svc.addHeader('X-Correlation-Key', key); +EOF +assert_blocks_with "secret preference read blocks regardless of later use" "$S20_MSG" "$cap" + +cap="$(mkcap)" +cat > "$cap/service.js" <<'EOF' +var LocalServiceRegistry = require('dw/svc/LocalServiceRegistry'); +var service = LocalServiceRegistry.createService('vendor.api', { + createRequest: function (svc) { + var user = svc.configuration.credential.getUser(); + var password = svc.configuration.credential.getPassword(); + svc.addHeader('Authorization', user + ':' + password); + } +}); +EOF +assert_passes "LocalServiceRegistry service credentials are allowed" "$cap" + +# String-typed secret preference — the shape the old site-preferences skill +# generated. BM won't mask it, so a secret-like ID must block regardless of type. +cap="$(mkcap)" +cat > "$cap/preferences.xml" <<'EOF' + + + string + + +EOF +assert_blocks_with "string-typed secret SitePreferences attribute blocks" "$S20_MSG" "$cap" + +# Non-secret string-typed preference must still pass (type is no longer the gate). +cap="$(mkcap)" +cat > "$cap/preferences.xml" <<'EOF' + + + string + + +EOF +assert_passes "non-secret string-typed SitePreferences attribute does not block" "$cap" + +# Malformed SitePreferences XML must fail closed — xmllint cannot parse it, so a +# secret pref would otherwise slip through the xpath silently. +cap="$(mkcap)" +cat > "$cap/preferences.xml" <<'EOF' + + + password + + +EOF +assert_blocks_with "malformed SitePreferences XML fails closed" "$S20_MSG" "$cap" + +# Malformed XML that does not mention SitePreferences is not S20's concern. +cap="$(mkcap)" +cat > "$cap/random.xml" <<'EOF' + + + string + + +EOF +assert_passes "malformed non-SitePreferences XML does not trigger S20" "$cap" + +echo "" + # --------------------------------------------------------------------------- # P1: Service profile timeout (BLOCK) # --------------------------------------------------------------------------- diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 0683e2c..b0e085b 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -16,6 +16,11 @@ jobs: ref: ${{ github.event.pull_request.head.sha || github.sha }} fetch-depth: 0 + - name: Install XML tools + run: | + sudo apt-get update + sudo apt-get install --yes libxml2-utils + - name: Extract Commerce App Packages shell: bash env: diff --git a/.github/workflows/test-scripts.yml b/.github/workflows/test-scripts.yml index 78dd376..3f30115 100644 --- a/.github/workflows/test-scripts.yml +++ b/.github/workflows/test-scripts.yml @@ -40,5 +40,10 @@ jobs: exit 1 fi + - name: Install XML tools + run: | + sudo apt-get update + sudo apt-get install --yes libxml2-utils + - name: Run script tests run: ./.github/scripts/run-all-tests.sh diff --git a/.github/workflows/verify-zip.yml b/.github/workflows/verify-zip.yml index 008efe0..689b93f 100644 --- a/.github/workflows/verify-zip.yml +++ b/.github/workflows/verify-zip.yml @@ -22,6 +22,11 @@ jobs: with: fetch-depth: 0 + - name: Install XML tools + run: | + sudo apt-get update + sudo apt-get install --yes libxml2-utils + - name: Step 1 - Verify sha256 in manifest.json for changed ZIPs shell: bash run: | diff --git a/AGENTS.md b/AGENTS.md index 4219faf..a80a93d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,7 +142,7 @@ Your response: ### 4. Security - NO hardcoded production credentials in impex - Use placeholders for API keys/secrets -- Use `` type for sensitive site preferences +- Do not store secret values in Site preferences; store authentication secrets in ecom service credentials and access them through `LocalServiceRegistry` - Mark sensitive data clearly in documentation - NO secret files (`.env`, `.key`, `.pem`, `.p12`, `.pfx`, `.jks`) in packages - NO direct `HTTPClient` usage — must use service framework @@ -505,8 +505,8 @@ Before suggesting `/submit-app-pr`, verify: **Security:** - [ ] No sensitive data in XML -- [ ] Passwords use `` type -- [ ] API keys are placeholders +- [ ] No secret values are stored in Site preferences; authentication secrets use ecom service credentials/`LocalServiceRegistry` +- [ ] API keys in service impex are placeholders - [ ] No direct HTTPClient usage (use service framework) - [ ] No secret files in package (.env, .key, .pem, .p12, .pfx, .jks) - [ ] No eval/innerHTML/outerHTML/insertAdjacentHTML/document write diff --git a/docs/testing.md b/docs/testing.md index 1e1aa2f..422ed5e 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -724,7 +724,7 @@ Before opening the PR: **Security:** - [ ] No hardcoded production credentials anywhere -- [ ] Sensitive site preferences use `` type +- [ ] No secret values are stored in Site preferences; authentication secrets use ecom service credentials/`LocalServiceRegistry` - [ ] Error messages don't leak internal details - [ ] Input validation at trust boundaries