Skip to content

Commit de50520

Browse files
committed
fix(auth): address claude oauth review hardening
1 parent 875fbd5 commit de50520

10 files changed

Lines changed: 110 additions & 35 deletions

File tree

packages/app/src/docker-git/controller-compose-files.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ const mapComposePathError = (error: PlatformError): ControllerBootstrapError =>
2525
// QUOTE(ТЗ): n/a
2626
// REF: issue-440-review-compose-overlay
2727
// SOURCE: n/a
28-
// FORMAT THEOREM: forall p: env(extra)=p and exists(resolve(p)) -> resolve(extra)=Some(resolve(p))
28+
// FORMAT THEOREM: forall p: env(extra)=p and regular_file(resolve(p)) -> resolve(extra)=Some(resolve(p))
2929
// PURITY: SHELL
3030
// EFFECT: Effect<string | null, ControllerBootstrapError, FileSystem | Path>
31-
// INVARIANT: non-empty extra compose env values either resolve to an existing file or fail before docker compose
31+
// INVARIANT: non-empty extra compose env values either resolve to a regular file or fail before docker compose
3232
// COMPLEXITY: O(1)
3333
export const loadControllerComposeExtraPath = (): Effect.Effect<
3434
string | null,
@@ -45,12 +45,23 @@ export const loadControllerComposeExtraPath = (): Effect.Effect<
4545
const path = yield* _(Path.Path)
4646
const extraOverlayPath = path.resolve(raw)
4747
const isExists = yield* _(fs.exists(extraOverlayPath).pipe(Effect.mapError(mapComposePathError)))
48-
return isExists
48+
if (!isExists) {
49+
return yield* _(
50+
Effect.fail(
51+
controllerBootstrapError(
52+
`${controllerComposeExtraFileEnvKey} points to ${extraOverlayPath}, but it was not found.`
53+
)
54+
)
55+
)
56+
}
57+
58+
const info = yield* _(fs.stat(extraOverlayPath).pipe(Effect.mapError(mapComposePathError)))
59+
return info.type === "File"
4960
? extraOverlayPath
5061
: yield* _(
5162
Effect.fail(
5263
controllerBootstrapError(
53-
`${controllerComposeExtraFileEnvKey} points to ${extraOverlayPath}, but it was not found.`
64+
`${controllerComposeExtraFileEnvKey} points to ${extraOverlayPath}, but it is not a regular file.`
5465
)
5566
)
5667
)

packages/app/tests/docker-git/controller-compose.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { NodeContext } from "@effect/platform-node"
2+
import * as FileSystem from "@effect/platform/FileSystem"
23
import * as Path from "@effect/platform/Path"
34
import { describe, expect, it } from "@effect/vitest"
45
import { Effect } from "effect"
@@ -114,6 +115,28 @@ describe("controller compose preparation", () => {
114115
).pipe(Effect.provide(NodeContext.layer))
115116
})
116117

118+
it.effect("rejects extra compose overlay paths that are directories", () =>
119+
withMinimalControllerRoot((rootDir) =>
120+
Effect.gen(function*(_) {
121+
const fs = yield* _(FileSystem.FileSystem)
122+
const path = yield* _(Path.Path)
123+
const extraComposePath = path.join(rootDir, "docker-compose.auth-claude-login.yml")
124+
yield* _(fs.makeDirectory(extraComposePath))
125+
yield* _(
126+
withControllerEnv([
127+
[controllerBuildSkillerEnvKey, "0"],
128+
[controllerComposeExtraFileEnvKey, extraComposePath],
129+
[controllerDockerRuntimeEnvKey, undefined],
130+
[controllerGpuModeEnvKey, undefined]
131+
])
132+
)
133+
134+
const error = yield* _(resolveControllerComposeFiles().pipe(Effect.flip))
135+
expect(error._tag).toBe("ControllerBootstrapError")
136+
expect(error.message).toContain("regular file")
137+
})
138+
).pipe(Effect.provide(NodeContext.layer)))
139+
117140
it.effect("does not initialize the Skiller submodule when package metadata already exists", () => {
118141
const startedCommands: Array<string> = []
119142

packages/auth-oauth/src/claude-docker-oauth.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { chmod, mkdtemp, mkdir, rm, writeFile } from "node:fs/promises"
1+
import { chmod, mkdtemp, mkdir, rename, rm, writeFile } from "node:fs/promises"
22
import { tmpdir } from "node:os"
33
import { join, resolve } from "node:path"
44
import { fileURLToPath } from "node:url"
@@ -285,8 +285,23 @@ const runDockerProbe = (spec: ClaudeDockerProbeSpec): Promise<number> =>
285285

286286
const writeCapturedToken = async (accountPath: string, token: string): Promise<void> => {
287287
const tokenPath = claudeOauthTokenPath(accountPath)
288-
await writeFile(tokenPath, formatClaudeOauthTokenFile(token), "utf8")
289-
await chmod(tokenPath, claudeOauthTokenFileMode)
288+
const tempDir = await mkdtemp(join(accountPath, ".oauth-token-write-"))
289+
const tempPath = join(tempDir, ".oauth-token")
290+
let renamed = false
291+
try {
292+
await writeFile(tempPath, formatClaudeOauthTokenFile(token), {
293+
encoding: "utf8",
294+
mode: claudeOauthTokenFileMode
295+
})
296+
await chmod(tempPath, claudeOauthTokenFileMode)
297+
await rename(tempPath, tokenPath)
298+
renamed = true
299+
} finally {
300+
await rm(tempDir, { recursive: true, force: true })
301+
if (!renamed) {
302+
await rm(tempPath, { force: true })
303+
}
304+
}
290305
}
291306

292307
const dockerProbeStatusFromExitCode = (exitCode: number): ClaudeDockerProbeStatus =>

packages/auth-oauth/src/claude-local-smoke.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,10 @@ export const persistClaudeLocalOauthToken = async (
9191
token: string
9292
): Promise<void> => {
9393
const tokenPath = claudeOauthTokenPath(accountPath)
94-
await writeFile(tokenPath, formatClaudeOauthTokenFile(token), "utf8")
94+
await writeFile(tokenPath, formatClaudeOauthTokenFile(token), {
95+
encoding: "utf8",
96+
mode: claudeOauthTokenFileMode
97+
})
9598
await chmod(tokenPath, claudeOauthTokenFileMode)
9699
}
97100

packages/auth-oauth/tests/claude-docker-oauth.test.ts

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdtemp, readFile, stat } from "node:fs/promises"
1+
import { mkdtemp, readFile, rm, stat } from "node:fs/promises"
22
import { tmpdir } from "node:os"
33
import { join } from "node:path"
44

@@ -41,12 +41,16 @@ const oauthTokenArbitrary = fc.array(fc.constantFrom(
4141
maxLength: 64
4242
}).map((chars) => `${oauthTokenPrefix}${chars.join("")}`)
4343

44+
const temporaryAccountPath = (prefix: string) =>
45+
Effect.acquireRelease(
46+
Effect.tryPromise(() => mkdtemp(join(tmpdir(), prefix))),
47+
(accountPath) => Effect.promise(() => rm(accountPath, { recursive: true, force: true }))
48+
)
49+
4450
describe("Claude Docker OAuth runner", () => {
4551
it.effect("runs Docker setup-token, persists token, then probes through the mounted token file", () =>
46-
Effect.gen(function*(_) {
47-
const accountPath = yield* _(
48-
Effect.tryPromise(() => mkdtemp(join(tmpdir(), "docker-git-auth-oauth-docker-test-")))
49-
)
52+
Effect.scoped(Effect.gen(function*(_) {
53+
const accountPath = yield* _(temporaryAccountPath("docker-git-auth-oauth-docker-test-"))
5054
const builds: Array<ClaudeDockerBuildSpec> = []
5155
const setupRuns: Array<ClaudeDockerSetupTokenSpec> = []
5256
const probeRuns: Array<ClaudeDockerProbeSpec> = []
@@ -96,13 +100,11 @@ describe("Claude Docker OAuth runner", () => {
96100
expect(probeRuns[0]?.args.slice(-3)).toEqual(["claude-test:latest", "-p", "ping"])
97101
const tokenMode = yield* _(Effect.tryPromise(() => stat(claudeOauthTokenPath(accountPath))))
98102
expect(tokenMode.mode & 0o777).toBe(claudeOauthTokenFileMode)
99-
}))
103+
})))
100104

101105
it.effect("keeps the captured token and file mode when Docker probe fails", () =>
102-
Effect.gen(function*(_) {
103-
const accountPath = yield* _(
104-
Effect.tryPromise(() => mkdtemp(join(tmpdir(), "docker-git-auth-oauth-docker-probe-test-")))
105-
)
106+
Effect.scoped(Effect.gen(function*(_) {
107+
const accountPath = yield* _(temporaryAccountPath("docker-git-auth-oauth-docker-probe-test-"))
106108
const result = yield* _(
107109
Effect.tryPromise(() =>
108110
runClaudeDockerOauth({
@@ -124,7 +126,7 @@ describe("Claude Docker OAuth runner", () => {
124126
const tokenMode = yield* _(Effect.tryPromise(() => stat(claudeOauthTokenPath(accountPath))))
125127
expect(tokenFile).toBe(`${oauthToken}\n`)
126128
expect(tokenMode.mode & 0o777).toBe(claudeOauthTokenFileMode)
127-
}))
129+
})))
128130

129131
it.effect("returns command failure when setup-token exits non-zero without token", () =>
130132
Effect.gen(function*(_) {

packages/lib/src/usecases/auth-claude-oauth.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,13 @@ const resolveClaudeDockerOauthTokenResult = (
177177
)
178178
)
179179
}
180+
if (result.probeStatus._tag === "ClaudeDockerProbeFailed") {
181+
yield* _(
182+
Effect.logWarning(
183+
`claude -p ping failed with exit=${result.probeStatus.exitCode}; OAuth token was saved. Run docker-git auth claude status to verify later.`
184+
)
185+
)
186+
}
180187
return result.token
181188
}
182189
if (result._tag === "ClaudeDockerOauthCommandFailed") {

packages/lib/src/usecases/auth-claude.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ const persistClaudeOauthToken = (
5757
): Effect.Effect<void, PlatformError> =>
5858
Effect.gen(function*(_) {
5959
const tokenPath = claudeOauthTokenPath(accountPath)
60-
yield* _(fs.writeFileString(tokenPath, formatClaudeOauthTokenFile(token)))
60+
yield* _(fs.writeFileString(tokenPath, formatClaudeOauthTokenFile(token), { mode: claudeOauthTokenFileMode }))
6161
yield* _(fs.chmod(tokenPath, claudeOauthTokenFileMode))
6262
})
6363

packages/lib/tests/usecases/auth-claude-local.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,8 @@ import {
1616
runClaudeLocalEnvTokenLoginFlow
1717
} from "../../src/usecases/auth-claude-local.js"
1818

19-
const oauthTokenPrefix = ["sk", "ant", ""].join("-")
20-
const oauthToken = `${oauthTokenPrefix}oat01-LOCAL0123456789abcdef`
21-
const lowerPriorityToken = `${oauthTokenPrefix}oat01-LOWERPRIORITY0123456789`
19+
const oauthToken = "TEST_CLAUDE_OAUTH_TOKEN_LOCAL"
20+
const lowerPriorityToken = "TEST_CLAUDE_OAUTH_TOKEN_LOWER_PRIORITY"
2221

2322
const makeExitCodeExecutor = (
2423
exitCode: number,

packages/lib/tests/usecases/auth-claude-login.test.ts

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as FileSystem from "@effect/platform/FileSystem"
44
import * as Path from "@effect/platform/Path"
55
import { NodeContext } from "@effect/platform-node"
66
import { describe, expect, it } from "@effect/vitest"
7-
import { Effect } from "effect"
7+
import { Effect, Logger } from "effect"
88
import * as Inspectable from "effect/Inspectable"
99
import * as Sink from "effect/Sink"
1010
import * as Stream from "effect/Stream"
@@ -13,8 +13,7 @@ import { authClaudeLogin } from "../../src/usecases/auth-claude.js"
1313

1414
const encode = (value: string): Uint8Array => new TextEncoder().encode(value)
1515

16-
const oauthTokenPrefix = ["sk", "ant", ""].join("-")
17-
const oauthToken = `${oauthTokenPrefix}oat01-EXAMPLE0123456789abcdef`
16+
const oauthToken = "TEST_CLAUDE_OAUTH_TOKEN_EXAMPLE"
1817

1918
// Mirrors the real `claude setup-token` output that the OAuth parser scans for.
2019
const setupTokenOutput = (token: string): string =>
@@ -130,10 +129,18 @@ const withPatchedEnv = <A, E, R>(
130129
const runLoginAndReadToken = (
131130
root: string,
132131
pingExitCode: number
133-
): Effect.Effect<string, unknown, FileSystem.FileSystem | Path.Path> =>
132+
): Effect.Effect<
133+
{ readonly logs: ReadonlyArray<string>; readonly tokenText: string },
134+
unknown,
135+
FileSystem.FileSystem | Path.Path
136+
> =>
134137
Effect.gen(function*(_) {
135138
const fs = yield* _(FileSystem.FileSystem)
136139
const path = yield* _(Path.Path)
140+
const logs: Array<string> = []
141+
const logger = Logger.make(({ message }) => {
142+
logs.push(String(message))
143+
})
137144
const claudeAuthPath = path.join(root, ".docker-git/.orch/auth/claude")
138145

139146
yield* _(
@@ -142,11 +149,13 @@ const runLoginAndReadToken = (
142149
label: null,
143150
claudeAuthPath
144151
}).pipe(
145-
Effect.provideService(CommandExecutor.CommandExecutor, makeFakeExecutor(oauthToken, pingExitCode))
152+
Effect.provideService(CommandExecutor.CommandExecutor, makeFakeExecutor(oauthToken, pingExitCode)),
153+
Effect.provide(Logger.replace(Logger.defaultLogger, logger))
146154
)
147155
)
148156

149-
return yield* _(fs.readFileString(path.join(claudeAuthPath, "default", ".oauth-token")))
157+
const tokenText = yield* _(fs.readFileString(path.join(claudeAuthPath, "default", ".oauth-token")))
158+
return { logs, tokenText }
150159
})
151160

152161
const runLoginWithoutCapturedToken = (
@@ -184,8 +193,9 @@ describe("authClaudeLogin", () => {
184193
withPatchedEnv(
185194
{ HOME: root, DOCKER_GIT_STATE_AUTO_SYNC: "0", DOCKER_GIT_PROJECTS_ROOT: undefined },
186195
Effect.gen(function*(_) {
187-
const persisted = yield* _(runLoginAndReadToken(root, 7))
188-
expect(persisted.trim()).toBe(oauthToken)
196+
const { logs, tokenText } = yield* _(runLoginAndReadToken(root, 7))
197+
expect(tokenText.trim()).toBe(oauthToken)
198+
expect(logs.some((message) => message.includes("claude -p ping failed with exit=7"))).toBe(true)
189199
})
190200
)
191201
).pipe(Effect.provide(NodeContext.layer)))
@@ -195,8 +205,8 @@ describe("authClaudeLogin", () => {
195205
withPatchedEnv(
196206
{ HOME: root, DOCKER_GIT_STATE_AUTO_SYNC: "0", DOCKER_GIT_PROJECTS_ROOT: undefined },
197207
Effect.gen(function*(_) {
198-
const persisted = yield* _(runLoginAndReadToken(root, 0))
199-
expect(persisted.trim()).toBe(oauthToken)
208+
const { tokenText } = yield* _(runLoginAndReadToken(root, 0))
209+
expect(tokenText.trim()).toBe(oauthToken)
200210
})
201211
)
202212
).pipe(Effect.provide(NodeContext.layer)))

scripts/e2e/auth-claude-login.sh

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ chmod 0700 "$ROOT"
1313
KEEP="${KEEP:-0}"
1414
COMPOSE_OVERRIDE_FILE="$ROOT/docker-compose.auth-claude-login.yml"
1515
LOGIN_TIMEOUT_SECONDS="${DOCKER_GIT_E2E_AUTH_CLAUDE_LOGIN_TIMEOUT_SECONDS:-900}"
16+
OAUTH_TOKEN_MARKER="docker-git-e2e-oauth-token-marker"
1617

1718
export DOCKER_GIT_PROJECTS_ROOT="$ROOT"
1819
export DOCKER_GIT_STATE_AUTO_SYNC=0
@@ -21,11 +22,11 @@ export DOCKER_GIT_PROJECTS_ROOT_VOLUME="docker-git-e2e-auth-claude-$RUN_ID-proje
2122
export DOCKER_GIT_CONTROLLER_COMPOSE_EXTRA_FILE="$COMPOSE_OVERRIDE_FILE"
2223
export COMPOSE_PROJECT_NAME="docker-git"
2324

24-
cat > "$COMPOSE_OVERRIDE_FILE" <<'YAML'
25+
cat > "$COMPOSE_OVERRIDE_FILE" <<YAML
2526
services:
2627
api:
2728
environment:
28-
DOCKER_GIT_CLAUDE_OAUTH_TOKEN: docker-git-e2e-oauth-token-marker
29+
DOCKER_GIT_CLAUDE_OAUTH_TOKEN: ${OAUTH_TOKEN_MARKER}
2930
YAML
3031

3132
LOG_FILE="/tmp/docker-git-auth-claude-login-$RUN_ID.log"
@@ -74,6 +75,10 @@ if [[ "$login_exit" -ne 0 ]]; then
7475
fail "docker-git auth claude login failed (exit: $login_exit)"
7576
fi
7677

78+
if grep -Fq -- "$OAUTH_TOKEN_MARKER" "$LOG_FILE"; then
79+
fail "expected OAuth token marker to be absent from auth claude login output"
80+
fi
81+
7782
grep -Fq -- "Claude OAuth token saved" "$LOG_FILE" \
7883
|| fail "expected saved-token warning in auth claude login output"
7984

0 commit comments

Comments
 (0)