From fea09774aebf9ea5b5be8f1d0db7bcd43c1a35d3 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 20:48:12 -0400 Subject: [PATCH 01/12] fix(fields): accept custom and third-party field types in gf_add_field gf_add_field threw "Unknown field type" for any type not in the static 46-key registry, rejecting third-party fields (Gravity Perks, Gravity Wiz, etc.) that Gravity Forms itself accepts. The registry is now an enhancement, not a gate: unknown types are created with graceful degradation plus a warning. Also stop leaking the internal _unknown flag to Gravity Forms on form create/update, and align validateFieldConfig to tolerate unknown types. Convert the orphan field-operations-integration harness to exercise the real field-operations layer. Bump 2.4.0 -> 2.4.1. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- AGENTS.md | 2 +- CHANGELOG.md | 9 ++ mcp.json | 2 +- package.json | 2 +- src/config/field-validation.js | 6 +- src/field-definitions/field-registry.js | 10 +- src/field-operations/field-manager.js | 51 +++++--- test/field-manager.test.js | 48 ++++++- test/field-operations-integration.test.js | 148 +++++----------------- test/field-registry.test.js | 23 +++- test/field-validation.test.js | 10 +- test/forms.test.js | 22 ++-- 12 files changed, 168 insertions(+), 165 deletions(-) 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..fb2ae23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ 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. + +### ๐Ÿ› Fixed +- **`gf_add_field` now accepts custom and third-party field types.** Field types added by other plugins (Gravity Perks, Gravity Wiz, and similar add-ons) 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. +- **Unrecognized field types no longer leave an internal marker in saved forms** when creating or updating a form. + ## [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 +244,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/config/field-validation.js b/src/config/field-validation.js index 9834205..b07dcfc 100644 --- a/src/config/field-validation.js +++ b/src/config/field-validation.js @@ -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/field-definitions/field-registry.js b/src/field-definitions/field-registry.js index fdf91f0..6e3380f 100644 --- a/src/field-definitions/field-registry.js +++ b/src/field-definitions/field-registry.js @@ -928,12 +928,12 @@ export function detectFieldVariant(field) { */ export function validateFieldConfig(field) { 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 diff --git a/src/field-operations/field-manager.js b/src/field-operations/field-manager.js index ac5d604..0145794 100644 --- a/src/field-operations/field-manager.js +++ b/src/field-operations/field-manager.js @@ -27,48 +27,59 @@ 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}`); - } + // 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; - // 2. Fetch current form via REST API + // 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 || {}); + + // Generate compound sub-inputs only for known compound types (address.1, + // name.3, โ€ฆ). Unknown types keep any caller-supplied `inputs` untouched. + const isCompoundType = fieldDef?.storage?.type === 'compound'; + if (isCompoundType) { field.inputs = this.generateSubInputs(field, fieldDef); } - // 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, diff --git a/test/field-manager.test.js b/test/field-manager.test.js index 80a7810..ae830db 100644 --- a/test/field-manager.test.js +++ b/test/field-manager.test.js @@ -225,17 +225,57 @@ 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('preserves caller-supplied inputs for an unknown compound-like type', async () => { + const apiClient = createMockApiClient(); + const registry = createMockRegistry(); + const validator = createMockValidator(); + const manager = new FieldManager(apiClient, registry, validator); + + const result = await manager.addField(1, 'custom_compound', { + label: 'Custom Compound', + inputs: [{ id: '4.1', label: 'Part A' }, { id: '4.2', label: 'Part B' }] + }); + + assert.strictEqual(result.success, true); + assert.deepStrictEqual(result.field.inputs, [ + { id: '4.1', label: 'Part A' }, + { id: '4.2', label: 'Part B' } + ]); + }); }); test('FieldManager - updateField', async (t) => { 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..5448f46 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 } from '../src/field-definitions/field-registry.js'; test('assignFieldIds', async (t) => { await t.test('assigns sequential ids when none are provided', () => { @@ -163,6 +163,27 @@ 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('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'); }); // ================================= From 30dbb6ebe436b437d7e98dd8c690cf466635d8b2 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 21:04:35 -0400 Subject: [PATCH 02/12] fix(fields): rebase caller sub-input ids onto the generated field id For unknown/third-party compound fields, gf_add_field preserved caller inputs verbatim, so dotted sub-input ids kept their assumed parent (e.g. 9.1) instead of the id generateFieldId assigns, orphaning the sub-inputs. Rebase dotted ids onto the generated field id (mirrors assignFieldIds); known compound types keep generateSubInputs unchanged. Live-verified on dev.test: 99.x inputs persist as 4.x. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- src/field-operations/field-manager.js | 28 +++++++++++++++++++++++++-- test/field-manager.test.js | 23 ++++++++++++++++++++-- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/field-operations/field-manager.js b/src/field-operations/field-manager.js index 0145794..6417334 100644 --- a/src/field-operations/field-manager.js +++ b/src/field-operations/field-manager.js @@ -44,11 +44,15 @@ export class FieldManager { // Create field with type-specific defaults (none for unknown types) const field = this.createField(fieldId, fieldType, properties, fieldDef || {}); - // Generate compound sub-inputs only for known compound types (address.1, - // name.3, โ€ฆ). Unknown types keep any caller-supplied `inputs` untouched. + // 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); } // Normalize layout grid properties (layoutGroupId, layoutGridColumnSpan) @@ -202,6 +206,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/test/field-manager.test.js b/test/field-manager.test.js index ae830db..474812f 100644 --- a/test/field-manager.test.js +++ b/test/field-manager.test.js @@ -259,23 +259,42 @@ test('FieldManager - addField', async (t) => { ); }); - await t.test('preserves caller-supplied inputs for an unknown compound-like type', async () => { + 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: '4.1', label: 'Part A' }, { id: '4.2', label: 'Part B' }] + 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'] + ); + }); }); test('FieldManager - updateField', async (t) => { From 7838d78e45698158d4f0442e1f6f75ebb3b3ed7a Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 21:26:13 -0400 Subject: [PATCH 03/12] fix(robustness): harden field/validation helpers; fix update-field persistence Adversarial pass (50+ probes) surfaced 15 defects, all fixed test-first: gf_update_field saved a change before reporting it blocked (dependency guard now runs before the write, like deleteField); gf_add_field crashed on null properties/position and accepted empty/null/non-string field_type; gf_create_form fields:[null] threw an opaque TypeError instead of a clean error; null-input crashes in validateFieldConfig/detectFieldVariant/generateCompoundInputs/stripEntryMeta/DependencyTracker; stripEmpty/sanitize could stack-overflow on circular input; buildToolList emitted an undefined entry when gkReloadDef was omitted; validateFieldFilter silently stringified object values. Offline gate green (test:node 377, test:unit 100%, field-validation, publint, lint:docs); adversarial flags 13->3 (remaining 3 are non-bugs); live-verified on dev.test. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- CHANGELOG.md | 2 + src/config/field-validation.js | 4 +- src/config/validation.js | 7 +++ src/field-definitions/field-registry.js | 9 +++ src/field-operations/field-dependencies.js | 13 +++- src/field-operations/field-manager.js | 44 ++++++++++---- src/field-operations/index.js | 23 +------- src/server-runtime.js | 2 +- src/utils/compact.js | 13 +++- src/utils/sanitize.js | 10 +++- test/compact.test.js | 12 ++++ test/field-dependencies.test.js | 15 +++++ test/field-manager.test.js | 69 ++++++++++++++++++---- test/field-registry.test.js | 14 ++++- test/sanitize-hardening.test.js | 9 +++ test/server-runtime.test.js | 6 ++ test/validation-hardening.test.js | 16 +++++ 17 files changed, 212 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb2ae23..1ff5538 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ This release lets the field tools work with custom and third-party field types, ### ๐Ÿ› Fixed - **`gf_add_field` now accepts custom and third-party field types.** Field types added by other plugins (Gravity Perks, Gravity Wiz, and similar add-ons) 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. - **Unrecognized field types no longer leave an internal marker in saved forms** when creating or updating a form. +- **`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. ## [2.4.0] - 2026-06-19 diff --git a/src/config/field-validation.js b/src/config/field-validation.js index b07dcfc..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 }); } 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 6e3380f..bd0b3ed 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,6 +930,9 @@ 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 @@ -1059,6 +1065,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 6417334..b1b039d 100644 --- a/src/field-operations/field-manager.js +++ b/src/field-operations/field-manager.js @@ -27,6 +27,14 @@ export class FieldManager { * @returns {object} Field creation result with warnings */ async addField(formId, fieldType, properties = {}, position = {}) { + 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 || {}; + // 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 @@ -95,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], @@ -128,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]) } }; 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/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..f6ad2a1 100644 --- a/src/utils/sanitize.js +++ b/src/utils/sanitize.js @@ -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; } 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 474812f..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 @@ -321,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 () => { @@ -527,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-registry.test.js b/test/field-registry.test.js index 5448f46..04f3faa 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, validateFieldConfig } 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', () => { @@ -184,6 +184,18 @@ test('validateFieldConfig', async (t) => { }); }); +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/sanitize-hardening.test.js b/test/sanitize-hardening.test.js index cca41d6..22f7b5d 100644 --- a/test/sanitize-hardening.test.js +++ b/test/sanitize-hardening.test.js @@ -15,6 +15,15 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { sanitize, sanitizeUrl } from '../src/utils/sanitize.js'; +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. const SECRET_VALUE = 'PLAINTEXT-DO-NOT-LEAK-0123456789abcdef'; 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..d5f47b6 100644 --- a/test/validation-hardening.test.js +++ b/test/validation-hardening.test.js @@ -21,9 +21,25 @@ import { BaseValidator, } from '../src/config/validation.js'; import { buildEntriesQuery } from '../src/gravity-forms-client.js'; +import FieldAwareValidator from '../src/config/field-validation.js'; 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 From 6eb77f3a5a8bd59a08185733587d99d8ea9a2c1a Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 21:53:02 -0400 Subject: [PATCH 04/12] fix(security): correct loopback detection, mask cookies, relax URL/id validation Five more genuine defects, fixed test-first: isLocalUrl treated remote 127.x domains (e.g. 127.example.com) as local, which could allow Basic auth over plain HTTP; sanitize/sanitizeHeaders never masked Cookie; validateURL rejected dotless hosts (localhost/intranet) for confirmation redirects; assignFieldIds let duplicate explicit field ids survive; formatErrorMessage only replaced the first {field} and interpreted $-sequences in values. Offline gate green (test:node 384, test:unit 100%, auth, field-validation, publint, lint:docs). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- CHANGELOG.md | 1 + src/config/auth.js | 6 +++++- src/config/validation-config.js | 14 ++++++++------ src/field-definitions/field-registry.js | 20 ++++++++++++-------- src/utils/sanitize.js | 4 ++-- test/field-registry.test.js | 7 +++++++ test/sanitize-hardening.test.js | 20 +++++++++++++++++++- test/validation-hardening.test.js | 17 +++++++++++++++++ 8 files changed, 71 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ff5538..9c70313 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ This release lets the field tools work with custom and third-party field types, - **Unrecognized field types no longer leave an internal marker in saved forms** when creating or updating a form. - **`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 explicit field ids are de-duplicated so generated forms stay valid. ## [2.4.0] - 2026-06-19 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/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/field-definitions/field-registry.js b/src/field-definitions/field-registry.js index bd0b3ed..2e54db4 100644 --- a/src/field-definitions/field-registry.js +++ b/src/field-definitions/field-registry.js @@ -984,27 +984,31 @@ export function assignFieldIds(fields) { return fields; } - const used = new Set(); + // Seed the auto-id counter above the highest explicit id. + let next = 1; for (const field of fields) { const id = Number(field?.id); - if (Number.isInteger(id) && id > 0) { - used.add(id); + if (Number.isInteger(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) { + const hasFreshExplicitId = Number.isInteger(id) && id > 0 && !claimed.has(id); + if (hasFreshExplicitId) { + claimed.add(id); return field; } - while (used.has(next)) { + while (claimed.has(next)) { next++; } const newId = next++; - used.add(newId); + claimed.add(newId); // Re-base any provided compound sub-input ids onto the new field id. if (Array.isArray(field?.inputs)) { diff --git a/src/utils/sanitize.js b/src/utils/sanitize.js index f6ad2a1..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' ]; /** @@ -102,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/test/field-registry.test.js b/test/field-registry.test.js index 04f3faa..55ea72d 100644 --- a/test/field-registry.test.js +++ b/test/field-registry.test.js @@ -184,6 +184,13 @@ test('validateFieldConfig', async (t) => { }); }); +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); diff --git a/test/sanitize-hardening.test.js b/test/sanitize-hardening.test.js index 22f7b5d..ec3c08f 100644 --- a/test/sanitize-hardening.test.js +++ b/test/sanitize-hardening.test.js @@ -13,7 +13,25 @@ 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' }; diff --git a/test/validation-hardening.test.js b/test/validation-hardening.test.js index d5f47b6..36424c3 100644 --- a/test/validation-hardening.test.js +++ b/test/validation-hardening.test.js @@ -22,6 +22,23 @@ import { } 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); From 265df942eea9270165a5f5aba1a7320b05ce7d35 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:01:42 -0400 Subject: [PATCH 05/12] ci(release): announce releases to Slack via SLACK_RELEASE_NOTIFICATION_WEBHOOK Adds an "Announce release to Slack" step after "Create GitHub Release" that POSTs the gktools {link,version,changelog,product} contract to SLACK_RELEASE_NOTIFICATION_WEBHOOK, so GravityKit MCP shows up in #release-announcements like every other GravityKit plugin (which get this for free via gktools' "Notify Slack" block). - product = "GravityKit MCP"; version reuses steps.guard.outputs.version; link = .../releases/tag/v; changelog = the CHANGELOG.md section for that version (awk between "## []" and the next "## ["). - jq builds the JSON so changelog newlines/quotes are safely escaped. - Non-fatal, mirroring gktools: a missing webhook secret skips cleanly, and a response without "ok":true only warns. The release never fails on Slack. - Gated on a new steps.release.outputs.created output so it fires only when a release was actually created this run (not on an idempotent skip). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- .github/workflows/publish.yml | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 1b68bd1..bdbab6b 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,45 @@ 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 }} + run: | + if [ -z "${SLACK_RELEASE_NOTIFICATION_WEBHOOK:-}" ]; then + echo "SLACK_RELEASE_NOTIFICATION_WEBHOOK not set โ€” skipping Slack announcement." + exit 0 + fi + VERSION="${{ steps.guard.outputs.version }}" + LINK="https://github.com/${{ github.repository }}/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 From a697d3b6f50ae1be230d2ed82473a6f0dc11c0c5 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:06:41 -0400 Subject: [PATCH 06/12] fix(client): read timeout/TLS from resolved test config, not raw config In test mode resolveEnv() remaps GRAVITY_FORMS_TEST_TIMEOUT into this.config, but the GravityFormsClient constructor read the raw config object, so the test-site timeout override was silently ignored (stayed 30000). Read timeout and self-signed-cert settings from this.config like every other setting. Found by reading the constructor; verified test-first. Gate green (test:node 385). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- src/gravity-forms-client.js | 4 ++-- test/client-hardening.test.js | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/gravity-forms-client.js b/src/gravity-forms-client.js index e706201..fc71150 100644 --- a/src/gravity-forms-client.js +++ b/src/gravity-forms-client.js @@ -94,7 +94,7 @@ export class GravityFormsClient { // 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 +113,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: (this.config.GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS || this.config.MCP_ALLOW_SELF_SIGNED_CERTS) !== 'true' }) }); diff --git a/test/client-hardening.test.js b/test/client-hardening.test.js index de92e28..b9bd65f 100644 --- a/test/client-hardening.test.js +++ b/test/client-hardening.test.js @@ -36,6 +36,20 @@ 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); +}); + // 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) { From eaa4c798fa05ba98ec1368364de766026496343f Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:15:56 -0400 Subject: [PATCH 07/12] fix(fields,ci): assignFieldIds infinite-loop + sub-input rebase; harden publish.yml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more genuine bugs, test-first: (1) assignFieldIds infinite-loops on an out-of-range field id (1e308 etc.) because next++ can't advance past float precision โ€” now only safe integers count as explicit ids (DoS, found via fuzzing). (2) assignFieldIds preserved-explicit-id fast path left dotted sub-inputs attached to a stale parent โ€” now rebases inputs to the final id in both branches via a shared helper. (3) publish.yml Slack step interpolated steps.guard.outputs.version / github.repository directly into the shell body โ€” now bound via env and referenced as quoted shell vars (template-to-shell injection). Gate green (test:node 387, test:unit 100%, publish.yml YAML/actionlint clean). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- .github/workflows/publish.yml | 7 ++-- src/field-definitions/field-registry.js | 46 +++++++++++++++---------- test/field-registry.test.js | 19 ++++++++++ 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index bdbab6b..2b82250 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -109,13 +109,16 @@ jobs: 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 - VERSION="${{ steps.guard.outputs.version }}" - LINK="https://github.com/${{ github.repository }}/releases/tag/v$VERSION" + 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" ' diff --git a/src/field-definitions/field-registry.js b/src/field-definitions/field-registry.js index 2e54db4..f4cb6e6 100644 --- a/src/field-definitions/field-registry.js +++ b/src/field-definitions/field-registry.js @@ -984,11 +984,33 @@ export function assignFieldIds(fields) { return fields; } - // Seed the auto-id counter above the highest explicit id. + // 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 && id >= next) { + if (Number.isSafeInteger(id) && id > 0 && id >= next) { next = id + 1; } } @@ -998,10 +1020,11 @@ export function assignFieldIds(fields) { const claimed = new Set(); return fields.map((field) => { const id = Number(field?.id); - const hasFreshExplicitId = Number.isInteger(id) && id > 0 && !claimed.has(id); + const hasFreshExplicitId = Number.isSafeInteger(id) && id > 0 && !claimed.has(id); if (hasFreshExplicitId) { claimed.add(id); - return field; + // Id preserved, but sub-inputs may still reference a stale parent โ€” rebase. + return rebaseInputs(field, id); } while (claimed.has(next)) { @@ -1010,20 +1033,7 @@ export function assignFieldIds(fields) { const newId = next++; claimed.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 }; - } - - return { ...field, id: newId }; + return rebaseInputs({ ...field, id: newId }, newId); }); } diff --git a/test/field-registry.test.js b/test/field-registry.test.js index 55ea72d..4a940c6 100644 --- a/test/field-registry.test.js +++ b/test/field-registry.test.js @@ -184,6 +184,25 @@ test('validateFieldConfig', async (t) => { }); }); +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); From 0f14becbaec8076a850aa7c8d6e263b4b019d203 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:18:21 -0400 Subject: [PATCH 08/12] fix(abilities): methodForAbility tolerates null/non-object annotations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit methodForAbility(null) threw 'Cannot read properties of null (reading readonly)' โ€” the = {} default only covers undefined. Now guarded with optional chaining (returns POST). Surfaced by a fuzz pass over the exported surface. Verified test-first. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- src/abilities/loader.js | 4 ++-- test/abilities-loader.test.js | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) 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/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) { From d893c3535d0dd002f1b85aeec9242b0e8ac9c457 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:26:18 -0400 Subject: [PATCH 09/12] docs(changelog): round out 2.4.1 with compound sub-input + out-of-range id fixes [ci skip] Reflect the net user-facing 2.4.1 changes since v2.4.0: compound sub-input ids stay consistent with the field id, and out-of-range/duplicate explicit field ids are corrected (an out-of-range id no longer stalls form creation). CI/release tooling (Slack announce), the test-only timeout fix, and the internal methodForAbility guard are intentionally omitted (not user-facing). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c70313..034957e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,14 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [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. +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 (Gravity Perks, Gravity Wiz, and similar add-ons) 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. - **Unrecognized field types no longer leave an internal marker in saved forms** when creating or updating a form. +- **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 explicit field ids are de-duplicated so generated forms stay valid. +- **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 From 64b128e02b2847bc0d5fce23b797744b11bc9685 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:29:17 -0400 Subject: [PATCH 10/12] docs(changelog): drop redundant add-on parenthetical in 2.4.1 [ci skip] Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 034957e..c85dcc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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 (Gravity Perks, Gravity Wiz, and similar add-ons) 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. +- **`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. - **Unrecognized field types no longer leave an internal marker in saved forms** when creating or updating a form. - **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`). From 60f237586e122388bc297f75b72528c91053e009 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:36:55 -0400 Subject: [PATCH 11/12] fix(tls): self-signed cert flags enable independently (no masking) rejectUnauthorized used (A || B) !== 'true', but 'false' is a truthy string, so GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS='false' short-circuited and masked MCP_ALLOW_SELF_SIGNED_CERTS='true' (and vice versa). Now each flag is compared === 'true' separately, in both GravityFormsClient and WordPressClient (same bug). Test-first; gate green (test:node 389). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- src/gravity-forms-client.js | 9 ++++++++- src/wp-client.js | 6 +++++- test/client-hardening.test.js | 11 +++++++++++ test/wp-client.test.js | 5 +++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/gravity-forms-client.js b/src/gravity-forms-client.js index fc71150..159a9ab 100644 --- a/src/gravity-forms-client.js +++ b/src/gravity-forms-client.js @@ -91,6 +91,13 @@ 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, @@ -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: (this.config.GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS || this.config.MCP_ALLOW_SELF_SIGNED_CERTS) !== 'true' + rejectUnauthorized: !allowSelfSignedCerts }) }); 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/client-hardening.test.js b/test/client-hardening.test.js index b9bd65f..450336e 100644 --- a/test/client-hardening.test.js +++ b/test/client-hardening.test.js @@ -50,6 +50,17 @@ test('test-mode honors GRAVITY_FORMS_TEST_TIMEOUT', () => { 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/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' })); }); From 4536ad438d35a7cc41299b5b5c0fc80d8679cb01 Mon Sep 17 00:00:00 2001 From: Zack Katz Date: Thu, 25 Jun 2026 22:49:28 -0400 Subject: [PATCH 12/12] docs(changelog): finalize 2.4.1 release notes Drop the internal-marker bullet (not customer-facing) ahead of the 2.4.1 release. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c85dcc8..c11654f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,6 @@ This release lets the field tools work with custom and third-party field types ( ### ๐Ÿ› 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. -- **Unrecognized field types no longer leave an internal marker in saved forms** when creating or updating a form. - **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.