fix(security): the desktop code scanning alerts - #41
Merged
Merged
Conversation
Closes #24. **A game name could move a recording out of the library.** `MoveClipToGameAction` joined whatever came in as `targetGame` onto the videos root, so `..\..\Somewhere` was a folder outside it and the clip was renamed there. `services/gameFolder.ts` refuses a name holding a separator, a drive colon, a control character, a leading dot (the scan would never see the clip again), a trailing dot or space (Windows drops them), or a device name; the route answers 400 with the reason, and the action checks the resolved folder sits directly inside the root anyway, because one guard is a single point of failure on the line that moves a recording. Refused rather than cleaned, unlike `safeName`: this is a name somebody typed, and filing their clip under a different spelling is a surprise. **A tag rule could hang the library.** Compiling a regular expression the user wrote is the feature, so it is not escaped. What is refused now is the shape that backtracks without end, a quantified group that already contains a quantifier (`(a+)+`), plus anything over 200 characters. `tagPatternProblem` in `src/shared` is the one answer the route and the DTO both use. The 34 rules in the real library all pass (checked read only). Rules already saved are not touched. **Two linear trims for two quadratic ones.** `/^'+|'+$/` in the search tokeniser and `/\.+$/` on an export's name retry from every position of a long run that does not end the string. `utils/trimChar.ts` is a loop; a test holds it under 50 ms on 200,000 dots. `tests/unit/main/codeScanning.spec.ts`, 38 cases. `npm run check` green. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
DarrellVS
added
bug
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.
Closes #24. Code scanning alerts 4, 5, 7, 8, 9, 15.
Path injection (alerts 7, 8, 9)
MoveClipToGameActionjoinedtargetGameonto the videos root, so..\..\Somewheremoved the recording outside the library. Now:services/gameFolder.tsrefuses separators, drive colon, control characters, a leading dot (the scan would never see the clip again), a trailing dot or space, and Windows device names. The route answers 400 with the reason.Regex injection (alert 15)
Compiling the user's own expression is the feature, so it is not escaped. Refused now: a quantified group that already contains a quantifier (
(a+)+), and anything over 200 characters.tagPatternProbleminsrc/sharedis used by both the route and the DTO. All 34 rules in the real library pass (checked read-only). Saved rules are not touched.This alert will likely stay open, because CodeQL flags any
new RegExp(userInput). Once merged I will dismiss it as won't fix, pointing at this PR.Polynomial regex (alerts 4, 5)
/^'+|'+$/(search tokeniser) and/\.+$/(export name) replaced with a linearutils/trimChar.ts; a test holds it under 50 ms on 200,000 dots.Gates
tests/unit/main/codeScanning.spec.ts, 38 cases.npm run check: 681 tests.🤖 Generated with Claude Code