Avoid possible shredding of passed cred on reject (#28448)

JuliaLang · Aug 5, 2018 · 696700f · 696700f
1 parent e4d933d
commit 696700f
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 7 deletions.
diff --git a/stdlib/LibGit2/src/types.jl b/stdlib/LibGit2/src/types.jl
@@ -1303,8 +1303,9 @@ end
 
 function approve(cache::CachedCredentials, cred::AbstractCredential, url::AbstractString)
     cred_id = credential_identifier(url)
-    if haskey(cache.cred, cred_id) && cred !== cache.cred[cred_id]
-         Base.shred!(cache.cred[cred_id])
+    if haskey(cache.cred, cred_id)
+        # Shred the cached credential we'll be overwriting if it isn't identical
+        cred !== cache.cred[cred_id] && Base.shred!(cache.cred[cred_id])
     end
     cache.cred[cred_id] = cred
     nothing
@@ -1313,7 +1314,8 @@ end
 function reject(cache::CachedCredentials, cred::AbstractCredential, url::AbstractString)
     cred_id = credential_identifier(url)
     if haskey(cache.cred, cred_id)
-        Base.shred!(cache.cred[cred_id])
+        # Shred the cached credential if it isn't the `cred` passed in
+        cred !== cache.cred[cred_id] && Base.shred!(cache.cred[cred_id])
         delete!(cache.cred, cred_id)
     end
     nothing
@@ -1413,6 +1415,8 @@ function approve(p::CredentialPayload; shred::Bool=true)
     cred = p.credential
     cred === nothing && return  # No credential was used
 
+    # Each `approve` call needs to avoid shredding the passed in credential as we need
+    # the credential information intact for subsequent approve calls.
     if p.cache !== nothing
         approve(p.cache, cred, p.url)
         shred = false  # Avoid wiping `cred` as this would also wipe the cached copy
@@ -1441,6 +1445,8 @@ function reject(p::CredentialPayload; shred::Bool=true)
     cred = p.credential
     cred === nothing && return  # No credential was used
 
+    # Note: each `reject` call needs to avoid shredding the passed in credential as we need
+    # the credential information intact for subsequent reject calls.
     if p.cache !== nothing
         reject(p.cache, cred, p.url)
     end

diff --git a/stdlib/LibGit2/test/libgit2.jl b/stdlib/LibGit2/test/libgit2.jl
@@ -1741,20 +1741,35 @@ mktempdir() do dir
 
         # Overwrite an already cached credential
         dup_cred = deepcopy(cred)
-        LibGit2.approve(cache, dup_cred, url)  # Shreds `cred`
+        LibGit2.approve(cache, dup_cred, url)  # Shreds overwritten `cred`
         @test haskey(cache, cred_id)
         @test cache[cred_id] === dup_cred
-        @test dup_cred.pass == password
+        @test cred.user != "julia"
         @test cred.pass != password
+        @test dup_cred.user == "julia"
+        @test dup_cred.pass == password
 
         cred = dup_cred
 
-        # Reject an approved should cause it to be removed and shredded
-        LibGit2.reject(cache, cred, url)
+        # Reject an approved credential
+        @test cache[cred_id] === cred
+        LibGit2.reject(cache, cred, url)  # Avoids shredding the credential passed in
+        @test !haskey(cache, cred_id)
+        @test cred.user == "julia"
+        @test cred.pass == password
+
+        # Reject and shred an approved credential
+        dup_cred = deepcopy(cred)
+        LibGit2.approve(cache, cred, url)
+
+        LibGit2.reject(cache, dup_cred, url)  # Shred `cred` but not passed in `dup_cred`
         @test !haskey(cache, cred_id)
         @test cred.user != "julia"
         @test cred.pass != password
+        @test dup_cred.user == "julia"
+        @test dup_cred.pass == password
 
+        Base.shred!(dup_cred)
         Base.shred!(cache)
         Base.shred!(password)
     end