diff --git a/packages/@overeng/megarepo/README.md b/packages/@overeng/megarepo/README.md index f0fbdd76c8..d645a66804 100644 --- a/packages/@overeng/megarepo/README.md +++ b/packages/@overeng/megarepo/README.md @@ -53,6 +53,14 @@ For CI: mr apply --git-protocol=https ``` +For authenticated HTTP(S) clones, configure a Git credential helper (for example, +`gh auth setup-git`) or use SSH. Megarepo rejects HTTP(S) userinfo in member +configuration, lock files, and `mr add` inputs, including username-only tokens and +percent-encoded credentials. Errors show only `https://***@host/...` and explain +how to migrate to a credential helper or SSH. Remove embedded credentials from +existing configuration or lock files before running commands; accepted source identities are +never rewritten. SSH URLs (including scp-style `git@host:path`) are unchanged. + ### Standalone Nix lock synchronization Check or update standalone flake locks from an already resolved root input, or diff --git a/packages/@overeng/megarepo/src/cli/commands/add.ts b/packages/@overeng/megarepo/src/cli/commands/add.ts index e1726348ab..74bf4b1b7f 100644 --- a/packages/@overeng/megarepo/src/cli/commands/add.ts +++ b/packages/@overeng/megarepo/src/cli/commands/add.ts @@ -14,6 +14,8 @@ import { MegarepoConfig, parseSourceString, readMegarepoConfig, + redactSourceCredentials, + sourceCredentialError, writeMegarepoConfig, } from '../../core/config.ts' import * as Git from '../../core/git.ts' @@ -106,12 +108,17 @@ export const addCommand = Cli.Command.make( // Parse the repo reference const parsed = parseRepoRef(repo) if (parsed === undefined) { + const credentialError = sourceCredentialError(repo) + const message = + credentialError ?? `Invalid repo reference: ${redactSourceCredentials(repo)}` tui.dispatch({ _tag: 'SetError', error: 'invalid_repo', - message: `Invalid repo reference: ${repo}`, + message, + }) + return yield* new AddCommandError({ + message: credentialError ?? 'Invalid repo reference', }) - return yield* new AddCommandError({ message: 'Invalid repo reference' }) } const memberName = Option.getOrElse(name, () => parsed.suggestedName) @@ -191,7 +198,7 @@ export const addCommand = Cli.Command.make( command: 'add', label: Option.getOrElse(name, () => parseRepoRef(repo)?.suggestedName ?? 'add'), output: outputMode, - repo, + repo: redactSourceCredentials(repo), }), ), ), diff --git a/packages/@overeng/megarepo/src/cli/commands/store/mod.ts b/packages/@overeng/megarepo/src/cli/commands/store/mod.ts index d7c9cd14f8..9e2788eb9f 100644 --- a/packages/@overeng/megarepo/src/cli/commands/store/mod.ts +++ b/packages/@overeng/megarepo/src/cli/commands/store/mod.ts @@ -30,6 +30,8 @@ import { isRemoteSource, getSourceRef, readMegarepoConfig, + redactSourceCredentials, + sourceCredentialError, } from '../../../core/config.ts' import * as Git from '../../../core/git.ts' import { LOCK_FILE_NAME, readLockFile } from '../../../core/lock.ts' @@ -3964,8 +3966,10 @@ const storeAddCommand = Cli.Command.make( tui.dispatch({ _tag: 'SetError', error: 'invalid_source', - message: `Invalid source string: ${sourceString}`, - source: sourceString, + message: + sourceCredentialError(sourceString) ?? + `Invalid source string: ${redactSourceCredentials(sourceString)}`, + source: redactSourceCredentials(sourceString), }) }), { view: React.createElement(StoreView, { stateAtom: StoreApp.stateAtom }) }, @@ -4099,7 +4103,7 @@ const storeAddCommand = Cli.Command.make( Effect.provide(StoreLayer), Observability.withStoreSourceSpan({ name: 'megarepo/store/add', - source: sourceString, + source: redactSourceCredentials(sourceString), }), ), ).pipe(Cli.Command.withDescription('Add a repository to the store (without adding to megarepo)')) @@ -4354,8 +4358,10 @@ const storeWorktreeNewCommand = Cli.Command.make( tui.dispatch({ _tag: 'SetError', error: 'invalid_source', - message: `Invalid repository: ${repoString}`, - source: repoString, + message: + sourceCredentialError(repoString) ?? + `Invalid repository: ${redactSourceCredentials(repoString)}`, + source: redactSourceCredentials(repoString), }) }), { view: React.createElement(StoreView, { stateAtom: StoreApp.stateAtom }) }, @@ -4573,7 +4579,7 @@ const storeWorktreeNewCommand = Cli.Command.make( Effect.provide(StoreLayer), Observability.withStoreSourceSpan({ name: 'megarepo/store/worktree/new', - source: repoString, + source: redactSourceCredentials(repoString), ...(Option.isSome(refOpt) === true ? { ref: refOpt.value } : {}), ...(Option.isSome(baseOpt) === true ? { base: baseOpt.value } : {}), ...(Option.isSome(commitOpt) === true ? { commit: commitOpt.value } : {}), diff --git a/packages/@overeng/megarepo/src/core/config.ts b/packages/@overeng/megarepo/src/core/config.ts index b9efd1e9d7..9e3eb78760 100644 --- a/packages/@overeng/megarepo/src/core/config.ts +++ b/packages/@overeng/megarepo/src/core/config.ts @@ -17,6 +17,7 @@ import path from 'node:path' import { Effect, Option, Schema } from 'effect' import * as FileSystem from 'effect/FileSystem' +import * as SchemaIssue from 'effect/SchemaIssue' import { EffectPath, @@ -154,6 +155,43 @@ export class GeneratorsConfig extends Schema.Class('Generators // Megarepo Configuration // ============================================================================= +const credentialSourceUrl = (source: string): string | undefined => { + if (/^\s*https?:\/\/[^/?#]*@/i.test(source) === true) { + return source.replace(/^(\s*https?:\/\/)[^/?#]*@/i, '$1***@') + } + if (/^\s*https?:/i.test(source) === false) return undefined + try { + const url = new URL(source) + if (url.username === '' && url.password === '') return undefined + url.username = '***' + url.password = '' + return url.href + } catch { + return undefined + } +} + +/** Render HTTP userinfo safely without changing accepted source identities. */ +export const redactSourceCredentials = (source: string): string => + credentialSourceUrl(source) ?? source + +/** Shared rejection for configuration and CLI source inputs. */ +export const sourceCredentialError = (source: string): string | undefined => { + const safeUrl = credentialSourceUrl(source) + return safeUrl === undefined + ? undefined + : `HTTP(S) URL userinfo is not supported: ${safeUrl}. Fix the lock or config and use a Git credential helper (e.g. gh auth setup-git) or SSH instead.` +} + +/** Source strings shared by configuration and lock decoding. */ +export const MemberSourceString = Schema.String.check( + Schema.makeFilter((source) => { + const message = sourceCredentialError(source) + // Omit the rejected value from this leaf issue. + return message === undefined ? undefined : new SchemaIssue.InvalidValue({ message }) + }), +) + /** * Main megarepo configuration schema * @@ -169,7 +207,7 @@ export class MegarepoConfig extends Schema.Class('MegarepoConfig $schema: Schema.optional(Schema.String), /** Members: repos to include in this megarepo (name -> source string) */ - members: Schema.Record(Schema.String, Schema.String), + members: Schema.Record(Schema.String, MemberSourceString), /** Generators: optional config file generation */ generators: Schema.optional(GeneratorsConfig), @@ -297,6 +335,7 @@ export const decodeMegarepoConfigContent = ({ readonly content: string readonly format: ConfigFormat }) => + // Keep reportInput disabled: enclosing schema issues otherwise retain credentialed inputs. Schema.decodeEffect( format === 'kdl' ? MegarepoConfigFromKdl : Schema.fromJsonString(MegarepoConfig), )(content) @@ -483,6 +522,7 @@ const isLocalPath = (s: string): boolean => { * - Local path: "./...", "../...", "/...", "~..." (no #ref support) */ export const parseSourceString = (sourceString: string): MemberSource | undefined => { + if (sourceCredentialError(sourceString) !== undefined) return undefined // Local paths don't support #ref syntax - the entire string is the path if (isLocalPath(sourceString) === true) { return { type: 'path', path: sourceString } diff --git a/packages/@overeng/megarepo/src/core/config.unit.test.ts b/packages/@overeng/megarepo/src/core/config.unit.test.ts index d7a1ab7e2d..4b6c1dfe72 100644 --- a/packages/@overeng/megarepo/src/core/config.unit.test.ts +++ b/packages/@overeng/megarepo/src/core/config.unit.test.ts @@ -18,6 +18,7 @@ import { isValidMemberName, MegarepoConfig, parseSourceString, + sourceCredentialError, rejectRetiredRootConfig, validateMemberName, } from './config.ts' @@ -143,6 +144,50 @@ describe('config', () => { }) describe('parseSourceString', () => { + it.each([ + 'https://user:secret@example.com/org/repo#main', + 'http://secret@example.com/org/repo', + 'https://us%65r:s%65cret@example.com/org/repo', + 'https://s%65cret@example.com/org/repo', + ])('rejects HTTP userinfo without exposing it: %s', (source) => { + expect(parseSourceString(source)).toBeUndefined() + const safeUrl = source.replace(/\/\/.*@/, '//***@') + expect(sourceCredentialError(source)).toContain(safeUrl) + for (const format of ['json', 'kdl'] as const) { + const content = + format === 'json' + ? JSON.stringify({ members: { repo: source } }) + : `members { repo "${source}" }` + const exit = Effect.runSyncExit(decodeMegarepoConfigContent({ content, format })) + expect(Exit.isFailure(exit)).toBe(true) + if (Exit.isFailure(exit) === true) { + const message = Cause.pretty(exit.cause) + expect(message).toContain(safeUrl) + expect(message).toContain('credential helper') + expect(message).toContain('SSH') + expect(message).not.toContain(source) + expect(message).not.toMatch(/secret|s%65cret|us%65r/) + const error = Effect.runSync( + decodeMegarepoConfigContent({ content, format }).pipe(Effect.flip), + ) + expect(JSON.stringify({ cause: exit.cause, issue: error.issue })).not.toMatch( + /secret|s%65cret|us%65r/, + ) + } + } + }) + + it.each(['ssh://git@example.com/org/repo.git', 'git@example.com:org/repo.git'])( + 'preserves SSH sources: %s', + (source) => { + expect(parseSourceString(source)?.type).toBe('url') + expect(Schema.decodeSync(MegarepoConfig)({ members: { repo: source } }).members.repo).toBe( + source, + ) + expect(sourceCredentialError(source)).toBeUndefined() + }, + ) + describe('GitHub shorthand', () => { it('should parse owner/repo', () => { const source = parseSourceString('owner/repo') diff --git a/packages/@overeng/megarepo/src/core/git-timeout.integration.test.ts b/packages/@overeng/megarepo/src/core/git-timeout.integration.test.ts index d74a363048..e095aa5c5e 100644 --- a/packages/@overeng/megarepo/src/core/git-timeout.integration.test.ts +++ b/packages/@overeng/megarepo/src/core/git-timeout.integration.test.ts @@ -12,7 +12,7 @@ import { NodeServices } from '@effect/platform-node' import { describe, it } from '@effect/vitest' -import { Effect } from 'effect' +import { Effect, Tracer } from 'effect' import * as FileSystem from 'effect/FileSystem' import * as Command from 'effect/process/ChildProcess' import { ChildProcessSpawner } from 'effect/process/ChildProcessSpawner' @@ -94,6 +94,69 @@ describe('git operation-aware timeout', () => { }).pipe(Effect.provide(NodeServices.layer)), ) + for (const bare of [false, true]) { + for (const [userinfo, protocol] of [ + ['fixture-user:fixture-password', 'https'], + ['fixture-token', 'http'], + ]) { + it.live( + `strips ${protocol} credentials from ${bare === true ? 'bare' : 'working'} clone origins and spans`, + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem + const { tmp, remote } = yield* makeCloneSource() + const target = cloneTargetIn(tmp, 'clone-credentialed') + const cleanUrl = `${protocol}://example.com/owner/repo.git` + const credentialedUrl = `${protocol}://${userinfo}@example.com/owner/repo.git` + const spans: Array = [] + const tracer = Tracer.make({ + span: (options) => { + const span = new Tracer.NativeSpan(options) + spans.push(span) + return span + }, + }) + // Rewrite both forms so even a regression never contacts the network. + const rewrites = [ + ['GIT_CONFIG_COUNT', '2'], + ['GIT_CONFIG_KEY_0', `url.file://${remote}.insteadOf`], + ['GIT_CONFIG_VALUE_0', cleanUrl], + ['GIT_CONFIG_KEY_1', `url.file://${remote}.insteadOf`], + ['GIT_CONFIG_VALUE_1', credentialedUrl], + ] as const + const cloned = ( + bare === true + ? Git.cloneBare({ url: credentialedUrl, targetPath: target }) + : Git.clone({ url: credentialedUrl, targetPath: target }).pipe(Effect.as(true)) + ).pipe(Effect.withTracer(tracer)) + yield* rewrites.reduceRight( + (effect, [key, value]) => withEnv(key, value, effect), + cloned, + ) + + // Read the persisted config, not the transport URL after insteadOf rewriting. + const configPath = bare === true ? 'config' : '.git/config' + const config = yield* fs.readFileString( + EffectPath.ops.join(target, EffectPath.unsafe.relativeFile(configPath)), + ) + expect(config).toContain(`url = ${cleanUrl}`) + expect(config).not.toContain('fixture-') + for (const name of bare === true ? ['git/clone', 'git/clone-bare'] : ['git/clone']) { + const span = spans.find((candidate) => candidate.name === name) + expect(span).toBeDefined() + expect(span?.attributes.get('git.url')).toBe(cleanUrl) + } + for (const span of spans) { + expect(span.name).not.toContain('fixture-') + for (const [key, value] of span.attributes) { + expect(`${key}=${String(value)}`).not.toContain('fixture-') + } + } + }).pipe(Effect.provide(NodeServices.layer)), + ) + } + } + it.live('a network op is bounded by the network budget (1ms → times out)', () => Effect.gen(function* () { const { tmp, remote } = yield* makeCloneSource() diff --git a/packages/@overeng/megarepo/src/core/git.ts b/packages/@overeng/megarepo/src/core/git.ts index a47242b182..faa4116f72 100644 --- a/packages/@overeng/megarepo/src/core/git.ts +++ b/packages/@overeng/megarepo/src/core/git.ts @@ -454,25 +454,31 @@ const runGitCommandWithRetry = ({ args, cwd }: { args: ReadonlyArray; cw }), ) +/** Strip HTTP userinfo before commands or instrumentation; Git authentication uses credential helpers. */ +export const withoutHttpCredentials = (value: string): string => + value.replace(/^(https?:\/\/)[^/?#]*@/i, '$1') + /** * Clone a git repository */ -const cloneUnlocked = (args: { url: string; targetPath: string; bare?: boolean }) => - Effect.gen(function* () { +const cloneUnlocked = (args: { url: string; targetPath: string; bare?: boolean }) => { + const url = withoutHttpCredentials(args.url) + return Effect.gen(function* () { const cmdArgs = ['clone'] if (args.bare === true) { cmdArgs.push('--bare') } - cmdArgs.push(args.url, args.targetPath) + cmdArgs.push(url, args.targetPath) yield* runGitCommandWithRetry({ args: cmdArgs }) }).pipe( Observability.withGitUrlSpan({ name: 'git/clone', - label: args.url, - url: args.url, + label: url, + url, bare: args.bare ?? false, }), ) +} /** Clone a repository, serializing shared bare targets by their common-directory identity. */ export const clone = (args: { url: string; targetPath: string; bare?: boolean }) => @@ -878,8 +884,9 @@ export const repairBareOriginFetchRefspec = ({ repoPath }: { repoPath: string }) * Configures fetch refspec so remote tracking refs are created on fetch. * (git clone --bare doesn't set this up by default) */ -export const cloneBare = (args: { url: string; targetPath: string }) => - Effect.gen(function* () { +export const cloneBare = (args: { url: string; targetPath: string }) => { + const url = withoutHttpCredentials(args.url) + return Effect.gen(function* () { // Recheck after acquiring the common-dir lock: a sibling may have finished cloning. const exists = yield* Effect.tryPromise(() => lstat(args.targetPath).then( @@ -903,7 +910,7 @@ export const cloneBare = (args: { url: string; targetPath: string }) => ) } } else { - yield* cloneUnlocked({ url: args.url, targetPath: args.targetPath, bare: true }) + yield* cloneUnlocked({ url, targetPath: args.targetPath, bare: true }) } // Complete initialization even if a previous holder died after cloning. yield* runGitCommand({ @@ -915,10 +922,11 @@ export const cloneBare = (args: { url: string; targetPath: string }) => withGitWriterLock({ path: args.targetPath, create: true }), Observability.withGitUrlSpan({ name: 'git/clone-bare', - label: args.url, - url: args.url, + label: url, + url, }), ) +} /** * Refresh a bare store repo from its remote. diff --git a/packages/@overeng/megarepo/src/core/git.unit.test.ts b/packages/@overeng/megarepo/src/core/git.unit.test.ts index d28ceee23b..e90da311fc 100644 --- a/packages/@overeng/megarepo/src/core/git.unit.test.ts +++ b/packages/@overeng/megarepo/src/core/git.unit.test.ts @@ -8,10 +8,33 @@ import { isTransientGitError, parseGitRemoteUrl, type ParsedGitRemote, + withoutHttpCredentials, worktreeRegistrationRef, } from './git.ts' describe('git', () => { + describe('withoutHttpCredentials', () => { + it.each([ + [ + 'https://fixture-user:fixture-password@example.com/owner/repo.git', + 'https://example.com/owner/repo.git', + ], + ['http://fixture-token@example.com/owner/repo.git', 'http://example.com/owner/repo.git'], + [ + 'HTTPS://fixture-user:p%40ss@example.com:8443/repo.git#main', + 'HTTPS://example.com:8443/repo.git#main', + ], + ['https://example.com/owner/repo.git', 'https://example.com/owner/repo.git'], + ['https://example.com/owner@ref/repo.git', 'https://example.com/owner@ref/repo.git'], + ['https://example.com/repo.git?ref=owner@ref', 'https://example.com/repo.git?ref=owner@ref'], + ['git@example.com:owner/repo.git', 'git@example.com:owner/repo.git'], + ['ssh://git@example.com/owner/repo.git', 'ssh://git@example.com/owner/repo.git'], + ['/tmp/local@source/repo', '/tmp/local@source/repo'], + ])('sanitizes only HTTP(S) authority userinfo: %s', (input, expected) => { + expect(withoutHttpCredentials(input)).toBe(expected) + }) + }) + it('derives budget refs from Git rather than the registered directory name', () => { const registration = { path: '/store/repository/.hidden/arbitrary', diff --git a/packages/@overeng/megarepo/src/core/lock.ts b/packages/@overeng/megarepo/src/core/lock.ts index 646a4960be..be3c8c68d5 100644 --- a/packages/@overeng/megarepo/src/core/lock.ts +++ b/packages/@overeng/megarepo/src/core/lock.ts @@ -18,6 +18,7 @@ import { type PlatformError } from 'effect/PlatformError' import type { AbsoluteFilePath } from '@overeng/effect-path' import { authorizeAuthorWorkspaceWrite } from '../store/store-path.ts' +import { MemberSourceString } from './config.ts' // ============================================================================= // Lock File Schema @@ -34,7 +35,7 @@ export const LOCK_FILE_NAME = 'megarepo.lock' */ export class LockedMember extends Schema.Class('LockedMember')({ /** Resolved URL (GitHub shorthand expanded) */ - url: Schema.String, + url: MemberSourceString, /** Original ref from config (for context) */ ref: Schema.String, @@ -63,6 +64,8 @@ export class LockFile extends Schema.Class('LockFile')({ // ============================================================================= // Lock File Operations // ============================================================================= +/** Keep reportInput disabled: enclosing schema issues otherwise retain credentialed inputs. */ +export const decodeLockFileContent = Schema.decodeEffect(Schema.fromJsonString(LockFile)) /** * Read and parse a lock file @@ -83,7 +86,7 @@ export const readLockFile = ( } const content = yield* fs.readFileString(lockPath) - const parsed = yield* Schema.decodeEffect(Schema.fromJsonString(LockFile))(content) + const parsed = yield* decodeLockFileContent(content) return Option.some(parsed) }) diff --git a/packages/@overeng/megarepo/src/core/lock.unit.test.ts b/packages/@overeng/megarepo/src/core/lock.unit.test.ts index 8eb09e4605..d34ac35b50 100644 --- a/packages/@overeng/megarepo/src/core/lock.unit.test.ts +++ b/packages/@overeng/megarepo/src/core/lock.unit.test.ts @@ -1,9 +1,10 @@ -import { Option, Schema } from 'effect' +import { Effect, Option, Schema } from 'effect' import { describe, expect, it } from 'vitest' import { checkLockStaleness, createEmptyLockFile, + decodeLockFileContent, createLockedMember, getLockedMember, hasMember, @@ -22,6 +23,27 @@ import { describe('lock', () => { describe('LockFile schema', () => { + it('rejects credentialed legacy locks without retaining the secret', () => { + const content = JSON.stringify({ + version: 1, + members: { + repo: { + url: 'https://fixture-secret@example.com/org/repo.git', + ref: 'main', + commit: 'abc123', + pinned: false, + lockedAt: '2024-01-15T10:30:00Z', + }, + }, + }) + const effect = decodeLockFileContent(content) + const cause = Effect.runSyncExit(effect) + const error = Effect.runSync(effect.pipe(Effect.flip)) + expect(error.message).toContain('https://***@example.com/org/repo.git') + expect(error.message).toContain('Fix the lock or config') + expect(JSON.stringify({ cause, issue: error.issue })).not.toContain('fixture-secret') + }) + it('should decode valid lock file', () => { const input = { version: 1, diff --git a/packages/@overeng/megarepo/src/sync/member.ts b/packages/@overeng/megarepo/src/sync/member.ts index 4765fa9cdf..24316ac6e6 100644 --- a/packages/@overeng/megarepo/src/sync/member.ts +++ b/packages/@overeng/megarepo/src/sync/member.ts @@ -17,6 +17,8 @@ import { getSourceRef, type MemberSource, parseSourceString, + redactSourceCredentials, + sourceCredentialError, validateMemberName, } from '../core/config.ts' import * as Git from '../core/git.ts' @@ -425,13 +427,14 @@ export const syncMember = ({ } satisfies MemberSyncResult } - // Parse the source string const source = parseSourceString(sourceString) if (source === undefined) { return { name, status: 'error', - message: `Invalid source string: ${sourceString}`, + message: + sourceCredentialError(sourceString) ?? + `Invalid source string: ${redactSourceCredentials(sourceString)}`, } satisfies MemberSyncResult } @@ -1281,5 +1284,8 @@ export const syncMember = ({ } satisfies MemberSyncResult }) }), - Observability.withSyncMemberSpan({ name, source: sourceString }), + Observability.withSyncMemberSpan({ + name, + source: redactSourceCredentials(sourceString), + }), )