Repository navigation
Conversation
…ngs on save - LibraryManager: topic and snippet names must be single safe path components that resolve inside the library root. An imported pythonbox-snippets-v1 file could previously overwrite arbitrary .py files via names such as ../../dir or absolute paths. - MultiTabEditor: remember the newline style of an opened file and write it back unchanged, so LF files are no longer rewritten to CRLF on Windows. Never-saved files keep the platform default. - on_editor_changed: use Qt.ConnectionType.UniqueConnection instead of a failing disconnect() that logged a libpyside warning on every tab switch. - Tests: 5 new regression tests (126 total); badge and changelog updated. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01THB2EzdrTmcSvNq8hpiYJL
|
Welcome! 👋 Thanks for your first pull request in this repository. A maintainer will review it soon. Please make sure:
Thanks for contributing! |
lukisch
left a comment
There was a problem hiding this comment.
Zweitreview (Cloud-Bot): ✅ freigabefähig.
Diff von PythonBox_v8.py vollständig gelesen, alle 10 Checks grün (regression-tests auf Windows mit Python 3.10–3.13, Smoke auf Ubuntu und macOS, CodeQL), mergeable_state: clean. Lokal konnte ich die Suite nicht ausführen (in der Cloud-Umgebung fehlt libEGL), die Bewertung stützt sich auf Code-Lektüre und CI.
- Pfad-Traversal:
save_snippet,get_contentunddelete_itemlaufen jetzt alle über_snippet_path. Die Kombination aus Komponentenprüfung undresolve().relative_to(root)deckt auch Symlinks ab. - Zeilenenden: Lesen mit
newline='', Normalisieren für den Editor und Zurückschreiben mitnewline=''ist korrekt. Da die Regressionstests auf Windows laufen, ist der LF-Fall dort tatsächlich abgedeckt. UniqueConnection: für die gebundene Methode einesQMainWindowzulässig.
Hinweise, keine Blocker
- Windows-Gerätenamen (
CON,NUL,AUX,COM1…) gelten weiterhin als gültige Namen. Das ist kein Ausbruch aus der Bibliothek, das Speichern schlägt dort nur fehl oder landet auf dem Gerät. get_snippetsbautself.root / topicweiterhin ungeprüft. Das ist nur ein lesendesglobund ändert nichts, könnte der Einheitlichkeit halber aber dieselbe Prüfung nutzen.- Dateien mit gemischten Zeilenenden werden beim Speichern auf das häufigere Zeilenende vereinheitlicht. Das war vorher ebenfalls so (damals auf den Plattformstandard).
Freigabe-Empfehlung: mergen. Kein formales Approve möglich, weil der PR vom selben Konto stammt.
Generated by Claude Code
lukisch
left a comment
There was a problem hiding this comment.
Zweitprüfung (Cloud-Bot): keine Einwände, aus meiner Sicht mergebar. Formales „Approve“ ist nicht möglich, weil der PR vom selben Konto stammt.
Geprüft auf Head 90c9465, Linux, Python 3.11, PySide6 6.11.3, offscreen:
pytest: 126 passed. CI ist auf allen 10 Checks grün.- Pfad-Traversal: Gegen eine temporäre Bibliothek probiert:
../,.., absoluter Pfad,../../xals Snippet-Name,a\..\..,C:, sowie ein Symlink-Thema, das aus der Bibliothek herauszeigt. Alle werden abgelehnt, die Zieldatei außerhalb bleibt unverändert. Aufmasterüberschreibt derselbe Aufruf die Datei. - Zeilenenden: LF bleibt LF, CRLF bleibt CRLF (Bytevergleich nach Öffnen und Speichern). Gemischte Dateien werden auf den häufigeren Stil vereinheitlicht, bei Gleichstand auf CRLF.
UniqueConnection: Nach dreimaligemon_editor_changedauf demselben Editor hatcursorPositionInfogenau einen Empfänger.
Nicht blockierend, alles schon auf master so und nicht Teil dieses PRs:
import_payloadspeichert erst und prüftlockeddanach. Bei ungültigemlocked-Wert wird das Snippet wieder gelöscht. Existierte unter dem Namen schon ein Snippet, ist es danach weg (reproduziert: vorhandenesok/keepverschwindet nach Import mit"locked": "maybe"). Dafür kommt ein eigener kleiner PR.- Eine Datei mit UTF-8-BOM verliert das BOM beim Speichern.
- Der Windows-Fall (LF → CRLF) ist weiterhin nur indirekt abgedeckt, weil die CI-Regressionstests auf Linux laufen.
Generated by Claude Code
lukisch
left a comment
There was a problem hiding this comment.
Second-model review: no blocking findings, ready to merge. (Posted as a comment because the bot account is also the PR author and cannot approve.)
Checked on head 90c9465, Linux, Python 3.11, PySide6 6.11.3, offscreen:
pytest: 126 passed, no libpyside warnings. All 10 CI checks are green.- Path traversal:
..,a/b,C:,..\x, trailing dot, leading space,a:band empty names are all rejected bysave_snippet. A symlinked topic directory pointing outside the library is rejected too, and nothing is written outside. UniqueConnection: connecting the same slot three times and emitting once calls it exactly once, with warnings turned into errors. The replacement fordisconnect()works as described.- Line endings: a CRLF file edited and saved stays CRLF; an LF file stays LF.
Two small things, neither blocking:
_is_safe_componentaccepts control characters such as\nor\tin a name. This cannot leave the library, but on Linux/macOS it creates a file with a line break in its name. Rejectingch < ' 'would close it.- A file with mixed endings is rewritten to the majority style on save (a tie goes to CRLF).
masteralso normalised such files, so this is not a regression.
Merge order: this PR and #2 both add a test at the same place in tests/test_pythonbox_regressions.py and conflict there. The source changes do not overlap. Whichever is merged second needs a rebase, and the tests badge should then read 127.
Generated by Claude Code
lukisch
left a comment
There was a problem hiding this comment.
Zweitprüfung (Cloud-Bot, 2026-11-11) auf 90c9465: freigabefähig, keine Blocker. Eine formale Approval ist nicht möglich, weil der Bot unter demselben Konto läuft wie der PR-Autor.
Nachgeprüft
pytestlokal: 126 passed (Linux, Python 3.11, PySide6 6.11.3, offscreen). CI: alle 10 Checks grün.- Pfadprüfung von Hand getestet:
..,../x,C:,/etc,x\y, Namen mit Leerzeichen am Rand und mit Punkt am Ende werden abgelehnt. Ein Symlink innerhalb der Bibliothek, der nach außen zeigt, wird ebenfalls abgelehnt; es wird nichts geschrieben. UniqueConnection: nach drei Aufrufen vonon_editor_changedmit demselben Editor gibt es genau einen Empfänger, und die Positionsanzeige wird aktualisiert. Das gilt auch mit PySide6 6.5.3, der Untergrenze ausrequirements.txt.- Zusammen mit #2 gemergt: kein Konflikt, 127 passed.
Hinweise, nicht blockierend
- Dateien mit gemischten Zeilenenden werden beim Speichern auf das häufigere Zeilenende vereinheitlicht. Dateien, die nur
\rverwenden, bekommen den Plattformstandard. LibraryManager.get_snippets(topic)baut den Pfad weiterhin ungeprüft (self.root / topic). Die Methode liest nur Dateinamen; der Vollständigkeit halber könnte sie dieselbe Prüfung bekommen.- Windows-Gerätenamen wie
CONoderNULgelten als gültige Namen. - Der LF-Erhalt unter Windows ist weder hier noch in der CI gelaufen (kein Windows-Job).
- Nach dem Merge von #1 und #2 muss das Tests-Badge auf 127.
Generated by Claude Code
lukisch
left a comment
There was a problem hiding this comment.
Zweitprüfung auf 90c9465: inhaltlich freigegeben, zwei nicht blockierende Hinweise. (Als Kommentar, weil GitHub eine formale Freigabe des eigenen PRs nicht zulässt.)
Geprüft:
- Pfad-Traversal:
get_content,save_snippetunddelete_itemlaufen jetzt alle über_snippet_path; die Prüfung kombiniert Zeichenfilter (/ \ : NUL,./.., Punkt am Ende) mitresolve().relative_to(root). Damit ist auch der Importpfad abgedeckt, weilimport_payloadübersave_snippetschreibt. - Zeilenenden:
open_fileliest mitnewline='',save_tabschreibt mitnewline=''und dem gemerkten Zeilenende zurück. Neue Dateien behalten den Plattformstandard. UniqueConnection: Das Entfernen destry/exceptumdisconnectist sicher. Ich habe das doppelte Verbinden einer gebundenen Methode gegen PySide6 6.5.3 (Untergrenze ausrequirements.txt) und 6.11.3 ausprobiert: Der zweiteconnectliefert eine ungültigeConnection, wirft nicht, und der Slot wird genau einmal aufgerufen.- CI auf dem Head ist grün. Die vollständige Suite konnte ich lokal nicht ausführen (kein
libEGLim Container).
Hinweise:
get_items(topic)baut weiterself.root / topicohne Prüfung. Das ist nur lesend und listet lediglich Dateinamen, ein_is_safe_component(topic)dort würde die Regel aber einheitlich machen.- Dateien mit gemischten Zeilenenden werden beim Speichern auf das häufigere Zeilenende vereinheitlicht. Das ist besser als vorher, steht aber nicht im Changelog-Eintrag („unverändert zurück“).
Lässt sich konfliktfrei mit #2 zusammenführen; der Test-Badge müsste danach von 126 auf 127.
Generated by Claude Code
Code review of
PythonBox_v8.py(first bot pass over this repo) found three defects. Each is fixed here with regression tests.Fixes
1. Snippet import could write outside the library (security)
LibraryManager.save_snippetbuilt its target asroot / topic / f"{name}.py"from unchecked names. Apythonbox-snippets-v1JSON file with a topic such as../../somewhereor an absolute path made the import overwrite any.pyfile the user can write.get_contentanddelete_itemhad the same gap.Names must now be a single path component (no
/,\,:, NUL,./.., trailing dot) and the resolved path must stay inside the library root. Unsafe entries are skipped and counted asskipped; the dialog path returns "Ungültiger Themen- oder Snippet-Name."2. Saving rewrote LF files to CRLF on Windows
open_fileread with universal newlines andsave_tabwrote with the platform default, so an LF file became CRLF on the first save and Git reported every line as changed. The tab now remembers the file's newline style (detect_newline) and writes it back unchanged. Files that were never saved keep the platform default.3.
libpyside: Failed to disconnectwarning on every tab switchon_editor_changedcalleddisconnect()before connecting; current PySide6 logs aRuntimeWarninginstead of raising. It now connects withQt.ConnectionType.UniqueConnection.Verification
pytest: 126 passed (121 before, 5 new), Linux, Python 3.11, PySide6 6.11.3, offscreen. The seven libpyside warnings from the previous run are gone.ruff check tests: clean.PythonBox_v8.pyhas the same 369 pre-existing findings asmaster(mostly whitespace), left untouched.Not changed
1.0.1; the changelog entry is under[Unreleased].README.md/README_de.mdupdated from 121 to 126.Generated by Claude Code