Harden CI dependency installs against lifecycle-script execution - #304
Merged
Conversation
Blocks third-party install hooks in both the test-github and test-forgejo jobs, addressing SonarCloud githubactions:S6505 and githubactions:S8541. npm ci now runs with --ignore-scripts. The repo's own postinstall (copy-assets) is load-bearing: it vendors monaco, xterm and quadlet-lint into static/vendor/, so it is invoked explicitly as a separate step rather than left to run implicitly alongside dependency hooks. pip installs now pass --only-binary :all:, so no sdist setup.py runs during install. Verified with a dry-run resolve of requirements.txt and requirements-test.txt on Python 3.12, matching the workflow's PYTHON_VERSION: both resolve wheel-only, exit 0. The six githubactions:S8544 findings on the same lines are deliberately left open. Satisfying them needs a genuinely pinned lockfile rather than the current >= ranges, which is a dependency-management decision with Dependabot implications and does not belong in a workflow-only change.
Contributor
Up to standards ✅🟢 Issues
|
|
Owner
Author
|
I will merge this PR now as Sonarqube's reported issues are now in #305 and will complete in another time. |
karoltheguy
added a commit
that referenced
this pull request
Aug 3, 2026
Third PR in the SonarCloud cleanup series (group 2: HTML accessibility), after #303 and #304. Clears all 62 open accessibility findings in the templates and fixes #287. | Rule | Count | |---|---| | `Web:InputWithoutLabelCheck` | 32 | | `Web:S6853` | 27 | | `Web:S5255` | 2 | | `Web:S5254` | 1 | ## Not one fix repeated 62 times The findings look uniform and are not. Six distinct cases, and applying the obvious fix to all of them would have produced broken markup in two: 1. **Label paired with one control.** Add `for`/`id`, named `<context>-<field>`. The bulk of the change. 2. **The same, but inside a `{% for %}` loop.** `settings_servers.html`'s edit form renders once per server, so a static `id="name"` would be emitted N times and every label would resolve to the first server's field. The id has to interpolate the loop key. That file already set the precedent one line up with `id="server-edit-row-{{ s[0] }}"`, so these follow it. 3. **Control with only a placeholder.** A placeholder is not an accessible name: it is not exposed as one and it disappears once the field has content. These get `aria-label`. 4. **A `<label>` sitting before a `<div class="radio-group">`.** A `<label for>` must reference a labelable control, and a div is not one, so `for` would have satisfied nothing and Sonar would have kept flagging it. These become a `<span>` plus `role="radiogroup"` and `aria-labelledby`. 5. **The machine-only `<textarea>` in the hidden HTMX save form.** Gets an `aria-label` rather than a visible label, since it is never user-facing. 6. **Document level.** `lang="en"` on `<html>`, and distinct `aria-label`s on the two `<nav>` landmarks. ## The test `tests/test_form_label_a11y.py` asserts the general contract (every control has an accessible name, no label is left dangling) rather than checking off the individual fixes, extending the source-assertion style of `tests/test_settings_a11y.py` from #221. Written before the fix, it independently reproduced Sonar's finding set exactly, per file: 32 unnamed controls and 27 dangling labels, without being given Sonar's line numbers. That match is the reason to trust it as a stand-in for the analyzer going forward. It also carries a guard that is green today and earns its place anyway: `TestLoopScopedIdsAreUnique` fails if a static `id` appears inside a `{% for %}`, which is precisely the mistake case 2 invites. ## Verification * `-m unit`: 939 passed, 3 skipped * unmarked suite: 47 passed * `name=` attributes verified byte-identical before and after across all seven templates, since form submission and the backend depend on them * no pre-existing `id` value changed or dropped * checked for id collisions across `dashboard.html` and the partials injected into it. The four that exist (`admin-root`, `systemctl-actions`, `systemd-status`, `themes-root`) are pre-existing HTMX swap-target pairs, unchanged by this PR ## Out of scope The design hook flags monotonous spacing in `dashboard.html`. That is real, already tracked as #228, and unrelated: this PR adds attributes and touches no layout. Left unchanged and unsuppressed.
karoltheguy
added a commit
that referenced
this pull request
Aug 3, 2026
…ons (#309) Fourth PR in the SonarCloud cleanup series (group 3: Python test hygiene), after #303, #304 and #308. Clears 43 of the 44 findings; the 44th is blocked on an app-side fix and explained below. | Rule | Count | Status | |---|---|---| | `python:S5778` exception test has multiple invocations | 24 | Fixed | | `python:S9073` composite assertion | 12 | Fixed | | `python:S8997` global state mutation | 5 | Fixed, found a real bug | | `python:S5958` assertion too broad | 3 | 2 fixed, 1 blocked | ## The one that was a real bug, not hygiene `tests/test_config.py`'s `tearDown` **deleted** three environment variables instead of restoring them: ```python if "QUADLET_MASTER_KEY" in os.environ: del os.environ["QUADLET_MASTER_KEY"] ``` `.github/workflows/tests.yml:22` sets `QUADLET_MASTER_KEY` for the entire job, so running this TestCase silently unset a variable it never owned. It has not bitten yet only because `--dist=loadfile` puts each file in its own worker, which masks the contamination. `setUp` now snapshots the three variables and `tearDown` restores the exact prior state, including restoring "was absent" as absent. Demonstrated by running the TestCase in-process with the variable set: ``` BEFORE tests=4 failures=0 QUADLET_MASTER_KEY after = None AFTER tests=4 failures=0 QUADLET_MASTER_KEY after = 'sentinel_value_from_ci' ``` Note the rule's own suggested fix is impossible in that file: it is a `unittest.TestCase`, and `monkeypatch` is a pytest fixture that cannot be injected into `TestCase` methods. Those three use `patch.dict` instead, which restores prior values rather than clearing them. The two findings in `test_crypto.py` are plain pytest functions and do use `monkeypatch`; both genuinely leaked, one via a `finally` that restored `DATABASE_PATH` but not the env var, the other via a `pop` on the last line that never runs if an earlier assert fails. ## The blocked one `tests/test_crypto.py` keeps `pytest.raises(Exception)` with a comment saying why. `services/ssh_manager.py:118` genuinely raises bare `Exception` on the decrypt-failure path, so there is no more specific type to narrow to. That line is already tracked as `python:S112` ("replace this generic exception class with a more specific one") and is scheduled in group 8. Once the app raises a dedicated exception, this test can narrow. Inventing a type here would have made the test pass for the wrong reason. ## What the mechanical fixes actually change `S5778`: the offending second invocation was always mock construction *inside* the block, so it could pass because `MagicMock()` threw rather than the code under test. Hoisted to a local; each block now contains exactly the call under test. `S9073`: one split is worth a look. It exposed a decorative half: ```python assert "(" in teardown_block, "Expected window._quadletLintDetach() to be invoked..." ``` Standing alone, "this JavaScript contains an open paren" is vacuously true. Replaced with `assert "window._quadletLintDetach(" in teardown_block`, which implies both original halves and actually tests invocation: | Scenario | Old | New | |---|---|---| | function invoked | passes | passes | | merely referenced, never invoked | **passes** | **fails** | ## Verification Test hygiene changes can quietly delete coverage, so this was checked by AST rather than by eye: * **159 test functions before and after**, none removed * **assert statements 251 to 262**, up from splitting composites, never down * **no exception type broadened**: `pytest.raises(Exception)` went 3 to 1, the remainder being the blocked case * `-m unit`: 939 passed, 3 skipped. Unmarked suite: 47 passed * Re-run in random order (`-p randomly` enabled) to catch any new cross-test contamination: 939 passed * The two e2e files cannot run locally and were syntax-checked
karoltheguy
added a commit
that referenced
this pull request
Aug 3, 2026
Fixes the quality gate on this PR. Adding DualProtocolCM to both test_log_level.py and test_session_duration.py pushed new-code duplication to 14% against a 3% threshold. The duplication was mostly pre-existing: the two files carried byte-identical copies of the login mock, the login helper and SettingsMockDB, 75 duplicated lines each. Touching those lines made them count as new code, which is the same inherit-the-debt effect seen on PR #304. Extracting the shared scaffolding fixes the cause rather than the symptom. The two files drop from 147 and 127 lines to 79 and 56, and shared blocks fall from 75 lines each to 22, which is now just imports and fixtures. Follows the existing tests/sudo_permissions.py precedent for a non-test helper module under tests/. test_sync_engine.py has its own DualProtocolCM copy, deliberately left alone here since it is modified on PR #309 and touching it would conflict. Unit 942 and unmarked 47, both unchanged.
karoltheguy
added a commit
that referenced
this pull request
Aug 3, 2026
…art 1) (#310) Fifth PR in the SonarCloud cleanup series (group 8: Python maintainability), after #303, #304, #308 and #309. Clears 13 of 19 findings. The 6 `python:S3776` cognitive-complexity refactors are held back for a second PR, since a control-flow change deserves isolated review and a revert that does not drag constant extraction with it. | Rule | Count | Status | |---|---|---| | `python:S1192` duplicated literals | 7 | Fixed | | `python:S108` empty block | 2 | Fixed, exposed two bad test mocks | | `python:S1066` nested if | 2 | Fixed | | `python:S112` generic exception | 2 | Fixed, unblocks a finding left open in #309 | | `python:S3776` cognitive complexity | 6 | Deferred to part 2 | ## S1192, and a count discrepancy worth knowing Sonar reports 3 occurrences each of `"/login"` and `"/change-password"`. The file actually contains **5 of each**, since Sonar's duplicate-literal count excludes route decorators. All occurrences were replaced, not just the flagged subset. Verified by AST rather than by eye: each of the seven literals now appears exactly once, as its constant definition, and **no other string literal in the file changed count**. That check is what makes a bulk find-and-replace across a 1700-line routing module trustworthy. ## S108 found a test that was propping up the outlier The two `async with db.execute(...): pass` blocks become `await db.execute(...)`, which is what **every other write site in the codebase** does. These two were the only exceptions. That broke two tests, and the tests were wrong. Their hand-rolled `SettingsMockDB.execute` returned a cursor supporting only `async with`, while a real `aiosqlite` `execute()` result supports both protocols. The repo already knows this: `tests/test_sync_engine.py:90` defines `DualProtocolCM` whose docstring says *"Objects returned by aiosqlite execute() support both `async with obj` and `await obj`."* Both mocks now use that same pattern, so the fake stops dictating which idiom production code may use. Worth being explicit, since "the change broke tests so I changed the tests" deserves scrutiny: the mock was asserting an implementation detail, not behavior. The tests still prove the same thing, that settings persist and apply live. ## S112, and why these do not subclass SSHCommandError The two bare `raise Exception` in `ssh_manager.py` become `ServerConfigurationError` and `KeyDecryptionError`, both subclassing `Exception` directly. Not `SSHCommandError`, because that type carries `exit_status` and `stderr` for a command that actually ran, and `tests/test_ssh_manager.py:137-138` asserts on exactly those fields. Both new errors fire while assembling the connection, before anything reaches the host, so typing them as command failures would hand callers a null exit status and imply a command failed when none was issued. Caller audit: all ~41 handlers in the codebase use `except Exception`, and there are no exact-type comparisons such as `type(e) is Exception`, so nothing downstream changes. Three new tests cover both types plus `__cause__` chaining, and they fail against the previous code at import time. **Follow-up this enables:** #309 left one `python:S5958` open because `ssh_manager.py:118` raised bare `Exception`, with a comment saying so. Once both merge, that comment is stale and the test can narrow to `KeyDecryptionError`. It could not be done in either PR alone without a conflict, since `tests/test_crypto.py` is modified in #309. ## S1066 Two nested ifs merged. `and` short-circuits, so `_drop_if_stale()`'s side effect still fires only when the connection is cached, exactly as before, and the conditions are not reordered. **One deliberate extra, not flagged by Sonar:** the structurally identical re-check inside `get_connection`'s lock was merged too. Fixing only the flagged one left two adjacent, semantically identical checks written differently, which is worse than either form. Easy to drop if you would rather keep the diff strictly to the findings. ## Verification * `-m unit`: 939 to 942, the three new exception tests, nothing lost * unmarked suite: 47 passed, unchanged * re-run in random order to catch cross-test contamination: 942 passed * AST verification of literal extraction as described above ## Deliberately skipped, with reasons Three `S3776` findings will not be fixed even in part 2: * `sync_engine.py:166` `check_quadlets` (complexity 25) and `sync_engine.py:114` `_fetch_mtimes`. These are the subject of open bug #283: the function iterates the `quadlets` table, and the only `INSERT INTO quadlets` anywhere in the repo is in test files, so it is dead in production. Its uncovered branches are **uncoverable by construction** while the work set is always empty, meaning no test can protect a refactor of the most complex function in the group. Whoever fixes #283 rewrites it anyway. * `scripts/import_servers.py:12`, a one-off script not referenced by `Dockerfile` or `start.sh`, so not shipped, and not covered. Restructuring untested, unshipped code to satisfy a metric is risk without benefit.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Second PR in the SonarCloud cleanup series (group 7: CI/build supply-chain hardening), following #303.
Fixes 6 of the 12
githubactions:*findings in.github/workflows/tests.yml, applied identically to both thetest-githubandtest-forgejojobs.npm (
githubactions:S6505, 2 findings)npm cibecomes:The naive fix of just appending
--ignore-scriptswould have broken CI. The workflow's own comment says so:postinstallis what vendors monaco, xterm and quadlet-lint intostatic/vendor/, and the docker suites bind-mount the host tree over the app container. Suppressing it silently would have taken out every browser suite. Splitting it means third-party lifecycle scripts stay blocked while our own vendoring step is invoked deliberately.Verified in an isolated copy of
package.json+package-lock.json:npm ci --ignore-scriptsleaves nostatic/vendor/at all, confirming the flag actually suppresses the hooknpm run copy-assetsproduces the full vendor tree, byte-identical tonode_modules/monaco-editor/min/vsand the xterm/quadlet-lint sourcespip (
githubactions:S8541, 4 findings)Both requirements installs now pass
--only-binary :all:, so no sdistsetup.pyexecutes during install.Verified rather than assumed, since a missing wheel would fail CI hard: a
pip install --dry-run --only-binary :all:of both requirements files on Python 3.12 (matchingPYTHON_VERSION) resolves cleanly, exit 0.Deliberately not fixed
The 6
githubactions:S8544findings ("using dependencies without locking resolved versions") on the same lines stay open. Both requirements files use>=ranges, so satisfying that rule means introducing a genuinely pinned lockfile (pip-compile, ideally with hashes). That is a dependency-management decision that interacts with Dependabot, and folding it into a workflow-only change would make neither part reviewable. Worth its own issue.Note unrelated to this PR
While verifying, I found that a local
static/vendor/monaco/checkout can go stale: same pinnedmonaco-editor0.56.0 innode_modules, but an older vendored layout on disk. CI is unaffected (its cache is keyed onhashFiles('package-lock.json')), but a local dev server may be serving old monaco untilnpm run copy-assetsis re-run.