test: cover the untested auth paths — and fix the three bugs that found - #49
Merged
Conversation
Measured line coverage first rather than guessing: 50% overall, and the gaps
were not where you'd assume. app.py 42% is mostly install/bootstrap work that
needs a real host. What stood out instead were small, fully-testable functions
that decide who may do what, with ZERO executed lines.
Three real bugs, each found by a test written for something else:
1. ssh_manager._is_protected_path — the file manager's only guard against
deleting the game install. It read the RAW path string while delete_path
`rm -rf`s the RESOLVED one, so anything that normalised onto a protected
tree walked straight through:
./lgsm -> rm -rf /home/<user>/lgsm (LinuxGSM tree)
x/../serverfiles -> rm -rf /home/<user>/serverfiles (the game install)
a/b/../../.ssh -> rm -rf /home/<user>/.ssh (the host's keys)
Six forms in total. Now normalised inside the guard, so both callers are
covered and anything climbing out of the home dir collapses to "refused".
2. models.host_key_fingerprint — base64.b64decode's lenient default DISCARDS
characters outside the alphabet, so a corrupted pin like "ssh-rsa ***"
decoded to b"" and printed the sha256 of nothing: a confident-looking
fingerprint for a key that isn't there, in the one place an operator is
asked to eyeball what they're trusting. validate=True now.
3. terminal.strip_escapes — an ESC that starts a sequence matching none of the
three grammars survived to the page: a trailing "\x1b", or "\x1b\t" /
"\x1b\x00". Console text carries player names and chat, so those bytes are
authored, not accidental.
New tests, all verified by mutation:
- API tokens (unit + smoke). A Bearer token authenticates as its owner and
inherits their full RBAC, and app.py exempts Bearer requests from CSRF —
a whole authentication path with no test. Asserts only the hash is stored,
that REPLAYING the stored hash does not authenticate, that a token sees only
its owner's servers, and that deactivating or revoking kills it.
- can_run_custom_command (smoke). Eight branches deciding who may run a
superadmin-authored console command; the one that matters is that access to
the server is not access to the command.
- Privilege escalation (rbac). A delegated MANAGE_GROUPS admin can edit a group
they belong to; _grantable_perms is all that stops self-promotion. The
existing "can't grant super_admin" check passes even with the guard deleted,
because super_admin is filtered separately. Now asserts a real permission
cannot be granted, and that an edit preserves one the editor cannot grant.
- Bulk actions (rbac). /api/servers/bulk-action is not an <int:server_id>
route, so the structural sweep never saw it. Asserts a server on a
non-granted host is refused AND that no SSH command is issued.
- The file-manager guard (unit), including delete_path end-to-end asserting no
shell command runs at all for a protected path.
- A fifth fuzz target, console, over terminal.py — the only parser whose input
is partly attacker-AUTHORED. It asserts no raise, no ESC and no CR survive;
the ESC property is what found bug 3. 12 seeds, wired into the fuzz matrix.
unit 763 -> 821, smoke 253 -> 268, rbac 60 -> 65.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`* text=auto eol=lf` rewrote CRLF out of two console seeds on the way into the index, so crlf_log and jline_echo were committed as LF — deleting the exact byte sequence they exist to feed the renderer (a CRLF log line, and JLine's in-place echo ending in CRLF). Corpora are binary inputs, not text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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.
I measured coverage first rather than guessing. 50% overall, and the gaps weren't where you'd assume:
app.pyat 42% is mostly install/bootstrap work that needs a real host. What stood out were small, fully-testable functions that decide who may do what, with zero executed lines.Writing tests for them turned up three real bugs.
Bug 1 — the file manager could delete the game install
_is_protected_pathis the only guard between the delete button and an unrecoverable wipe. It read the raw path string, whiledelete_pathrunsrm -rfon the resolved one. Anything that normalised onto a protected tree walked straight through:rm -rfactually gets./lgsm/home/<user>/lgsm— the LinuxGSM control treex/../serverfiles/home/<user>/serverfiles— the entire game installa/b/../../.ssh/home/<user>/.ssh— the host's SSH keysSix forms in total, all now closed by normalising inside the guard, so both callers are covered and anything climbing out of the home dir collapses to "refused". Ordinary paths stay deletable.
Bug 2 — a corrupt host key showed a confident fingerprint
base64.b64decode's lenient default discards characters outside the alphabet, so a pin like"ssh-rsa ***"decoded tob""and printed the SHA-256 of nothing — a real-looking fingerprint for a key that isn't there, in the one place an operator is asked to eyeball what they're trusting.validate=Truenow.Bug 3 — a player-authored escape byte reached the page
strip_escapesdropped sequences matching its three grammars, but an ESC starting a sequence that matched none of them survived: a trailing\x1b, or\x1b\t/\x1b\x00. Console text carries player names and chat, so those bytes are authored, not accidental. Found by the new fuzz target's property, not by hand.The tests
Every one verified by mutation.
can_run_custom_command(smoke) — eight branches deciding who may run a superadmin-authored console command. The one that matters: access to the server is not access to the command.MANAGE_GROUPSadmin can edit a group they belong to, so_grantable_permsis all that stops self-promotion. The existing "can't grant super_admin" check passes even with the guard deleted, because super_admin is filtered separately. Now asserts a real permission can't be granted, and that an edit preserves one the editor can't grant./api/servers/bulk-actionisn't an<int:server_id>route, so the structural sweep never saw it. Asserts a server on a non-granted host is refused and that no SSH command is issued. Under mutation this one caught the real command that would have fired.delete_pathend-to-end, asserting no shell command runs at all for a protected path.console—terminal.pyis the only parser whose input is partly attacker-authored. Asserts no raise, no ESC and no CR survive. 12 seeds, wired into the fuzz matrix.Also:
.gitattributesnow keeps corpora byte-exact —* text=auto eol=lfhad rewritten CRLF out of two console seeds on the way into the index, deleting the exact bytes they exist to test.unit 763 → 821 · smoke 253 → 268 · rbac 60 → 65, lint clean.
🤖 Generated with Claude Code