From 6ac30911de15376428d406bca74e667b179f4dd2 Mon Sep 17 00:00:00 2001 From: Sang Date: Sat, 5 Sep 2026 03:46:00 +0700 Subject: [PATCH] Stop the generator reading commented-out code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A controller keeping a line for reference: def create # Legacy: params.require(:admin).permit(:superuser) params.require(:user).permit(:name) end drafted root: :admin and a :superuser field. Both came from a line that does not execute, and both are wrong — the second in a security-flavoured way, since the draft then suggests permitting a privilege escalation parameter. =begin/=end blocks and trailing comments on live lines had the same effect. Comments are removed before scanning, with Ripper rather than a regexp, because `#` is only sometimes a comment. Two consequences the regexp approach would get wrong, and which specs now pin: * A permit call inside #{...} interpolation IS live code, and is still read. * String CONTENT is deliberately kept. permit("name") is a supported spelling and its keys live in string tokens, so dropping string bodies — the obvious next step — would silently lose them. Ripper is stdlib, so no dependency is added, and a file it cannot lex falls back to the raw source: a syntactically odd controller scans exactly as it did before rather than not at all. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + README.md | 1 + lib/permittable/generator.rb | 28 +++++++++++++++++++- spec/generator_spec.rb | 51 ++++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c509b4..fc0f285 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ - **An array outside its `length:` bound was still fully examined, so an oversized payload cost far more to reject than to accept.** `length:` recorded its violation and then cast, checked and reported on every element anyway. A payload of 200,000 non-string elements against `array :tags, of: :string, length: 0..10` produced **200,001 violations and a ~9.5 MB error body after ~9.2 seconds of CPU** — for a request already refused by its first check, and against the very bound a developer declares to prevent exactly that. `length:` is now a bound rather than a report: an array outside it returns immediately, so the same payload costs **one violation, ~40 bytes and ~57 ms** of contract work (the rest of the wall time is the `HashWithIndifferentAccess` conversion of the payload, which happens before any field is examined). A consequence worth knowing: `validate:` and `transform:` are no longer handed an array the contract has already rejected, matching the rule `transform:` already followed for element violations. Arrays within their bounds, and arrays with no `length:` declared, behave exactly as before — note in particular that there is still **no default cap**, so an array with no `length:` remains unbounded and every element of it is cast and checked. `benchmark/oversized_array.rb` re-runs the measurement. An authored `default:`/`example:` on an array is now also checked against that array's own `length:` at class load, instead of loading and handing the action an out-of-bounds default. - **`sensitive: true` on a nested block or array was a complete no-op, and logged the values it promised to redact.** Rails' parameter filtering walks into hashes and arrays itself and asks a proc filter about the **leaf values only**, handing it the leaf's own key and never the path that led there — so registering only the container's name redacted nothing: the filter descended and asked about `"card_number"`, which the container's name does not match. A contract declaring `optional :payment, sensitive: true do required :card_number, :string end` printed the card number in the clear. `sensitive:` now **cascades** to every field inside a nested or array container, at any depth, and a spec proves it through `ActiveSupport::ParameterFilter` rather than only asserting on the registry. The cascade is resolved onto the field data at class load, so every reader of a contract agrees with the redaction: the exported JSON Schema marks a cascaded child `writeOnly`, and the RSpec matcher's `.sensitive` chain passes for it. +- **`permittable:generate` read commented-out code as if it ran.** A controller keeping a `# params.require(:admin).permit(:superuser)` line for reference had `:admin` drafted as the contract's `root:` and `:superuser` drafted as a permitted field — a wrong suggestion, and a security-flavoured one, from a line that does not execute. The same applied to `=begin`/`=end` blocks and to trailing comments on live lines. Comments are now removed before scanning, using `Ripper` (stdlib, no new dependency) rather than a regexp, because `#` is only sometimes a comment: a permit call inside `#{'#{...}'}` interpolation **is** live code and is still read, and string **content** is deliberately kept because `permit("name")` is a supported spelling whose keys live in string tokens. A file `Ripper` cannot lex falls back to the raw source, so a syntactically odd controller scans exactly as it did before rather than not at all. ### Added - **`sensitive: false` opts a sub-field out of an inherited cascade.** Matching is a case-insensitive **substring** match, so cascading a generic name like `:id` or `:name` would redact every parameter in the app that happens to contain it — occasionally a worse outcome than the leak it prevents. An explicit `sensitive: false` on a field (or on a container, for its whole subtree) keeps it readable. Only `false` opts out — `sensitive: nil` reads as "not stated" and still inherits. diff --git a/README.md b/README.md index af27a57..dfa8e2b 100644 --- a/README.md +++ b/README.md @@ -688,6 +688,7 @@ The generator's one rule is **draft, don't guess** — everything it cannot know - Drafts come out in **monitor mode**, so pasting one changes nothing until you flip it. - A permitted key that isn't a column becomes `virtual: true` with a TODO; a column type with no scalar equivalent (`json`, `binary`) becomes a TODO comment; a permit argument the conservative parser can't read (`*dynamic_keys`) is kept verbatim in a TODO instead of dropped. +- **Comments are not code.** A commented-out `params.require(:admin).permit(:superuser)` kept for reference is skipped, so it can't contribute a root or a field to the draft. The source is tokenised with `Ripper` for this, because `#` is only sometimes a comment — a permit call inside `#{'#{...}'}` interpolation is live code and is still read, and quoted keys like `permit("name")` still work. - A database default is noted in a comment but **not** copied into `default:` — a contract default is injected on every request that omits the field, which would overwrite columns on partial updates. The database already handles creation. - `key: [:a, :b]` in a permit call drafts as a nested block, with a TODO noting it may be an array of hashes. diff --git a/lib/permittable/generator.rb b/lib/permittable/generator.rb index 2340889..6241668 100644 --- a/lib/permittable/generator.rb +++ b/lib/permittable/generator.rb @@ -1,3 +1,5 @@ +require "ripper" + module Permittable # Drafts a permit_params contract from what the app already knows: the # model's columns (types, NOT NULL, database defaults) and, when the @@ -54,14 +56,38 @@ def found? ARRAY_ARG = /\A(\w+):\s*\[\s*\]\z/m NESTED_ARG = /\A(\w+):\s*\[([^\[\]]*)\]\z/m + # Comment tokens. Ripper (stdlib) is used rather than a regexp because `#` + # is only a comment sometimes — it also appears inside string literals and + # `#{}` interpolation, and a permit call inside interpolation IS live code. + # String CONTENT is deliberately kept: `permit("name")` is a supported + # spelling, and its keys live in string tokens. + COMMENT_TOKENS = %i[on_comment on_embdoc on_embdoc_beg on_embdoc_end].freeze + module_function + # `source` with its comments removed. A controller keeping a commented-out + # `params.require(:admin).permit(:superuser)` for reference had :admin + # drafted as its root and :superuser as a permitted field — a wrong + # suggestion, and a security-flavoured one, from a line that does not run. + # + # Anything Ripper cannot lex falls back to the source unchanged, so a + # syntactically odd file scans exactly as it did before rather than not at + # all. + def executable_source(source) + tokens = Ripper.lex(source) + return source if tokens.nil? || tokens.empty? + + tokens.reject { |token| COMMENT_TOKENS.include?(token[1]) }.map { |token| token[2] }.join + rescue StandardError + source + end + # Merge every permit call found in `source` into one Scan. The first # `.require(:root)` seen wins, matching how a controller normally sticks # to one envelope across actions. def scan(source) result = Scan.new(root: nil, scalars: [], arrays: [], nested: {}, unparsed: [], calls: 0) - (source || "").scan(PERMIT_CALL) do |root, args| + executable_source(source.to_s).scan(PERMIT_CALL) do |root, args| result.calls += 1 result.root ||= root&.to_sym split_args(args).each { |arg| classify_arg(result, arg) } diff --git a/spec/generator_spec.rb b/spec/generator_spec.rb index e1ad4f9..d858ab9 100644 --- a/spec/generator_spec.rb +++ b/spec/generator_spec.rb @@ -86,6 +86,57 @@ def update end end + describe ".scan and things that are not code" do + it "does not read a commented-out permit call" do + scan = described_class.scan(<<~RUBY) + def create + # Legacy, kept for reference: + # params.require(:admin).permit(:superuser, :impersonate_id) + params.require(:user).permit(:name) + end + RUBY + expect(scan.root).to eq(:user) + expect(scan.scalars).to eq(%i[name]) + expect(scan.calls).to eq(1) + end + + it "does not read a trailing comment on a live line" do + scan = described_class.scan('params.permit(:name) # was params.permit(:admin)') + expect(scan.scalars).to eq(%i[name]) + expect(scan.calls).to eq(1) + end + + it "does not read a permit call inside an =begin/=end block" do + scan = described_class.scan(<<~RUBY) + =begin + params.require(:old).permit(:legacy) + =end + params.permit(:name) + RUBY + expect(scan.root).to be_nil + expect(scan.scalars).to eq(%i[name]) + end + + it "keeps a `#` that is part of a string or interpolation, not a comment" do + scan = described_class.scan(<<~RUBY) + LABEL = "tracking #1" + def create = params.require(:user).permit(:name) + RUBY + expect(scan.root).to eq(:user) + expect(scan.scalars).to eq(%i[name]) + end + + it "still reads quoted permit keys, which live in string tokens" do + scan = described_class.scan(%(params.permit("name", 'email', :age))) + expect(scan.scalars).to eq(%i[name email age]) + end + + it "falls back to the raw source when the file cannot be lexed" do + scan = described_class.scan("def broken( ; params.permit(:name)") + expect(scan.scalars).to eq(%i[name]) + end + end + describe ".draft from a model's columns" do before do ActiveRecord::Schema.define do