Skip to content

Comments

Adds type hinting for scalar types in ICrypto->decrypt#7825

Merged
rullzer merged 2 commits intomasterfrom
add-type-hinting-for-ICrypto-decrypt
Jan 13, 2018
Merged

Adds type hinting for scalar types in ICrypto->decrypt#7825
rullzer merged 2 commits intomasterfrom
add-type-hinting-for-ICrypto-decrypt

Conversation

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Jan 12, 2018

A side effect of this is that failures could now happen regarding the type of a parameter even if the other file does not use strict typing. But it would make the original error of #7824 a lot more clear:

It now would cause something like this (which immediately points out the root of the issue):

{
    "Code": 0,
    "Exception": "TypeError",
    "File": "lib/private/Security/Crypto.php",
    "Line": 112,
    "Message": "Argument 1 passed to OC\\Security\\Crypto::decrypt() must be of the type string, null given, called in lib/private/Session/CryptoSessionData.php on line 82",
    "Trace": "
        #0 lib/private/Session/CryptoSessionData.php(82): OC\\Security\\Crypto->decrypt(*** sensitive parameters replaced ***)
        #1 lib/private/Session/CryptoSessionData.php(63): OC\\Session\\CryptoSessionData->initializeSession()
        #2 lib/private/Session/CryptoWrapper.php(101): OC\\Session\\CryptoSessionData->__construct(Object(OC\\Session\\Memory), Object(OC\\Security\\Crypto), '7tkRtGTgI79Dtd7...')
        #3 cron.php(60): OC\\Session\\CryptoWrapper->wrapSession(Object(OC\\Session\\Memory))
        #4 {main}
    "
}

instead of this:

{
    "Code": 0,
    "Exception": "TypeError",
    "File": "lib/private/Security/Crypto.php",
    "Line": 118,
    "Message": "explode() expects parameter 2 to be string, null given",
    "Trace": "
        #0 lib/private/Security/Crypto.php(118): explode('|', NULL)
        #1 lib/private/Session/CryptoSessionData.php(82): OC\\Security\\Crypto->decrypt(*** sensitive parameters replaced ***)
        #2 lib/private/Session/CryptoSessionData.php(63): OC\\Session\\CryptoSessionData->initializeSession()
        #3 lib/private/Session\\/CryptoWrapper.php(101): OC\\Session\\CryptoSessionData->__construct(Object(OC\\Session\\Memory), Object(OC\\Security\\Crypto), '7tkRtGTgI79Dtd7...')
        #4 cron.php(60): OC\\Session\\CryptoWrapper->wrapSession(Object(OC\\Session\\Memory))
        #5 {main}
    "
}

@schiessle @blizzz @skjnldsv @juliushaertl @LukasReschke @irgendwie @Henni @pixelipo @rullzer @nickvergessen @daita @oparoz @karlitschek Does this makes sense for you? I guess it helps us a lot, but will also cause quite a lot of (valid) issues in the existing code.

@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Jan 12, 2018
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Jan 12, 2018
@rullzer
Copy link
Member

rullzer commented Jan 12, 2018

YES!!! This is one of the major benefits of >=7.0! Doing this will make our code more robust. The phpdoc is already telling you since forever what to pass. Less strict declarations to allow undefined behavior are a bad idea :)

So yes please :) 👍 🚀

@MorrisJobke MorrisJobke mentioned this pull request Jan 12, 2018
Closed
@rullzer rullzer force-pushed the add-type-hinting-for-ICrypto-decrypt branch from cf8c2ad to d5dc1bd Compare January 12, 2018 21:41
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 12, 2018
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Jan 12, 2018

This also then can be used by the strict type hinting in #7392

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jan 12, 2018
@MorrisJobke MorrisJobke force-pushed the add-type-hinting-for-ICrypto-decrypt branch from d5dc1bd to ca28df6 Compare January 12, 2018 23:06
@codecov
Copy link

codecov bot commented Jan 12, 2018 •
edited
Loading

Codecov Report

Merging #7825 into master will decrease coverage by 16.49%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #7825      +/-   ##
============================================
- Coverage     51.23%   34.73%   -16.5%     
+ Complexity    24975    24956      -19     
============================================
  Files          1607     1607              
  Lines         95008    95046      +38     
  Branches       1376     1376              
============================================
- Hits          48674    33018   -15656     
- Misses        46334    62028   +15694
Impacted Files Coverage Δ Complexity Δ
lib/private/Security/Crypto.php 11.76% <0%> (-85.3%) 9 <4> (ø)
lib/private/App/AppStore/Bundles/Bundle.php 0% <0%> (-100%) 2% <0%> (ø)
apps/files/lib/Activity/Settings/FileDeleted.php 0% <0%> (-100%) 8% <0%> (ø)
...eware/Security/Exceptions/NotLoggedInException.php 0% <0%> (-100%) 1% <0%> (ø)
...mments/tests/Unit/Controller/NotificationsTest.php 0% <0%> (-100%) 4% <0%> (ø)
apps/files_trashbin/appinfo/app.php 0% <0%> (-100%) 0% <0%> (ø)
lib/public/AppFramework/Http/StreamResponse.php 0% <0%> (-100%) 6% <0%> (ø)
lib/public/AppFramework/Middleware.php 0% <0%> (-100%) 4% <0%> (ø)
lib/public/AppFramework/Http/JSONResponse.php 0% <0%> (-100%) 5% <0%> (ø)
...te/Authentication/Token/DefaultTokenCleanupJob.php 0% <0%> (-100%) 1% <0%> (ø)
... and 556 more

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Jan 12, 2018
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Jan 12, 2018

Rebased to get the fix for the failing unit tests in this PR: #7824

@rullzer
Fix tests
8d1dd19 
Signed-off-by: Roeland Jago Douma <[email protected]>
@codecov-io
Copy link

codecov-io commented Jan 13, 2018

Codecov Report

Merging #7825 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7825      +/-   ##
============================================
- Coverage     51.23%   51.21%   -0.03%     
+ Complexity    24975    24956      -19     
============================================
  Files          1607     1607              
  Lines         95008    95046      +38     
  Branches       1376     1376              
============================================
  Hits          48674    48674              
- Misses        46334    46372      +38
Impacted Files Coverage Δ Complexity Δ
lib/private/Security/Crypto.php 97.05% <100%> (ø) 9 <4> (ø) ⬇️
apps/federation/composer/composer/ClassLoader.php 0% <0%> (ø) 64% <0%> (-1%) ⬇️
...pps/comments/composer/composer/autoload_static.php 0% <0%> (ø) 1% <0%> (ø) ⬇️
apps/systemtags/composer/composer/ClassLoader.php 0% <0%> (ø) 64% <0%> (-1%) ⬇️
.../admin_audit/composer/composer/autoload_static.php 83.33% <0%> (ø) 2% <0%> (ø) ⬇️
apps/oauth2/composer/composer/ClassLoader.php 0% <0%> (ø) 64% <0%> (-1%) ⬇️
apps/oauth2/composer/composer/autoload_static.php 0% <0%> (ø) 1% <0%> (ø) ⬇️
...ps/files_sharing/composer/composer/ClassLoader.php 0% <0%> (ø) 64% <0%> (-1%) ⬇️
.../sharebymail/composer/composer/autoload_static.php 0% <0%> (ø) 1% <0%> (ø) ⬇️
...ratedfilesharing/composer/composer/ClassLoader.php 0% <0%> (ø) 64% <0%> (-1%) ⬇️
... and 29 more

@rullzer rullzer merged commit 60f38d3 into master Jan 13, 2018
@rullzer rullzer deleted the add-type-hinting-for-ICrypto-decrypt branch January 13, 2018 14:11
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Jan 16, 2018

ref (#7392)

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

Reviewers

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@blizzz blizzz blizzz approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

5 participants

@MorrisJobke @rullzer @codecov-io @ChristophWurst @blizzz