Prevent set_attrs deep merge from writing nested keys into top-level attrs#484
Open
tabuchid wants to merge 1 commit into
Open
Prevent set_attrs deep merge from writing nested keys into top-level attrs#484tabuchid wants to merge 1 commit into
tabuchid wants to merge 1 commit into
Conversation
…attrs When set_attrs is called with clobber=false and the incoming hash contains a nested hash under a key the target does not yet have (target[k] is nil), the recursive call received nil as its target. The 'target ||= @attrs' fallback then silently redirected the merge to the TOP-LEVEL attrs hash, writing the nested hash's keys there. Example: on an issue built from a sparse search result whose attrs lack fields.project, save! merging {"fields" => {"project" => {"key" => "REL"}}} overwrote the issue's own top-level "key" (e.g. "REL-77") with the project key "REL", corrupting every URL built from issue.key afterwards. Fix: before recursing, ensure target[k] is a Hash. Create it when missing, and replace it when a scalar is being superseded by a hash (previously that case raised IndexError by indexing into a String). The nil target fallback now only applies to the initial call, as the 'internal use only' comment intended.
Author
|
@SimonMiaou I saw the previous PR stalled from lack of tests. Hopefully this covers it and unblocks. LMK if there is any feedback or anything I can do |
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.
Same root cause as #176, open since 2016. The review there asked for a test before merging, so this PR brings the tests, plus coverage for a second edge that patch misses.
set_attrs(hash, false)deep-merges into@attrs, but when the incoming hash has a nested hash under a key the resource doesn't have yet, the recursion getstarget[k]= nil and thetarget ||= @attrsfallback sends the merge to the top level instead:In practice you hit this through
save!, which merges the request payload back into attrs. Any resource with sparse attrs is exposed, e.g. an issue built from a/search/jqlresult. In our app, saving fields onto an issue loaded from a search result overwrote the issue's ownkeywith the project key, which broke every URL built fromissue.keyafterwards.There's a second edge: when
target[k]is a scalar and the incoming value is a hash, the recursion raisesIndexError: string not matched(it indexes into a String). The #176 patch (target[k] = {} if !target.key?(k) || target[k].nil?) fixes the nil case but not this one.The fix is one line:
target[k] = {} unless target[k].is_a?(Hash)before recursing. It creates the missing intermediate hash, replaces a scalar being superseded by a hash, and confines the nil-target fallback to the initial call, which is what the "internal use only" comment ontargetintended.Tests: three new examples in the
set_attrsblock ofbase_spec.rb— the repro above, a merge into an existing nested key to show unchanged behavior, and the scalar case. Full suite is green apart from the 78 pre-existing failures inspec/jira/atlassian/jwt_spec.rb, which fail identically on master.