Repository navigation
Resolve permit_params in controller context, and close three form-description gaps - #20
Closed
lloydwatkin wants to merge 1 commit into
Closed
lloydwatkin wants to merge 1 commit into
lloydwatkin wants to merge 1 commit into
Conversation
Fixes the four findings in #18. `RecordWriter#resolve_permitted` and `FormDescription#permitted_names` each built a bare `@config.controller.new` to ask a resource what it permits. ActiveAdmin `instance_exec`s a block-form `permit_params` on the controller, so a block reading `current_admin_user` — the documented way to vary the writable set by user — raised `NameError` on that instance. Both call sites rescue to nil, and nil is how "this resource never declared permit_params" is signalled, so `create`, `update` and `describe_form` all refused such a resource outright and said something untrue about why. Both now take the controller from `ControllerDispatcher#controller_with_mcp_user`, which is the context the `permission:` proc fix already established. A form block declaring no inputs of its own was described as `source: "form"` with an empty attribute list, because `[]` is truthy. A bare `f.inputs` is legal and Formtastic only expands it at render time, so the block describes nothing and the description now falls back to `permit_params`. `FormFieldCollector#inputs` ignored its options, so `inputs for: :author` flattened the author's fields into the record's own attributes, where a client would read them as writable and `create` and `update` would drop them. It now reports a nested group, as `has_many` already did. The `permit_params` probe recovers names by offering the controller every model column, so a permitted param that is not a column — `tag_ids`, `*_attributes` — never appears on the fallback path. That limit is now written into the README rather than left undocumented. Each fix is covered end to end by a new fixture resource — `Newsletter`, `Bulletin` and `Dispatch` — and every new e2e example was watched failing against the unfixed library first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
🤖 Superseded by #19, which landed the same four fixes while this was in flight — the library diffs turned out to be line-for-line equivalent, including the Closing in favour of #21. |
lloydwatkin
added a commit
that referenced
this pull request
Sep 21, 2026
Follow-up to #19, which fixed all four findings in #18. This closes two gaps in the e2e coverage of the block-form `permit_params` shape, and documents two behaviours #19 changed but did not describe. No library changes. ## 1. `update` was not covered `resolve_permitted` gates `update` as well as `create`, and #19 added only a `create` example against `Assignment`. Reverting the fix in `RecordWriter` leaves the new update example failing, so the second call site is now covered by something that can actually see it. ## 2. Nothing proved the block filters Both branches of `Assignment`'s block — `%i[name notes]` and `%i[name]` — permit only columns the fixture already writes, so every example passed whether the block's result was honoured or ignored entirely. `secret_note` is a new column neither branch permits, and an example asserts a write never reaches it. Mutating the block to permit `secret_note` fails that example and no other. ## 3. Two behaviours left undocumented The README now says that an associated record's fields are reported under `nested` whether declared with `has_many` or with `inputs for:`, and that a form block declaring no inputs of its own is described from `permit_params` — both changed by #19, neither described. It also notes that a block-form `permit_params` is evaluated as the authenticated MCP user. ## Testing `rake e2e` — 69 examples, 0 failures (67 on `main`, plus 2). Both new examples were verified against two separate mutations: - reverting `resolve_permitted` to `@config.controller.new` fails both; - leaving the fix in place but adding `secret_note` to the block's permitted list fails only the withholding example. ## Note on #20 I had an in-flight branch fixing all four findings independently, opened as #20 before #19 landed. Its library diff turned out to be line-for-line equivalent to #19's, so rather than resolve the conflicts and re-land a duplicate, #20 is closed and this carries only the part that was not already covered. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
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.
Fixes #18 — all four findings.
1. Block-form
permit_paramsmade a resource unwritableRecordWriter#resolve_permittedandFormDescription#permitted_nameseach built a bare@config.controller.newto ask a resource what it permits. ActiveAdmininstance_execs a block-formpermit_paramson the controller, so a block readingcurrent_admin_user— the documented way to vary the writable set by user — raisedNameErroron that instance. Both call sites rescue tonil, andnilis how "this resource never declaredpermit_params" is signalled, socreate,updateanddescribe_formall refused such a resource outright with a message that was not merely unhelpful but wrong.Both now take the controller from
ControllerDispatcher#controller_with_mcp_user— the same context thepermission:proc fix in #10 established.2. A form block declaring no inputs reported zero writable attributes
[]is truthy, so a resource whose form block declares no inputs of its own was described assource: "form"with an emptyattributesarray, which a client reads as "nothing may be written here". A baref.inputsis legal and Formtastic only expands it at render time, so the description now falls back topermit_params.3.
inputs for: :associationwas flattened into the parent's attributesFormFieldCollector#inputsignored its options, sof.inputs for: :authorreported the author'snameas a top-level writable attribute of the record being described — whichcreateandupdatewould then drop. It now reports anested:group, ashas_manyalready did. Handles bothfor: :authorand Formtastic'sfor: [:author, object]array form.4. The
permit_paramsprobe only offers column namesDocumented rather than widened. The probe recovers names by offering the controller every column the model has, so a permitted param that is not a column —
tag_ids,*_attributes— never appears on the fallback path. Each permitted shape would need a differently-shaped probe value, so a partial widening would still under-report while looking complete. The README's "Two limits worth knowing" is now three, and points at declaring aformblock as the way to get the full read.Coverage
Three new e2e fixture resources, one per defect — the absence of which is why all three got through:
Newsletter— block-formpermit_paramsreadingcurrent_admin_user, with asecret_notepermitted to nobody.Bulletin— a form block whose onlyf.inputsis the bare one.Dispatch— a form block naming an association withfor:, reusing the existingAuthor.Each has a model, migration, seed rows and an entry in the fixture app's README.
The new e2e examples were watched failing first. Restoring
lib/toHEADleaves the fixture build cache intact, so the rerun exercises the new fixtures against the unfixed library: 9 of the 10 new examples failed. The tenth was a bare "control record untouched" example, which passes trivially when the write is refused, so its assertion was folded into the example that does fail rather than left as an example that passes regardless.Both suites green locally:
rake e2e— 72 examples, 0 failuresform_description,record_writer,form_field_collector,controller_dispatcher,resource_registry,request_handler,action_catalog) — 147 examples, 0 failures🤖 Generated with Claude Code