-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add new Bundler/InsecureProtocolSource
cop
#4720
Conversation
|
||
def on_send(node) | ||
insecure_protocol_source?(node) do | ||
source = source_node(node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send
nodes are decorated with node extensions that expose send_node.arguments
, send_node.first_argument
and send_node.last_argument
. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use captures in the node pattern to extract the parts you want, using $
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice. I updated with node extensions API and $
. 5efbbe1
Description: >- | ||
The source `:gemcutter`, `:rubygems` and `:rubyforge` are deprecated | ||
because HTTP requests are insecure. Please change your source to | ||
'https://rubygems.org' if possible, or 'http://rubygems.org' if not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is using http://rubygems.org
better than using :rubygems
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It is preferable to use source 'http://rubygems.org'
instead of specifying a symbol such as source :rubygems
.
Of course, the best thing is to use source 'https://rubygems.org'
.
The following is an example of converting symbol into HTTP strings.
If source :rubygems
is specified for Gemfile, remote: http://rubygems.org/
(HTTP) is output to Gemfile.lock.
This is the same when specified the string source 'http://rubygems.org'
.
% cat Gemfile
# frozen_string_literal: true
source :rubygems
gem 'rubocop'
% head Gemfile.lock
GEM
remote: http://rubygems.org/
specs:
ast (2.3.0)
parallel (1.12.0)
parser (2.4.0.0)
ast (~> 2.2)
powerpack (0.1.1)
rainbow (2.2.2)
rake
On the other hand, If source 'https://rubygems.org'
is specified for Gemfile, remote: https://rubygems.org/
(HTTPS) is output to Gemfile.lock.
% cat Gemfile
# frozen_string_literal: true
source 'https://rubygems.org'
gem 'rubocop'
% head Gemfile.lock
GEM
remote: https://rubygems.org/
specs:
ast (2.3.0)
parallel (1.12.0)
parser (2.4.0.0)
ast (~> 2.2)
powerpack (0.1.1)
rainbow (2.2.2)
rake
I'm sorry if I misunderstood the intention of question.
# | ||
# # good | ||
# source 'https://rubygems.org' # strongly recommended | ||
# source 'http://rubygems.org' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me this should be flagged as well. As should any source referred by a symbol or starting with http://.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we do this auto-correction will be limitted, but I think the cop would be more useful it were more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I have reasons why I can not make a decision to replace all of source
with http://
to https://
. For example, when specifying an internal gem server using HTTP on the intranet, a use case where HTTPS can not be specified was considered. For this reason, this cop replaces only the special case rubygems.org with http://
to https://
.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this if the class documentation explains all of this, so people wouldn't wonder why some sources are flagged and others are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the class documentation with 48676d6.
2e55cba
to
4ddedb9
Compare
4ddedb9
to
48676d6
Compare
I guess that's good enough for now, but we can add to it some option to forbid completely |
Thanks for the review. I will be noted about the option to forbid |
Feature
This cop emulates a Bundler's deprecation warning.
Target Problem
This cop emulates Bundler's deprecation warning as below.
https://github.com/bundler/bundler/blob/v1.16.0.pre.2/lib/bundler/dsl.rb#L442-L446
This warning recommends using as secure a
https
protocol as possible. The argument (:gemcutter
,:rubygems
and:rubyforge
) to replace the insecurehttp
is deprecated.Other Information
As shown in Bundler's warning, users who can not use
https
may want to replace them withhttp
. However, in many cases I think that it will be replaced withhttps
.And, the default in Gemfile generated by famous generators like
rails new
andbundle init
is already usingsource 'https://rubygems.org'
.Gemfile template of
rails new
.https://github.com/rails/rails/blob/v5.1.4/railties/lib/rails/generators/rails/app/templates/Gemfile#L1
Gemfile template of
bundle init
.https://github.com/bundler/bundler/blob/v1.16.0.pre.2/lib/bundler/templates/Gemfile#L3
Therefore, using autocorrect will replace with
source 'https://rubygems.org'
.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).rake spec
) are passing.rake internal_investigation
.and description in grammatically correct, complete sentences.
rake generate_cops_documentation
(required only when you've added a new cop or changed the configuration/documentation of an existing cop).