forked from fatihak/InkyPi
-
Notifications
You must be signed in to change notification settings - Fork 1
fix: correct refresh error accounting, sidebar nav clipping, and weather rendering; add unattended update recovery #632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jtn0123
wants to merge
23
commits into
main
Choose a base branch
from
claude/app-fork-feature-review-09e2f7
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+6,720
−637
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
4172f5d
fix(weather): correct Open-Meteo units, feels-like, hourly icons and …
jtn0123 414dcbc
fix(display): drive epd3in7-class panels instead of failing on them
jtn0123 f446cde
fix(plugins): resolve background colours against the target image mode
jtn0123 44b4a6a
fix(refresh): gate the systemd watchdog on refresh-loop progress
jtn0123 e2004f6
feat(install): verify updates are serving, and roll back unattended
jtn0123 63a1432
feat(refresh): record crash breadcrumbs and quarantine the culprit pl…
jtn0123 ba304b9
feat(plugins): skip-a-turn hook, image-less plugins, screenshot optio…
jtn0123 5e7e657
feat(images): add Auto fit mode with a central padImage -> fitMode mi…
jtn0123 796a27c
fix(stats): stop counting every successful refresh as an error
jtn0123 3aaee02
fix(ui): unclip the sidebar nav, and correct three misleading labels
jtn0123 a6ee47e
test: add a simulation tier and fix the silently-skipping systemd gate
jtn0123 98cff06
test: annotate the new test files to satisfy the mypy tests ratchet
jtn0123 b6d5ce4
fix: address CodeRabbit review findings on PR #632
jtn0123 392c89a
ci: run the systemd container gates once instead of across the matrix
jtn0123 d9580ca
ci: add a manual workflow to refresh visual baselines on a CI runner
jtn0123 d21871c
docs: surround the skip_display_condition example with blank lines (M…
jtn0123 57dd71d
fix: restore Open-Meteo wind and count rollback in start-limit events
jtn0123 c367554
fix: never skip a user-requested refresh, and harden breadcrumb inputs
jtn0123 cc58c2b
docs: record the Open-Meteo timezone finding as a tracked follow-up
jtn0123 ac77331
test: refresh the dashboard-desktop layout baseline from a CI render
jtn0123 2699029
fix(deps): clear all 45 known vulnerabilities in the lockfiles
jtn0123 5212659
docs: track the SonarCloud S2083 finding as a reviewed false positive
jtn0123 2689ec3
refactor: address the valid SonarCloud findings on new code
jtn0123 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| name: Refresh visual baselines | ||
|
|
||
| # Regenerate layout/plugin snapshot baselines in the *same* environment the | ||
| # `Browser smoke` job renders them in, and upload the PNGs as an artifact to | ||
| # download and commit. | ||
| # | ||
| # Why this exists: the baselines are pixel comparisons and are documented as | ||
| # reproducible only on Linux x86_64 with ubuntu-24.04's font set | ||
| # (tests/snapshots/README.md). That leaves a contributor on Apple Silicon with | ||
| # no way to refresh them after an intentional CSS change — the documented | ||
| # `--platform linux/amd64` docker one-liner installs fine but Chromium SIGABRTs | ||
| # under emulation. Rather than commit baselines that cannot be verified, render | ||
| # them on the same runner CI compares against. | ||
| # | ||
| # Usage: | ||
| # gh workflow run refresh-visual-baselines.yml --ref <branch> | ||
| # gh run download <run-id> -n refreshed-visual-baselines | ||
| # # copy over tests/snapshots/, inspect the diff, commit | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| target: | ||
| description: 'Which baselines to regenerate' | ||
| required: true | ||
| default: 'layout' | ||
| type: choice | ||
| options: | ||
| - layout | ||
| - plugins | ||
| - both | ||
|
|
||
| jobs: | ||
| refresh: | ||
| name: Refresh baselines | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.13' | ||
| cache: 'pip' | ||
| cache-dependency-path: | | ||
| install/requirements.txt | ||
| install/requirements-dev.txt | ||
| - name: Install OS dependencies | ||
| # Matches the browser-smoke job — the font set is what makes these | ||
| # baselines reproducible. | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y \ | ||
| libopenjp2-7 \ | ||
| libopenblas-dev \ | ||
| libfreetype6-dev \ | ||
| fonts-noto-color-emoji | ||
| - name: Install Python dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip wheel | ||
| pip install -r install/requirements.txt -r install/requirements-dev.txt | ||
| - name: Install Playwright Chromium | ||
| run: python -m playwright install --with-deps chromium | ||
| - name: Build CSS | ||
| run: python scripts/build_css.py | ||
| - name: Regenerate baselines | ||
| env: | ||
| REQUIRE_BROWSER_SMOKE: '1' | ||
| INKYPI_ENV: dev | ||
| INKYPI_NO_REFRESH: '1' | ||
| PYTHONPATH: src | ||
| run: | | ||
| case "${{ inputs.target }}" in | ||
| layout) targets="tests/integration/test_visual_regression.py" ;; | ||
| plugins) targets="tests/snapshots/" ;; | ||
| both) targets="tests/integration/test_visual_regression.py tests/snapshots/" ;; | ||
| esac | ||
| # --update-snapshots makes every assert_image_snapshot() write instead | ||
| # of compare, so the run is expected to pass trivially; the artifact is | ||
| # the point. | ||
| pytest $targets -q --update-snapshots | ||
| - name: Show what changed | ||
| run: git --no-pager diff --stat -- tests/snapshots/ || true | ||
| - name: Upload refreshed baselines | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: refreshed-visual-baselines | ||
| path: tests/snapshots/**/*.png | ||
| if-no-files-found: error |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| # Tracking: SonarCloud S2083 on `utils/crash_breadcrumb.py` | ||
|
|
||
| Created: 2026-08-20 | ||
|
|
||
| ## Finding | ||
|
|
||
| - Rule: `pythonsecurity:S2083` — "Change this code to not construct the path from user-controlled data." | ||
| - Severity: Blocker (drives `new_security_rating` to **E**, failing the PR quality gate) | ||
| - Location: `src/utils/crash_breadcrumb.py`, the `write_text` call inside `_write_json` | ||
| - First reported: PR [#632](https://github.com/jtn0123/InkyPi/pull/632) | ||
|
|
||
| ## Why It Fires | ||
|
|
||
| Sonar's taint analysis treats `os.getenv()` as an attacker-controlled source and | ||
| `Path.write_text()` as a file-write sink. The breadcrumb's directories come from | ||
| `INKYPI_RUNTIME_DIR` / `INKYPI_LOCKFILE_DIR` / `INKYPI_STATE_DIR`, so there is a | ||
| source-to-sink path and the rule reports it. | ||
|
|
||
| ## Assessment: false positive, but the code was hardened anyway | ||
|
|
||
| **Not a privilege boundary.** These variables are set by the systemd unit that | ||
| launches the service. Anyone able to change them can already execute code as the | ||
| service user, so redirecting a breadcrumb write gains an attacker nothing they | ||
| did not already have. This is configuration, not untrusted input. | ||
|
|
||
| The environment override exists so tests and dev runs can redirect state to a | ||
| temp directory — the same contract `install/update.sh` and | ||
| `blueprints/settings/_update_status.py` already honour. Those modules read the | ||
| same variables and are not flagged, because they have no write sink. | ||
|
|
||
| Hardening applied in #632 regardless, because one part of the finding pointed at | ||
| a real (if minor) bug: | ||
|
|
||
| - The directory must now be **absolute**, and is resolved. A relative value used | ||
| to scatter breadcrumbs relative to the service's working directory instead of | ||
| where the next boot reads them — a genuine correctness bug, not just a | ||
| security one. | ||
| - `_in_dir()` refuses a filename that resolves outside its directory, so these | ||
| helpers cannot become an arbitrary-write primitive if a future caller passes | ||
| something that is not a module constant. | ||
| - Values read back out of the breadcrumb are sanitised before they reach logs or | ||
| `disabled_reason` (this closed the three companion `S5145` findings). | ||
|
|
||
| Sonar's engine does not model any of that as a sanitizer. It recognises | ||
| allow-list comparison against literals, which is not usable here: the tests that | ||
| exercise crash recovery need arbitrary `tmp_path` directories. | ||
|
|
||
| ## Deliberately Not Done | ||
|
|
||
| - **No `# NOSONAR`.** Suppressing the marker in code hides the finding from | ||
| future readers and from any genuinely unsafe path added later. | ||
| - **No laundering the value** through string/`Path` round-trips to break taint | ||
| propagation. That would clear the gate only by confusing the analyser, and | ||
| would silence the rule for real issues in this file afterwards. | ||
|
|
||
| ## Resolution Required | ||
|
|
||
| Mark the issue **Safe** (or *Won't Fix*) in the SonarCloud UI, referencing this | ||
| document. This needs a maintainer with project permissions; it is a review | ||
| decision rather than a code change, which is why it is not automated. | ||
|
|
||
| Until then `SonarCloud Scan`, `SonarCloud Code Analysis`, and the aggregate | ||
| `CI gate` stay red on any PR touching this file. Note `main`'s Sonar gate is | ||
| independently red on `new_reliability_rating`. | ||
|
|
||
| ## Closure Criteria | ||
|
|
||
| Close this tracking item when either: | ||
|
|
||
| - The issue is marked Safe in SonarCloud and `new_security_rating` returns to A; or | ||
| - The environment override is removed from the breadcrumb write path entirely | ||
| (for example, resolved once at startup in `config.py` and injected), which | ||
| would remove the source-to-sink flow rather than mask it. | ||
|
|
||
| ## GitHub Issue Attempt | ||
|
|
||
| Preferred tracking was a GitHub issue, but the `jtn0123/InkyPi` repository has | ||
| issues disabled — same constraint recorded in | ||
| [the pip advisory tracking doc](./pip-ghsa-58qw-9mgm-455v-tracking.md). |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.