Merge pull request electron#11329 from electron/remove-classes-key

fix: Properly cleanup in `removeAsDefaultProtocolClient`

Author: ckerr

atom/browser/browser_win.cc

@@ -153,15 +153,19 @@ bool Browser::RemoveAsDefaultProtocolClient(const std::string& protocol,
 
   // Main Registry Key
   HKEY root = HKEY_CURRENT_USER;
-  base::string16 keyPath = base::UTF8ToUTF16("Software\\Classes\\" + protocol);
+  base::string16 keyPath = L"Software\\Classes\\";
 
   // Command Key
-  base::string16 cmdPath = keyPath + L"\\shell\\open\\command";
+  base::string16 wprotocol = base::UTF8ToUTF16(protocol);
+  base::string16 shellPath = wprotocol + L"\\shell";
+  base::string16 cmdPath = keyPath + shellPath + L"\\open\\command";
 
-  base::win::RegKey key;
+  base::win::RegKey classesKey;
   base::win::RegKey commandKey;
-  if (FAILED(key.Open(root, keyPath.c_str(), KEY_ALL_ACCESS)))
-    // Key doesn't even exist, we can confirm that it is not set
+
+  if (FAILED(classesKey.Open(root, keyPath.c_str(), KEY_ALL_ACCESS)))
+    // Classes key doesn't exist, that's concerning, but I guess
+    // we're not the default handler
     return true;
 
   if (FAILED(commandKey.Open(root, cmdPath.c_str(), KEY_ALL_ACCESS)))
@@ -179,9 +183,25 @@ bool Browser::RemoveAsDefaultProtocolClient(const std::string& protocol,
 
   if (keyVal == exe) {
     // Let's kill the key
-    if (FAILED(key.DeleteKey(L"shell")))
+    if (FAILED(classesKey.DeleteKey(shellPath.c_str())))
       return false;
 
+    // Let's clean up after ourselves
+    base::win::RegKey protocolKey;
+    base::string16 protocolPath = keyPath + wprotocol;
+
+    if (SUCCEEDED(protocolKey
+      .Open(root, protocolPath.c_str(), KEY_ALL_ACCESS))) {
+      protocolKey.DeleteValue(L"URL Protocol");
+
+      // Overwrite the default value to be empty, we can't delete it right away
+      protocolKey.WriteValue(L"", L"");
+      protocolKey.DeleteValue(L"");
+    }
+
+    // If now empty, delete the whole key
+    classesKey.DeleteEmptyKey(wprotocol.c_str());
+
     return true;
   } else {
     return true;

spec/api-app-spec.js

@@ -504,9 +504,34 @@ describe('app module', () => {
       '--process-start-args', `"--hidden"`
     ]
 
+    let Winreg
+    let classesKey
+
     before(function () {
       if (process.platform !== 'win32') {
         this.skip()
+      } else {
+        Winreg = require('winreg')
+
+        classesKey = new Winreg({
+          hive: Winreg.HKCU,
+          key: '\\Software\\Classes\\'
+        })
+      }
+    })
+
+    after(function (done) {
+      if (process.platform !== 'win32') {
+        done()
+      } else {
+        const protocolKey = new Winreg({
+          hive: Winreg.HKCU,
+          key: `\\Software\\Classes\\${protocol}`
+        })
+
+        // The last test leaves the registry dirty,
+        // delete the protocol key for those of us who test at home
+        protocolKey.destroy(() => done())
       }
     })
 
@@ -534,6 +559,61 @@ describe('app module', () => {
       assert.equal(app.isDefaultProtocolClient(protocol, updateExe, processStartArgs), true)
       assert.equal(app.isDefaultProtocolClient(protocol), false)
     })
+
+    it('creates a registry entry for the protocol class', (done) => {
+      app.setAsDefaultProtocolClient(protocol)
+
+      classesKey.keys((error, keys) => {
+        if (error) {
+          throw error
+        }
+
+        const exists = !!keys.find((key) => key.key.includes(protocol))
+        assert.equal(exists, true)
+
+        done()
+      })
+    })
+
+    it('completely removes a registry entry for the protocol class', (done) => {
+      app.setAsDefaultProtocolClient(protocol)
+      app.removeAsDefaultProtocolClient(protocol)
+
+      classesKey.keys((error, keys) => {
+        if (error) {
+          throw error
+        }
+
+        const exists = !!keys.find((key) => key.key.includes(protocol))
+        assert.equal(exists, false)
+
+        done()
+      })
+    })
+
+    it('only unsets a class registry key if it contains other data', (done) => {
+      app.setAsDefaultProtocolClient(protocol)
+
+      const protocolKey = new Winreg({
+        hive: Winreg.HKCU,
+        key: `\\Software\\Classes\\${protocol}`
+      })
+
+      protocolKey.set('test-value', 'REG_BINARY', '123', () => {
+        app.removeAsDefaultProtocolClient(protocol)
+
+        classesKey.keys((error, keys) => {
+          if (error) {
+            throw error
+          }
+
+          const exists = !!keys.find((key) => key.key.includes(protocol))
+          assert.equal(exists, true)
+
+          done()
+        })
+      })
+    })
   })
 
   describe('getFileIcon() API', () => {

spec/package.json

@@ -18,6 +18,7 @@
     "send": "^0.14.1",
     "temp": "^0.8.3",
     "walkdir": "0.0.11",
+    "winreg": "^1.2.4",
     "ws": "^1.1.1",
     "yargs": "^6.0.0"
   },