release: config write tool, contextual ACL, tool management UI, audit-log PII redaction - #29
Merged
Conversation
Adds coverage the reviewer proved missing by mutation: a known Field whose path hits the keyword blocklist, field-check-before-keyword ordering, case folding of both the field type and the path, and the subclass / leading backslash arms of isEncryptedBackend(). Also constrains the getAttribute stub to 'backend_model'.
…-by-default allowlist Two layers gate what the upcoming system.config.set may ever write. Protected prefixes are a di.xml array, not admin config, so weakening them needs filesystem write plus setup:di:compile — out of reach of an admin session and of the MCP surface itself, while satellites can still extend the set by merge. magebit_mcp is in that set so the tool cannot widen its own gates. The operator allowlist ships empty and the tool ships disabled, so enabling it and choosing what it may set stay two deliberate acts. Prefix matching is segment-bounded: "dev" protects dev/debug/* without catching developer/*. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tolerating them findProtectedPrefix() folded case and slashes but not whitespace, so " admin/x" returned null and cleared the protected layer. Nothing exploited it — allowlist entries are trimmed on read, so such a path died at in_array instead — but the containment was incidental, and it breaks the moment a consumer uses the public findProtectedPrefix() as its gate or the caller canonicalises after asserting. assertWritable() now refuses anything that is not already canonical section/group/field, which kills surrounding and embedded whitespace, leading and trailing slashes, empty segments and dot segments in one check, rather than normalising input the layers below then disagree about. The pattern is case tolerant because a mixed-case path is legitimate and the protected layer lowercases anyway; the /D modifier matters because PCRE otherwise lets "$" match before a trailing newline, which was the same bypass wearing a different hat. The allowlist comparison stays exact — folding it would widen the allowlist, which is the wrong direction. findProtectedPrefix() also trims whitespace now, as defence in depth for callers that reach it directly. Also adds a bare-CR dataset to ConfigWriteConfigTest: the CRLF case did not discriminate the splitter, since per-line trim() swallows the stray \r. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ssage The "allowlist comparison stays exact" invariant was upheld by the source but by nothing else: folding case on both sides of the in_array left the suite green, while making Tax/Foo/Bar match tax/foo/bar and TAX/FOO/BAR. A later "be forgiving about case" change would have widened the allowlist unopposed. Now pinned, with the exact entry asserted writable in the same test so the refusals are about case and not about the path being unreachable. The shape-guard branch is the one place a wholly unvalidated path reaches a message that ends up in the JSON-RPC error and the audit log, so the reflected value is now stripped of control characters, forced to valid UTF-8 (invalid sequences would fail json_encode downstream) and capped at 120 characters. The other two branches route through the same helper for consistency, though the shape guard has already constrained their input. findProtectedPrefix()'s docblock claimed it could only ever catch more, never less. Not true for form feed, non-ASCII whitespace, or anything embedded rather than surrounding. Narrowed the claim rather than widening the fold: the shape guard is the contract now, and a more forgiving fold would only make this method look safe to use standalone, which is the belief that caused the bug. Also reworded the guard message, which said "section/group/field" while the pattern accepts two segments. Task 4's writer enforces three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A throw from getContextualAclResources() escaped checkAcl and surfaced as -32603 Internal error with an audit row that carried no permission code. Catch it, and treat a blank or non-string resource the same way: -32004 with a message distinct from a genuine permission refusal, logged to the module channel.
…ig save path Writes go through Magento\Config\Model\Config rather than WriterInterface so the field's backend model and validation run exactly as an admin save does. Tightened against the plan draft in four places, each closing a silent-widening path rather than adding a feature: - parse() refuses non-canonical paths instead of normalising them, so a caller that skips ConfigWritePolicy cannot reach a path the allowlist never approved. - Unknown scope names are rejected rather than falling through to a default-scope write; the singular website/store spellings the framework itself normalises are accepted. - A non-default scope must carry a scope code, otherwise Config::retrieveScope() resolves it back to the default scope. - Rejection messages sanitise the echoed path (control chars, UTF-8, length) since they reach the JSON-RPC error string and the audit row. sectionFor() also rejects the blank Section that Structure::getElement() synthesises for an undeclared id, so the caller's contextual-ACL resolver gets null instead of an element with no <resource> - which would emit a blank ACL entry and turn a missing field into a -32004 denial. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…skip, or persist unvalidated CRITICAL. Config::getFieldPath() replaces the posted path with the field's <config_path>, so an allowlisted path could write a row the protected-prefix list forbids: payment_all_paypal/.../partner is not matched by the 'payment' prefix but stores at payment/payflowpro/partner. Every gate behaved as designed and the invariant still fell. Measured on this install: 8952 of 10955 structural field paths redirect, and 7049 of those go from an unprotected path to a protected row. currentValue() also read the named path, so previous_value described a row that never changed. write() now resolves the target through Structure::getFieldPaths() - keyed by the path a value lands on, listing the structural paths that reach it, which inverted is exactly the redirection - and refuses outright when the stored path differs. Resolving this way rather than via getElement() + a placeholder heuristic also fixes 39 real fields (payment/*/model and friends) that carry only id/path/ _elementType and are byte-identical to a synthesised placeholder. sectionFor() follows the same resolution so the contextual ACL lands on the section that would really be touched. Two further refusals fall out of having the field resolved: - A path absent from the map is not declared in system.xml. _processGroup() would fall back to a generic Value and persist the raw string - the tax/calculation/algorithm = "garbage" outcome routing through the admin save path exists to prevent. - SettingChecker::isReadOnly() is now checked up front. _processGroup() continues past a path pinned in app/etc/env.php or by a CONFIG__* variable and save() returns cleanly, so the tool would have written a truthful-looking audit row for a change that never happened. For a config writer whose audit row is the only undo trail, that is the worst failure mode available. The redirect refusal does not suggest allowlisting the stored path: zero of the 8952 redirecting paths have a stored path that is itself a declared field, because a <config_path> exists precisely to point several UI locations at one shared row that has no field of its own. The advice would dead-end on the undeclared-field refusal. Also: singular website/store scope aliases dropped so the writer and the tool's schema share one vocabulary; currentValue() runs the same scope assertion as write() so a read cannot silently target a different scope from the write that follows; and the canonical-path pattern plus the safe-echo helper move to Model/Util/ConfigPathFormat, shared with ConfigWritePolicy, which had a verbatim copy and a second, looser idea of what a canonical path is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ht, not the raw structure map CRITICAL, round two. storedPaths() inverted Structure::getFieldPaths(), which walks the raw merged _data array and reads only the literal <config_path> XML attribute. Config::getFieldPath() reads $field->getConfigPath() off the intercepted flyweight, and Magento_Paypal's FieldPlugin::afterGetConfigPath() synthesises payment/<rest> for every field under a payment_<country> section that declares none. The raw map cannot see a plugin, so 909 paths were classified writable while Magento redirected them - and all 909 went from an unprotected structural path to a protected payment/... row. payment_us 95, payment_ca 84, payment_au 78, payment_gb 77, and so on down twelve country sections. Now hybrid, as directed: existence still from getFieldPaths(), which is exact for 'does system.xml declare this field'; the stored path from getElement($path)->getConfigPath() with the same inner-slash guard getFieldPath() applies. That is not merely equivalent to what save() does - Structure is a shared singleton memoising flyweights in _elements, and Config::getField() resolves through the same instance, so the two read the same object in any area or DI context. sectionFor() resolves the same way. The regression test's getFieldPaths() double always reports identity, modelling the raw map's blindness, so a test can only observe a redirect by way of the flyweight. Also LOW: the env-lock probe forwarded the caller's scope code verbatim. _processGroup() probes with the resolved Config::getScopeCode(), and while SettingChecker's deployment-config branch normalises, its env-variable branch interpolates the code as given - so scope code '1' probed CONFIG__WEBSITES__1__X while the lock is CONFIG__WEBSITES__BASE__X, the check passed, _processGroup() skipped the field, and the tool would have reported a write that never happened. ScopeCodeResolver now resolves the code first; the default scope is left alone because both branches ignore the code there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d hand it to Config::save() system.xml only ever exists at etc/adminhtml/system.xml, and Structure\Data extends Config\Data\Scoped, which resolves files per area and memoises the result. This module's controller is registered in etc/frontend/routes.xml, so on the route it actually serves the plain Structure is EMPTY - 0 declared paths against 10955 in adminhtml. That is a live disclosure bug in already-shipped code, not just a gap in the unreleased write path. SensitiveFieldGuard had no field type and no backend_model to inspect, so its field_type_sensitive and encrypted_backend_model branches never fired and only the keyword blocklist stood between system.config.get and any password, obscure or encrypted field whose path happens to miss the blocklist. Measured from the frontend area: payment/payflowpro/pwd returned NULL (readable) before, field_type_sensitive after. State::emulateAreaCode() does not fix this - it swaps State::_areaCode while Structure\Data resolves through Framework\Config\ScopeInterface, which emulation never touches. Wired an adminhtml-scoped chain in etc/di.xml instead. Defined under Magebit\Mcp\Model\Config\Structure\* rather than referencing Magento's adminhtmlConfigStructure: a virtualType is not public API, and the chain's root lives in magento2-base's app/etc/di.xml. The cacheId is inherited from the type, so we share the admin UI's cache entry rather than duplicating it. write() forwards that same instance into Config. Fixing only this module's side would have re-opened the divergence closed in the previous commit - the redirect check reading the adminhtml structure while Config::save() resolved against the empty frontend one, with _processGroup() falling back to a generic Value and no backend model. Verified live from the frontend area: 10955 declared paths, 1983 accepted, 8972 refused, 0 accepted-but-redirected; and a write of tax/calculation/based_on emitted the three tax/notification rows that only Magento\Tax\Model\Config\Notification - the field's backend model - produces. Test rows were deleted and core_config_data verified back to 61 rows, max_id 112. Unit tests cannot reach any of this: they mock Structure, so the injected instance is a double and the area scoping is invisible. The unit-level guard is therefore a wiring test over etc/di.xml, plus an assertion on the exact ConfigFactory payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tions Structure::__construct pulls the whole 9 MB structure eagerly, and both consumers sit on the eager path from the MCP controller, so every request paid ~38 ms / ~36 MB — including initialize, ping, and tool calls that never read config. Magento wraps it for its own consumers for exactly this reason. Magento_Paypal rewrites payment_<country>/* onto payment/* through a plugin registered only in etc/adminhtml/di.xml. Interception is per-area and this module serves a frontend route, so the rewrite is invisible here: a write to payment_us/... would have passed the protected-prefix gate and landed in a row Magento never reads, reported as success. Protecting the 13 alias sections closes both the divergence from the admin UI and the dead-row write. The comment claiming the flyweight sees that plugin was simply wrong. The wiring test caught the proxy indirection on the first run; it now follows it, and asserts no Structure consumer is left unwired at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RecursiveIteratorIterator yields mixed at PHPStan level 9, so isFile(), getExtension() and getPathname() were three unchecked calls on mixed. My own error: the previous commit piped phpstan through tail inside an && chain, so the exit status came from tail and the commit ran over a failing gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Composes the write policy, the sensitivity guard and the config path writer behind one tool: four gates (allowlist, sensitivity, system.xml Field, section ACL) must pass before a core_config_data row is touched. The section ACL arrives through ContextualAclAwareInterface, which resolves before schema validation and so tolerates absent or mistyped arguments by returning []. Also renames the Structure\AdminhtmlProxy virtual type to AdminhtmlLazy: the DI compiler resolves every di.xml reference ending in "Proxy" as a generated proxy class, so the old name failed setup:di:compile outright with "Invalid proxy class". A wiring test now guards the naming rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…write gates Adds the config_write group to Stores > Configuration > Magebit > MCP Server, default-scope only because ConfigWriteConfig reads it there and a website-scope value would have no effect. Allowed Paths gets a backend model validating each line against ConfigPathFormat, so an entry the policy could never match is refused at save time instead of silently doing nothing; the line parsing moves into ConfigWriteConfig::parseAllowedPaths() so the validator and the policy read the field identically. README gains a Configuration writing section covering the gate stack, the protected set, and why system.config.get can report a value the database does not hold. CLAUDE.md bumps the tool count to 19, documents config_write, and fixes the error-code drift that omitted -32016 PROMPT_NOT_FOUND. Group sortOrder is 57, not 70: Magebit_McpDbTools already ships db_tools at 70 in the same section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tem.config.set Backend\File::beforeSave() reads $_FILES only. Handed a plain string it calls unsValue(), so AbstractDb::_prepareDataForTable() omits the value column: an existing row is left as it was, a new one is created NULL. Config::save() still returns cleanly, so the tool reported isError:false and the audit row — the only undo trail a config change gets — recorded a previous_value -> value transition that never happened. Three stock paths reach it through every other gate: sales/identity/logo, sales/identity/logo_html, catalog/placeholder/placeholder. Refused in ConfigPathWriter, symmetrically with assertNotLocked(), off the backend model first and the declared type as a secondary catch. Structure\Element\Section::isAllowed() is false for a section that declares no <resource>, and Config\Controller\Adminhtml\System\Config\Save requires it — so the admin UI refuses such a section for every role. getContextualAclResources() returned [] there and the dispatcher reads [] as "no extra gate", which handed MCP a section the admin UI cannot save. execute() now refuses it, which also makes the one shipped implementation obey the rule docs/EXTENDING.md publishes: every [] return must be backed by a refusal in execute(). Resolution itself still never throws. Stock exposure is web_api and payment_all_paypal (the latter already protected), but it generalises to any third-party section. Also drops the dead getElementByConfigPath() gate: getElementByPathParts() synthesises a Field flyweight for any unmatched three-segment path, so it never returned a non-Field there; the real existence check is the writer's, and SensitiveFieldGuard's non_field_path catches the rest one statement earlier. The Structure dependency, its di.xml wiring and the two tests that passed only against a mock go with it. README: the "only about a fifth" figure was wrong (measured 740/10,957) and would rot again, so it now describes why paths are refused instead of quoting a ratio; adds the file-upload refusal and the fact that Magento commits the row before dispatching admin_system_config_changed_section_*, so a throwing observer is reported as an error on a value that was written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Feature/mcp config set
Update README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Releases everything accumulated on
developsince the lastmastercut — 19 commits, 50 files, threefeature groups plus a security fix to the audit log.
What's in it
system.config.set— allowlisted config writes (new)Tool/System/ConfigSet.phpwrites through Magento's own admin config save path(
Model/Config/ConfigPathWriter.php), so backend models, validation and encryption all run exactly asthey do in the admin UI.
Model/Config/ConfigWritePolicy.phpgates every path: empty allowlist by default, plus aDI-contributed set of protected prefixes that an allowlist entry cannot override. Non-canonical paths
are rejected rather than normalised.
values that would persist unvalidated, file-upload fields, and sections with no
<resource>.Model/Config/Backend/AllowedPaths.phpvalidates the allowlist at save time, so a typo is reported inthe admin instead of failing later at call time.
ContextualAclAwareInterface— third ACL check (new contract)UnderlyingAclAwareInterfacevalue cannot express — a config writer needs the target section's own<resource>.when a resource cannot be resolved.
Tool management admin UI (new)
Magebit_Mcp::mcp_tool_management: enable/disable individual toolswithout touching DI, backed by
Model/Tool/DisabledTools.phpand honoured by bothtools/listandtools/call.Audit-log PII redaction (security)
Model/AuditLog/TextRedactor.phpruns pattern-based detection over free-text values and fails closedon any error, replacing the value with
[REDACTION_FAILED]rather than letting it through.Strategy/FingerprintStrategy.phpsubstitutes a keyed HMAC fingerprint instead of a fixed placeholder, sothe same input yields the same token and auditors can still group repeated lookups.
AuditLogger::encodeSummary()previously encodedresult_summary_jsonwithout redacting it; bothcolumns now go through the redactor.
Refactor
SensitiveFieldGuardextracted out ofConfigGet, behind characterisation tests, so the read and writetools share one definition of "sensitive".
Verification
OK (844 tests, 2113 assertions)setup:di:compile,cache:flush,magebit:mcp:tools:validate-aclall cleanReview notes
hardcoded in the core — a satellite that adds a config section must contribute its own prefix or its
settings become writable through
system.config.set.allow_writesconfig and theper-token flag) on top of the new allowlist.