fix(git-sync): address PR #119 review — close 403/404 space-existence leak + warnings/tests/arch
Security (must-fix):
- /git smart-HTTP gate: an authenticated NON-member of a git-sync space now gets
404 (not 403), so the 403<->404 difference can no longer be used to brute-force
which spaces exist / have git-sync enabled. 403 is reserved for a MEMBER who
lacks the required role (existence already known). New gate input
userIsSpaceMember; decision-table + service specs extended.
Config (must-fix):
- Remove the dead GIT_SYNC_SSH_KEY_PATH knob (getter + validation field + two
.env.example lines) — it had zero consumers and advertised a nonexistent push
capability.
Stability/docs (warnings):
- Wire the lost-lock AbortSignal into runReceivePack -> git http-backend so the
receive-pack child is killed if the per-space lock lapses mid-write.
- Raise the divergent-`docmost` (invariant §5) push refusal from info -> warn and
surface divergentDocmost in the run status (/status).
- Comment the stale read-after-debounced-collab-write updatedAt in
importPageMarkdown (deferred §10 loop-guard must not trust it).
- Fix the Dockerfile comment: the loader uses require.resolve + dynamic import(),
it deliberately does NOT require('@docmost/git-sync').
- Merge the two near-identical space toggle handlers into one parameterized
handler; add the 2 missing en-US i18n keys for the auto-merge switch (ru-RU not
maintained for these git-sync strings, mirrored).
Tests:
- isGitSyncHttpEnabled() default-branch (unset -> isGitSyncEnabled fallback).
- agentSourceFields 'git-sync' case (source stamped, chat key omitted).
- editor-ext name-level schema contract (vendored mirror superset of editor-ext
node/mark types) + the new shared resolver + non-member 404 gate cases.
Architecture:
- Extract resolveRequestWorkspace shared by DomainMiddleware + GitHttpService
(the two real self-hosted/cloud copies; McpService has no cloud branch).
- Document the in-process setInterval multi-replica limitation + BullMQ/fencing
future direction (deferred, not implemented).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -122,4 +122,54 @@ describe('EnvironmentService', () => {
|
||||
expect(withEnv('1').isGitSyncEnabled()).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// isGitSyncHttpEnabled is the master gate of the /git smart-HTTP trust boundary.
|
||||
// When GIT_SYNC_HTTP_ENABLED is UNSET it FALLS BACK to isGitSyncEnabled(); when
|
||||
// set it is honored verbatim ('true' -> on, anything else -> off). The fallback
|
||||
// (default) branch is what these tests pin.
|
||||
describe('isGitSyncHttpEnabled', () => {
|
||||
const withEnv = (values: Record<string, string | undefined>) =>
|
||||
new EnvironmentService({
|
||||
get: (key: string, fallback?: string) => values[key] ?? fallback,
|
||||
} as any);
|
||||
|
||||
it('DEFAULT branch: unset -> falls back to isGitSyncEnabled() === true', () => {
|
||||
expect(
|
||||
withEnv({ GIT_SYNC_ENABLED: 'true' }).isGitSyncHttpEnabled(),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('DEFAULT branch: unset -> falls back to isGitSyncEnabled() === false', () => {
|
||||
// Neither key set: the fallback resolves to isGitSyncEnabled() which is
|
||||
// false by default.
|
||||
expect(withEnv({}).isGitSyncHttpEnabled()).toBe(false);
|
||||
expect(
|
||||
withEnv({ GIT_SYNC_ENABLED: 'false' }).isGitSyncHttpEnabled(),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it('explicit "true" enables the host regardless of GIT_SYNC_ENABLED', () => {
|
||||
expect(
|
||||
withEnv({
|
||||
GIT_SYNC_HTTP_ENABLED: 'true',
|
||||
GIT_SYNC_ENABLED: 'false',
|
||||
}).isGitSyncHttpEnabled(),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('explicit non-"true" disables the host even when sync is enabled', () => {
|
||||
expect(
|
||||
withEnv({
|
||||
GIT_SYNC_HTTP_ENABLED: 'false',
|
||||
GIT_SYNC_ENABLED: 'true',
|
||||
}).isGitSyncHttpEnabled(),
|
||||
).toBe(false);
|
||||
expect(
|
||||
withEnv({
|
||||
GIT_SYNC_HTTP_ENABLED: 'maybe',
|
||||
GIT_SYNC_ENABLED: 'true',
|
||||
}).isGitSyncHttpEnabled(),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -419,9 +419,4 @@ export class EnvironmentService {
|
||||
getGitSyncServiceUserId(): string | undefined {
|
||||
return this.configService.get<string>('GIT_SYNC_SERVICE_USER_ID');
|
||||
}
|
||||
|
||||
/** Optional path to the SSH key used for git remote access. */
|
||||
getGitSyncSshKeyPath(): string | undefined {
|
||||
return this.configService.get<string>('GIT_SYNC_SSH_KEY_PATH');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -216,10 +216,6 @@ export class EnvironmentVariables {
|
||||
@IsNotEmpty()
|
||||
@IsString()
|
||||
GIT_SYNC_SERVICE_USER_ID: string;
|
||||
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
GIT_SYNC_SSH_KEY_PATH: string;
|
||||
}
|
||||
|
||||
export function validate(config: Record<string, any>) {
|
||||
|
||||
Reference in New Issue
Block a user