Rails 4.0 patterns: validation anchors + accepts_nested_attributes_for#105
Open
JuanVqz wants to merge 5 commits into
Open
Rails 4.0 patterns: validation anchors + accepts_nested_attributes_for#105JuanVqz wants to merge 5 commits into
JuanVqz wants to merge 5 commits into
Conversation
Rails 4 raises ArgumentError: regular expression is using multiline
anchors (^ or $), which may present a security risk whenever a
validates_format_of regex uses ^ or $. The validator runs
check_options_validity at model class-load, so the failure is
boot-fatal.
Fix is mechanical: replace ^...$ with \A...\z (absolute start/end of
string). Ruby's regex engine accepts both forms on every Rails
version, so the rewrite ships pre-bump without a NextRails.next?
branch.
The primary regex catches only the literal validates_format_of
token. Two known blind spots documented in the explanation/fix:
- Block-options form: validates :col, format: { with: /^.../ } or
validates :col, format: /^.../. Same error, different AST. The
fix field provides a secondary grep targeting this shape.
- Method-call regex args: validates_format_of :col, with:
regex_for(:something). The regex body is invisible to grep. Fix
at the helper method's return value — one fix covers every
caller.
Classified high_priority / kind: breaking — boot-fatal.
Companion to VALIDATES_FORMAT_OF_ANCHORS. The primary anchor grep
looks for literal /^.../ regexes inline in validates_format_of
calls; it does not see regex constants defined via Regexp.new:
EMAIL = Regexp.new(%q{^[^@]+@[^@]+$})
validates_format_of :address, with: EMAIL
The validator-call line has no anchors visible to grep, but Rails 4
still raises at model class-load when check_options_validity
examines the constant's compiled regex.
This entry has its own detection target — constant definitions, not
validator calls — so it gets its own pattern and variable_name.
Regex covers the three common construction forms:
Regexp.new(%q{^...$})
Regexp.new("^...$")
Regexp.new(%r{^...$})
Fix is the same rewrite as the primary entry but applied at the
constant's definition. One fix covers every caller, so detection
plus per-constant rewrite is a small fixed-cost effort.
Per-match judgment is required because not every Regexp.new
constant feeds a validator. String#match / String#scan callers
operating on multi-line input may need ^ / $ for start/end-of-line
semantics; the fix field calls this out and recommends a call-site
grep before rewriting.
Classified high_priority / kind: breaking — boot-fatal at model
class-load.
Rails 3.2 accepted accepts_nested_attributes_for :foo even when :foo was not a declared association — projects sometimes paired it with a hand-rolled reader (polymorphic lookup, memoized custom join) and Rails generated the foo_attributes= writer against whatever the override returned. Rails 4.0 looks up the reflection at class-load and raises ArgumentError: No association found for name 'foo' at boot. The protected_attributes bridge gem (commonly added during the 3.2 → 4.0 hop to keep attr_accessible alive) hits the same check earlier in its accessible_attributes setup, so the error can surface before AR's own reflection lookup runs. Boot-fatal — the offending class fails to load before any spec runs, taking the suite down. Fix is branched: add belongs_to :foo, polymorphic: true (or the matching has_one / has_many) under `if NextRails.next?`. Ruby method lookup resolves the explicit `def foo` first, so the hand-rolled override keeps winning while the reflection check passes on the next side. Why the gate: declaring unconditionally on the current side introduces a new auto-generated reader+writer on Rails 3.2 where neither existed. Even if `def foo` shadows the reader (placement-dependent), the new writer makes `record.foo = ...` silently succeed where it previously raised NoMethodError. Inheritance pitfall documented in the explanation/fix: the missing reflection may be a side effect of a parallel gem bump (e.g. paper_trail 2.x's top-level Version moves to PaperTrail::Version in 3.x — the inherited reflection disappears with no app code change). Check the gem's migration docs before patching the subclass. The primary regex catches all accepts_nested_attributes_for calls; most matches are benign (model declares both the macro and the matching association). Operator filters to the breaking ones by inspecting each model — the explanation calls this out so reviewers don't expect every hit to be actionable. Classified high_priority / kind: breaking.
…loss The prior entry claimed Rails 4.0 newly raises on a missing association vs 3.2 silently allowing it. That is false: the ArgumentError raise in accepts_nested_attributes_for is byte-identical at v3.2.22.5 and v4.0.0 and predates both. A genuinely-missing association never booted on 3.2, so it is not a 3.2 to 4.0 regression. Reframe around the real trigger: an inherited association reflection from a gem-supplied base class disappears when that gem is bumped in Gemfile.next (paper_trail 2.x top-level Version to 3.x PaperTrail::Version). Document both remedies: root-cause re-point of the base class on the next side, and the per-model gated re-declaration. Strip app-specific detail.
VALIDATES_FORMAT_OF_ANCHORS only matched a leading caret, missing
trailing-dollar-only regexes (with: /foo$/) that Rails 4 also rejects.
Widen to [\^$]. VALIDATES_FORMAT_OF_ANCHORS_CONSTANT_BOUND truncated at
the first ) so grouped regexes (Regexp.new("(a|b)$")) were missed; drop
the [^)] cap. Quote the verbatim v4.0.0 ArgumentError message, note the
check is new in 4.0, and document that only a leading ^ or trailing $
actually raises (char-class ^ is a false positive). Drop the stray
NextRails.next? reference from the anchors fix - the rewrite is
backwards-compatible and needs no gate.
3fa9665 to
aac8272
Compare
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.
Summary
Three new Rails 4.0 detection patterns. All three break boot when the model class loads, and the current skill does not catch them.
VALIDATES_FORMAT_OF_ANCHORS— Rails 4 raisesArgumentError: The provided regular expression is using multiline anchors (^ or $)...when avalidates_format_ofregex starts with^or ends with$. This check is new in 4.0 (Rails 3.2 does not have it), checked againstformat.rbat v3.2.22.5 vs v4.0.0. Fix is\A/\z, works on both versions, no gate. The regex now matches both anchors ([\^$]), so trailing-$-only forms likewith: /foo$/are caught. Only a leading^or trailing$actually raises, so a^inside a character class (/[^@]+/) is a false positive — noted in the entry. Block-options and method-call forms are not caught by the main regex; a second grep is given for them.VALIDATES_FORMAT_OF_ANCHORS_CONSTANT_BOUND— catches regex constants built withRegexp.new(%q{^...$})that feed validators, which the inline grep misses. The regex no longer stops at the first), so grouped patterns likeRegexp.new("(a|b)$")are caught. Check each hit by hand — not everyRegexp.newconstant feeds a validator, and only a leading^or trailing$raises.ACCEPTS_NESTED_NO_REFLECTION—accepts_nested_attributes_for :fooraisesArgumentError: No association found for name 'foo'at class-load when:foohas no association. This raise is not new in Rails 4.0 (it fires on any version when the association is missing), so a missing association is not a 3.2 → 4.0 regression. It shows up as a new failure on the next side only when the association comes from a gem's base class through inheritance, and a gem bump inGemfile.nextremoves it. Example: paper_trail 2.x has a top-levelVersion(withbelongs_to :item); 3.x renames it toPaperTrail::Versionand drops the top-levelVersion, so models written as< Versionlose the inherited association on the next side only. A hit is only a problem when the association is inherited from a gem's base class and that gem is being bumped inGemfile.next. Two fixes given: re-point the base class on the next side in one place (Version = PaperTrail::Version if NextRails.next?), or add the association per model (belongs_to ... if NextRails.next?, gated so the current side does not get a duplicate reader/writer). A hand-writtendef foooverride still wins either way.All three classified
high_priority/kind: breaking.Test plan
bin/validate-patterns rails-upgrade/detection-scripts/patterns/rails-40-patterns.ymlclean.validates_format_of,Regexp.newconstants,accepts_nested_attributes_formacros).