fix(git-sync): address PR #119 review #2 — throttle /git Basic auth, fix mcp schema drift + warnings/tests
Must-fix:
- Throttle the raw /git HTTP-Basic path: it bypasses Nest/ThrottlerGuard, so
verifyUserCredentials (bcrypt) ran unthrottled. Wrap it in the SAME
FailedLoginLimiter the /mcp path uses (5/60s; per-IP, per-IP+email, global
per-email keys; atomic tryReserve BEFORE bcrypt; success resets, non-credential
errors release). The (threshold+1)-th attempt now gets 429 pre-bcrypt. Sweep
timer + onModuleDestroy mirror McpService.
- Fix the mcp schema mirror drift: packages/mcp details `open` attr now reads via
hasAttribute (matches editor-ext canon + git-sync copy); getAttribute dropped a
bare `<details open>` state. (build/ is gitignored — rebuilt locally.)
Tests added:
- /git brute-force throttle: pre-bcrypt 429 on the 6th failure; success resets;
non-credential error releases the budget.
- git-http-backend lost-lock AbortSignal: already-aborted -> no spawn + 500;
live abort mid-request -> SIGTERM + response closed.
- orchestrator divergentDocmost -> WARN + flag surfaced in status (+ clean case).
- pollTick re-entrancy guard skips an overlapping tick.
- datasource NotFound early-throws (getPageJson/move/rename) + updatedAt:undefined
stale-read branch (importPageMarkdown/createPage).
Suggestions:
- space.repo updateGitSyncSettings: parameterize the jsonb key (`${prefKey}::text`)
instead of sql.raw (latent-injection footgun); value stays sql.lit. Spec updated.
- pollTick re-entrancy guard (private `polling` flag).
- page-change.listener docstring: honest about the move/rename/delete over-skip
(loop-guard keys only on lastUpdatedSource) -> ~poll-interval latency, not loss.
- AGENTS.md: document the root /git smart-HTTP route + GitSyncModule.
- Remove redundant redteam-provenance.spec.ts (covered e2e in
persistence.extension.spec.ts:145).
- Extract the duplicated SIGTERM->SIGKILL+finish block (watchdog + abort) into
terminateChild; centralize watchdog-timer teardown in done().
Architecture (deferred, documented): mcp schema header now carries the three-copy
keep-in-sync + schema-core note; the editor-ext contract test documents that the
mcp copy and attribute-behaviour drift (details `open`) are not mechanically
covered yet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -13,6 +13,7 @@ import {
|
||||
NotFoundException,
|
||||
UnauthorizedException,
|
||||
} from '@nestjs/common';
|
||||
import { CREDENTIALS_MISMATCH_MESSAGE } from '../../../core/auth/auth.constants';
|
||||
import {
|
||||
SpaceCaslAction,
|
||||
SpaceCaslSubject,
|
||||
@@ -488,4 +489,103 @@ describe('GitHttpService.handle', () => {
|
||||
expect(state.headers['WWW-Authenticate']).toBe('Basic realm="gitmost"');
|
||||
expect(built.backend.run).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// --- brute-force throttle (must-fix #1, mirrors the /mcp Basic limiter) -----
|
||||
describe('HTTP-Basic brute-force throttle', () => {
|
||||
/** A request with wrong credentials for the given email. */
|
||||
const wrongCredReq = (email = 'dev@example.com') =>
|
||||
fakeRequest({
|
||||
url: '/git/space-1.git/info/refs?service=git-upload-pack',
|
||||
method: 'GET',
|
||||
authorization: basic(email, 'wrong'),
|
||||
});
|
||||
|
||||
it('rejects the (threshold+1)-th failed attempt with 429 BEFORE bcrypt', async () => {
|
||||
const built = build();
|
||||
// Realistic credential failure: verifyUserCredentials throws the SAME
|
||||
// UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE) production throws, so
|
||||
// isCredentialsFailure matches and the reservation is KEPT (counted).
|
||||
built.authService.verifyUserCredentials.mockRejectedValue(
|
||||
new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE),
|
||||
);
|
||||
|
||||
// 5 failed attempts (threshold = 5): each runs the credential check -> 401.
|
||||
for (let i = 0; i < 5; i++) {
|
||||
const { reply, state } = fakeReply();
|
||||
await built.service.handle(wrongCredReq(), reply);
|
||||
expect(state.statusCode).toBe(401);
|
||||
}
|
||||
expect(built.authService.verifyUserCredentials).toHaveBeenCalledTimes(5);
|
||||
|
||||
// The 6th attempt is throttled: 429, Retry-After, and bcrypt is NOT run.
|
||||
const { reply, state } = fakeReply();
|
||||
await built.service.handle(wrongCredReq(), reply);
|
||||
expect(state.statusCode).toBe(429);
|
||||
expect(state.headers['Retry-After']).toBe('60');
|
||||
expect(state.headers['WWW-Authenticate']).toBe('Basic realm="gitmost"');
|
||||
// Still 5 — the 6th never reached verifyUserCredentials (pre-bcrypt reject).
|
||||
expect(built.authService.verifyUserCredentials).toHaveBeenCalledTimes(5);
|
||||
expect(built.backend.run).not.toHaveBeenCalled();
|
||||
|
||||
built.service.onModuleDestroy();
|
||||
});
|
||||
|
||||
it('a successful auth resets the limiter so later attempts are not throttled', async () => {
|
||||
const built = build();
|
||||
const verify = built.authService.verifyUserCredentials;
|
||||
// First 4 attempts fail (credential mismatch), then one SUCCEEDS.
|
||||
verify
|
||||
.mockRejectedValueOnce(new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE))
|
||||
.mockRejectedValueOnce(new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE))
|
||||
.mockRejectedValueOnce(new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE))
|
||||
.mockRejectedValueOnce(new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE))
|
||||
.mockResolvedValueOnce({ id: 'user-1', email: 'dev@example.com' });
|
||||
|
||||
for (let i = 0; i < 4; i++) {
|
||||
const { reply } = fakeReply();
|
||||
await built.service.handle(wrongCredReq(), reply);
|
||||
}
|
||||
// 5th attempt succeeds -> proceeds (not throttled) and clears the budget.
|
||||
const okReply = fakeReply();
|
||||
await built.service.handle(
|
||||
fakeRequest({
|
||||
url: '/git/space-1.git/info/refs?service=git-upload-pack',
|
||||
method: 'GET',
|
||||
authorization: basic('dev@example.com', 'right'),
|
||||
}),
|
||||
okReply.reply,
|
||||
);
|
||||
expect(okReply.state.hijacked).toBe(true); // proceeded to the backend
|
||||
|
||||
// After the reset, a fresh wrong attempt is evaluated (401), NOT a 429 —
|
||||
// proving the per-IP/per-IP+email budget was cleared by the success.
|
||||
verify.mockRejectedValueOnce(
|
||||
new UnauthorizedException(CREDENTIALS_MISMATCH_MESSAGE),
|
||||
);
|
||||
const { reply, state } = fakeReply();
|
||||
await built.service.handle(wrongCredReq(), reply);
|
||||
expect(state.statusCode).toBe(401);
|
||||
|
||||
built.service.onModuleDestroy();
|
||||
});
|
||||
|
||||
it('a non-credential error releases the reservation (does not burn the budget)', async () => {
|
||||
const built = build();
|
||||
// A DB error (not a credentials mismatch) must NOT count toward the limiter.
|
||||
built.authService.verifyUserCredentials.mockRejectedValue(
|
||||
new Error('db down'),
|
||||
);
|
||||
|
||||
// 10 such failures — far beyond the threshold — must all be 401, never 429,
|
||||
// because each releases its reservation.
|
||||
for (let i = 0; i < 10; i++) {
|
||||
const { reply, state } = fakeReply();
|
||||
await built.service.handle(wrongCredReq(), reply);
|
||||
expect(state.statusCode).toBe(401);
|
||||
}
|
||||
expect(built.authService.verifyUserCredentials).toHaveBeenCalledTimes(10);
|
||||
|
||||
built.service.onModuleDestroy();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user