Skip to content

fix(library,editor): block snippet path traversal, preserve line endings on save - #1

Open
lukisch wants to merge 1 commit into
masterfrom
claude/determined-mayer-marfcu
Open

lukisch wants to merge 1 commit into
masterfrom
claude/determined-mayer-marfcu

Conversation

@lukisch

@lukisch lukisch commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

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_snippet built its target as root / topic / f"{name}.py" from unchecked names. A pythonbox-snippets-v1 JSON file with a topic such as ../../somewhere or an absolute path made the import overwrite any .py file the user can write. get_content and delete_item had 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 as skipped; the dialog path returns "Ungültiger Themen- oder Snippet-Name."

2. Saving rewrote LF files to CRLF on Windows
open_file read with universal newlines and save_tab wrote 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 disconnect warning on every tab switch
on_editor_changed called disconnect() before connecting; current PySide6 logs a RuntimeWarning instead of raising. It now connects with Qt.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.
  • Without the source change, 4 of the 5 new tests fail. The LF round-trip test passes either way on Linux; it guards the Windows case, which I could not run here.
  • ruff check tests: clean. PythonBox_v8.py has the same 369 pre-existing findings as master (mostly whitespace), left untouched.

Not changed

  • Version stays 1.0.1; the changelog entry is under [Unreleased].
  • Tests badge in README.md / README_de.md updated from 121 to 126.
  • A snippet or topic name ending in a dot is now rejected. Existing library entries with such names (unlikely) would no longer load.

Generated by Claude Code

…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
@github-actions

Copy link
Copy Markdown

Welcome! 👋 Thanks for your first pull request in this repository.

A maintainer will review it soon. Please make sure:

  • Your changes are tested
  • Documentation is updated if needed
  • The PR description explains what and why

Thanks for contributing!

@lukisch lukisch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_content und delete_item laufen jetzt alle über _snippet_path. Die Kombination aus Komponentenprüfung und resolve().relative_to(root) deckt auch Symlinks ab.
  • Zeilenenden: Lesen mit newline='', Normalisieren für den Editor und Zurückschreiben mit newline='' ist korrekt. Da die Regressionstests auf Windows laufen, ist der LF-Fall dort tatsächlich abgedeckt.
  • UniqueConnection: für die gebundene Methode eines QMainWindow zulässig.

Hinweise, keine Blocker

  1. 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.
  2. get_snippets baut self.root / topic weiterhin ungeprüft. Das ist nur ein lesendes glob und ändert nichts, könnte der Einheitlichkeit halber aber dieselbe Prüfung nutzen.
  3. 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 lukisch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ../../x als Snippet-Name, a\..\.., C:, sowie ein Symlink-Thema, das aus der Bibliothek herauszeigt. Alle werden abgelehnt, die Zieldatei außerhalb bleibt unverändert. Auf master ü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 dreimaligem on_editor_changed auf demselben Editor hat cursorPositionInfo genau einen Empfänger.

Nicht blockierend, alles schon auf master so und nicht Teil dieses PRs:

  1. import_payload speichert erst und prüft locked danach. Bei ungültigem locked-Wert wird das Snippet wieder gelöscht. Existierte unter dem Namen schon ein Snippet, ist es danach weg (reproduziert: vorhandenes ok/keep verschwindet nach Import mit "locked": "maybe"). Dafür kommt ein eigener kleiner PR.
  2. Eine Datei mit UTF-8-BOM verliert das BOM beim Speichern.
  3. Der Windows-Fall (LF → CRLF) ist weiterhin nur indirekt abgedeckt, weil die CI-Regressionstests auf Linux laufen.

Generated by Claude Code

@lukisch lukisch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:b and empty names are all rejected by save_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 for disconnect() works as described.
  • Line endings: a CRLF file edited and saved stays CRLF; an LF file stays LF.

Two small things, neither blocking:

  1. _is_safe_component accepts control characters such as \n or \t in a name. This cannot leave the library, but on Linux/macOS it creates a file with a line break in its name. Rejecting ch < ' ' would close it.
  2. A file with mixed endings is rewritten to the majority style on save (a tie goes to CRLF). master also 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 lukisch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • pytest lokal: 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 von on_editor_changed mit demselben Editor gibt es genau einen Empfänger, und die Positionsanzeige wird aktualisiert. Das gilt auch mit PySide6 6.5.3, der Untergrenze aus requirements.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 \r verwenden, 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 CON oder NUL gelten 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 lukisch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_snippet und delete_item laufen jetzt alle über _snippet_path; die Prüfung kombiniert Zeichenfilter (/ \ : NUL, ./.., Punkt am Ende) mit resolve().relative_to(root). Damit ist auch der Importpfad abgedeckt, weil import_payload über save_snippet schreibt.
  • Zeilenenden: open_file liest mit newline='', save_tab schreibt mit newline='' und dem gemerkten Zeilenende zurück. Neue Dateien behalten den Plattformstandard.
  • UniqueConnection: Das Entfernen des try/except um disconnect ist sicher. Ich habe das doppelte Verbinden einer gebundenen Methode gegen PySide6 6.5.3 (Untergrenze aus requirements.txt) und 6.11.3 ausprobiert: Der zweite connect liefert eine ungültige Connection, 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 libEGL im Container).

Hinweise:

  1. get_items(topic) baut weiter self.root / topic ohne Prüfung. Das ist nur lesend und listet lediglich Dateinamen, ein _is_safe_component(topic) dort würde die Regel aber einheitlich machen.
  2. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants