Skip to content

Commit 94df5d5

Browse files
committed
fix: kilobot findings from PR #14
1. Plugin-managed authToken now falls through to file-based auto re-auth instead of dead-ending at a 'update your openclaw.json' message on 401. Added isPluginManagedAuthToken() in token-store; Path 0 in runShellSecurityFlow now skips when the raw config's authToken is a SecretRef pointing at our own provider (the shape writeStoredToken() always writes). Covered by 5 new unit tests in token-store.test.ts. 2. getPublicIp() now clears its 5s abort timer in a finally block so dangling timeouts don't accumulate across failed checkups. 3. Device-auth poll requests now carry a per-request 10s AbortController so a hung HTTP call can't outlive the overall 30s POLL_TIMEOUT_MS. Cleared in finally so every loop iteration is interruptible. 4. CHANGELOG regained its '## [Unreleased]' heading per the release workflow documented in AGENTS.md + RELEASING.md, and the three fixes above are logged under it.
1 parent 8b5cb7c commit 94df5d5

6 files changed

Lines changed: 159 additions & 8 deletions

File tree

CHANGELOG.md

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,24 @@ All notable changes to `@kilocode/shell-security` (formerly
66
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
77
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
88

9+
## [Unreleased]
10+
11+
### Fixed
12+
13+
- `getPublicIp()` now clears its 5-second abort timer on error paths as
14+
well as success, so repeated checkups on a flaky network don't leak
15+
dangling timeouts.
16+
- Device-auth poll requests now carry a per-request `AbortController`
17+
(10s) so a hung HTTP call can no longer outlive the overall 30s
18+
`POLL_TIMEOUT_MS` budget.
19+
- Expired plugin-managed auth tokens now fall through to the file-based
20+
auto re-auth path (Path B) instead of returning the "update your
21+
openclaw.json" message. `runShellSecurityFlow` inspects the raw
22+
config via `isPluginManagedAuthToken()` and skips Path 0 when the
23+
`authToken` is a SecretRef pointing at our own provider — that shape
24+
is only ever written by `writeStoredToken()` after device auth, so
25+
the plugin (not the user) owns recovery.
26+
927
## [0.2.0]
1028

1129
First release under the new `@kilocode/shell-security` name. The plugin

index.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
readPendingCode,
1111
writePendingCode,
1212
clearPendingCode,
13+
isPluginManagedAuthToken,
1314
type PluginLogger,
1415
type PluginRuntimeConfig,
1516
} from "./src/auth/token-store.js";
@@ -166,8 +167,23 @@ async function runShellSecurityFlow(
166167
// going through device auth, and it respects the schema contract
167168
// documented in openclaw.plugin.json + README. Explicit user config
168169
// wins over everything else.
170+
//
171+
// Skip this path when the raw config shows a SecretRef aimed at our
172+
// OWN provider — that shape is only written by writeStoredToken()
173+
// after device auth, and the plugin's file-based auto re-auth path
174+
// (Path B below) should own recovery in that case. Without this
175+
// check, a plugin-managed token that expires would hit the
176+
// "update your openclaw.json" message here instead of falling through
177+
// to clear + redo device auth.
169178
const configToken = api.pluginConfig?.authToken;
170-
if (typeof configToken === "string" && configToken.length > 0) {
179+
const pluginManaged = isPluginManagedAuthToken(
180+
api.runtime.config.loadConfig(),
181+
);
182+
if (
183+
!pluginManaged &&
184+
typeof configToken === "string" &&
185+
configToken.length > 0
186+
) {
171187
try {
172188
return await doCheckup(api, apiBase, configToken, channel);
173189
} catch (err) {

src/audit.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,17 +107,18 @@ export function isValidIp(candidate: string): boolean {
107107
*/
108108
export async function getPublicIp(): Promise<string | undefined> {
109109
const fetchFn: typeof fetch = resolveFetch() ?? globalThis.fetch;
110+
const controller = new AbortController();
111+
const timeout = setTimeout(() => controller.abort(), 5_000);
110112
try {
111-
const controller = new AbortController();
112-
const timeout = setTimeout(() => controller.abort(), 5_000);
113113
const resp = await fetchFn("https://ifconfig.me/ip", {
114114
signal: controller.signal,
115115
});
116-
clearTimeout(timeout);
117116
if (!resp.ok) return undefined;
118117
const text = (await resp.text()).trim();
119118
return isValidIp(text) ? text : undefined;
120119
} catch {
121120
return undefined;
121+
} finally {
122+
clearTimeout(timeout);
122123
}
123124
}

src/auth/device-auth.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,13 @@ import type { PluginLogger } from "./token-store.js";
1313
*/
1414
const POLL_TIMEOUT_MS = 30 * 1_000;
1515
const POLL_INTERVAL_MS = 3_000;
16+
/**
17+
* Per-request deadline for a single poll HTTP call. Without this,
18+
* a hung connection could outlive the overall POLL_TIMEOUT_MS budget,
19+
* because the loop only re-checks the deadline between iterations.
20+
* Capped below the overall budget so the loop stays interruptible.
21+
*/
22+
const POLL_REQUEST_TIMEOUT_MS = 10 * 1_000;
1623

1724
type DeviceAuthInitResponse = {
1825
code: string;
@@ -110,8 +117,13 @@ export async function pollDeviceAuth(
110117

111118
while (Date.now() < deadline) {
112119
await sleep(POLL_INTERVAL_MS);
120+
const controller = new AbortController();
121+
const requestTimeout = setTimeout(
122+
() => controller.abort(),
123+
POLL_REQUEST_TIMEOUT_MS,
124+
);
113125
try {
114-
const resp = await fetchFn(pollUrl);
126+
const resp = await fetchFn(pollUrl, { signal: controller.signal });
115127
if (resp.status === 202) continue; // pending
116128
if (resp.status === 403) return { kind: "denied" };
117129
if (resp.status === 410) return { kind: "expired" };
@@ -123,10 +135,13 @@ export async function pollDeviceAuth(
123135
if (data.status === "expired") return { kind: "expired" };
124136
}
125137
} catch (err) {
126-
// Transient network error. Log at debug level so it's visible
127-
// when investigating real failures but not noisy on the happy path.
138+
// Transient network error (including per-request abort due to a
139+
// hung connection). Log at debug level so it's visible when
140+
// investigating real failures but not noisy on the happy path.
128141
const message = err instanceof Error ? err.message : String(err);
129142
logger?.debug?.(`shell-security: poll transient error: ${message}`);
143+
} finally {
144+
clearTimeout(requestTimeout);
130145
}
131146
}
132147

src/auth/token-store.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,53 @@ export function secretFilePath(): string {
3939
return join(homedir(), ".openclaw", "secrets", `${PLUGIN_ID}-auth-token`);
4040
}
4141

42+
/**
43+
* True when the raw openclaw.json has a SecretRef at
44+
* `plugins.entries.shell-security.config.authToken` that points at
45+
* OUR provider (`kilocode_shell_security`). That shape is only ever
46+
* written by writeStoredToken() — a user configuring the plugin by
47+
* hand would use a plain string or a SecretRef aimed at a different
48+
* provider.
49+
*
50+
* The plugin can't tell from `api.pluginConfig.authToken` alone
51+
* whether the resolved string came from a user-typed value or from
52+
* OpenClaw resolving our own SecretRef. Callers use this helper to
53+
* treat those cases differently (see `runShellSecurityFlow` in
54+
* index.ts): plugin-managed tokens should auto re-auth on 401, but
55+
* user-managed ones should surface an "update your openclaw.json"
56+
* message.
57+
*/
58+
export function isPluginManagedAuthToken(config: unknown): boolean {
59+
const root = (config && typeof config === "object" ? config : {}) as Record<
60+
string,
61+
unknown
62+
>;
63+
const plugins = (
64+
root.plugins && typeof root.plugins === "object" ? root.plugins : {}
65+
) as Record<string, unknown>;
66+
const entries = (
67+
plugins.entries && typeof plugins.entries === "object"
68+
? plugins.entries
69+
: {}
70+
) as Record<string, unknown>;
71+
const entry = (
72+
entries[PLUGIN_ID] && typeof entries[PLUGIN_ID] === "object"
73+
? entries[PLUGIN_ID]
74+
: {}
75+
) as Record<string, unknown>;
76+
const entryConfig = (
77+
entry.config && typeof entry.config === "object" ? entry.config : {}
78+
) as Record<string, unknown>;
79+
const authToken = entryConfig.authToken;
80+
if (!authToken || typeof authToken !== "object") return false;
81+
const ref = authToken as Record<string, unknown>;
82+
return (
83+
ref.source === "file" &&
84+
ref.provider === PROVIDER_ID &&
85+
typeof ref.id === "string"
86+
);
87+
}
88+
4289
function pendingCodeFilePath(): string {
4390
return join(homedir(), ".openclaw", "secrets", `${PLUGIN_ID}-pending-code`);
4491
}

test/token-store.test.ts

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, test, expect } from "bun:test";
2-
import { patchConfig } from "../src/auth/token-store";
2+
import { patchConfig, isPluginManagedAuthToken } from "../src/auth/token-store";
33

44
const TEST_PATH = "/home/node/.openclaw/secrets/shell-security-auth-token";
55

@@ -124,3 +124,57 @@ describe("patchConfig", () => {
124124
expect(getAuthToken(next)).toBeDefined();
125125
});
126126
});
127+
128+
describe("isPluginManagedAuthToken", () => {
129+
test("true for a SecretRef pointing at our own provider", () => {
130+
const cfg = patchConfig({}, TEST_PATH);
131+
expect(isPluginManagedAuthToken(cfg)).toBe(true);
132+
});
133+
134+
test("false for a plain-string authToken (user-set)", () => {
135+
const cfg = {
136+
plugins: {
137+
entries: {
138+
"shell-security": { config: { authToken: "user-typed-string" } },
139+
},
140+
},
141+
};
142+
expect(isPluginManagedAuthToken(cfg)).toBe(false);
143+
});
144+
145+
test("false for a SecretRef pointing at a different provider", () => {
146+
const cfg = {
147+
plugins: {
148+
entries: {
149+
"shell-security": {
150+
config: {
151+
authToken: {
152+
source: "file",
153+
provider: "some_other_provider",
154+
id: "value",
155+
},
156+
},
157+
},
158+
},
159+
},
160+
};
161+
expect(isPluginManagedAuthToken(cfg)).toBe(false);
162+
});
163+
164+
test("false when authToken is unset", () => {
165+
expect(isPluginManagedAuthToken({})).toBe(false);
166+
expect(isPluginManagedAuthToken({ plugins: {} })).toBe(false);
167+
expect(
168+
isPluginManagedAuthToken({
169+
plugins: { entries: { "shell-security": { config: {} } } },
170+
}),
171+
).toBe(false);
172+
});
173+
174+
test("tolerates null/undefined/non-object config", () => {
175+
expect(isPluginManagedAuthToken(null)).toBe(false);
176+
expect(isPluginManagedAuthToken(undefined)).toBe(false);
177+
expect(isPluginManagedAuthToken("string")).toBe(false);
178+
expect(isPluginManagedAuthToken({ plugins: "not-an-object" })).toBe(false);
179+
});
180+
});

0 commit comments

Comments
 (0)