From df556602915f05b07b0bc8fabf457eb3fe41b9e1 Mon Sep 17 00:00:00 2001 From: Martin Tranberg Date: Sun, 12 Apr 2026 10:22:54 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20task=207=20=E2=80=94=20production=20hard?= =?UTF-8?q?ening=20quick=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace 4 bare `except:` with `except Exception:` (load_settings, get_txt, get_icon_idx_for_file, process_file cleanup block) so SystemExit and KeyboardInterrupt are no longer swallowed - Replace 2 print() calls with logger.error() (__init__ MSAL init, ensure_valid_token) so errors appear in the configurable log output - Sanitize item['name'] with os.path.basename() in on_download_clicked and _download_folder_recursive_sync to prevent path traversal from server-controlled filenames - Add 8 new unit tests covering all Task 7 changes Co-Authored-By: Claude Sonnet 4.6 --- sharepoint_browser.py | 16 +++++------ tests/test_review_fixes.py | 59 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/sharepoint_browser.py b/sharepoint_browser.py index 0335bb1..f6232f0 100644 --- a/sharepoint_browser.py +++ b/sharepoint_browser.py @@ -67,7 +67,7 @@ def load_settings(): with open(SETTINGS_FILE, 'r', encoding='utf-8') as f: try: return json.load(f) - except: + except Exception: return default_settings def save_settings(new_settings): @@ -708,7 +708,7 @@ class SharePointApp(wx.Frame): try: self.msal_app = msal.PublicClientApplication(CLIENT_ID, authority=AUTHORITY) except Exception as e: - print(f"MSAL Init Error: {e}") + logger.error(f"MSAL Init Error: {e}") self.InitUI() self.Centre() @@ -737,7 +737,7 @@ class SharePointApp(wx.Frame): if kwargs: try: return text.format(**kwargs) - except: + except Exception: pass return text @@ -1303,7 +1303,7 @@ class SharePointApp(wx.Frame): with wx.DirDialog(self, self.get_txt("msg_select_folder"), style=wx.DD_DEFAULT_STYLE | wx.DD_DIR_MUST_EXIST) as dd: if dd.ShowModal() == wx.ID_OK: parent_path = dd.GetPath() - dest_path = os.path.join(parent_path, item['name']) + dest_path = os.path.join(parent_path, os.path.basename(item['name'])) threading.Thread(target=self._download_folder_bg_task, args=(item, dest_path), daemon=True).start() def _download_file_bg_task(self, item, dest_path): @@ -1343,7 +1343,7 @@ class SharePointApp(wx.Frame): res_data = res.json() items = res_data.get('value', []) for item in items: - item_path = os.path.join(local_dir, item['name']) + item_path = os.path.join(local_dir, os.path.basename(item['name'])) if 'folder' in item: self._download_folder_recursive_sync(drive_id, item['id'], item_path) else: @@ -1666,7 +1666,7 @@ class SharePointApp(wx.Frame): self.headers = {'Authorization': f'Bearer {self.access_token}'} return True except Exception as e: - print(f"Token refresh error: {e}") + logger.error(f"Token refresh error: {e}") self.set_status(self.get_txt("status_login_needed")) return False @@ -2073,7 +2073,7 @@ class SharePointApp(wx.Frame): idx = self.image_list.Add(bmp) self.ext_icons[ext] = idx return idx - except: + except Exception: pass self.ext_icons[ext] = self.idx_file @@ -2410,7 +2410,7 @@ class SharePointApp(wx.Frame): try: os.remove(local_path) os.rmdir(working_dir) - except: + except Exception: pass self.set_status(self.get_txt("msg_update_success", name=file_name)) diff --git a/tests/test_review_fixes.py b/tests/test_review_fixes.py index 96a38de..fc71d89 100644 --- a/tests/test_review_fixes.py +++ b/tests/test_review_fixes.py @@ -221,5 +221,64 @@ class TestIsBreakcrumbRemoved(unittest.TestCase): self.assertIn("tree_item", sig.parameters) +# --------------------------------------------------------------------------- +# Task 7: bare except → except Exception, print → logger, basename sanitization +# --------------------------------------------------------------------------- + +class TestTask7BareExcept(unittest.TestCase): + """Verify that bare except: clauses have been replaced with except Exception:.""" + + def _get_source_lines(self): + return inspect.getsource(sb).splitlines() + + def test_no_bare_except_in_load_settings(self): + """7a: load_settings() uses except Exception, not bare except.""" + source = inspect.getsource(sb.load_settings) + self.assertNotIn("except:", source, + "load_settings still has a bare except:") + + def test_no_bare_except_in_get_txt(self): + """7a: get_txt() uses except Exception, not bare except.""" + source = inspect.getsource(sb.SharePointApp.get_txt) + self.assertNotIn("except:", source, + "get_txt still has a bare except:") + + def test_no_bare_except_in_get_icon_idx(self): + """7a: get_icon_idx_for_file() uses except Exception, not bare except.""" + source = inspect.getsource(sb.SharePointApp.get_icon_idx_for_file) + self.assertNotIn("except:", source, + "get_icon_idx_for_file still has a bare except:") + + def test_no_bare_except_in_process_file(self): + """7a: process_file() cleanup block uses except Exception, not bare except.""" + source = inspect.getsource(sb.SharePointApp.process_file) + self.assertNotIn("except:", source, + "process_file still has a bare except: in cleanup block") + + def test_no_print_in_msal_init(self): + """7b: MSAL init error uses logger, not print().""" + source = inspect.getsource(sb.SharePointApp.__init__) + self.assertNotIn('print(f"MSAL Init Error', source, + "__init__ still uses print() for MSAL Init Error") + + def test_no_print_in_ensure_valid_token(self): + """7b: ensure_valid_token() uses logger, not print().""" + source = inspect.getsource(sb.SharePointApp.ensure_valid_token) + self.assertNotIn('print(f"Token refresh error', source, + "ensure_valid_token still uses print() for token error") + + def test_basename_in_download_folder_recursive(self): + """7c: _download_folder_recursive_sync uses os.path.basename on item name.""" + source = inspect.getsource(sb.SharePointApp._download_folder_recursive_sync) + self.assertIn("os.path.basename", source, + "_download_folder_recursive_sync does not sanitize item['name'] with basename") + + def test_basename_in_on_download_clicked(self): + """7c: on_download_clicked uses os.path.basename when building dest_path for folders.""" + source = inspect.getsource(sb.SharePointApp.on_download_clicked) + self.assertIn("os.path.basename", source, + "on_download_clicked does not sanitize item['name'] with basename for folder downloads") + + if __name__ == "__main__": unittest.main(verbosity=2)