martin/sharepoint-browser
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix: task 7 — production hardening quick fixes

Browse Source 
- 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 <noreply@anthropic.com>
This commit is contained in:
Martin Tranberg
2026-04-12 10:22:54 +02:00
parent e8e1d8b60d
commit df55660291
2 changed files with 67 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
sharepoint_browser.py
View File

@@ -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))

59
tests/test_review_fixes.py
View File

@@ -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)