Refactor: improve code quality, security, and testability#5
Open
TerrifiedBug wants to merge 10 commits intomainfrom
Open
Refactor: improve code quality, security, and testability#5TerrifiedBug wants to merge 10 commits intomainfrom
TerrifiedBug wants to merge 10 commits intomainfrom
Conversation
Introduces Deps struct with DiskOps, GadgetOps, ArchiveOps, SystemOps, KeepAwaker, and Notifier interfaces. Concrete adapters in internal/wire wrap existing package-level functions. State machine is now unit-testable with mock implementations.
Greptile SummaryThis PR is a well-structured refactoring of the teslausb-go codebase, introducing dependency injection across the state machine, extracting shared helpers, fixing path traversal vulnerabilities, and cleaning up WebSocket dead-connection handling. The vast majority of the changes are clean improvements with good test coverage. Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Boot([Boot]) --> Run
Run["Run()\nDisk.Exists? → Create\nGadget.Enable → gadgetEnabled=true"] --> Away
Away["runAway()\nevery 30s: tryEnableGadget()\nArchive.IsReachable?"] -->|reachable| Arriving
Away -->|ctx.Done| Stop([Stop])
tryEnable["tryEnableGadget()\ngadgetEnabled==true → skip\nGadget.Enable → gadgetEnabled=true"] -.->|called from| Away
tryEnable -.->|called from| Idle
Arriving["runArriving()\nGadget.WaitForIdle\nGadget.Disable → gadgetEnabled=false"]
Arriving -->|Disable fails| DisableErr["gadgetEnabled=false ⚠️\n(gadget still active)\n→ StateAway"]
Arriving -->|Disk.Mount fails| MountErr["Gadget.Enable (no flag update ⚠️)\n→ StateAway"]
Arriving -->|Archive.MountArchive fails| ArchErr["Gadget.Enable (no flag update ⚠️)\n→ StateAway"]
Arriving -->|success| Archiving
DisableErr --> Away
MountErr --> Away
ArchErr --> Away
Archiving["runArchiving()\nstartKeepAlive\nArchive.ArchiveClips\nupdateAndPersistStats\nArchive.ManageFreeSpace"] --> Idle
Idle["runIdle()\nGadget.Enable → gadgetEnabled=true\nevery 30s: tryEnableGadget\nArchive.IsReachable?"] -->|unreachable| Away
Idle -->|ctx.Done| Stop
Last reviewed commit: 09a825e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+218
to
222
| if err := m.deps.Disk.Mount(); err != nil { | ||
| log.Printf("mount cam: %v", err) | ||
| gadget.Enable(disk.BackingFile) | ||
| m.deps.Gadget.Enable(m.deps.Disk.BackingFilePath()) | ||
| m.setState(StateAway) | ||
| return |
There was a problem hiding this comment.
gadgetEnabled not updated after fallback Enable() calls
When Disk.Mount() fails (and similarly when Archive.MountArchive() fails at line 229), the code calls Enable() to restore the gadget, but m.gadgetEnabled is never set back to true. It was set to false a few lines earlier (line 214) when Disable() succeeded. This leaves the flag permanently false while the hardware is actually enabled.
The consequence: on the very next tick of runAway, tryEnableGadget() sees gadgetEnabled == false and calls Enable() again on an already-configured USB gadget. The architecture notes flag this as critical: calling Enable() when the gadget is already set up writes to /sys/kernel/config/usb_gadget/teslausb/ and rebinds the UDC, which can briefly disconnect the USB drive from Tesla or trigger a kernel oops on the Pi Zero.
Both fallback paths should update the flag:
if err := m.deps.Disk.Mount(); err != nil {
log.Printf("mount cam: %v", err)
if enableErr := m.deps.Gadget.Enable(m.deps.Disk.BackingFilePath()); enableErr == nil {
m.gadgetEnabled = true
}
m.setState(StateAway)
return
}Same fix applies to the Archive.MountArchive() error branch at line 229.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/state/machine.go
Line: 218-222
Comment:
**`gadgetEnabled` not updated after fallback `Enable()` calls**
When `Disk.Mount()` fails (and similarly when `Archive.MountArchive()` fails at line 229), the code calls `Enable()` to restore the gadget, but `m.gadgetEnabled` is never set back to `true`. It was set to `false` a few lines earlier (line 214) when `Disable()` succeeded. This leaves the flag permanently `false` while the hardware is actually enabled.
The consequence: on the very next tick of `runAway`, `tryEnableGadget()` sees `gadgetEnabled == false` and calls `Enable()` again on an already-configured USB gadget. The architecture notes flag this as critical: calling `Enable()` when the gadget is already set up writes to `/sys/kernel/config/usb_gadget/teslausb/` and rebinds the UDC, which can briefly disconnect the USB drive from Tesla or trigger a kernel oops on the Pi Zero.
Both fallback paths should update the flag:
```go
if err := m.deps.Disk.Mount(); err != nil {
log.Printf("mount cam: %v", err)
if enableErr := m.deps.Gadget.Enable(m.deps.Disk.BackingFilePath()); enableErr == nil {
m.gadgetEnabled = true
}
m.setState(StateAway)
return
}
```
Same fix applies to the `Archive.MountArchive()` error branch at line 229.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+208
to
213
| if err := m.deps.Gadget.Disable(); err != nil { | ||
| log.Printf("disable gadget: %v", err) | ||
| m.gadgetEnabled = false | ||
| m.setState(StateAway) | ||
| return | ||
| } |
There was a problem hiding this comment.
gadgetEnabled set to false despite Disable() failing
When Disable() returns an error, the gadget is still enabled — the Disable call did nothing. Setting m.gadgetEnabled = false here misrepresents the hardware state. Back in runAway, tryEnableGadget() will see gadgetEnabled == false and call Enable() on a gadget that never stopped running. Depending on whether Enable() is idempotent for the configfs/UDC setup, this either produces harmless log spam or causes a brief USB reconnect visible to Tesla.
The flag should be left as true when Disable() fails — the gadget is still active:
Suggested change
| if err := m.deps.Gadget.Disable(); err != nil { | |
| log.Printf("disable gadget: %v", err) | |
| m.gadgetEnabled = false | |
| m.setState(StateAway) | |
| return | |
| } | |
| if err := m.deps.Gadget.Disable(); err != nil { | |
| log.Printf("disable gadget: %v", err) | |
| // gadgetEnabled stays true — Disable did not succeed, USB is still active | |
| m.setState(StateAway) | |
| return | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/state/machine.go
Line: 208-213
Comment:
**`gadgetEnabled` set to `false` despite Disable() failing**
When `Disable()` returns an error, the gadget is **still enabled** — the Disable call did nothing. Setting `m.gadgetEnabled = false` here misrepresents the hardware state. Back in `runAway`, `tryEnableGadget()` will see `gadgetEnabled == false` and call `Enable()` on a gadget that never stopped running. Depending on whether `Enable()` is idempotent for the configfs/UDC setup, this either produces harmless log spam or causes a brief USB reconnect visible to Tesla.
The flag should be left as `true` when `Disable()` fails — the gadget is still active:
```suggestion
if err := m.deps.Gadget.Disable(); err != nil {
log.Printf("disable gadget: %v", err)
// gadgetEnabled stays true — Disable did not succeed, USB is still active
m.setState(StateAway)
return
}
```
How can I resolve this? If you propose a fix, please make it concise.
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.
Summary
filepath.Clean+strings.Contains("..")path validation with robustsafePath()helper usingfilepath.Relto prevent directory traversal in 3 web handlersBroadcast()now removes dead connections instead of accumulating stale entries;config.Get()returns defensive copy to prevent mutation leakstryEnableGadget(), decompose 67-linerunArchivinginto focused helpers, extract shared NFS/CIFS mount-test scaffoldingDiskOps,GadgetOps,ArchiveOps,SystemOps,KeepAwaker,Notifier) with concrete adapters ininternal/wire, making the state machine fully unit-testable with mock implementationsFiles Changed
internal/web/pathutil.go,internal/web/pathutil_test.go,internal/web/mounttest.go,internal/web/ws_test.go,internal/state/deps.go,internal/wire/wire.gointernal/state/machine.go,internal/state/machine_test.go,internal/web/server.go,internal/web/ws.go,internal/config/config.go,internal/config/config_test.go,internal/web/server_test.go,internal/disk/disk.go,internal/gadget/idle.go,internal/archive/archive.go,cmd/teslausb/main.goTest plan
go test ./...)TestSafePathcovers 8 path traversal scenariosTestHubBroadcastRemovesDeadClientsverifies dead WS cleanup with real httptest serverTestGetReturnsCopyproves config mutation isolationTestTryEnableGadget,TestRunAwayTransitionsOnReachable,TestRunIdleNotifiesOnGadgetEnable) use mock dependenciesgo build ./...)