Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect from crashes when using Hash#dig #450

Merged
merged 5 commits into from
Feb 8, 2022
Merged

Protect from crashes when using Hash#dig #450

merged 5 commits into from
Feb 8, 2022

Conversation

jonabc
Copy link
Contributor

@jonabc jonabc commented Feb 7, 2022

resolves #448 #449

This PR makes two changes to protect against failures reported String does not have #dig method (TypeError)

  1. In the npm source, it's possible for a peerDependencies value to be set as "[Circular]". I've only been able to reproduce this on npm 6.x. This was tricky to track down as the error is being thrown internally from Hash#dig. From the line of code in licensed parent&.dig("peerDependencies", name), parent here is a hash but parent["peerDependencies"] is a string. If I had to guess, I'd bet that the dig method is implemented using recursion like:
def dig(*parts)
  key = parts[0]
  value = self[key]
  # no more key path parts to evaluate or value is nil, complete recursion
  return value if parts.length == 1 || value == nil

  # call dig on remaining key path parts
  # this is not resilient to the value not being a hash
  # and most likely value is not previously tested with respond_to?(:dig)
  value.dig(parts[1..])
end
  1. A safety check in the pip source when using dig to evaluate the python config. There is an expectation that a user has set the python key in the configuration file to a YAML hash, but in this case it looks like it was set to a string

@jonabc
Copy link
Contributor Author

jonabc commented Feb 7, 2022

I put up a change to the npm test fixture that I used to reproduce the failure case for the npm source on it's own to kick off CI. It should show the failure, where a successful CI run from a commit including the fix will validate the fix. Since this issue is not related to an expected output from licensed, I don't think there's anything to add a specific test case or validation around.

EDIT: 👍 failing run. The fix has resolved the issue as shown below.

@jonabc jonabc merged commit 10a6de9 into master Feb 8, 2022
@jonabc jonabc deleted the dig-protection branch February 8, 2022 02:44
jonabc added a commit that referenced this pull request Feb 8, 2022
## 3.4.4

2022-02-07

### Fixed

- The npm and pip sources have better protection from strings causing crashes in `Hash#dig` (#450)
@jonabc jonabc mentioned this pull request Feb 8, 2022
@RobbieTheWagner
Copy link

I'm still getting this error with 4.0.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Licensed crashing evaluating NPM sources
2 participants