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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show warning when SDK provider tries to push a version with an unencrypted HTTP source #7250

Merged
merged 10 commits into from Dec 5, 2017

Conversation

Projects
None yet
6 participants
@KrauseFx
Contributor

KrauseFx commented Dec 1, 2017

Related to #7249, fixes #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 馃憤

Prohibit SDK providers to use non-encrypted HTTP links for SDKs
Related to #7249, fixes #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 馃憤
@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 2, 2017

LGTM.

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 2, 2017

Alright, thanks @dnkoutso, will update the Changelog and add a test 馃憤

KrauseFx added some commits Dec 2, 2017

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 2, 2017

Use bundle exec rubocop -a to ensure its using the rules of rubocop with the lib.

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 2, 2017

Ha, I did the rubocop command, it replaced the code style of 10,000 files (feels like). That's because the rubocop version isn't locked in the Gemfile, and I ran bundle update. What version do you have installed? Happy to submit a PR to the Gemfile 馃憤

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 2, 2017

Ready for review 馃憤

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 2, 2017

It looks like its locked in Gemfile.lock

rubocop (0.37.2)
@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 2, 2017

Ah right, yah, that's why it didn't work for me, as I ran bundle update locally :)

@orta

This comment has been minimized.

Member

orta commented Dec 2, 2017

OK, let's make this a warning in 1.4.0 and tell people it will be an error in 1.6.0. Warnings can be skipped, but errors cannot. So I'd rather give people some time to adjust to the change.

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 3, 2017

Makes sense, will update the PR and the tests 馃憤

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 3, 2017

Done 馃憤 Ready to be merged from my side whenever :)

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 4, 2017

@paulb777 @justinseanmartin @dantoml are you OK with this change?

@dantoml

This comment has been minimized.

Member

dantoml commented Dec 4, 2017

Yep - I have a couple of wording changes I want to make, WRT a timeline for requiring https, will review properly today

@paulb777

Looks good! Just a few wording nits ...

return if url.downcase.start_with?('https://')
warning('http', "The URL (#{url}) doesn't use the encrypted HTTPs protocol. " \
'It is crucial for Pods to be transferred over a secure protocol to protect your users from man-in-the-middle attacks. '\
'Please update the URL to use https and try again.')

This comment has been minimized.

@paulb777

paulb777 Dec 4, 2017

Member

'This will be an error in future releases. Please update the URL to use https.'

@@ -16,6 +16,10 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
[Eric Amorde](https://github.com/amorde)
[#7093](https://github.com/CocoaPods/CocoaPods/pull/7093)
* Prohibit SDK providers to use non-encrypted HTTP links for SDKs

This comment has been minimized.

@paulb777

paulb777 Dec 4, 2017

Member

s/to use/from using/

This comment has been minimized.

@justinseanmartin

justinseanmartin Dec 4, 2017

Contributor

Would also soften the wording of "Prohibit" to "Warn against" or something like that, but not a big deal.

@dantoml

This comment has been minimized.

Member

dantoml commented Dec 4, 2017

That said, I'm ok with merging this is as is and following up in a second pr

@justinseanmartin

This comment has been minimized.

Contributor

justinseanmartin commented Dec 4, 2017

IIUC, this will generate a warning on pod spec lint / pod lib lint, but not prevent or warn someone from consuming the podspec on the other side. I assume the publisher can still bypass the warning with --allow-warnings as well. Is that right?

Assuming the above, this seems like a good first step. As a followup, on the consumption side we should require an explicit installation option of install! 'cocoapods', allow_insecure_source_urls: true to protect users from installing pods that have previously been published with http URLs.

KrauseFx added some commits Dec 5, 2017

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 5, 2017

Updated 馃憤 Thanks for the feedback everyone 馃憤

return if spec.source.nil? || spec.source[:http].nil?
url = spec.source[:http]
return if url.downcase.start_with?('https://')
warning('http', "The URL (#{url}) doesn't use the encrypted HTTPs protocol. " \

This comment has been minimized.

@dnkoutso

dnkoutso Dec 5, 2017

Contributor

this is a nit but in most places around this file the strings are wrapped with backticks, so "The URL (`#{url}`)..."

@@ -16,6 +16,10 @@ 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 SDK provider tries to push a version with an unencrypted HTTP source

This comment has been minimized.

@dnkoutso

dnkoutso Dec 5, 2017

Contributor

nit, add two trailing spaces in the end of this line

This comment has been minimized.

@dnkoutso

dnkoutso Dec 5, 2017

Contributor

this line could also be updated to the commit message, as the word "prohibit" is not in effect here.

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 5, 2017

Tiny nits.

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

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 5, 2017

Best nits still :) Will address

@orta

This comment has been minimized.

Member

orta commented Dec 5, 2017

Assuming the above, this seems like a good first step. As a followup, on the consumption side we should require an explicit installation option of install! 'cocoapods', allow_insecure_source_urls: true to protect users from installing pods that have previously been published with http URLs.

I think this makes sense as a part of the "1.6" or whenever we declare switch it to an error. Worth making this a TODO: issue for it. 馃憤

@KrauseFx KrauseFx changed the title from Prohibit SDK providers to use non-encrypted HTTP links for SDKs to Show warning when SDK provider tries to push a version with an unencrypted HTTP source Dec 5, 2017

KrauseFx added some commits Dec 5, 2017

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 5, 2017

Updated the PR, thanks @dnkoutso 馃憤

@KrauseFx

This comment has been minimized.

Contributor

KrauseFx commented Dec 5, 2017

Mh, it doesn't seem like the Travis error was introduced with my PR?

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Dec 5, 2017

Nope its not related to you. I need to fix this but I rekicked yours. Should go green.

@dnkoutso dnkoutso merged commit 33a2951 into CocoaPods:master Dec 5, 2017

3 checks passed

Danger All green. Good on 'ya.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@KrauseFx KrauseFx deleted the KrauseFx:patch-1 branch Dec 5, 2017

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

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