feat(runtime-auth): centralized token manager (editor half — fixes upload after token expiry) - #903
Merged
Conversation
…hority Mirrors the web change: the editor now centralizes JWT management in the main process via the shared RuntimeTokenManager, so every runtime call self-heals on expiry — including project upload, which previously did a raw HTTPS POST with no refresh (the reason a long session could keep polling status while uploads 401'd). - main process owns the token + credentials via the shared manager; login sets the session, clearCredentials clears it, and a single onTokenChanged subscription broadcasts runtime:token-refreshed to the renderer. - makeRuntimeApiRequest / makeRuntimeApiPostRequest now run through tokens.withAuth (replacing the bespoke attemptTokenRefresh + per-call broadcast). - New makeRuntimeApiUpload on the bridge does the multipart upload through tokens.withAuth; the upload pipeline (editor-compiler-platform-port) routes to it and the old refresh-less CompilerModule.sendRuntimeUpload is removed. - Shared, byte-identical pieces with web: RuntimeTokenManager (+tests), RuntimePort.getAccessToken, and use-runtime-polling adopting refreshed tokens into the store connection flag. - IPC handlers still accept (but ignore) the legacy jwtToken arg; the param is removed from the signatures in the follow-up cleanup. Validated the runtime auth contract against a live SLM-RP4: TTL 900s, expiry 401. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PC surface With the main process as the token authority, the renderer no longer holds or passes a token. Removes jwtToken from the runtime + EtherCAT IPC handlers, the renderer bridge signatures, and the editor adapter (which now tracks only a loggedIn flag for isReadyForDebug); makeRuntimeApiRequest/Post drop the param too. The debugger's separate connectionParams.jwtToken is untouched. deviceContext.jwt is kept solely as the 'are we logged in' gate for the upload phase; the compile pipeline no longer threads it into runtime calls. Brings the rewritten editor runtime-adapter to 100% coverage (adds the previously-untested EtherCAT delegators). tsc --build, eslint, prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThe PR centralizes runtime JWT handling in a shared token manager, removes ChangesRuntime token ownership refactor
Sequence Diagram(s)sequenceDiagram
participant MainProcessBridge
participant RuntimeTokenManager
participant rendererProcessBridge
participant runtimeAdapter
participant useRuntimePolling
participant deviceActions
MainProcessBridge->>RuntimeTokenManager: withAuth(runtime request)
RuntimeTokenManager->>RuntimeTokenManager: refresh() after 401/403
RuntimeTokenManager-->>MainProcessBridge: onTokenChanged(newToken)
MainProcessBridge-->>rendererProcessBridge: runtime:token-refreshed(newToken)
rendererProcessBridge-->>runtimeAdapter: onTokenRefreshed(newToken)
runtimeAdapter-->>useRuntimePolling: callback(newToken)
useRuntimePolling->>deviceActions: setRuntimeJwtToken(newToken)
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Keeps the editor layer validator aligned with web's classification of the shared RuntimeTokenManager directory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/editor/compiler/editor-compiler-platform-port.ts (1)
551-555: 🎯 Functional Correctness | 🟠 MajorProbe
/api/versionmust bypass the token-protected bridge.
editor-compiler-platform-port.tsroutes the unauthenticated version probe throughmakeRuntimeApiRequest, which injects a Bearer token. Runtimes treating this as an auth failure will block the probe.Implement a direct HTTPS request (bypassing the bridge/token authority) similar to the implementation in
compiler-module.ts(lines 1330‑1337) to allow unauthenticated version detection.Problematic code
const result = await context.mainProcessBridge.makeRuntimeApiRequest<{ version: string }>( deviceContext.ip, '/api/version', (data: string) => JSON.parse(data) as { version: string }, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/editor/compiler/editor-compiler-platform-port.ts` around lines 551 - 555, The `/api/version` probe in `editor-compiler-platform-port.ts` is being sent through `makeRuntimeApiRequest`, which adds token authentication and can fail against runtimes that expect this check to be unauthenticated. Update the version-detection path that uses `context.mainProcessBridge.makeRuntimeApiRequest` to perform a direct HTTPS request instead, following the same approach used in `compiler-module.ts` for unauthenticated version probing, and keep the JSON parsing for the `{ version: string }` response.
🧹 Nitpick comments (2)
src/middleware/shared/runtime-auth/runtime-token-manager.ts (1)
107-111: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep subscriber failures from failing refresh.
A throwing
onTokenChangedcallback currently rejects the refresh chain aftertokenis updated, sowithAuthcan skip the retry even though refresh succeeded. Isolate each callback.Proposed fix
token = result.token const fresh = result.token - subscribers.forEach((cb) => cb(fresh)) + subscribers.forEach((cb) => { + try { + cb(fresh) + } catch { + // Subscribers must not affect token refresh success. + } + }) return true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/middleware/shared/runtime-auth/runtime-token-manager.ts` around lines 107 - 111, The refresh path in runtime-token-manager’s token update flow lets a throwing subscriber callback break the successful refresh chain. Update the token-notification logic around subscribers.forEach in the token refresh branch so each onTokenChanged callback is isolated with its own error handling, preventing one failure from rejecting the refresh after token has already been updated; keep the success path in the refresh/token manager flow intact.src/frontend/hooks/use-runtime-polling.ts (1)
181-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
openPLCStoreBase.getState()in this subscription callback.This direct store access runs from an external token-refresh event callback, so it should use the base store accessor rather than the hook export.
As per coding guidelines, "
**/*.ts: UseopenPLCStoreBase.getState()for direct state access outside React".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/hooks/use-runtime-polling.ts` around lines 181 - 184, The token refresh subscription in useRuntimePolling is accessing store state through the hook export, which is not allowed outside React. Update the runtime.onTokenRefreshed callback to use openPLCStoreBase.getState() instead of useOpenPLCStore.getState() when calling deviceActions.setRuntimeJwtToken, keeping the change localized to useRuntimePolling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/editor/compiler/compiler-module.ts`:
- Around line 2407-2414: The `deviceContext` gate in `compiler-module.ts` is
still blocking runtime uploads unless `runtimeJwtToken` is present, even though
`makeRuntimeApiUpload` now handles auth refresh internally. Update the
`deviceContext` construction so it depends on `runtimeIpAddress` alone, or make
`jwt` optional, and keep the context flowing into the upload pipeline when only
`runtimeIpAddress` is available. Use the `deviceContext` logic near the runtime
upload setup and the `makeRuntimeApiUpload` bridge contract as the key symbols
to adjust.
In `@src/main/modules/ipc/main.ts`:
- Around line 253-260: The runtime token flow in handleRuntimeLogin and the
other tokens.withAuth call sites should be bound to the authenticated runtime
IP. Update the IPC handlers to compare the caller-supplied ipAddress against
this.runtimeIp and reject any runtime-authenticated call when they do not match,
using isAuthenticatedRuntime(ipAddress) before each tokens.withAuth path. Also
clear any stale token session on failed login so an old bearer token cannot be
reused after an IP change or unsuccessful authentication.
In `@src/middleware/adapters/editor/__tests__/runtime-adapter.test.ts`:
- Around line 465-480: The EtherCAT adapter tests are using `as never` with
invalid payload shapes, which bypasses the `RuntimePort` contract. Update the
cases in `runtime-adapter.test.ts` for `scanEthercatDevices`,
`testEthercatConnection`, and `validateEthercatConfig` to pass properly typed
request-shaped objects that match the real method signatures, so the bridge
contract is actually validated instead of suppressed.
In `@src/middleware/adapters/editor/runtime-adapter.ts`:
- Around line 44-54: The readiness check in runtime-adapter should not rely on
only the session boolean in isReadyForDebug and login; store the IP returned by
requireIp() on successful authentication and have isReadyForDebug() verify that
the current getIpAddress() matches that authenticated IP. Update login() so it
sets the authenticated IP only on a successful result, and make sure failed
login attempts do not leave a previous loggedIn state or authenticated IP in
place.
In `@src/middleware/shared/runtime-auth/runtime-token-manager.ts`:
- Around line 93-118: Invalidate any in-flight refresh completion when the
session is cleared in runtime-token-manager’s clear() and refresh() logic. Add a
session generation or cancellation guard around the existing
refreshInFlight/transport.login flow so that if clear() runs before the promise
settles, the later .then() path in refresh() does not update token or call
subscribers. Make sure the guard is checked in the refresh() continuation and is
reset appropriately when clearing or starting a new session.
---
Outside diff comments:
In `@src/backend/editor/compiler/editor-compiler-platform-port.ts`:
- Around line 551-555: The `/api/version` probe in
`editor-compiler-platform-port.ts` is being sent through
`makeRuntimeApiRequest`, which adds token authentication and can fail against
runtimes that expect this check to be unauthenticated. Update the
version-detection path that uses
`context.mainProcessBridge.makeRuntimeApiRequest` to perform a direct HTTPS
request instead, following the same approach used in `compiler-module.ts` for
unauthenticated version probing, and keep the JSON parsing for the `{ version:
string }` response.
---
Nitpick comments:
In `@src/frontend/hooks/use-runtime-polling.ts`:
- Around line 181-184: The token refresh subscription in useRuntimePolling is
accessing store state through the hook export, which is not allowed outside
React. Update the runtime.onTokenRefreshed callback to use
openPLCStoreBase.getState() instead of useOpenPLCStore.getState() when calling
deviceActions.setRuntimeJwtToken, keeping the change localized to
useRuntimePolling.
In `@src/middleware/shared/runtime-auth/runtime-token-manager.ts`:
- Around line 107-111: The refresh path in runtime-token-manager’s token update
flow lets a throwing subscriber callback break the successful refresh chain.
Update the token-notification logic around subscribers.forEach in the token
refresh branch so each onTokenChanged callback is isolated with its own error
handling, preventing one failure from rejecting the refresh after token has
already been updated; keep the success path in the refresh/token manager flow
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b76cd87-c366-44a5-8697-7dd87d972d2e
📒 Files selected for processing (11)
src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.tssrc/backend/editor/compiler/compiler-module.tssrc/backend/editor/compiler/editor-compiler-platform-port.tssrc/frontend/hooks/use-runtime-polling.tssrc/main/modules/ipc/main.tssrc/main/modules/ipc/renderer.tssrc/middleware/adapters/editor/__tests__/runtime-adapter.test.tssrc/middleware/adapters/editor/runtime-adapter.tssrc/middleware/shared/ports/runtime-port.tssrc/middleware/shared/runtime-auth/__tests__/runtime-token-manager.test.tssrc/middleware/shared/runtime-auth/runtime-token-manager.ts
Comment on lines
+2407
to
+2414
| makeRuntimeApiUpload: (opts: { | ||
| ipAddress: string | ||
| fileBuffer: Buffer | ||
| filename: string | ||
| contentType: string | ||
| cleanBuild: boolean | ||
| onUploadAccepted?: (responseBody: string) => void | ||
| }) => Promise<{ success: true; data: string } | { success: false; error: string }> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify compileProgram argument shape and remaining runtime JWT plumbing.
rg -n -C 3 'runtimeJwtToken|jwtToken|compileProgram\(|makeRuntimeApiUpload' src/backend/editor/compiler src/main src/middlewareRepository: Autonomy-Logic/openplc-editor
Length of output: 31981
🏁 Script executed:
grep -n -A 5 -B 5 "deviceContext" src/backend/editor/compiler/editor-compiler-platform-port.ts src/backend/editor/compiler/compiler-module.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 12890
🏁 Script executed:
cat -n src/backend/editor/compiler/editor-compiler-platform-port.ts | sed -n '30,80p'Repository: Autonomy-Logic/openplc-editor
Length of output: 2393
🏁 Script executed:
grep -n -A 10 "assertEditorHttpsContext" src/middleware/shared/ports/compiler-platform-port.ts src/backend/editor/compiler/editor-compiler-platform-port.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 4975
🏁 Script executed:
grep -n -A 20 "export.*PlatformDeviceContext" src/middleware/shared/ports/compiler-platform-port.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 1023
Remove the runtimeJwtToken requirement to enable token-free runtime uploads.
The deviceContext construction at lines 2675-2677 still gates the existence of the context on runtimeJwtToken. Since the makeRuntimeApiUpload bridge implementation in the main process handles token refresh internally and no longer accepts a JWT argument, the upload pipeline is unnecessarily skipped when only runtimeIpAddress is available.
Update the guard to check runtimeIpAddress alone (or treat jwt as optional) so the context is passed to the pipeline, allowing the bridge to manage authentication transparently.
Current Code
const deviceContext =
runtimeIpAddress && runtimeJwtToken
? { kind: 'editor-https' as const, ip: runtimeIpAddress, jwt: runtimeJwtToken }
: undefined🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/editor/compiler/compiler-module.ts` around lines 2407 - 2414, The
`deviceContext` gate in `compiler-module.ts` is still blocking runtime uploads
unless `runtimeJwtToken` is present, even though `makeRuntimeApiUpload` now
handles auth refresh internally. Update the `deviceContext` construction so it
depends on `runtimeIpAddress` alone, or make `jwt` optional, and keep the
context flowing into the upload pipeline when only `runtimeIpAddress` is
available. Use the `deviceContext` logic near the runtime upload setup and the
`makeRuntimeApiUpload` bridge contract as the key symbols to adjust.
Comment on lines
253
to
260
| handleRuntimeLogin = async (_event: IpcMainInvokeEvent, ipAddress: string, username: string, password: string) => { | ||
| const result = await this.performAuthentication(ipAddress, username, password) | ||
| if (result.success && result.accessToken) { | ||
| this.runtimeCredentials = { ipAddress, username, password } | ||
| // Hand the session to the token authority so it can transparently | ||
| // re-authenticate against this device when the token expires. | ||
| this.runtimeIp = ipAddress | ||
| this.tokens.setSession(result.accessToken, { username, password }) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind authenticated tokens to the runtime IP before sending them.
The manager refreshes against this.runtimeIp, but these helpers attach the current bearer token to the caller-supplied ipAddress without checking it matches the authenticated runtime. A stale session can leak runtime A’s token to runtime B after an IP change or failed login. Clear stale sessions on failed login and reject runtime calls when ipAddress !== this.runtimeIp.
Proposed direction
handleRuntimeLogin = async (_event: IpcMainInvokeEvent, ipAddress: string, username: string, password: string) => {
const result = await this.performAuthentication(ipAddress, username, password)
if (result.success && result.accessToken) {
@@
this.runtimeIp = ipAddress
this.tokens.setSession(result.accessToken, { username, password })
+ } else if (this.runtimeIp === ipAddress) {
+ this.tokens.clear()
+ this.runtimeIp = null
}
return result
}
+
+ private isAuthenticatedRuntime(ipAddress: string): boolean {
+ return this.runtimeIp === ipAddress && this.tokens.hasToken()
+ }Then check isAuthenticatedRuntime(ipAddress) before each tokens.withAuth(...) path.
Also applies to: 299-303, 392-397, 461-465
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/modules/ipc/main.ts` around lines 253 - 260, The runtime token flow
in handleRuntimeLogin and the other tokens.withAuth call sites should be bound
to the authenticated runtime IP. Update the IPC handlers to compare the
caller-supplied ipAddress against this.runtimeIp and reject any
runtime-authenticated call when they do not match, using
isAuthenticatedRuntime(ipAddress) before each tokens.withAuth path. Also clear
any stale token session on failed login so an old bearer token cannot be reused
after an IP change or unsuccessful authentication.
Comment on lines
+465
to
+480
| name: 'scanEthercatDevices', | ||
| bridge: 'etherCATScan', | ||
| invoke: (a) => a.scanEthercatDevices!({ interface: 'eth0' } as never), | ||
| expectArgs: ['192.168.1.100', { interface: 'eth0' }], | ||
| }, | ||
| { | ||
| name: 'testEthercatConnection', | ||
| bridge: 'etherCATTest', | ||
| invoke: (a) => a.testEthercatConnection!({ slave: 1 } as never), | ||
| expectArgs: ['192.168.1.100', { slave: 1 }], | ||
| }, | ||
| { | ||
| name: 'validateEthercatConfig', | ||
| bridge: 'etherCATValidate', | ||
| invoke: (a) => a.validateEthercatConfig!({ config: {} } as never), | ||
| expectArgs: ['192.168.1.100', { config: {} }], |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use valid EtherCAT request shapes instead of as never.
These test payloads bypass the RuntimePort contract and include fields that the real methods do not accept (slave, config). Use typed request-shaped objects so the test keeps guarding the bridge contract.
Proposed fix
{
name: 'scanEthercatDevices',
bridge: 'etherCATScan',
- invoke: (a) => a.scanEthercatDevices!({ interface: 'eth0' } as never),
+ invoke: (a) => a.scanEthercatDevices!({ interface: 'eth0' }),
expectArgs: ['192.168.1.100', { interface: 'eth0' }],
},
{
name: 'testEthercatConnection',
bridge: 'etherCATTest',
- invoke: (a) => a.testEthercatConnection!({ slave: 1 } as never),
- expectArgs: ['192.168.1.100', { slave: 1 }],
+ invoke: (a) => a.testEthercatConnection!({ interface: 'eth0', position: 1 }),
+ expectArgs: ['192.168.1.100', { interface: 'eth0', position: 1 }],
},
{
name: 'validateEthercatConfig',
bridge: 'etherCATValidate',
- invoke: (a) => a.validateEthercatConfig!({ config: {} } as never),
- expectArgs: ['192.168.1.100', { config: {} }],
+ invoke: (a) => a.validateEthercatConfig!({ interface: 'eth0', slaves: [] }),
+ expectArgs: ['192.168.1.100', { interface: 'eth0', slaves: [] }],
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: 'scanEthercatDevices', | |
| bridge: 'etherCATScan', | |
| invoke: (a) => a.scanEthercatDevices!({ interface: 'eth0' } as never), | |
| expectArgs: ['192.168.1.100', { interface: 'eth0' }], | |
| }, | |
| { | |
| name: 'testEthercatConnection', | |
| bridge: 'etherCATTest', | |
| invoke: (a) => a.testEthercatConnection!({ slave: 1 } as never), | |
| expectArgs: ['192.168.1.100', { slave: 1 }], | |
| }, | |
| { | |
| name: 'validateEthercatConfig', | |
| bridge: 'etherCATValidate', | |
| invoke: (a) => a.validateEthercatConfig!({ config: {} } as never), | |
| expectArgs: ['192.168.1.100', { config: {} }], | |
| name: 'scanEthercatDevices', | |
| bridge: 'etherCATScan', | |
| invoke: (a) => a.scanEthercatDevices!({ interface: 'eth0' }), | |
| expectArgs: ['192.168.1.100', { interface: 'eth0' }], | |
| }, | |
| { | |
| name: 'testEthercatConnection', | |
| bridge: 'etherCATTest', | |
| invoke: (a) => a.testEthercatConnection!({ interface: 'eth0', position: 1 }), | |
| expectArgs: ['192.168.1.100', { interface: 'eth0', position: 1 }], | |
| }, | |
| { | |
| name: 'validateEthercatConfig', | |
| bridge: 'etherCATValidate', | |
| invoke: (a) => a.validateEthercatConfig!({ interface: 'eth0', slaves: [] }), | |
| expectArgs: ['192.168.1.100', { interface: 'eth0', slaves: [] }], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/middleware/adapters/editor/__tests__/runtime-adapter.test.ts` around
lines 465 - 480, The EtherCAT adapter tests are using `as never` with invalid
payload shapes, which bypasses the `RuntimePort` contract. Update the cases in
`runtime-adapter.test.ts` for `scanEthercatDevices`, `testEthercatConnection`,
and `validateEthercatConfig` to pass properly typed request-shaped objects that
match the real method signatures, so the bridge contract is actually validated
instead of suppressed.
Comment on lines
44
to
+54
| return { | ||
| isReadyForDebug() { | ||
| return getIpAddress() !== '' && jwtToken !== '' | ||
| return getIpAddress() !== '' && loggedIn | ||
| }, | ||
|
|
||
| async login(params: LoginParams): Promise<LoginResult> { | ||
| try { | ||
| const ip = requireIp() | ||
| const result = await window.bridge.runtimeLogin(ip, params.username, params.password) | ||
| if (result.success && result.accessToken) { | ||
| jwtToken = result.accessToken | ||
| loggedIn = true |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Track the authenticated IP, not just a session boolean.
After a successful login, changing getIpAddress() to another runtime still leaves isReadyForDebug() true, and a failed login does not reset a previous loggedIn = true. Store the authenticated IP and require it to match the current IP.
Proposed fix
- let loggedIn = false
+ let authenticatedIp: string | null = null
@@
isReadyForDebug() {
- return getIpAddress() !== '' && loggedIn
+ const ip = getIpAddress()
+ return ip !== '' && authenticatedIp === ip
},
@@
const result = await window.bridge.runtimeLogin(ip, params.username, params.password)
if (result.success && result.accessToken) {
- loggedIn = true
+ authenticatedIp = ip
+ } else if (authenticatedIp === ip) {
+ authenticatedIp = null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| isReadyForDebug() { | |
| return getIpAddress() !== '' && jwtToken !== '' | |
| return getIpAddress() !== '' && loggedIn | |
| }, | |
| async login(params: LoginParams): Promise<LoginResult> { | |
| try { | |
| const ip = requireIp() | |
| const result = await window.bridge.runtimeLogin(ip, params.username, params.password) | |
| if (result.success && result.accessToken) { | |
| jwtToken = result.accessToken | |
| loggedIn = true | |
| return { | |
| isReadyForDebug() { | |
| const ip = getIpAddress() | |
| return ip !== '' && authenticatedIp === ip | |
| }, | |
| async login(params: LoginParams): Promise<LoginResult> { | |
| try { | |
| const ip = requireIp() | |
| const result = await window.bridge.runtimeLogin(ip, params.username, params.password) | |
| if (result.success && result.accessToken) { | |
| authenticatedIp = ip | |
| } else if (authenticatedIp === ip) { | |
| authenticatedIp = null | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/middleware/adapters/editor/runtime-adapter.ts` around lines 44 - 54, The
readiness check in runtime-adapter should not rely on only the session boolean
in isReadyForDebug and login; store the IP returned by requireIp() on successful
authentication and have isReadyForDebug() verify that the current getIpAddress()
matches that authenticated IP. Update login() so it sets the authenticated IP
only on a successful result, and make sure failed login attempts do not leave a
previous loggedIn state or authenticated IP in place.
Comment on lines
+93
to
+118
| function clear(): void { | ||
| token = null | ||
| credentials = null | ||
| } | ||
|
|
||
| function refresh(): Promise<boolean> { | ||
| // Single-flight: a second caller that arrives while a re-login is in | ||
| // progress joins the same promise instead of firing another login. | ||
| if (refreshInFlight) return refreshInFlight | ||
| if (!credentials) return Promise.resolve(false) | ||
|
|
||
| const pending = transport | ||
| .login(credentials) | ||
| .then((result) => { | ||
| if (result.success && result.token) { | ||
| token = result.token | ||
| const fresh = result.token | ||
| subscribers.forEach((cb) => cb(fresh)) | ||
| return true | ||
| } | ||
| return false | ||
| }) | ||
| .catch(() => false) | ||
| .finally(() => { | ||
| refreshInFlight = null | ||
| }) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Invalidate in-flight refreshes when clearing the session.
If clear() runs while refresh() is already awaiting transport.login, the pending .then() can still set token and notify subscribers after logout. Add a session generation/cancel guard so stale refresh completions are ignored.
Proposed fix
export function createRuntimeTokenManager(transport: TokenLoginTransport): RuntimeTokenManager {
let token: string | null = null
let credentials: RuntimeCredentials | null = null
let refreshInFlight: Promise<boolean> | null = null
+ let sessionGeneration = 0
const subscribers = new Set<(newToken: string) => void>()
@@
function setSession(newToken: string, newCredentials: RuntimeCredentials): void {
+ sessionGeneration += 1
token = newToken
credentials = newCredentials
}
function clear(): void {
+ sessionGeneration += 1
token = null
credentials = null
}
@@
- const pending = transport
+ const generation = sessionGeneration
+ const pending = transport
.login(credentials)
.then((result) => {
+ if (generation !== sessionGeneration) return false
if (result.success && result.token) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function clear(): void { | |
| token = null | |
| credentials = null | |
| } | |
| function refresh(): Promise<boolean> { | |
| // Single-flight: a second caller that arrives while a re-login is in | |
| // progress joins the same promise instead of firing another login. | |
| if (refreshInFlight) return refreshInFlight | |
| if (!credentials) return Promise.resolve(false) | |
| const pending = transport | |
| .login(credentials) | |
| .then((result) => { | |
| if (result.success && result.token) { | |
| token = result.token | |
| const fresh = result.token | |
| subscribers.forEach((cb) => cb(fresh)) | |
| return true | |
| } | |
| return false | |
| }) | |
| .catch(() => false) | |
| .finally(() => { | |
| refreshInFlight = null | |
| }) | |
| export function createRuntimeTokenManager(transport: TokenLoginTransport): RuntimeTokenManager { | |
| let token: string | null = null | |
| let credentials: RuntimeCredentials | null = null | |
| let refreshInFlight: Promise<boolean> | null = null | |
| let sessionGeneration = 0 | |
| const subscribers = new Set<(newToken: string) => void>() | |
| function setSession(newToken: string, newCredentials: RuntimeCredentials): void { | |
| sessionGeneration += 1 | |
| token = newToken | |
| credentials = newCredentials | |
| } | |
| function clear(): void { | |
| sessionGeneration += 1 | |
| token = null | |
| credentials = null | |
| } | |
| function refresh(): Promise<boolean> { | |
| // Single-flight: a second caller that arrives while a re-login is in | |
| // progress joins the same promise instead of firing another login. | |
| if (refreshInFlight) return refreshInFlight | |
| if (!credentials) return Promise.resolve(false) | |
| const generation = sessionGeneration | |
| const pending = transport | |
| .login(credentials) | |
| .then((result) => { | |
| if (generation !== sessionGeneration) return false | |
| if (result.success && result.token) { | |
| token = result.token | |
| const fresh = result.token | |
| subscribers.forEach((cb) => cb(fresh)) | |
| return true | |
| } | |
| return false | |
| }) | |
| .catch(() => false) | |
| .finally(() => { | |
| refreshInFlight = null | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/middleware/shared/runtime-auth/runtime-token-manager.ts` around lines 93
- 118, Invalidate any in-flight refresh completion when the session is cleared
in runtime-token-manager’s clear() and refresh() logic. Add a session generation
or cancellation guard around the existing refreshInFlight/transport.login flow
so that if clear() runs before the promise settles, the later .then() path in
refresh() does not update token or call subscribers. Make sure the guard is
checked in the refresh() continuation and is reset appropriately when clearing
or starting a new session.
…ed-runtime-token-manager
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/modules/ipc/main.ts (1)
122-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the token-refresh IPC send against a destroyed window. Optional chaining still allows
webContents.send()to throw after the BrowserWindow closes; add anisDestroyed()check here like the other IPC sends in this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/modules/ipc/main.ts` around lines 122 - 123, The token-refresh IPC send in the token change handler can still throw if the BrowserWindow has already been closed because optional chaining does not protect against a destroyed webContents. Update the onTokenChanged callback in main.ts to follow the same pattern used by the other IPC sends in this file: check mainWindow and its webContents for isDestroyed() before calling send, and skip the runtime:token-refreshed message when the window is no longer valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/modules/ipc/main.ts`:
- Around line 122-123: The token-refresh IPC send in the token change handler
can still throw if the BrowserWindow has already been closed because optional
chaining does not protect against a destroyed webContents. Update the
onTokenChanged callback in main.ts to follow the same pattern used by the other
IPC sends in this file: check mainWindow and its webContents for isDestroyed()
before calling send, and skip the runtime:token-refreshed message when the
window is no longer valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa0a9e4b-ba77-4c13-9d38-b74a1665eab6
📒 Files selected for processing (2)
src/main/modules/ipc/main.tssrc/main/modules/ipc/renderer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/modules/ipc/renderer.ts
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
Editor half of the centralized runtime-auth change (shared branch name with openplc-web PR #561). Makes the main process the single token authority using the same shared
RuntimeTokenManager, so every runtime call — including project upload — refreshes the JWT on expiry.The bug this fixes
On a long Runtime v4 session, stats kept working but uploading a project failed. Root cause: the JWT was managed in three uncoordinated places — the renderer adapter's closure (self-healed via the token-refresh IPC event), the main process's GET/POST helpers (refreshed on 401), and the Zustand store (never refreshed). The upload path (
sendRuntimeUpload) did a raw HTTPS POST with no refresh, using the stale store token threaded through the compile args — so it 401'd after ~15 min while status polling sailed on.Design
One authority = the layer that does the HTTP (here, the main process). It holds the token + credentials via the shared
RuntimeTokenManagerand runs all runtime HTTP throughtokens.withAuth(retry-once on 401/403):makeRuntimeApiRequest/makeRuntimeApiPostRequestrefactored ontowithAuth(replacing the bespokeattemptTokenRefresh).makeRuntimeApiUploaddoes the multipart upload throughwithAuth; the compile pipeline routes to it and the refresh-lessCompilerModule.sendRuntimeUploadis removed.onTokenChanged→ broadcastsruntime:token-refreshed;use-runtime-pollingadopts it into the store flag.jwtTokenremoved from the runtime + EtherCAT IPC handlers, the renderer bridge, and the editor adapter (now a thin forwarder tracking only aloggedInflag). The debugger's separateconnectionParams.jwtTokenis untouched.Shared, byte-identical with web:
RuntimeTokenManager(+tests),RuntimePort.getAccessToken,use-runtime-polling.Testing
runtime-token-manager19/19; editorruntime-adapter55/55 (now 100%, incl. EtherCAT delegators);editor-compiler-platform-port23/23;backend/editor/compiler212/212.tsc --build, eslint, prettier clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes