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

Fix typo_squatting? false positive for rubygems.org itself #3951

Merged
merged 1 commit into from Sep 25, 2020
Merged

Fix typo_squatting? false positive for rubygems.org itself #3951

merged 1 commit into from Sep 25, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 17, 2020

Description:

Correct typo_squatting behaviour to allow adding https://rubygems.org without triggering input

What was the end-user or developer problem that led to this PR?

AMI prebuilt using internal gem server + another scripted scenario for some of our builds which re-add https://rubygems.org

This triggers the prompt saying that https://rubygems.org is similar to https://rubygems.org which totally breaks the script due to a) No option to default -y b) Strict tty checks with no default which cause a failure and mean for horrible workarounds c) This specific issue which is independent but contributes to the overall problem.

What is your fix for the problem, implemented in this PR?

Ensure that correctly entered https://rubygems.org does not raise the result that it is similar, because it is not similar but identical and poses no issue with the desired effect (that stops folks accidentally configuring camped domains and getting supply chain attacks).

We do this by instead of using <= distrance_threshold (which also covers zero) by instead using a range check from 1..distance_threshold


Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@welcome
Copy link

welcome bot commented Sep 17, 2020

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

… entered

typo_squatting? feature was implemented to avoid accidental configuration
of gem sources where domain squatting was likely and as such uses
a levenshtein algo to spot similar domain names

a score of 0 means that the names entered actually match which triggers
the squatting safety check in the gem command when https://rubygems.org is
legitimately re-added

noting this breaks some automation code in the real world
@simi
Copy link
Member

simi commented Sep 17, 2020

I do agree on handling this better than current state, but wouldn't be better to check if that source is already present and just warn nothing is happening since that source is already present (ant thus there's nothing to do)?

@sean-msmg
Copy link

The typo_squatting? check is hard-coded to rubygems.org, not based on existing sources. The bug means you can't clear out all sources then re-add rubygems.org

if source.typo_squatting?("rubygems.org")

@ghost
Copy link
Author

ghost commented Sep 17, 2020

@simi this would be a workaround, I've tried to stay faithfully to ensuring the typo_squatting? method more accurately reflects what it was supposed to do as per the original case, which is to spot nearly identical but not identical matches and avoid folks becoming vulnerable to MITM/supply chain attacks.

There are a number of issues in working around it for us and this is only one of the defects really that should be raised against the current implementation. I chose the one that seemed to provide correctness in the single method implementation against the original driver.

@ghost
Copy link
Author

ghost commented Sep 17, 2020

The typo_squatting? check is hard-coded to rubygems.org, not based on existing sources. The bug means you can't clear out all sources then re-add rubygems.org

if source.typo_squatting?("rubygems.org")

For clarity @simi this is what we have.

We have a cloud pipeline with servers built with configurations that already exclude https://rubygems.org in favour of our servers. We have some mechanisms for individual hosts that need to reset this on system build and this mechanism works fine with earlier versions of gem but we are trying to migrate forward to gem 3.x

Copy link
Member

@simi simi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for additional info, that makes it clear.

PS: We should also support for -y or --force flag later as well to not block this quick fix.

@ghost
Copy link
Author

ghost commented Sep 17, 2020

Thanks for additional info, that makes it clear.

PS: We should also support for -y or --force flag later as well to not block this quick fix.

Thanks @simi , yes for the related impediments I was considering doing this also in a seperate PR.

@simi
Copy link
Member

simi commented Sep 17, 2020

@andy-smith-msm I have created placeholder for followup - #3955.

@ghost
Copy link
Author

ghost commented Sep 17, 2020

@andy-smith-msm I have created placeholder for followup - #3955.

good stuff, I think this will also be really useful to us so I'll try to spend some time on this and quote #3955 in the commit ID

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too!

@deivid-rodriguez deivid-rodriguez changed the title typo_squatting? should not return true when rubygems.org is correctly… Fix typo_squatting? false positive for rubygems.org itself Sep 18, 2020
@deivid-rodriguez
Copy link
Member

Thanks for this @andy-smith-msm!

@deivid-rodriguez deivid-rodriguez merged commit 481d806 into rubygems:master Sep 25, 2020
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
…_with_no_typo

Fix `typo_squatting?` false positive for `rubygems.org` itself

(cherry picked from commit 481d806)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
…_with_no_typo

Fix `typo_squatting?` false positive for `rubygems.org` itself

(cherry picked from commit 481d806)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants