diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 1b68bd1..2b82250 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -80,6 +80,7 @@ jobs: run: npm publish --access public - name: Create GitHub Release + id: release if: steps.guard.outputs.published == 'false' env: GH_TOKEN: ${{ github.token }} @@ -96,3 +97,48 @@ jobs: --notes "See [CHANGELOG.md](https://github.com/$GITHUB_REPOSITORY/blob/main/CHANGELOG.md) for details. Install: \`npm install @gravitykit/mcp@$VERSION\` ยท [View on npm](https://www.npmjs.com/package/@gravitykit/mcp/v/$VERSION)" + # Signals the Slack step that a release was actually created this run + # (not an idempotent skip), so announcements never double-fire. + echo "created=true" >> "$GITHUB_OUTPUT" + + - name: Announce release to Slack + # Mirrors gktools' "Notify Slack": POST {link,version,changelog,product} + # to SLACK_RELEASE_NOTIFICATION_WEBHOOK so GravityKit MCP shows up in + # #release-announcements like every other plugin. Only after a release is + # actually created; never fails the job. + if: steps.release.outputs.created == 'true' + env: + SLACK_RELEASE_NOTIFICATION_WEBHOOK: ${{ secrets.SLACK_RELEASE_NOTIFICATION_WEBHOOK }} + # Bind expressions through env (quoted shell vars below) instead of + # interpolating ${{ }} into the run script โ€” avoids template-to-shell injection. + VERSION: ${{ steps.guard.outputs.version }} + REPO: ${{ github.repository }} + run: | + if [ -z "${SLACK_RELEASE_NOTIFICATION_WEBHOOK:-}" ]; then + echo "SLACK_RELEASE_NOTIFICATION_WEBHOOK not set โ€” skipping Slack announcement." + exit 0 + fi + LINK="https://github.com/$REPO/releases/tag/v$VERSION" + # Extract this version's CHANGELOG section: lines between "## [VERSION]" + # and the next "## [" header. + NOTES="$(awk -v ver="$VERSION" ' + $0 ~ "^## \\[" ver "\\]" { capture=1; next } + capture && /^## \[/ { exit } + capture { print } + ' CHANGELOG.md)" + # jq builds the JSON so newlines/quotes in the changelog are escaped. + PAYLOAD="$(jq -n \ + --arg link "$LINK" \ + --arg version "$VERSION" \ + --arg changelog "$NOTES" \ + --arg product "GravityKit MCP" \ + '{link:$link,version:$version,changelog:$changelog,product:$product}')" + # Non-fatal, like gktools: a bad Slack response only warns. + RESPONSE="$(curl -sS -X POST \ + -H 'Content-Type: application/json' \ + --data "$PAYLOAD" \ + "$SLACK_RELEASE_NOTIFICATION_WEBHOOK" || true)" + echo "Slack response: $RESPONSE" + if ! printf '%s' "$RESPONSE" | grep -q '"ok":true'; then + echo "::warning::Slack release announcement did not return \"ok\":true." + fi diff --git a/AGENTS.md b/AGENTS.md index 6dd702d..6fec1be 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,7 +6,7 @@ This is the single canonical doc for the project (agents and humans). `CLAUDE.md ## Project Identity -- **Package:** `@gravitykit/mcp` v2.4.0 +- **Package:** `@gravitykit/mcp` v2.4.1 - **Type:** Node.js MCP server (ESM) - **Purpose:** Full Gravity Forms REST API v2 coverage (26 Gravity Forms tools), plus dynamic GravityKit product tools (GravityView so far) via the WordPress Abilities API - **Repo:** https://github.com/GravityKit/MCP diff --git a/CHANGELOG.md b/CHANGELOG.md index c45549e..c11654f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to GravityKit MCP (formerly GravityMCP) will be documented i The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [2.4.1] - 2026-06-25 + +This release lets the field tools work with custom and third-party field types (not just the ones built into Gravity Forms), and tightens input handling and security across the server. + +### ๐Ÿ› Fixed +- **`gf_add_field` now accepts custom and third-party field types.** Field types added by other plugins were previously rejected because they aren't in the built-in registry. They are now created normally, with a note when type-specific defaults or sub-inputs aren't available; pass `inputs`/`choices` explicitly for custom compound or choice fields. +- **Compound field sub-inputs stay consistent with the field's id.** When a field's id is auto-assigned or differs from the one supplied, its sub-input ids (e.g. `5.1`, `5.2`) are rebased to match, so `name`, `address`, and custom compound fields save to the right inputs. +- **`gf_update_field` no longer saves a change it then reports as blocked.** When a field had dependent conditional logic and `force` wasn't set, the update was written before the "use force to proceed" response, so the check protected nothing. The dependency check now runs before the write (matching `gf_delete_field`). +- **Hardened tool-input handling.** Malformed or hostile input to the field, validation, and dependency helpers (empty/`null`/non-string field types, `null` properties or position, a `null` entry in a form's `fields`, non-scalar filter values, self-referential objects) now returns a clean error or is handled safely instead of failing opaquely. +- **Security and validation hardening.** Loopback detection no longer treats a remote `127.x` domain as local (which could have allowed Basic auth over plain HTTP); `Cookie` headers and values are masked in logs; confirmation-redirect URL validation accepts dotless hosts (localhost/intranet); and duplicate or out-of-range explicit field ids are corrected so generated forms stay valid (an out-of-range id can no longer stall form creation). + ## [2.4.0] - 2026-06-19 This update adds additional guards to make sure Gravity Forms entries save correctly for every field type, so an AI assistant gets the entry format right the first time. Adds explicit support for the `password` field type and Gravity Wiz Nested Forms, plus a benchmark suite that ensures the MCP works well with small models. @@ -236,6 +247,7 @@ A correctness pass on the Gravity Forms (`gf_*`) plane, verified against Gravity - Field filters (1 tool) - Results/Analytics (1 tool) +[2.4.1]: https://github.com/GravityKit/MCP/releases/tag/v2.4.1 [2.4.0]: https://github.com/GravityKit/MCP/releases/tag/v2.4.0 [2.3.0]: https://github.com/GravityKit/MCP/releases/tag/v2.3.0 [2.2.0]: https://github.com/GravityKit/MCP/releases/tag/v2.2.0 diff --git a/mcp.json b/mcp.json index 98b6767..20a8d9a 100644 --- a/mcp.json +++ b/mcp.json @@ -1,6 +1,6 @@ { "name": "gravitykit-mcp", - "version": "2.4.0", + "version": "2.4.1", "description": "MCP server for Gravity Forms", "author": "GravityKit", "license": "MIT", diff --git a/package.json b/package.json index 303f2a9..ebc91b1 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@gravitykit/mcp", - "version": "2.4.0", + "version": "2.4.1", "description": "Full-featured MCP server for Gravity Forms", "main": "src/index.js", "type": "module", diff --git a/src/abilities/loader.js b/src/abilities/loader.js index 6892d02..3b1c6a7 100644 --- a/src/abilities/loader.js +++ b/src/abilities/loader.js @@ -58,13 +58,13 @@ const GK_NAME_PATTERN = /^gk-[a-z0-9-]+\//; * @returns {'GET'|'POST'|'DELETE'} */ export function methodForAbility(annotations = {}) { - if (annotations.readonly) return 'GET'; + if (annotations?.readonly) return 'GET'; // Foundation's run controller only accepts DELETE for abilities that // are BOTH destructive AND idempotent โ€” matching WP-REST conventions // for HTTP DELETE. Destructive-but-not-idempotent operations (e.g. // view-delete with `force` defaulting to soft trash) must go through // POST so their non-idempotent semantics are explicit on the wire. - if (annotations.destructive && annotations.idempotent) return 'DELETE'; + if (annotations?.destructive && annotations?.idempotent) return 'DELETE'; return 'POST'; } diff --git a/src/config/auth.js b/src/config/auth.js index cfa5d7e..124196c 100644 --- a/src/config/auth.js +++ b/src/config/auth.js @@ -18,11 +18,15 @@ import logger from '../utils/logger.js'; export function isLocalUrl(baseUrl) { try { const { hostname } = new URL(baseUrl); + // 127.0.0.0/8 loopback must be a genuine IPv4: a remote domain like + // "127.example.com" also `startsWith('127.')` and must NOT count as local, + // or Basic auth would be allowed in the clear over plain HTTP to a remote host. + const isLoopbackIp = /^127\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostname); return hostname === 'localhost' || hostname === '[::1]' || hostname === '::1' || hostname.endsWith('.localhost') - || hostname.startsWith('127.') + || isLoopbackIp || hostname.endsWith('.test') || hostname.endsWith('.local'); } catch { diff --git a/src/config/field-validation.js b/src/config/field-validation.js index 9834205..f6c4b93 100644 --- a/src/config/field-validation.js +++ b/src/config/field-validation.js @@ -43,8 +43,8 @@ export class FieldAwareValidator { } else { errors.push({ index: i, - fieldId: field.id, - fieldType: field.type, + fieldId: field?.id, + fieldType: field?.type, error: validation.error }); } @@ -89,10 +89,12 @@ export class FieldAwareValidator { logger.warn(`[FieldValidator] Unknown field type '${field.type}' at ${path}`); } - // Allow unknown types but mark them + // Allow unknown types (third-party add-ons, GravityKit, custom fields). + // Gravity Forms accepts them on save; pass the field through unchanged. No + // internal flag is added: it would be PUT verbatim and nothing reads it. return { isValid: true, - field: { ...field, _unknown: true } + field: { ...field } }; } diff --git a/src/config/validation-config.js b/src/config/validation-config.js index 338e4b5..7a11efa 100644 --- a/src/config/validation-config.js +++ b/src/config/validation-config.js @@ -20,7 +20,7 @@ export const VALIDATION_CONFIG = { maxLength: 254 // RFC 5321 }, url: { - pattern: /^https?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*)$/, + pattern: /^https?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}(\.[a-zA-Z0-9()]{1,6})?\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*)$/, maxLength: 2083 // IE limit }, slug: { @@ -104,13 +104,15 @@ export const VALIDATION_CONFIG = { * Get error message with field name and parameters */ export function formatErrorMessage(template, field, params = {}) { - let message = template.replace('{field}', field); - + // split/join replaces EVERY placeholder and treats the value literally. + // String.replace(str, str) only swaps the first match and interprets `$&`/`$1` + // sequences in the replacement string. + let message = template.split('{field}').join(field); + Object.keys(params).forEach(key => { - const placeholder = `{${key}}`; - message = message.replace(placeholder, params[key]); + message = message.split(`{${key}}`).join(String(params[key])); }); - + return message; } diff --git a/src/config/validation.js b/src/config/validation.js index 13d1c45..446743d 100644 --- a/src/config/validation.js +++ b/src/config/validation.js @@ -104,6 +104,13 @@ export class BaseValidator { throw new Error('Field filter must have a value'); } + // Reject non-array objects: String({}) becomes "[object Object]", a silent + // garbage filter value. Arrays stay valid for the multi-value operators + // handled below; a plain object never is. + if (typeof value === 'object' && !Array.isArray(value)) { + throw new Error('Field filter value must be a string, number, boolean, or array'); + } + // GF normalizes operators case-insensitively (IN/in/In all match), so accept // any case rather than rejecting valid GF operators. const validOperators = getEnumValues('fieldOperators'); diff --git a/src/field-definitions/field-registry.js b/src/field-definitions/field-registry.js index fdf91f0..f4cb6e6 100644 --- a/src/field-definitions/field-registry.js +++ b/src/field-definitions/field-registry.js @@ -902,6 +902,9 @@ export function getStorageFormat(fieldType) { * Helper function to detect field variant */ export function detectFieldVariant(field) { + if (!field || typeof field !== 'object') { + return 'default'; + } const definition = fieldRegistry[field.type]; if (!definition || !definition.variants) { return 'default'; @@ -927,13 +930,16 @@ export function detectFieldVariant(field) { * Validate field configuration */ export function validateFieldConfig(field) { + if (!field || typeof field !== 'object') { + return { isValid: false, error: 'Field must be an object' }; + } const definition = fieldRegistry[field.type]; - + + // Unknown / third-party types are tolerated, not rejected (Gravity Forms + // accepts them on save). With no config schema to check them against there is + // nothing to validate, so report valid and let GF own it. if (!definition) { - return { - isValid: false, - error: `Unknown field type: ${field.type}` - }; + return { isValid: true }; } // Check required properties @@ -978,42 +984,56 @@ export function assignFieldIds(fields) { return fields; } - const used = new Set(); + // Rebase any dotted compound sub-input ids onto `parentId` so they always + // track the field's FINAL id โ€” applied whether the id is preserved or newly + // generated, so a caller mismatch (id 5 with sub-input "9.1") never orphans + // sub-inputs. Non-array inputs / non-dotted ids pass through unchanged. + const rebaseInputs = (field, parentId) => { + if (!Array.isArray(field?.inputs)) { + return field; + } + const inputs = field.inputs.map((input) => { + const hasDottedId = input && typeof input.id === 'string' && input.id.includes('.'); + if (!hasDottedId) { + return input; + } + const sub = input.id.slice(input.id.indexOf('.') + 1); + return { ...input, id: `${parentId}.${sub}` }; + }); + return { ...field, inputs }; + }; + + // Only SAFE positive integers count as explicit ids. Number.isInteger is true + // for values like 1e308 where `next++` can never advance (1e308 + 1 === 1e308), + // which would hang the reassignment loop โ€” treat those as unset and give them a + // small sequential id instead. + let next = 1; for (const field of fields) { const id = Number(field?.id); - if (Number.isInteger(id) && id > 0) { - used.add(id); + if (Number.isSafeInteger(id) && id > 0 && id >= next) { + next = id + 1; } } - let next = (used.size ? Math.max(...used) : 0) + 1; - + // Keep the FIRST occurrence of each explicit id; reassign fields with no id OR + // a duplicate explicit id, so the result never contains colliding field ids. + const claimed = new Set(); return fields.map((field) => { const id = Number(field?.id); - if (Number.isInteger(id) && id > 0) { - return field; + const hasFreshExplicitId = Number.isSafeInteger(id) && id > 0 && !claimed.has(id); + if (hasFreshExplicitId) { + claimed.add(id); + // Id preserved, but sub-inputs may still reference a stale parent โ€” rebase. + return rebaseInputs(field, id); } - while (used.has(next)) { + while (claimed.has(next)) { next++; } const newId = next++; - used.add(newId); - - // Re-base any provided compound sub-input ids onto the new field id. - if (Array.isArray(field?.inputs)) { - const inputs = field.inputs.map((input) => { - const hasDottedId = input && typeof input.id === 'string' && input.id.includes('.'); - if (hasDottedId) { - const sub = input.id.slice(input.id.indexOf('.') + 1); - return { ...input, id: `${newId}.${sub}` }; - } - return input; - }); - return { ...field, id: newId, inputs }; - } + claimed.add(newId); - return { ...field, id: newId }; + return rebaseInputs({ ...field, id: newId }, newId); }); } @@ -1059,6 +1079,9 @@ export function getCompoundFieldInputs(fieldType) { * @returns {array|null} Array of input definitions or null if not a compound field. */ export function generateCompoundInputs(field) { + if (!field || typeof field !== 'object') { + return null; + } const fieldDef = fieldRegistry[field.type]; if (!fieldDef || !fieldDef.isCompound) { diff --git a/src/field-operations/field-dependencies.js b/src/field-operations/field-dependencies.js index 83ee456..dfd4170 100644 --- a/src/field-operations/field-dependencies.js +++ b/src/field-operations/field-dependencies.js @@ -18,6 +18,10 @@ export class DependencyTracker { dynamicPopulation: [] }; + if (!form || typeof form !== 'object') { + return dependencies; + } + // 1. Check conditional logic dependencies in all fields this.scanConditionalLogic(form, fieldId, dependencies); @@ -260,10 +264,13 @@ export class DependencyTracker { * Check if dependencies would break form functionality */ hasBreakingDependencies(dependencies) { + if (!dependencies || typeof dependencies !== 'object') { + return false; + } return ( - dependencies.conditionalLogic.length > 0 || - dependencies.calculations.length > 0 || - dependencies.mergeTags.length > 0 + (dependencies.conditionalLogic?.length > 0) || + (dependencies.calculations?.length > 0) || + (dependencies.mergeTags?.length > 0) ); } diff --git a/src/field-operations/field-manager.js b/src/field-operations/field-manager.js index ac5d604..b1b039d 100644 --- a/src/field-operations/field-manager.js +++ b/src/field-operations/field-manager.js @@ -27,48 +27,71 @@ export class FieldManager { * @returns {object} Field creation result with warnings */ async addField(formId, fieldType, properties = {}, position = {}) { - // 1. Validate field type against registry - const fieldDef = this.registry[fieldType]; - if (!fieldDef) { - throw new Error(`Unknown field type: ${fieldType}`); + if (typeof fieldType !== 'string' || fieldType.trim() === '') { + throw new Error('field_type is required and must be a non-empty string'); } + // Parameter defaults only cover `undefined`; coerce an explicit null so + // adversarial input can't crash createField or the position engine. + properties = properties || {}; + position = position || {}; - // 2. Fetch current form via REST API + // The registry is an ENHANCEMENT source, not a gate. Known types get + // type-specific defaults and sub-inputs. Unknown types (third-party add-ons, + // GravityKit, custom fields) are still created (Gravity Forms accepts them on + // save), just without those extras. Callers can pass `inputs`/`choices` + // explicitly for custom compound/choice fields. + const fieldDef = this.registry[fieldType] || null; + const isKnownType = fieldDef !== null; + + // Fetch current form via REST API const { form } = await this.api.getForm({ id: formId }); - // 3. Generate unique integer field ID (max + 1 pattern) + // Generate unique integer field ID (max + 1 pattern) const fieldId = this.generateFieldId(form.fields || []); - // 4. Create field with type-specific defaults - const field = this.createField(fieldId, fieldType, properties, fieldDef); - - // 5. Generate compound sub-inputs if needed (address.1, name.3, etc.) - if (fieldDef.storage?.type === 'compound') { + // Create field with type-specific defaults (none for unknown types) + const field = this.createField(fieldId, fieldType, properties, fieldDef || {}); + + // Known compound types (address, name, โ€ฆ) regenerate sub-inputs from the + // registry, keyed off the generated field id. Otherwise caller-supplied + // `inputs` are kept, but their dotted sub-input ids are rebased onto the + // generated field id so the parent reference matches (mirrors assignFieldIds). + const isCompoundType = fieldDef?.storage?.type === 'compound'; + if (isCompoundType) { field.inputs = this.generateSubInputs(field, fieldDef); + } else if (Array.isArray(field.inputs)) { + field.inputs = this.rebaseSubInputIds(field.inputs, fieldId); } - // 5b. Normalize layout grid properties (layoutGroupId, layoutGridColumnSpan) + // Normalize layout grid properties (layoutGroupId, layoutGridColumnSpan) this.normalizeLayoutProperties(field, formId); - // 6. Calculate insertion position (page-aware) + // Calculate insertion position (page-aware) const insertIndex = this.positionEngine?.calculatePosition( form.fields || [], position, form.pagination ) || form.fields?.length || 0; - // 7. Insert field at calculated position + // Insert field at calculated position if (!form.fields) form.fields = []; form.fields.splice(insertIndex, 0, field); - // 8. Replace form via direct PUT (no re-fetch โ€” we already have the full state) - const updatedForm = await this.api.replaceForm(formId, form); - - // 9. Return result with validation warnings + // Replace form via direct PUT (no re-fetch; we already have the full state) + await this.api.replaceForm(formId, form); + + // Surface field-shape warnings, plus a heads-up when the type is unrecognized. + const warnings = this.validator.getWarnings(field); + if (!isKnownType) { + warnings.unshift( + `Field type '${fieldType}' is not in the known field registry; created without type-specific defaults or sub-inputs. Pass 'inputs'/'choices' explicitly if this type needs them.` + ); + } + return { success: true, field: field, - warnings: this.validator.getWarnings(field), + warnings, form_id: formId, position: { index: insertIndex, @@ -80,31 +103,46 @@ export class FieldManager { /** * Update existing field with dependency checking */ - async updateField(formId, fieldId, updates = {}) { + async updateField(formId, fieldId, updates = {}, options = {}) { + const { force = false } = options; + // Fetch form const { form } = await this.api.getForm({ id: formId }); - + // Find field const fieldIndex = form.fields?.findIndex(f => f.id == fieldId); - if (fieldIndex === -1) { + if (fieldIndex === undefined || fieldIndex === -1) { throw new Error(`Field ${fieldId} not found in form ${formId}`); } - - // Check dependencies + + // Gate the write on dependencies BEFORE mutating, the way deleteField does. + // Saving first and reporting failure afterward persisted a "blocked" update + // and made the success:false response a lie. const dependencies = this.dependencyTracker?.scanFormDependencies(form, fieldId) || {}; - + const hasBreakingDeps = dependencies.conditionalLogic?.length > 0; + + if (hasBreakingDeps && !force) { + return { + success: false, + error: 'Field has dependencies that may be affected', + field_id: fieldId, + dependencies, + suggestion: 'Use force=true to update anyway' + }; + } + // Apply updates const originalField = { ...form.fields[fieldIndex] }; form.fields[fieldIndex] = { ...originalField, - ...updates, + ...(updates || {}), id: originalField.id // Preserve ID }; this.normalizeLayoutProperties(form.fields[fieldIndex], formId); - // Replace form via direct PUT (no re-fetch โ€” we already have the full state) + // Replace form via direct PUT (no re-fetch; we already have the full state) const result = await this.api.replaceForm(formId, form); - + return { success: true, field: result.form.fields[fieldIndex], @@ -113,8 +151,7 @@ export class FieldManager { after: result.form.fields[fieldIndex] }, warnings: { - dependencies: dependencies.conditionalLogic?.length > 0 ? - ['Field has conditional logic dependencies'] : [], + dependencies: hasBreakingDeps ? ['Field has conditional logic dependencies'] : [], validationIssues: this.validator.getWarnings(result.form.fields[fieldIndex]) } }; @@ -191,6 +228,26 @@ export class FieldManager { return maxId + 1; } + /** + * Rebase dotted sub-input ids (e.g. "9.1") onto a new parent field id so each + * sub-input's parent reference matches the field it belongs to. Non-dotted and + * non-string ids pass through. Mirrors assignFieldIds in the field registry. + * + * @param {Array} inputs + * @param {number|string} baseId + * @returns {Array} + */ + rebaseSubInputIds(inputs, baseId) { + return inputs.map((input) => { + const hasDottedId = input && typeof input.id === 'string' && input.id.includes('.'); + if (!hasDottedId) { + return input; + } + const sub = input.id.slice(input.id.indexOf('.') + 1); + return { ...input, id: `${baseId}.${sub}` }; + }); + } + /** * Create field with intelligent defaults from registry */ diff --git a/src/field-operations/index.js b/src/field-operations/index.js index ac40a79..dc473e2 100644 --- a/src/field-operations/index.js +++ b/src/field-operations/index.js @@ -72,26 +72,9 @@ export const fieldOperationHandlers = { async gf_update_field(params, { fieldManager }) { const { form_id, field_id, properties, force = false } = params; - const result = await fieldManager.updateField( - form_id, - field_id, - properties - ); - - // Check for breaking changes if not forced - if (!force && result.warnings?.dependencies?.length > 0) { - return { - success: false, - error: 'Field has dependencies that may be affected', - ...result, - suggestion: 'Use force=true to update anyway' - }; - } - - return { - success: true, - ...result - }; + // updateField gates the write on dependencies and returns the final + // success/failure shape; it no longer saves before reporting a block. + return fieldManager.updateField(form_id, field_id, properties, { force }); }, /** diff --git a/src/gravity-forms-client.js b/src/gravity-forms-client.js index e706201..159a9ab 100644 --- a/src/gravity-forms-client.js +++ b/src/gravity-forms-client.js @@ -91,10 +91,17 @@ export class GravityFormsClient { this.authManager = new AuthManager(this.config); this.baseURL = `${this.config.GRAVITY_FORMS_BASE_URL}/wp-json/gf/v2`; + // Allow self-signed certs when EITHER flag is explicitly 'true'. Compare each + // flag on its own โ€” `A || B` short-circuits on a truthy string like 'false', + // which would let one flag mask the other. + const allowSelfSignedCerts = + this.config.GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS === 'true' || + this.config.MCP_ALLOW_SELF_SIGNED_CERTS === 'true'; + // Initialize HTTP client with Basic Auth as primary method this.httpClient = axios.create({ baseURL: this.baseURL, - timeout: parseInt(config.GRAVITY_FORMS_TIMEOUT) || 30000, + timeout: parseInt(this.config.GRAVITY_FORMS_TIMEOUT, 10) || 30000, headers: { 'User-Agent': USER_AGENT, 'Accept': 'application/json' @@ -113,7 +120,7 @@ export class GravityFormsClient { // Allow self-signed certificates for local development // Set GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS=true in .env for local dev environments httpsAgent: new https.Agent({ - rejectUnauthorized: (config.GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS || config.MCP_ALLOW_SELF_SIGNED_CERTS) !== 'true' + rejectUnauthorized: !allowSelfSignedCerts }) }); diff --git a/src/server-runtime.js b/src/server-runtime.js index 4cc963c..4a46c9f 100644 --- a/src/server-runtime.js +++ b/src/server-runtime.js @@ -29,7 +29,7 @@ export function buildToolList({ gfReady, gfToolDefs = [], fieldOpTools = [], abi ...(gfReady ? [...gfToolDefs, ...fieldOpTools] : []), ...(abilityDefs ?? []), gkReloadDef, - ]; + ].filter(Boolean); } /** diff --git a/src/utils/compact.js b/src/utils/compact.js index 5b2aefa..0e877a3 100644 --- a/src/utils/compact.js +++ b/src/utils/compact.js @@ -12,15 +12,19 @@ * @param {*} obj - Value to compact * @returns {*} Compacted value */ -export function stripEmpty(obj) { +export function stripEmpty(obj, seen = new WeakSet()) { if (Array.isArray(obj)) { - return obj.map(stripEmpty); + if (seen.has(obj)) return obj; + seen.add(obj); + return obj.map((v) => stripEmpty(v, seen)); } if (obj !== null && typeof obj === 'object') { + if (seen.has(obj)) return obj; + seen.add(obj); const result = {}; for (const [key, value] of Object.entries(obj)) { if (value === null || value === '') continue; - result[key] = stripEmpty(value); + result[key] = stripEmpty(value, seen); } return result; } @@ -53,6 +57,9 @@ function isFieldKey(key) { * @returns {object} Entry with only core + field keys */ export function stripEntryMeta(entry) { + if (!entry || typeof entry !== 'object') { + return {}; + } const result = {}; for (const [key, value] of Object.entries(entry)) { if (CORE_ENTRY_KEYS.has(key) || isFieldKey(key)) { diff --git a/src/utils/sanitize.js b/src/utils/sanitize.js index 0eae0f8..d1a2e58 100644 --- a/src/utils/sanitize.js +++ b/src/utils/sanitize.js @@ -23,7 +23,7 @@ const SENSITIVE_KEYS = [ 'consumer_key', 'consumer_secret', 'authorization', 'auth', 'credential', 'oauth_signature', 'bearer', - 'credit_card', 'cvv', 'ssn' + 'credit_card', 'cvv', 'ssn', 'cookie' ]; /** @@ -43,11 +43,15 @@ function mask(value) { /** * Sanitize an object for logging */ -export function sanitize(obj) { +export function sanitize(obj, seen = new WeakSet()) { if (!obj || typeof obj !== 'object') return obj; + // Cut cycles so logging a self-referential object can never stack-overflow. + if (seen.has(obj)) return Array.isArray(obj) ? [] : {}; + seen.add(obj); + if (Array.isArray(obj)) { - return obj.map(sanitize); + return obj.map((v) => sanitize(v, seen)); } const result = {}; @@ -58,7 +62,7 @@ export function sanitize(obj) { if (isSensitive) { result[key] = mask(value); } else if (typeof value === 'object' && value !== null) { - result[key] = sanitize(value); + result[key] = sanitize(value, seen); } else { result[key] = value; } @@ -98,7 +102,7 @@ export function sanitizeHeaders(headers) { for (const [key, value] of Object.entries(headers)) { const keyLower = key.toLowerCase(); - if (keyLower === 'authorization' || keyLower.includes('api-key')) { + if (keyLower === 'authorization' || keyLower.includes('api-key') || keyLower.includes('cookie')) { result[key] = mask(String(value)); } else { result[key] = value; diff --git a/src/wp-client.js b/src/wp-client.js index adaf366..4a1d412 100644 --- a/src/wp-client.js +++ b/src/wp-client.js @@ -64,7 +64,11 @@ export class WordPressClient { } this.basicAuth = 'Basic ' + Buffer.from(`${username}:${password}`).toString('base64'); - this.allowSelfSigned = (this.config.GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS || this.config.MCP_ALLOW_SELF_SIGNED_CERTS) === 'true'; + // Compare each flag on its own: `A || B` short-circuits on a truthy string + // like 'false', so one flag could otherwise mask the other. + this.allowSelfSigned = + this.config.GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS === 'true' || + this.config.MCP_ALLOW_SELF_SIGNED_CERTS === 'true'; this.timeoutMs = parseInt(this.config.GRAVITYKIT_TIMEOUT || this.config.GRAVITY_FORMS_TIMEOUT, 10) || 30000; // Rooted at the WP install. Subclasses may replace this with a diff --git a/test/abilities-loader.test.js b/test/abilities-loader.test.js index 9c9a1f5..f6ee09f 100644 --- a/test/abilities-loader.test.js +++ b/test/abilities-loader.test.js @@ -677,6 +677,11 @@ suite.test('methodForAbility: readonly โ†’ GET, destructive+idempotent โ†’ DELET TestAssert.equal(methodForAbility(), 'POST'); }); +suite.test('methodForAbility: null / non-object annotations โ†’ POST (no throw)', () => { + TestAssert.equal(methodForAbility(null), 'POST'); + TestAssert.equal(methodForAbility('nope'), 'POST'); +}); + // Standalone runner const isMain = process.argv[1] && import.meta.url.endsWith(process.argv[1].replace(/.*\//, '')); if (isMain) { diff --git a/test/client-hardening.test.js b/test/client-hardening.test.js index de92e28..450336e 100644 --- a/test/client-hardening.test.js +++ b/test/client-hardening.test.js @@ -36,6 +36,31 @@ function makeClient() { return new GravityFormsClient(ENV); } +// In test mode, resolveEnv() remaps GRAVITY_FORMS_TEST_TIMEOUT โ†’ GRAVITY_FORMS_TIMEOUT +// on this.config; the constructor must read the timeout from the RESOLVED config, +// not the raw one, or the test-site override is silently ignored. +test('test-mode honors GRAVITY_FORMS_TEST_TIMEOUT', () => { + const c = new GravityFormsClient({ + GRAVITYKIT_MCP_TEST_MODE: 'true', + GRAVITY_FORMS_TEST_BASE_URL: 'https://t.test', + GRAVITY_FORMS_TEST_CONSUMER_KEY: 'wp_user', + GRAVITY_FORMS_TEST_CONSUMER_SECRET: 'app pass word', + GRAVITY_FORMS_TEST_TIMEOUT: '5000', + }); + assert.strictEqual(c.httpClient.defaults.timeout, 5000); +}); + +// Either ALLOW_SELF_SIGNED flag must enable self-signed independently. The old +// `(A || B) !== 'true'` short-circuited on a truthy string like "false", so a +// GRAVITY_FORMS flag of "false" masked an MCP flag of "true". +test('self-signed certs: either flag enables independently (no masking)', () => { + const base = { GRAVITY_FORMS_BASE_URL: 'https://x', GRAVITY_FORMS_CONSUMER_KEY: 'u', GRAVITY_FORMS_CONSUMER_SECRET: 'p', GRAVITY_FORMS_AUTH_METHOD: 'basic' }; + const reject = (cfg) => new GravityFormsClient({ ...base, ...cfg }).httpClient.defaults.httpsAgent.options.rejectUnauthorized; + assert.strictEqual(reject({ GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS: 'false', MCP_ALLOW_SELF_SIGNED_CERTS: 'true' }), false); + assert.strictEqual(reject({ GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS: 'true' }), false); + assert.strictEqual(reject({}), true); +}); + // Build a fake axios-style error: a rejection whose .response carries the // GF body and status โ€” exactly what axios exposes before any interceptor. function gfError(status, data) { diff --git a/test/compact.test.js b/test/compact.test.js index 861d7a1..3bd0795 100644 --- a/test/compact.test.js +++ b/test/compact.test.js @@ -12,6 +12,18 @@ import { TestRunner, TestAssert } from './helpers.js'; const runner = new TestRunner('Compact Utility Tests'); +runner.test('stripEntryMeta: null input returns {} without throwing', () => { + TestAssert.deepEqual(stripEntryMeta(null), {}); +}); + +runner.test('stripEmpty: terminates on a circular reference', () => { + const o = { a: 1, b: '' }; + o.self = o; + const out = stripEmpty(o); // must not throw RangeError + TestAssert.equal(out.a, 1, 'real value kept'); + TestAssert.equal(out.b, undefined, 'empty string still stripped'); +}); + // --- stripEmpty: basic value handling --- runner.test('stripEmpty: returns primitives unchanged', () => { diff --git a/test/field-dependencies.test.js b/test/field-dependencies.test.js index 275ca59..64ba150 100644 --- a/test/field-dependencies.test.js +++ b/test/field-dependencies.test.js @@ -7,6 +7,21 @@ import test from 'node:test'; import assert from 'node:assert'; import { DependencyTracker } from '../src/field-operations/field-dependencies.js'; +test('DependencyTracker null-input guards (no TypeError)', async (t) => { + const dt = new DependencyTracker(); + await t.test('scanFormDependencies(null) returns empty deps, no throw', () => { + assert.deepStrictEqual(dt.scanFormDependencies(null, 1), { + conditionalLogic: [], calculations: [], mergeTags: [], dynamicPopulation: [] + }); + }); + await t.test('hasBreakingDependencies(null) โ†’ false, no throw', () => { + assert.strictEqual(dt.hasBreakingDependencies(null), false); + }); + await t.test('hasBreakingDependencies({}) โ†’ false, no throw', () => { + assert.strictEqual(dt.hasBreakingDependencies({}), false); + }); +}); + // Sample form with various dependencies const createTestForm = () => ({ id: 1, diff --git a/test/field-manager.test.js b/test/field-manager.test.js index 80a7810..8285e90 100644 --- a/test/field-manager.test.js +++ b/test/field-manager.test.js @@ -6,6 +6,7 @@ import test from 'node:test'; import assert from 'node:assert'; import { FieldManager } from '../src/field-operations/field-manager.js'; +import { PositionEngine } from '../src/field-operations/field-positioner.js'; import FieldAwareValidator from '../src/config/field-validation.js'; // Mock dependencies. Mirrors the GravityFormsClient contract FieldManager @@ -225,15 +226,74 @@ test('FieldManager - addField', async (t) => { assert.strictEqual(result.position.index, 3); }); - await t.test('rejects unknown field type', async () => { + // Custom / third-party field types (Gravity Perks, add-ons, GravityKit, the + // MailPot field a customer hit) are not in the static registry, yet Gravity + // Forms accepts them on the form PUT. addField must not gate on the registry; + // it degrades gracefully: create from caller properties, skip registry-derived + // defaults/sub-inputs, and warn. + await t.test('accepts an unknown field type instead of throwing', async () => { const apiClient = createMockApiClient(); const registry = createMockRegistry(); const validator = createMockValidator(); const manager = new FieldManager(apiClient, registry, validator); - await assert.rejects( - async () => await manager.addField(1, 'unknown_type', {}), - /Unknown field type: unknown_type/ + const result = await manager.addField(1, 'mailpot_custom', { label: 'Custom Field' }); + + assert.strictEqual(result.success, true); + assert.strictEqual(result.field.type, 'mailpot_custom'); + assert.strictEqual(result.field.label, 'Custom Field'); + assert.strictEqual(result.field.id, 4); // Next ID after 1,2,3 + }); + + await t.test('warns when the field type is not in the registry', async () => { + const apiClient = createMockApiClient(); + const registry = createMockRegistry(); + const validator = createMockValidator(); + const manager = new FieldManager(apiClient, registry, validator); + + const result = await manager.addField(1, 'mailpot_custom', { label: 'Custom Field' }); + + assert.ok(Array.isArray(result.warnings)); + assert.ok( + result.warnings.some((m) => /mailpot_custom/.test(m) && /registr/i.test(m)), + 'expected a warning naming the unrecognized field type' + ); + }); + + await t.test('rebases caller-supplied dotted sub-input ids onto the generated field id (unknown type)', async () => { + const apiClient = createMockApiClient(); + const registry = createMockRegistry(); + const validator = createMockValidator(); + const manager = new FieldManager(apiClient, registry, validator); + + // The caller guessed parent id 9, but the form's next id is 4. Sub-inputs + // must follow the generated field id (4.x), not stay orphaned at 9.x. Labels + // and other props are preserved. + const result = await manager.addField(1, 'custom_compound', { + label: 'Custom Compound', + inputs: [{ id: '9.1', label: 'Part A' }, { id: '9.2', label: 'Part B' }] + }); + + assert.strictEqual(result.success, true); + assert.strictEqual(result.field.id, 4); + assert.deepStrictEqual(result.field.inputs, [ + { id: '4.1', label: 'Part A' }, + { id: '4.2', label: 'Part B' } + ]); + }); + + await t.test('known compound type still gets registry sub-inputs keyed on the generated field id', async () => { + const apiClient = createMockApiClient(); + const registry = createMockRegistry(); + const validator = createMockValidator(); + const manager = new FieldManager(apiClient, registry, validator); + + const result = await manager.addField(1, 'address', { label: 'Mailing Address' }); + + assert.strictEqual(result.field.id, 4); + assert.deepStrictEqual( + result.field.inputs.map((i) => i.id), + ['4.1', '4.2', '4.3', '4.4', '4.5', '4.6'] ); }); }); @@ -262,23 +322,38 @@ test('FieldManager - updateField', async (t) => { assert.strictEqual(result.field.id, 2); // ID preserved }); - await t.test('warns about dependencies', async () => { + // A blocking dependency must be detected BEFORE the form is written. The old + // code saved the change, then the handler returned success:false โ€” so a + // "blocked" update was already persisted and the response lied. + await t.test('does not persist and reports failure when a dependency would break and force is false', async () => { const apiClient = createMockApiClient(); - const registry = createMockRegistry(); - const validator = createMockValidator(); - const manager = new FieldManager(apiClient, registry, validator); - - // Mock dependency tracker with dependencies + let saved = false; + apiClient.replaceForm = async (id, form) => { saved = true; return { form }; }; + const manager = new FieldManager(apiClient, createMockRegistry(), createMockValidator()); manager.dependencyTracker = { - scanFormDependencies: () => ({ - conditionalLogic: [{ field_id: 1, field_label: 'Name' }] - }) + scanFormDependencies: () => ({ conditionalLogic: [{ field_id: 1, field_label: 'Name' }] }) }; - const result = await manager.updateField(1, 2, { label: 'Updated' }); - + const result = await manager.updateField(1, 2, { label: 'Updated' }, { force: false }); + + assert.strictEqual(result.success, false); + assert.strictEqual(saved, false, 'must NOT persist the change when blocked'); + assert.match(result.suggestion || '', /force/); + }); + + await t.test('persists when force is true despite dependencies', async () => { + const apiClient = createMockApiClient(); + let saved = false; + apiClient.replaceForm = async (id, form) => { saved = true; return { form }; }; + const manager = new FieldManager(apiClient, createMockRegistry(), createMockValidator()); + manager.dependencyTracker = { + scanFormDependencies: () => ({ conditionalLogic: [{ field_id: 1, field_label: 'Name' }] }) + }; + + const result = await manager.updateField(1, 2, { label: 'Updated' }, { force: true }); + assert.strictEqual(result.success, true); - assert.ok(result.warnings.dependencies.length > 0); + assert.strictEqual(saved, true); }); await t.test('throws for non-existent field', async () => { @@ -468,3 +543,32 @@ test('FieldAwareValidator.getWarnings', async (t) => { assert.deepStrictEqual(v.getWarnings('nope'), []); }); }); + +test('FieldManager - addField hardening (adversarial input)', async (t) => { + const mk = () => new FieldManager(createMockApiClient(), createMockRegistry(), createMockValidator()); + + await t.test('does not crash when properties is null', async () => { + const result = await mk().addField(1, 'text', null); + assert.strictEqual(result.success, true); + assert.strictEqual(result.field.type, 'text'); + }); + + await t.test('does not crash when position is null (real PositionEngine)', async () => { + const m = mk(); + m.positionEngine = new PositionEngine(); + const result = await m.addField(1, 'text', { label: 'X' }, null); + assert.strictEqual(result.success, true); + }); + + await t.test('rejects an empty-string field_type', async () => { + await assert.rejects(() => mk().addField(1, '', { label: 'X' }), /field_type/); + }); + + await t.test('rejects a null field_type', async () => { + await assert.rejects(() => mk().addField(1, null, { label: 'X' }), /field_type/); + }); + + await t.test('rejects a non-string field_type', async () => { + await assert.rejects(() => mk().addField(1, 123, { label: 'X' }), /field_type/); + }); +}); diff --git a/test/field-operations-integration.test.js b/test/field-operations-integration.test.js index 2148c5a..849ecf9 100644 --- a/test/field-operations-integration.test.js +++ b/test/field-operations-integration.test.js @@ -3,7 +3,7 @@ * Tests the complete flow from MCP tool calls to API interactions */ -import { fieldOperationHandlers } from '../src/field-operations/index.js'; +import { fieldOperationHandlers, createFieldOperations } from '../src/field-operations/index.js'; import { testConfig, TestFormManager } from '../src/config/test-config.js'; import GravityFormsClient from '../src/gravity-forms-client.js'; import fieldRegistry from '../src/field-definitions/field-registry.js'; @@ -43,100 +43,12 @@ async function setup() { // Create test form manager testFormManager = new TestFormManager(apiClient, testConfig); - // Initialize field operations + // Exercise the REAL field-operations layer (FieldManager plus the dependency + // and position engines), not a hand-rolled reimplementation. A local mock + // drifts from production behavior; createFieldOperations is exactly what + // src/index.js wires up, so this test catches real regressions. const validator = new FieldAwareValidator(); - fieldOperations = { - fieldManager: { - addField: async (formId, fieldType, properties, position) => { - // Mock field manager for integration test - const form = await apiClient.getForm(formId); - const nextId = Math.max(...form.fields.map(f => parseInt(f.id) || 0), 0) + 1; - - const fieldDef = fieldRegistry[fieldType]; - if (!fieldDef) { - throw new Error(`Unknown field type: ${fieldType}`); - } - - const field = { - id: nextId, - type: fieldType, - label: properties.label || fieldDef.label || 'Untitled', - isRequired: properties.isRequired || false, - ...properties - }; - - // Add field to form - form.fields.push(field); - await apiClient.updateForm(form); - - return { - success: true, - field, - form_id: formId, - position: { index: form.fields.length - 1 } - }; - }, - updateField: async (formId, fieldId, properties) => { - const form = await apiClient.getForm(formId); - const fieldIndex = form.fields.findIndex(f => f.id == fieldId); - - if (fieldIndex === -1) { - throw new Error(`Field ${fieldId} not found`); - } - - const originalField = { ...form.fields[fieldIndex] }; - form.fields[fieldIndex] = { - ...originalField, - ...properties, - id: originalField.id - }; - - await apiClient.updateForm(form); - - return { - success: true, - field: form.fields[fieldIndex], - changes: { - before: originalField, - after: form.fields[fieldIndex] - }, - warnings: { dependencies: [] } - }; - }, - deleteField: async (formId, fieldId, options = {}) => { - const form = await apiClient.getForm(formId); - const field = form.fields.find(f => f.id == fieldId); - - if (!field) { - throw new Error(`Field ${fieldId} not found`); - } - - // Remove field - form.fields = form.fields.filter(f => f.id != fieldId); - await apiClient.updateForm(form); - - return { - success: true, - deleted_field: { - id: field.id, - type: field.type, - label: field.label - }, - dependencies: {}, - actions_taken: [] - }; - } - }, - dependencyTracker: { - scanFormDependencies: () => ({ conditionalLogic: [] }), - hasBreakingDependencies: () => false - }, - positionEngine: { - calculatePosition: (fields, config) => fields.length - }, - config: testConfig, - fieldRegistry - }; + fieldOperations = createFieldOperations(apiClient, fieldRegistry, validator); console.log('โœ… Test environment initialized'); } catch (error) { @@ -203,7 +115,7 @@ async function testAddField() { // Verify field was actually added const updatedForm = await apiClient.getForm(testForm.id); const addedField = updatedForm.fields.find(f => f.label === 'Comments'); - + if (addedField) { console.log(' โœ… Field verified in form'); return true; @@ -254,7 +166,7 @@ async function testUpdateField() { // Verify changes const updatedForm = await apiClient.getForm(testForm.id); const updatedField = updatedForm.fields.find(f => f.id == fieldToUpdate.id); - + if (updatedField && updatedField.label === 'Email Address') { console.log(' โœ… Update verified in form'); return true; @@ -275,30 +187,30 @@ async function testListFieldTypes() { console.log('\n๐Ÿ”ง Testing gf_list_field_types...'); try { + // gf_list_field_types returns { field_types, total } (or { error }) โ€” no + // success/categories envelope. const result = await fieldOperationHandlers.gf_list_field_types({ - category: 'standard', - include_variants: false + category: 'standard' }, fieldOperations); - if (result.success) { - console.log(` โœ… Listed ${result.total} field types`); - console.log(` ๐Ÿ“‹ Categories: ${result.categories.join(', ')}`); - - // Check for expected field types - const hasText = result.field_types.some(f => f.type === 'text'); - const hasEmail = result.field_types.some(f => f.type === 'email'); - - if (hasText && hasEmail) { - console.log(' โœ… Expected field types found'); - return true; - } else { - console.log(' โŒ Missing expected field types'); - return false; - } - } else { + if (result.error) { console.log(` โŒ Failed: ${result.error}`); return false; } + + console.log(` โœ… Listed ${result.total} field types`); + + // Check for expected field types + const hasText = result.field_types.some(f => f.type === 'text'); + const hasEmail = result.field_types.some(f => f.type === 'email'); + + if (hasText && hasEmail) { + console.log(' โœ… Expected field types found'); + return true; + } else { + console.log(' โŒ Missing expected field types'); + return false; + } } catch (error) { console.log(` โŒ Error: ${error.message}`); return false; @@ -337,7 +249,7 @@ async function testDeleteField() { // Verify field was deleted const updatedForm = await apiClient.getForm(testForm.id); const deletedField = updatedForm.fields.find(f => f.id == fieldToDelete.id); - + if (!deletedField) { console.log(' โœ… Deletion verified in form'); return true; @@ -375,7 +287,7 @@ async function runTests() { await createTestForm(); const results = []; - + // Run all tests results.push(await testAddField()); results.push(await testUpdateField()); @@ -387,7 +299,7 @@ async function runTests() { // Report results const passed = results.filter(r => r).length; const total = results.length; - + console.log('\n๐Ÿ“Š Integration Test Results:'); console.log(` โœ… Passed: ${passed}/${total}`); console.log(` โŒ Failed: ${total - passed}/${total}`); @@ -409,4 +321,4 @@ process.on('SIGTERM', cleanup); runTests().catch(error => { console.error('\n๐Ÿ’ฅ Test runner error:', error); cleanup().finally(() => process.exit(1)); -}); \ No newline at end of file +}); diff --git a/test/field-registry.test.js b/test/field-registry.test.js index a735b19..4a940c6 100644 --- a/test/field-registry.test.js +++ b/test/field-registry.test.js @@ -4,7 +4,7 @@ import test from 'node:test'; import assert from 'node:assert'; -import { generateCompoundInputs, isCompoundField, getFieldDefinition, assignFieldIds } from '../src/field-definitions/field-registry.js'; +import { generateCompoundInputs, isCompoundField, getFieldDefinition, assignFieldIds, validateFieldConfig, detectFieldVariant } from '../src/field-definitions/field-registry.js'; test('assignFieldIds', async (t) => { await t.test('assigns sequential ids when none are provided', () => { @@ -163,6 +163,65 @@ test('generateCompoundInputs - non-compound fields', async (t) => { }); }); +test('validateFieldConfig', async (t) => { + // Unknown / third-party types are tolerated, not rejected: consistent with + // FieldManager.addField and the form-create validator. There is no config + // schema for a type we don't know, so there is nothing to reject. + await t.test('tolerates an unknown field type', () => { + const result = validateFieldConfig({ id: 7, type: 'mailpot_custom', label: 'Custom' }); + assert.strictEqual(result.isValid, true); + }); + + await t.test('still rejects a known type missing required config (choice field without choices)', () => { + const result = validateFieldConfig({ id: 7, type: 'select', label: 'Pick' }); + assert.strictEqual(result.isValid, false); + assert.match(result.error, /choices/i); + }); + + await t.test('accepts a well-formed known field', () => { + const result = validateFieldConfig({ id: 7, type: 'text', label: 'Name' }); + assert.strictEqual(result.isValid, true); + }); +}); + +test('assignFieldIds rebases dotted sub-inputs to a PRESERVED explicit id', () => { + // A field can keep its explicit id but carry sub-inputs that reference a stale + // parent (caller mismatch). They must be rebased onto the final id, not just + // when a new id is allocated. + const out = assignFieldIds([{ id: 5, type: 'address', inputs: [{ id: '9.1' }, { id: '9.2' }] }]); + assert.strictEqual(out[0].id, 5); + assert.deepStrictEqual(out[0].inputs.map((i) => i.id), ['5.1', '5.2']); +}); + +test('assignFieldIds terminates and stays unique with an out-of-range (1e308) id', () => { + // 1e308 is Number.isInteger-true but 1e308 + 1 === 1e308, so the old + // max+1 / next++ loop could never advance โ€” an infinite-loop DoS. Such ids + // are treated as unset and reassigned a small sequential id. + const out = assignFieldIds([{ id: 1e308, type: 't' }, { type: 'e' }]); + const ids = out.map((f) => f.id); + assert.strictEqual(new Set(ids).size, ids.length, 'unique ids'); + assert.ok(ids.every((id) => Number.isSafeInteger(id) && id > 0), 'all ids are safe positive integers'); +}); + +test('assignFieldIds reassigns duplicate explicit ids so none collide', () => { + const out = assignFieldIds([{ id: 5, type: 't' }, { id: 5, type: 'e' }]); + const ids = out.map((f) => f.id); + assert.strictEqual(new Set(ids).size, ids.length, 'no duplicate field ids'); + assert.strictEqual(out[0].id, 5, 'first explicit id is preserved'); +}); + +test('field-registry null-input guards (no TypeError on hostile input)', async (t) => { + await t.test('validateFieldConfig(null) โ†’ {isValid:false}, no throw', () => { + assert.strictEqual(validateFieldConfig(null).isValid, false); + }); + await t.test('detectFieldVariant(null) โ†’ "default", no throw', () => { + assert.strictEqual(detectFieldVariant(null), 'default'); + }); + await t.test('generateCompoundInputs(null) โ†’ null, no throw', () => { + assert.strictEqual(generateCompoundInputs(null), null); + }); +}); + test('isCompoundField', async (t) => { await t.test('returns true for address', () => { assert.strictEqual(isCompoundField('address'), true); diff --git a/test/field-validation.test.js b/test/field-validation.test.js index 3105dc6..561ad59 100644 --- a/test/field-validation.test.js +++ b/test/field-validation.test.js @@ -451,7 +451,7 @@ test('Processes checkbox submission', () => { }); // Test 10: Unknown Field Types -test('Handles unknown field types gracefully', () => { +test('Handles unknown field types gracefully without leaking an internal flag', () => { const fields = [ { id: '1', @@ -462,9 +462,11 @@ test('Handles unknown field types gracefully', () => { const validated = FieldAwareValidator.validateFormFields(fields); assertEqual(validated.length, 1, 'Should allow unknown field types'); - // _unknown is intentionally retained (signals downstream code to handle gracefully) - // unlike _meta/_variant which are processing-only and stripped before return - assert(validated[0]._unknown === true, 'Should mark as unknown'); + assertEqual(validated[0].type, 'custom_field_type', 'Should preserve the field type'); + // The internal _unknown flag must NOT leak into the validated field: no + // production code reads it, and it was PUT verbatim to Gravity Forms. Unknown + // types are tolerated, not tagged. + assert(!('_unknown' in validated[0]), 'Should not leak the internal _unknown flag to Gravity Forms'); }); // Test 11: Validation Summary diff --git a/test/forms.test.js b/test/forms.test.js index 638286a..84f7dca 100644 --- a/test/forms.test.js +++ b/test/forms.test.js @@ -250,26 +250,32 @@ suite.test('Create Form: Should handle unicode and special characters', async () TestAssert.equal(result.form.title, unicodeForm.title); }); -suite.test('Create Form: Should handle unknown field type gracefully', async () => { - const invalidForm = { - title: 'Invalid Form', +suite.test('Create Form: Should accept an unknown field type without leaking _unknown to the API', async () => { + const customForm = { + title: 'Custom Field Form', fields: [ { id: 1, type: 'custom_field_type', label: 'Custom Field' } ] }; - // Mock should include the _unknown flag that would be added by field validation mockHttpClient.setMockResponse('POST', '/forms', new MockResponse({ - ...invalidForm, + ...customForm, id: 40, fields: [ - { id: 1, type: 'custom_field_type', label: 'Custom Field', _unknown: true } + { id: 1, type: 'custom_field_type', label: 'Custom Field' } ] })); - const result = await client.createForm(invalidForm); + mockHttpClient.clearRequests(); + const result = await client.createForm(customForm); + + // Unknown types are tolerated (no throw) and round-trip back to the caller. + TestAssert.equal(result.form.fields[0].type, 'custom_field_type'); - TestAssert.equal(result.form.fields[0]._unknown, true); + // The validated payload actually POSTed must not carry the internal _unknown flag. + const postReq = mockHttpClient.getRequests().find((r) => r.method === 'POST' && r.path === '/forms'); + TestAssert.exists(postReq, 'expected a POST /forms request'); + TestAssert.isFalse('_unknown' in postReq.config.data.fields[0], 'must not POST the internal _unknown flag'); }); // ================================= diff --git a/test/sanitize-hardening.test.js b/test/sanitize-hardening.test.js index cca41d6..ec3c08f 100644 --- a/test/sanitize-hardening.test.js +++ b/test/sanitize-hardening.test.js @@ -13,7 +13,34 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { sanitize, sanitizeUrl } from '../src/utils/sanitize.js'; +import { sanitize, sanitizeUrl, sanitizeHeaders } from '../src/utils/sanitize.js'; +import { isLocalUrl } from '../src/config/auth.js'; + +test('sanitizeHeaders masks a Cookie header (session credentials)', () => { + const out = sanitizeHeaders({ Cookie: 'wordpress_logged_in_x=secretsession12345' }); + assert.notEqual(out.Cookie, 'wordpress_logged_in_x=secretsession12345'); +}); + +test('sanitize masks a cookie-named key', () => { + const out = sanitize({ cookie: 'sessiondata1234567890' }); + assert.notEqual(out.cookie, 'sessiondata1234567890'); +}); + +test('isLocalUrl does not classify a remote 127.x domain as local', () => { + assert.equal(isLocalUrl('http://127.example.com'), false); + assert.equal(isLocalUrl('http://127.0.0.1.evil.com'), false); + assert.equal(isLocalUrl('http://127.0.0.1'), true); + assert.equal(isLocalUrl('http://localhost'), true); +}); + +test('sanitize terminates on a circular reference (no stack overflow)', () => { + const o = { token: 'abcdefghijklmnop' }; + o.self = o; + let out, err; + try { out = sanitize(o); } catch (e) { err = e; } + assert.ok(!err, 'must not throw on circular input'); + assert.notStrictEqual(out.token, 'abcdefghijklmnop', 'token still masked'); +}); // A value long enough that mask() returns the "abc****yz" form, so we can // assert the full value never survives anywhere. diff --git a/test/server-runtime.test.js b/test/server-runtime.test.js index 281a088..47d7045 100644 --- a/test/server-runtime.test.js +++ b/test/server-runtime.test.js @@ -7,6 +7,12 @@ import test from 'node:test'; import assert from 'node:assert'; import { runPlaneInit, buildToolList, classifyAbilityCall, resolveAbilitiesListTimeoutMs } from '../src/server-runtime.js'; +test('buildToolList: omits a missing gkReloadDef instead of emitting undefined', () => { + const list = buildToolList({ gfReady: false }); + assert.ok(!list.includes(undefined), 'tool list must not contain undefined'); + assert.ok(list.every((t) => t && typeof t.name === 'string'), 'every advertised tool has a name'); +}); + // --- runPlaneInit (#1: WP not blocked by a slow GF probe) --- test('runPlaneInit: WP plane initializes before a slow GF probe resolves', async () => { diff --git a/test/validation-hardening.test.js b/test/validation-hardening.test.js index c1238e3..36424c3 100644 --- a/test/validation-hardening.test.js +++ b/test/validation-hardening.test.js @@ -21,9 +21,42 @@ import { BaseValidator, } from '../src/config/validation.js'; import { buildEntriesQuery } from '../src/gravity-forms-client.js'; +import FieldAwareValidator from '../src/config/field-validation.js'; +import { formatErrorMessage } from '../src/config/validation-config.js'; + +test('validateURL accepts a dotless host (localhost/intranet) for confirmation redirects', () => { + assert.equal(BaseValidator.validateURL('https://localhost/thanks'), 'https://localhost/thanks'); + assert.equal(BaseValidator.validateURL('https://intranet/thanks'), 'https://intranet/thanks'); +}); + +test('validateURL still rejects non-http(s) schemes', () => { + assert.throws(() => BaseValidator.validateURL('javascript:alert(1)')); + assert.throws(() => BaseValidator.validateURL('file:///etc/passwd')); + assert.throws(() => BaseValidator.validateURL('https://has space/x')); +}); + +test('formatErrorMessage replaces every {field} and treats values literally', () => { + assert.equal(formatErrorMessage('{field} or {field}', 'X'), 'X or X'); + assert.equal(formatErrorMessage('{field} bad', 'a$&b'), 'a$&b bad'); +}); const validate = (tool, input) => ValidationFactory.validateToolInput(tool, input); +test('validateFormFields([null]) throws a clean validation error, not a TypeError', () => { + let err; + try { FieldAwareValidator.validateFormFields([null]); } catch (e) { err = e; } + assert.ok(err, 'should throw'); + assert.notStrictEqual(err.constructor.name, 'TypeError'); + assert.match(err.message, /must be an object/); +}); + +test('validateFieldFilter rejects a non-scalar (object) value instead of stringifying it', () => { + assert.throws( + () => BaseValidator.validateFieldFilter({ key: '1', operator: 'is', value: { a: 1 } }), + /value/ + ); +}); + // --------------------------------------------------------------------------- // Lax integer coercion is rejected (PositiveIntegerRule + BaseValidator.validateId) // GF types these as integers and 400s non-integer-formatted input. Only genuine diff --git a/test/wp-client.test.js b/test/wp-client.test.js index 2d287cb..445a2b3 100644 --- a/test/wp-client.test.js +++ b/test/wp-client.test.js @@ -11,6 +11,11 @@ import { WordPressClient } from '../src/wp-client.js'; const creds = { GRAVITYKIT_WP_USERNAME: 'admin', GRAVITYKIT_WP_APP_PASSWORD: 'pw' }; const make = (extra) => () => new WordPressClient({ ...creds, ...extra }); +test('self-signed certs: MCP flag enables it even when GF flag is the string "false"', () => { + const c = new WordPressClient({ ...creds, GRAVITYKIT_WP_URL: 'https://remote.example.com', GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS: 'false', MCP_ALLOW_SELF_SIGNED_CERTS: 'true' }); + assert.strictEqual(c.allowSelfSigned, true); +}); + test('allows HTTPS remote URLs', () => { assert.doesNotThrow(make({ GRAVITYKIT_WP_URL: 'https://remote.example.com' })); });