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

Show warning when Pod source uses unencrypted HTTP #7249

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@KrauseFx
Contributor

KrauseFx commented Dec 1, 2017

Please let me know if this should be part of cocoapods/cocoapods-downloader instead. There are 2 reasons why I decided to add it to pod_source_installer.rb as part of cocoapods/cocoapods instead:

  1. It seems like cocoapods/cocoapods-downloader was designed to not show any output, so it would add quite some extra complexity to show a warning as part of the check, without raising an exception
  2. cocoapods/cocoapods-downloader doesn't have the knowledge of what Pod it's trying to install, just the necessary information to download, cache and store the Pod. I thought it makes sense to show the Pod name as part of the warning.

This PR addresses the first half of #7238, related to #7250

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 1, 2017

LGTM.

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 1, 2017

Can you add tests for this?

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 1, 2017

Yeah, happy to add tests, just wanted to make sure this is the right approach first 👍

def verify_source_is_secure(root_spec)
return if root_spec.source.nil? || root_spec.source[:http].nil?
http_source = root_spec.source[:http]
# `:http` doesn't mean it's "http", might as well be "https://" here

This comment has been minimized.

@orta

orta Dec 1, 2017

Member

I'm not sure this comment makes sense in general - unless it's a dig at the key value, in which case it's not really adding much, so it can probably go :P

@orta

This comment has been minimized.

Member

orta commented Dec 1, 2017

Looks 👍 to me also with a test or two

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 1, 2017

Alright, just pushed tests, and they pass locally 👍

@@ -3,6 +3,7 @@
module Pod
describe Installer::PodSourceInstaller do
FIXTURE_HEAD = Dir.chdir(SpecHelper.fixture('banana-lib')) { `git rev-parse HEAD`.chomp }
WARNING_TEXT = 'Please reach out to the library author to notify them of this security issue'

This comment has been minimized.

@segiddins

segiddins Dec 1, 2017

Member

this probably shouldn't be a constant, and just be inlined into the specs

This comment has been minimized.

@KrauseFx

KrauseFx Dec 1, 2017

Contributor

Alright, will get rid of it 👍

http_source = root_spec.source[:http]
return if http_source.start_with?("https://")
UI.warn "'#{root_spec.name}' uses the unencrypted http protocol to transfer the Pod. " \

This comment has been minimized.

@segiddins

segiddins Dec 1, 2017

Member

my guess is this unconditionally warning might bother a bunch of people?

This comment has been minimized.

@KrauseFx

KrauseFx Dec 1, 2017

Contributor

Yes, I think that's kind of the idea, similar to what chrome does with the Not Secure badge, this way the users/customers of an SDK will also add pressure to the SDK providers to fix their infrastructure and get a (free, or cheap) SSL certificate.

This comment has been minimized.

@segiddins

segiddins Dec 1, 2017

Member

I personally don't like that.

This comment has been minimized.

@dnkoutso

dnkoutso Dec 1, 2017

Contributor

could we possibly go through an installation option that is on by default?

This comment has been minimized.

@justinseanmartin

justinseanmartin Dec 5, 2017

Contributor

I'm personally fine with this for now, and getting more strict in the future per #7260. With the change proposed in #7260, we can get rid of this warning, as consumers have explicitly opted in.

KrauseFx added a commit to KrauseFx/CocoaPods that referenced this pull request Dec 1, 2017

Prohibit SDK providers to use non-encrypted HTTP links for SDKs
Related to CocoaPods#7249, fixes CocoaPods#7238

I'll add tests, once I get the ok that this is the right location and the right approach. For now I used the `error` method to fail the validation. I believe that it's the right thing to do, there is no good excuse to transfer executable over non-encrypted protocols, however I do understand that this might throw off some SDK providers, and them being overwhelmed when they quickly need to push an update.

You all have more context around what the users are like, and if they have a way to work around this, just let me know what you're deciding on 👍
@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 1, 2017

Addressed the feedback and submitted the second PR with #7250

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 5, 2017

@KrauseFx when you get a chance can you rebase and add the installation option mentioned by #7260?

@dnkoutso dnkoutso added this to the 1.4.0 milestone Dec 5, 2017

@@ -30,6 +30,22 @@ module Pod
pod_folder = config.sandbox.pod_dir('BananaLib')
pod_folder.should.exist
end
it 'works fine if the source is encrypted using https' do

This comment has been minimized.

@dnkoutso

dnkoutso Dec 6, 2017

Contributor

nit: wording works fine is a bit generic here and can be it 'does not show warning if the source is encrypted using https'

This comment has been minimized.

@KrauseFx

KrauseFx Dec 6, 2017

Contributor

Done

end
it 'shows a warning if the source is unencrypted (e.g. http)' do
@spec.source = { :http => 'http://orta.io/sdk.zip' }

This comment has been minimized.

@orta

orta Dec 6, 2017

Member

hah - yeah, I should add https support sometime to my site

@@ -30,6 +30,22 @@ module Pod
pod_folder = config.sandbox.pod_dir('BananaLib')
pod_folder.should.exist
end
it 'does not show warning if the source is encrypted using https'

This comment has been minimized.

@dnkoutso

dnkoutso Dec 7, 2017

Contributor

missing do

@@ -20,6 +20,11 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
[Eric Amorde](https://github.com/amorde)
[#7093](https://github.com/CocoaPods/CocoaPods/pull/7093)
* Show warning when Pod source uses unencrypted HTTP

This comment has been minimized.

@dnkoutso

dnkoutso Dec 7, 2017

Contributor

nit of nits and since there is a build failure, remove extra line

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 9, 2017

Just needs a rebase to get rid of merge commits as Danger says and it should go green.

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 11, 2017

Moved over to #7276 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment