feat: OAuth SSO, decoder programming, and wizard drive support - #59
Conversation
Add OAuth clients/authorize with layout_id pinning, CV/addr programming on dcc-bus, Basic function template seed, and Impersonate-As on the dcc-bus WS proxy so the wizard can pulse functions as the vehicle owner. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This Pull Request introduces two major features: DCC decoder programming and OAuth SSO, along with several related improvements. The implementation demonstrates a high level of detail and robustness, with careful consideration for safety, error handling, and operational flexibility. The addition of comprehensive tests for these new features is commendable.
| programmingReadRetries = 1 | ||
| // programmingSettle lets a decoder finish its internal write cycle | ||
| // before the next CV is pushed (matches the loco CLI default). | ||
| programmingSettle = 300 * time.Millisecond |
There was a problem hiding this comment.
ℹ️ [LOW] PRIORITY:LOW: The programmingSettle constant (300ms) is noted as matching the loco CLI default. While this is good for consistency, consider if this value might need to be configurable in the future, perhaps as a command station setting, if different decoders or standards require varying settle times. For now, it's a reasonable default.
| } | ||
| go func() { | ||
| defer w.Close() | ||
| debounce := time.NewTimer(0) |
There was a problem hiding this comment.
ℹ️ [LOW] PRIORITY:LOW: The debounce logic in StartWatch resets the timer on every event. In a scenario with a very rapid, sustained burst of file system events, this could theoretically delay the reload() call indefinitely until the event stream completely stops for 300ms. For a configuration directory, this is unlikely to be a practical issue, but a more robust debounce might ensure a reload eventually happens even during sustained activity (e.g., a maximum wait time).
|
|
||
| // programmingGate rejects every CV / address use case when the daemon | ||
| // was started without --enable-programming, or when no driver is bound. | ||
| func (r *Router) programmingGate() (Result, bool) { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The introduction of programmingGate is an excellent safety measure, ensuring that programming operations are only attempted when explicitly enabled and a command station is available. This prevents accidental or unauthorized modifications to decoders.
| } | ||
|
|
||
| r.progMu.Lock() | ||
| defer r.progMu.Unlock() |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The progMu mutex in the Router is a crucial addition for concurrency safety. It correctly serializes multi-round-trip programming sequences (like CV batches or address rewrites) to prevent interleaving and potential corruption on a single command station.
| @@ -0,0 +1,57 @@ | |||
| package cmd | |||
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The dedicated programming_addr.go file for NMRA S-9.2.2 address encoding/decoding logic is a great example of clear separation of concerns. This complex logic is well-encapsulated and tested.
| out = s.ackOrFail(ctx, sess, env.ID, false, errors.WsCodeBadPayload) | ||
| break | ||
| } | ||
| if !s.programmingEnabled { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The early-exit checks for s.programmingEnabled and payload validation within the WebSocket handler (handleFrame) are good for efficiency and security. Requests for disabled functionality or invalid payloads are rejected before reaching the core router logic.
| } | ||
|
|
||
| // StartWatch watches the drop-in directory and reloads on change. | ||
| func (r *OAuthClientsRegistry) StartWatch(stop <-chan struct{}) error { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The OAuthClientsRegistry with fsnotify for dynamic reloading of client configurations is a well-designed and flexible solution. This allows for updating OAuth client settings without requiring a service restart, improving operational agility.
| return signed, expiry, nil | ||
| } | ||
|
|
||
| // IssueImpersonatedToken signs a session JWT for login when actor is an |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The IssueImpersonatedToken function, with its robust role and user activity checks, provides a secure mechanism for administrative impersonation, which is valuable for features like the wizard drive support mentioned in the PR summary.
| // scanning upwards from autoAllocateFirstDCCAddress, and returns them | ||
| // merged into as few contiguous ranges as possible. It returns | ||
| // ErrDCCPoolExhausted when fewer than count addresses remain free. | ||
| func allocateFreeDCCAddresses(count int, existing []domain.DCCAddressRange) ([]PoolRange, error) { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The allocateFreeDCCAddresses function and its associated tests provide a solid foundation for automatic DCC address allocation. The choice to start allocation from uint16(50) is a reasonable heuristic to leave lower addresses free for common factory defaults.
| return out | ||
| } | ||
|
|
||
| // listAvailableStations returns the stations a throttle on layoutID may |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: Filtering command stations marked HideInThrottle from the listAvailableStations improves the user experience by preventing dedicated programming stations from cluttering the regular throttle interface. This is a thoughtful UX improvement.
keskad
left a comment
keskad
left a comment
There was a problem hiding this comment.
Review Summary
This PR delivers three substantial features in one commit: OAuth2 SSO (authorize/token + drop-in clients), dcc-bus decoder programming (CV read/write + address get/set), and wizard drive support via X-BigFred-Impersonate-As on the dcc-bus WS proxy. The implementation is generally high-quality with strong test coverage on the new logic (programming_addr, dcc_pool allocation, oauth_clients, seed_helpers).
Local verification: go build ./..., go vet ./..., go test on all touched packages, and tsc -b --noEmit (web) all pass clean.
What's done well
- Defense-in-depth on programming:
programmingEnabledis checked twice (WS handler +programmingGate), andprogMucorrectly serializes multi-roundtrip sequences.programming_addr.gocleanly isolates NMRA S-9.2.2 logic with no I/O deps — trivially testable. - OAuth code flow:
GetDelenforces single-use, 128-bit random codes,clientID+redirectURIbinding on exchange.RedirectURIAllowedis exact-match (no prefix/wildcard — good). - Impersonation:
IssueImpersonatedTokenre-validates admin + subject active, mirroringMaybeImpersonate. Layout preserved from actor ("participants inherit the organizer makieta") — matches PR intent. - Migrations:
Basictemplate seed is idempotent (WHERE NOT EXISTS), andforceMomentaryOverridefor F3/F4 is a clean extension of the existing seed helper. - UX:
HideInThrottlefiltering keeps dedicated programming stations out of the throttle picker — thoughtful.
Issues to address (by severity)
Security / correctness
- Client secret comparison is not constant-time (
oauth.go:105). Usesubtle.ConstantTimeCompare. Also reject empty configured secrets atreload()time, not just at exchange. - No PKCE for the public client (
oauth.go). If the browser drives token exchange, theclientSecretis observable. Clarify whether the wizard Rust backend is the token-exchange party; if so, document it. If not, add PKCE (S256). - WS proxy does not verify
claims.LID == daemon.layoutID(dcc_bus_proxy.go:88). An admin on layout A could drive a daemon serving layout B via impersonation if they know a vehicle owner on B. Recommend the daemon rejectsLIDmismatch on upgrade. stateparam is unvalidated (http/oauth.go:82). Add length + charset bounds to harden against response-splitting regressions and match RFC 6749 §10.12.
Reliability
5. progMu held during time.Sleep(programmingSettle) (programming.go:95). For long CV batches this stalls all programming on the daemon and is not ctx-cancellable. Use select with ctx.Done().
6. uint8(value) truncation on CV read (programming.go:129, :160). Range-check before cast; a misbehaving driver returning out-of-range values would silently wrap.
7. Debounce starvation in StartWatch (oauth_clients.go:90). Sustained event bursts can delay reload indefinitely. Add a max-wait cap or drop the debounce (dir scan is cheap).
8. Empty-branch filter in StartWatch (oauth_clients.go:89) is a confusing no-op that still debounces on non-JSON events. Either continue on non-JSON or handle dir events explicitly.
9. Control-channel programming rejections are fire-and-forget (control_redis.go:59). Server-side callers have no feedback when CodeProgrammingDisabled fires. Add a result channel or at minimum surface it to the publisher.
Minor / UX
10. validatePoolRanges(userID=0, ...) in User.Create (user.go:196) uses a magic 0 — name it or wrap, so future readers don't read it as a bug.
11. redirectToLogin's HasPrefix guard (http/oauth.go:91) is effectively dead on first entry — simplify or document the re-entry case.
12. LocoAddrGet validation (validation/ws.go:119) allows Address==0 for POM, deferring the error to the router as WsCodeBadPayload — could reject earlier with a clearer message.
13. Wizard layout step summary (ConnectionWizardPage.tsx:627) omits the programming/track choices just made — add them to the final review.
14. oauthCodeTTL = 60s is within spec but on the high side; 30s would tighten the replay window. Defensible for a slow SPA exchange.
15. allocateFreeDCCAddresses (dcc_pool.go:188) is O(address space) per call — fine at current scale, worth a complexity comment.
16. seedTemplateFunctionsWithForce UNION ALL (seed_helpers.go:67) doesn't scale to arbitrary template sizes — cap if templates ever exceed ~50 functions.
Verdict
The core logic is sound and well-tested. The security items (1-4) are the most important to address before merge — particularly (3) the daemon-side LID check, which is a real cross-layout escalation path, and (1) the constant-time comparison. The reliability items (5-9) are worth fixing in this PR or a fast follow-up. The minor items can be addressed opportunistically.
Recommendation: REQUEST_CHANGES on the security items; the rest can land as follow-ups. (Posting as COMMENT since GitHub does not allow requesting changes on one's own PR.)
| qq := u.Query() | ||
| qq.Set("code", code) | ||
| if state != "" { | ||
| qq.Set("state", state) |
There was a problem hiding this comment.
state is fine, but state is never validated after redirect.
More importantly: when state is non-empty, it is echoed back into the redirect URL (qq.Set("state", state)) without any validation. Since redirectURI is allowlisted (good), the code and state land on a trusted host — but state is attacker-controllable and is placed verbatim into the query string of a 302 redirect. A malicious state containing newlines/CR could enable HTTP response splitting if http.Redirect ever serializes it raw. In practice Go's url.Values.Encode escapes these, so this is low-risk today, but I recommend explicitly validating state length (e.g. <= 512 chars) and charset (^[A-Za-z0-9._~-]*$) at the top of Authorize to harden against future regressions and to match RFC 6749 §10.12 (CSRF binders should be opaque but bounded).
| var p oauthCodePayload | ||
| if err := json.Unmarshal(raw, &p); err != nil { | ||
| return TokenExchangeResult{}, svcerrors.ErrOAuthInvalidGrant | ||
| } | ||
| if p.ClientID != in.ClientID || p.RedirectURI != in.RedirectURI { |
There was a problem hiding this comment.
clientID+redirectURI, but NOT to the user session.
IssueCode stores UserID/LayoutID/ClientID/RedirectURI in Redis. ExchangeCode validates p.ClientID == in.ClientID && p.RedirectURI == in.RedirectURI — good. However, the code is a 32-char hex (128 bits) with a 60s TTL and GetDel ensures single-use — this is correct and matches RFC 6749 §4.2.2.
One gap: there is no PKCE (code_challenge/code_verifier) support. For a public client like bigfred-wizard (a React SPA that cannot hold a client secret confidentially), RFC 6749 §10.6 explicitly recommends PKCE. The current design relies on the SPA shipping the clientSecret in JS, which is observable via devtools/network. Consider:
- Adding PKCE (S256) as the primary protection for public clients, OR
- Documenting that the wizard backend (
bigfred-wizardRust) is the actual token-exchange party (not the browser), so the secret never reaches the SPA.
If the wizard backend performs the exchange, this is fine — but the PR description ("silent SSO") suggests the browser may drive the flow. Worth clarifying.
| if err != nil { | ||
| return TokenExchangeResult{}, err | ||
| } | ||
| if c.ClientSecret == "" || c.ClientSecret != in.ClientSecret { |
There was a problem hiding this comment.
clientSecret is missing.
c.ClientSecret == in.ClientSecret uses a plain == comparison, which is vulnerable to timing side-channels. For a secret comparison, use subtle.ConstantTimeCompare([]byte(c.ClientSecret), []byte(in.ClientSecret)) != 1. The same applies to the code comparison in ExchangeCode (line 123) — though there GetDel already consumes the code so timing is less critical. The client-secret comparison happens on every token request and is the more sensitive path.
Also: if c.ClientSecret == "" (empty), the check c.ClientSecret == "" || c.ClientSecret != in.ClientSecret rejects — good, but an empty configured secret should ideally be rejected at reload() time in oauth_clients.go rather than only at exchange time, so a misconfigured client fails loudly on load.
| filepath.Ext(ev.Name) != "" { | ||
| // still reload on dir events that may affect json files | ||
| } | ||
| debounce.Reset(300 * time.Millisecond) |
There was a problem hiding this comment.
🐛 [BUG] Debounce timer can starve reload indefinitely under sustained event bursts.
debounce.Reset(300ms) on every event means a continuous burst of fs events (e.g. an editor writing many temp files, or rsync replacing the dir) will keep pushing the reload out forever — the <-debounce.C case never fires. The bot already flagged this as LOW, but for a config directory this can mask a failed reload for the whole lifetime of the burst.
Suggested fix: cap the delay with a max-wait timer:
maxWait := time.NewTimer(2 * time.Second)
...
case <-maxWait.C:
// force reload even if events are still coming
if !debounce.Stop() { <--debounce.C }
reload()
maxWait.Reset(2 * time.Second)Alternatively, since reload() reads the whole dir and replaces byID atomically under mu, you could simply call reload() directly on each event and drop the debounce entirely — the cost of a dir scan is negligible for a handful of JSON files.
| if !strings.HasSuffix(strings.ToLower(ev.Name), ".json") && | ||
| filepath.Ext(ev.Name) != "" { | ||
| // still reload on dir events that may affect json files | ||
| } |
There was a problem hiding this comment.
🐛 [BUG] Empty-branch filter silently reloads on every non-JSON event.
The condition if !strings.HasSuffix(...) && filepath.Ext(ev.Name) != "" evaluates to true for e.g. foo.txt, then falls through with an empty body — so it still calls debounce.Reset(300ms) below. The comment says "still reload on dir events that may affect json files" but the logic actually reloads on ANY event with a non-empty extension that isn't .json. For a directory containing oauth-clients/*.json plus a stray README.md or .swp editor temp file, this triggers spurious reloads.
Recommend either:
continuewhen the file is not.json(and rely onfsnotifydir events, which have emptyev.Name), or- explicitly handle
ev.Op & fsnotify.Rename == 0 && filepath.Ext == ""for directory-level events.
As written this is functionally a no-op that always debounces, which is confusing to read.
| </> | ||
| )} | ||
|
|
||
| {activeStep === "slots" && ( |
There was a problem hiding this comment.
layout step summary omits the programming/hideInThrottle/track choices. The summary Alert on the layout step lists name, kind, uri, remotes, and bootStop — but not the programming/track decisions just made on this new programming step. For a wizard whose headline feature is decoder programming, the final review before finish() should show e.g. programming: yes (prog track), hidden in throttle: yes. Minor, but improves the "are you sure?" moment.
| ))} | ||
| </Select> | ||
| </FormControl> | ||
| )} |
There was a problem hiding this comment.
✨ [POSITIVE] CommandStationsPage correctly gates defaultProgrammingTrackOutput behind programmingInput. The track selector only renders when programmingInput is true (line 620), and the help text is conditionally shown too. The openEdit initializer (line 209-211) correctly falls back to DEFAULT_PROGRAMMING_TRACK_OUTPUT when the stored value is missing — so legacy stations created before this PR don't crash the edit dialog. Good defensive handling of the nullable field.
| svcerrors "github.com/keskad/loco/pkgs/bigfred/server/errors" | ||
| ) | ||
|
|
||
| const oauthCodeTTL = 60 * time.Second |
There was a problem hiding this comment.
oauthCodeTTL = 60s is on the high side for an authorization code. RFC 6749 §4.2.2 recommends the shortest practical lifetime; 10 minutes is the max. 60s is within spec, but since GetDel already enforces single-use, a shorter TTL (e.g. 30s) reduces the window for a leaked code to be replayed. Not blocking — 60s is defensible for a slow-decoder-wizard flow where the SPA may take a moment to POST the token exchange. Just flagging for awareness.
| func (h *OAuthHandler) redirectToLogin(w http.ResponseWriter, r *http.Request, layoutID uint) { | ||
| returnTo := r.URL.RequestURI() | ||
| if !strings.HasPrefix(returnTo, "/api/v1/auth/oauth/authorize") { | ||
| returnTo = "/api/v1/auth/oauth/authorize?" + r.URL.RawQuery |
There was a problem hiding this comment.
redirectToLogin builds return_to from r.URL.RequestURI() but the guard strings.HasPrefix(returnTo, "/api/v1/auth/oauth/authorize") is always false on the first call — because RequestURI() returns the full path+query starting with /api/v1/auth/oauth/authorize?..., which DOES match the prefix. So the guard's else branch (line 91) only fires if RequestURI() somehow returns something unexpected. The intent seems to be: "if we got here via a re-entry (return_to already set), preserve it" — but return_to is derived from the current request, not from a query param. So this branch is effectively dead code on first entry and the else reconstructs the same URL. Harmless, but the guard reads as a safety check that never triggers. Consider simplifying to returnTo := r.URL.RequestURI() directly, or document the re-entry scenario it protects against.
| if !subject.Active { | ||
| return "", time.Time{}, svcerrors.ErrAccountDeactivated | ||
| } | ||
| return a.IssueToken(Identity{User: subject, Layout: actor.Layout}) |
There was a problem hiding this comment.
✨ [POSITIVE] IssueImpersonatedToken re-validates admin role and subject active state before minting. This mirrors MaybeImpersonate's gate, so the WS proxy path (which calls IssueImpersonatedToken directly, bypassing MaybeImpersonate) cannot escalate: a non-admin actor's token request returns ErrImpersonationForbidden, and a deactivated subject returns ErrAccountDeactivated. The layout is preserved from the actor ("participants inherit the organizer makieta") — consistent with the PR summary. The duplicated admin-check logic between here and MaybeImpersonate is a minor DRY violation but acceptable for a security-critical path where explicitness aids auditing.
…ctions Skip non-JSON fsnotify events in the OAuth clients drop-in watcher and publish control.programming.rejected on the dcc-bus event channel when Redis control-channel programming commands fail. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
This Pull Request introduces significant new features, including OAuth SSO, comprehensive DCC decoder programming capabilities, and improved user management with DCC address auto-allocation. The implementation demonstrates a strong focus on security, robustness, and testability. The addition of progMu for serializing programming operations is a critical and well-considered design choice for correctness. The OAuth implementation follows best practices, and the dynamic client registry is a flexible solution. Overall, this is a high-quality and impactful set of changes.
| "github.com/keskad/loco/pkgs/bigfred/dcc-bus/protocol" | ||
| ) | ||
|
|
||
| // Address encoding per NMRA S-9.2.2. Mirrors pkgs/loco/app/addr.go, |
There was a problem hiding this comment.
ℹ️ [LOW] PRIORITY:LOW: The duplication of address encoding/decoding logic from pkgs/loco/app/addr.go is noted and explained. While currently necessary due to export limitations, it's worth considering if these helpers could be made more generic or moved to a shared utility package in the future to reduce code duplication.
| // the isolated programming output cannot disturb locos on the main track. | ||
| const DefaultProgrammingTrack = protocol.ProgrammingModeProg | ||
|
|
||
| // ProgrammingTrackFromFlag normalises --default-programming-track. An |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The ProgrammingTrackFromFlag function provides clear validation and normalization for the programming track configuration, improving robustness and user experience.
| // did not run (loco-server can log it, surface it to an admin HUD, or | ||
| // retry against a different station). On success the daemon log is | ||
| // enough; no event is emitted. | ||
| func (r *Router) logControlProgramming(frameType string, res Result) { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The logControlProgramming function, which publishes a rejection event to Redis for fire-and-forget commands, is an excellent pattern for providing feedback in an asynchronous system. This ensures that the server has visibility into failed programming attempts.
| SendAck(ctx context.Context, requestID string, payload protocol.AckPayload) error | ||
| } | ||
|
|
||
| // noopResponder satisfies Responder for commands that arrive without a |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The noopResponder is a clean and effective way to satisfy the Responder interface for commands that do not require a direct client response, such as those originating from the Redis control channel.
| } | ||
|
|
||
| r.progMu.Lock() | ||
| defer r.progMu.Unlock() |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The introduction of progMu to serialize decoder programming sequences is crucial for preventing race conditions and ensuring the integrity of multi-round-trip operations on a single command station. This is a strong design choice for correctness.
| // scanning upwards from autoAllocateFirstDCCAddress, and returns them | ||
| // merged into as few contiguous ranges as possible. It returns | ||
| // ErrDCCPoolExhausted when fewer than count addresses remain free. | ||
| func allocateFreeDCCAddresses(count int, existing []domain.DCCAddressRange) ([]PoolRange, error) { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The allocateFreeDCCAddresses function provides a valuable new feature for automatically managing DCC address pools. The logic for scanning, skipping occupied addresses, and merging contiguous ranges is well-implemented and robust.
| svcerrors "github.com/keskad/loco/pkgs/bigfred/server/errors" | ||
| ) | ||
|
|
||
| func TestAllocateFreeDCCAddresses(t *testing.T) { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The extensive test cases for allocateFreeDCCAddresses cover a wide range of scenarios, including edge cases and complex allocations, demonstrating a high level of confidence in the correctness of the auto-allocation logic.
| return TokenExchangeResult{}, fmt.Errorf("oauth: redis unavailable") | ||
| } | ||
| key := oauthCodeKey(in.Code) | ||
| raw, err := o.redis.GetDel(ctx, key).Bytes() |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The OAuth implementation correctly uses redis.GetDel for authorization codes, ensuring that each code is single-use. This is a critical security measure for preventing replay attacks in the authorization code flow.
| } | ||
|
|
||
| // StartWatch watches the drop-in directory and reloads on change. | ||
| func (r *OAuthClientsRegistry) StartWatch(stop <-chan struct{}) error { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The OAuthClientsRegistry with fsnotify for dynamic reloading of client configurations is a flexible and powerful solution. This allows for client management without requiring a service restart, improving operational agility.
| return visibleInThrottle(stations), nil | ||
| } | ||
|
|
||
| func visibleInThrottle(rows []domain.CommandStation) []domain.CommandStation { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The HideInThrottle flag and visibleInThrottle helper provide a useful administrative control, allowing specific command stations (e.g., dedicated programming tracks) to be hidden from regular throttle users, improving usability and preventing accidental operations.
1 participant
Summary
layout_idon authorize pins the issued session for the wizardX-BigFred-Impersonate-Ason the dcc-bus WS proxy so wizard F2 pulse runs as the vehicle ownerTest plan
layout_idissues a code for that layout (silent SSO and fresh login)layout_idfrom query when presentBasic(F3/F4 momentary 1s); attach works via impersonationloco.setFunctionfor the subject ownerMade with Cursor