Skip to content

Commit 85eea5f

Browse files
bendruckerbendrucker
committed
refactor: inline subprocess helpers into platform backends
Each backend now owns its subprocess logic instead of importing a shared run() from index.ts. This removes the circular-feeling dependency and makes spawn error messages specific to the actual binary (security, secret-tool) rather than a generic platformHint. Also: document CREDENTIALW struct layout and GetLastError FFI limitation in windows.ts, extract mock backend in keyring tests, fix stale PowerShell reference in docs.
1 parent d562098 commit 85eea5f

6 files changed

Lines changed: 112 additions & 116 deletions

File tree

‎docs/authentication.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ API keys are not stored in this file. they are stored in the system keyring and
8282
- **Linux**: requires `secret-tool` from libsecret
8383
- Debian/Ubuntu: `apt install libsecret-tools`
8484
- Arch: `pacman -S libsecret`
85-
- **Windows**: uses CredentialManager via PowerShell (built-in)
85+
- **Windows**: uses Credential Manager via `advapi32.dll` (built-in)
8686

8787
if the keyring is unavailable, set `LINEAR_API_KEY` as a fallback.
8888

‎src/keyring/index.ts‎

Lines changed: 0 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,6 @@ export function _setBackend(b: KeyringBackend | null): void {
1616
backend = b
1717
}
1818

19-
function platformHint(): string {
20-
switch (Deno.build.os) {
21-
case "darwin":
22-
return "Could not find /usr/bin/security. Is this a macOS system?"
23-
case "linux":
24-
return "Could not find secret-tool. Install libsecret (e.g. apt install libsecret-tools, pacman -S libsecret).\n" +
25-
"Alternatively, set the LINEAR_API_KEY environment variable."
26-
case "windows":
27-
return "Could not load advapi32.dll. Is this a Windows system?"
28-
default:
29-
return `Unsupported platform: ${Deno.build.os}`
30-
}
31-
}
32-
3319
function getBackend(): KeyringBackend {
3420
if (backend != null) return backend
3521
switch (Deno.build.os) {
@@ -44,47 +30,6 @@ function getBackend(): KeyringBackend {
4430
}
4531
}
4632

47-
export async function run(
48-
cmd: string[],
49-
options?: { stdin?: string },
50-
): Promise<{ success: boolean; code: number; stdout: string; stderr: string }> {
51-
let process: Deno.ChildProcess
52-
try {
53-
const command = new Deno.Command(cmd[0], {
54-
args: cmd.slice(1),
55-
stdin: options?.stdin != null ? "piped" : "null",
56-
stdout: "piped",
57-
stderr: "piped",
58-
})
59-
process = command.spawn()
60-
} catch (error) {
61-
const detail = error instanceof Error ? error.message : String(error)
62-
throw new Error(`${platformHint()}\n (${detail})`)
63-
}
64-
65-
if (options?.stdin != null) {
66-
try {
67-
const writer = process.stdin.getWriter()
68-
await writer.write(new TextEncoder().encode(options.stdin))
69-
await writer.close()
70-
} catch (error) {
71-
try {
72-
process.kill()
73-
} catch { /* already exited */ }
74-
const detail = error instanceof Error ? error.message : String(error)
75-
throw new Error(`Failed to write to stdin of ${cmd[0]}: ${detail}`)
76-
}
77-
}
78-
79-
const { success, code, stdout, stderr } = await process.output()
80-
return {
81-
success,
82-
code,
83-
stdout: new TextDecoder().decode(stdout).trim(),
84-
stderr: new TextDecoder().decode(stderr).trim(),
85-
}
86-
}
87-
8833
export async function getPassword(account: string): Promise<string | null> {
8934
return await getBackend().get(account)
9035
}

‎src/keyring/linux.ts‎

Lines changed: 53 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,58 @@
11
import type { KeyringBackend } from "./index.ts"
2-
import { run, SERVICE } from "./index.ts"
2+
import { SERVICE } from "./index.ts"
3+
4+
function spawnError(error: unknown): never {
5+
const detail = error instanceof Error ? error.message : String(error)
6+
throw new Error(
7+
"Could not run secret-tool. Install libsecret " +
8+
"(e.g. apt install libsecret-tools, pacman -S libsecret).\n" +
9+
"Alternatively, set the LINEAR_API_KEY environment variable.\n" +
10+
` (${detail})`,
11+
)
12+
}
13+
14+
async function secretTool(
15+
args: string[],
16+
options?: { stdin?: string },
17+
) {
18+
let process: Deno.ChildProcess
19+
try {
20+
process = new Deno.Command("secret-tool", {
21+
args,
22+
stdin: options?.stdin != null ? "piped" : "null",
23+
stdout: "piped",
24+
stderr: "piped",
25+
}).spawn()
26+
} catch (error) {
27+
spawnError(error)
28+
}
29+
30+
if (options?.stdin != null) {
31+
try {
32+
const writer = process.stdin.getWriter()
33+
await writer.write(new TextEncoder().encode(options.stdin))
34+
await writer.close()
35+
} catch (error) {
36+
try {
37+
process.kill()
38+
} catch { /* already exited */ }
39+
const detail = error instanceof Error ? error.message : String(error)
40+
throw new Error(`Failed to write to stdin of secret-tool: ${detail}`)
41+
}
42+
}
43+
44+
const result = await process.output()
45+
return {
46+
success: result.success,
47+
code: result.code,
48+
stdout: new TextDecoder().decode(result.stdout).trim(),
49+
stderr: new TextDecoder().decode(result.stderr).trim(),
50+
}
51+
}
352

453
export const linuxBackend: KeyringBackend = {
554
async get(account) {
6-
const result = await run([
7-
"secret-tool",
55+
const result = await secretTool([
856
"lookup",
957
"service",
1058
SERVICE,
@@ -24,9 +72,8 @@ export const linuxBackend: KeyringBackend = {
2472
},
2573

2674
async set(account, password) {
27-
const result = await run(
75+
const result = await secretTool(
2876
[
29-
"secret-tool",
3077
"store",
3178
"--label",
3279
`${SERVICE}: ${account}`,
@@ -45,8 +92,7 @@ export const linuxBackend: KeyringBackend = {
4592
},
4693

4794
async delete(account) {
48-
const result = await run([
49-
"secret-tool",
95+
const result = await secretTool([
5096
"clear",
5197
"service",
5298
SERVICE,

‎src/keyring/macos.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,37 @@
11
import type { KeyringBackend } from "./index.ts"
2-
import { run, SERVICE } from "./index.ts"
2+
import { SERVICE } from "./index.ts"
3+
4+
async function security(...args: string[]) {
5+
try {
6+
const result = await new Deno.Command("/usr/bin/security", {
7+
args,
8+
stdout: "piped",
9+
stderr: "piped",
10+
}).output()
11+
return {
12+
success: result.success,
13+
code: result.code,
14+
stdout: new TextDecoder().decode(result.stdout).trim(),
15+
stderr: new TextDecoder().decode(result.stderr).trim(),
16+
}
17+
} catch (error) {
18+
const detail = error instanceof Error ? error.message : String(error)
19+
throw new Error(
20+
`Could not run /usr/bin/security. Is this a macOS system?\n (${detail})`,
21+
)
22+
}
23+
}
324

425
export const macosBackend: KeyringBackend = {
526
async get(account) {
6-
const result = await run([
7-
"/usr/bin/security",
27+
const result = await security(
828
"find-generic-password",
929
"-a",
1030
account,
1131
"-s",
1232
SERVICE,
1333
"-w",
14-
])
34+
)
1535
if (!result.success) {
1636
// exit 44 = errSecItemNotFound
1737
if (result.code === 44) return null
@@ -23,17 +43,16 @@ export const macosBackend: KeyringBackend = {
2343
},
2444

2545
async set(account, password) {
26-
const result = await run([
27-
"/usr/bin/security",
46+
const result = await security(
2847
"add-generic-password",
2948
"-a",
3049
account,
3150
"-s",
3251
SERVICE,
3352
"-w",
3453
password,
35-
"-U", // update the item if it already exists
36-
])
54+
"-U",
55+
)
3756
if (!result.success) {
3857
throw new Error(
3958
`security add-generic-password failed (exit ${result.code}): ${result.stderr}`,
@@ -42,14 +61,13 @@ export const macosBackend: KeyringBackend = {
4261
},
4362

4463
async delete(account) {
45-
const result = await run([
46-
"/usr/bin/security",
64+
const result = await security(
4765
"delete-generic-password",
4866
"-a",
4967
account,
5068
"-s",
5169
SERVICE,
52-
])
70+
)
5371
// exit 44 = errSecItemNotFound
5472
if (!result.success && result.code !== 44) {
5573
throw new Error(

‎src/keyring/windows.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@ import { SERVICE } from "./index.ts"
44
const ERROR_NOT_FOUND = 1168
55
const CRED_TYPE_GENERIC = 1
66
const CRED_PERSIST_LOCAL_MACHINE = 2
7+
// CREDENTIALW struct layout on 64-bit Windows (80 bytes):
8+
// 0: Flags (u32) 4: Type (u32) 8: TargetName (ptr)
9+
// 16: Comment (ptr) 24: LastWritten (i64) 32: CredentialBlobSize (u32)
10+
// 36: (padding) 40: CredentialBlob (ptr) 48: Persist (u32)
11+
// 52: AttributeCount 56: Attributes (ptr) 64: TargetAlias (ptr)
12+
// 72: UserName (ptr)
713
const CREDENTIAL_SIZE = 80
814

915
type FfiBuffer = Uint8Array<ArrayBuffer>
@@ -86,6 +92,9 @@ function credGet(account: string): string | null {
8692
const ok = lib.symbols.CredReadW(target, CRED_TYPE_GENERIC, 0, outBuf)
8793
if (!ok) {
8894
const err = getLastError()
95+
// Deno's FFI boundary clobbers the Win32 thread-local error before
96+
// GetLastError can read it through a separate dlopen call. Treat 0
97+
// (no error set) as "not found" since that's the only expected failure.
8998
if (err === ERROR_NOT_FOUND || err === 0) return null
9099
throw new Error(`CredReadW failed (error ${err})`)
91100
}
@@ -138,6 +147,7 @@ function credDelete(account: string): void {
138147
const ok = lib.symbols.CredDeleteW(target, CRED_TYPE_GENERIC, 0)
139148
if (!ok) {
140149
const err = getLastError()
150+
// See credGet for why err === 0 is treated as "not found"
141151
if (err === ERROR_NOT_FOUND || err === 0) return
142152
throw new Error(`CredDeleteW failed (error ${err})`)
143153
}

‎test/keyring.test.ts‎

Lines changed: 19 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,19 @@ import { fromFileUrl } from "@std/path"
44
const keyringUrl = new URL("../src/keyring/index.ts", import.meta.url)
55
const denoJsonPath = fromFileUrl(new URL("../deno.json", import.meta.url))
66

7+
const MOCK_BACKEND = `
8+
const _store = new Map();
9+
_setBackend({
10+
get(account) { return Promise.resolve(_store.get(account) ?? null) },
11+
set(account, password) { _store.set(account, password); return Promise.resolve() },
12+
delete(account) { _store.delete(account); return Promise.resolve() },
13+
});
14+
`.trim()
15+
16+
function mockAndImport(imports: string): string {
17+
return `import { ${imports}, _setBackend } from "${keyringUrl}";\n${MOCK_BACKEND}`
18+
}
19+
720
async function runWithKeyring(code: string): Promise<string> {
821
const command = new Deno.Command("deno", {
922
args: [
@@ -28,13 +41,7 @@ async function runWithKeyring(code: string): Promise<string> {
2841

2942
Deno.test("keyring - getPassword returns null when not set", async () => {
3043
const code = `
31-
import { getPassword, _setBackend } from "${keyringUrl}";
32-
_setBackend({
33-
store: new Map(),
34-
get(account) { return Promise.resolve(this.store.get(account) ?? null) },
35-
set(account, password) { this.store.set(account, password); return Promise.resolve() },
36-
delete(account) { this.store.delete(account); return Promise.resolve() },
37-
});
44+
${mockAndImport("getPassword")}
3845
const result = await getPassword("missing");
3946
console.log(result === null ? "null" : result);
4047
`
@@ -44,13 +51,7 @@ Deno.test("keyring - getPassword returns null when not set", async () => {
4451

4552
Deno.test("keyring - setPassword and getPassword round-trip", async () => {
4653
const code = `
47-
import { getPassword, setPassword, _setBackend } from "${keyringUrl}";
48-
_setBackend({
49-
store: new Map(),
50-
get(account) { return Promise.resolve(this.store.get(account) ?? null) },
51-
set(account, password) { this.store.set(account, password); return Promise.resolve() },
52-
delete(account) { this.store.delete(account); return Promise.resolve() },
53-
});
54+
${mockAndImport("getPassword, setPassword")}
5455
await setPassword("my-account", "secret123");
5556
const result = await getPassword("my-account");
5657
console.log(result);
@@ -61,13 +62,7 @@ Deno.test("keyring - setPassword and getPassword round-trip", async () => {
6162

6263
Deno.test("keyring - deletePassword removes stored password", async () => {
6364
const code = `
64-
import { getPassword, setPassword, deletePassword, _setBackend } from "${keyringUrl}";
65-
_setBackend({
66-
store: new Map(),
67-
get(account) { return Promise.resolve(this.store.get(account) ?? null) },
68-
set(account, password) { this.store.set(account, password); return Promise.resolve() },
69-
delete(account) { this.store.delete(account); return Promise.resolve() },
70-
});
65+
${mockAndImport("getPassword, setPassword, deletePassword")}
7166
await setPassword("my-account", "secret123");
7267
await deletePassword("my-account");
7368
const result = await getPassword("my-account");
@@ -79,13 +74,7 @@ Deno.test("keyring - deletePassword removes stored password", async () => {
7974

8075
Deno.test("keyring - setPassword overwrites existing value", async () => {
8176
const code = `
82-
import { getPassword, setPassword, _setBackend } from "${keyringUrl}";
83-
_setBackend({
84-
store: new Map(),
85-
get(account) { return Promise.resolve(this.store.get(account) ?? null) },
86-
set(account, password) { this.store.set(account, password); return Promise.resolve() },
87-
delete(account) { this.store.delete(account); return Promise.resolve() },
88-
});
77+
${mockAndImport("getPassword, setPassword")}
8978
await setPassword("my-account", "first");
9079
await setPassword("my-account", "second");
9180
const result = await getPassword("my-account");
@@ -97,13 +86,7 @@ Deno.test("keyring - setPassword overwrites existing value", async () => {
9786

9887
Deno.test("keyring - multiple accounts are independent", async () => {
9988
const code = `
100-
import { getPassword, setPassword, _setBackend } from "${keyringUrl}";
101-
_setBackend({
102-
store: new Map(),
103-
get(account) { return Promise.resolve(this.store.get(account) ?? null) },
104-
set(account, password) { this.store.set(account, password); return Promise.resolve() },
105-
delete(account) { this.store.delete(account); return Promise.resolve() },
106-
});
89+
${mockAndImport("getPassword, setPassword")}
10790
await setPassword("account-a", "password-a");
10891
await setPassword("account-b", "password-b");
10992
const a = await getPassword("account-a");
@@ -118,13 +101,7 @@ Deno.test("keyring - multiple accounts are independent", async () => {
118101

119102
Deno.test("keyring - deletePassword on missing account is a no-op", async () => {
120103
const code = `
121-
import { deletePassword, _setBackend } from "${keyringUrl}";
122-
_setBackend({
123-
store: new Map(),
124-
get(account) { return Promise.resolve(this.store.get(account) ?? null) },
125-
set(account, password) { this.store.set(account, password); return Promise.resolve() },
126-
delete(account) { this.store.delete(account); return Promise.resolve() },
127-
});
104+
${mockAndImport("deletePassword")}
128105
await deletePassword("nonexistent");
129106
console.log("ok");
130107
`

0 commit comments

Comments
 (0)