Skip to content

Commit 5dffc13

Browse files
committed
server: fail closed when Apple disabled status cannot be verified
When resolving an Apple user without email (subsequent logins), if both Firebase getUserByProviderUid and getUserByEmail throw (e.g., Firebase is temporarily unreachable), reject the login rather than allowing it through with an unverified disabled status. Previously, transient Firebase failures would leave isDisabled=false and allow potentially disabled accounts to log in.
1 parent a9d8760 commit 5dffc13

2 files changed

Lines changed: 59 additions & 1 deletion

File tree

src/server/auth/oauth-handlers.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,22 +175,32 @@ async function resolveAppleUserWithoutEmail(
175175
// cross-provider collisions
176176
const existingUser = await users.findOneByScan({ providerUserId: appleSub, provider: 'apple' });
177177
if (existingUser) {
178+
// Fail closed: if we can't verify the account isn't disabled (e.g.
179+
// Firebase is temporarily unreachable), reject the login rather than
180+
// risk letting a disabled account through.
181+
let statusVerified = false;
178182
let isDisabled = false;
179183
try {
180184
const fbUser = await firebaseAdmin.getUserByProviderUid('apple.com', appleSub);
181185
isDisabled = fbUser?.disabled ?? false;
186+
statusVerified = true;
182187
} catch {
183188
// If provider lookup fails, fallback to email lookup
184189
if (existingUser.getEmail()) {
185190
try {
186191
const fbUser = await firebaseAdmin.getUserByEmail(existingUser.getEmail());
187192
isDisabled = fbUser?.disabled ?? false;
193+
statusVerified = true;
188194
} catch {
189-
// If neither lookup works, proceed with login
195+
// Neither lookup succeeded
190196
}
191197
}
192198
}
193199

200+
if (!statusVerified) {
201+
logger.error(`Cannot verify disabled status for Apple user ${appleSub}, rejecting login`);
202+
return { status: 'disabled' };
203+
}
194204
if (isDisabled) {
195205
return { status: 'disabled' };
196206
}

src/server/tests/oauth-handlers.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -769,6 +769,54 @@ describe('createAppleOAuthCallbackHandler', () => {
769769
expect(req.login).not.toHaveBeenCalled();
770770
});
771771

772+
it('should reject login when Firebase disabled status cannot be verified (fail-closed)', async () => {
773+
const deps = createAppleMockDeps();
774+
const handler = createAppleOAuthCallbackHandler(deps);
775+
776+
(deps.stateStore as jest.Mocked<OAuthStateStore>).validate.mockResolvedValue({
777+
valid: true,
778+
returnUrl: '/projects/test',
779+
});
780+
(deps.stateStore as jest.Mocked<OAuthStateStore>).invalidate.mockResolvedValue();
781+
782+
(exchangeAppleCode as jest.Mock).mockResolvedValue({
783+
access_token: 'test-access-token',
784+
id_token: 'test-id-token',
785+
expires_in: 3600,
786+
token_type: 'Bearer',
787+
});
788+
789+
(verifyAppleIdToken as jest.Mock).mockResolvedValue({
790+
sub: 'apple-user-unverifiable',
791+
});
792+
793+
const existingUser = new User();
794+
existingUser.setId('user-unverifiable');
795+
existingUser.setEmail('user@example.com');
796+
existingUser.setProvider('apple');
797+
existingUser.setProviderUserId('apple-user-unverifiable');
798+
799+
(deps.users as jest.Mocked<Table<User>>).findOneByScan.mockResolvedValue(existingUser);
800+
801+
// Both Firebase lookups fail (e.g., Firebase is temporarily unavailable)
802+
(deps.firebaseAdmin as jest.Mocked<admin.auth.Auth>).getUserByProviderUid.mockRejectedValue(
803+
new Error('Service unavailable'),
804+
);
805+
(deps.firebaseAdmin as jest.Mocked<admin.auth.Auth>).getUserByEmail.mockRejectedValue(
806+
new Error('Service unavailable'),
807+
);
808+
809+
const req = createMockRequest({}, { code: 'test-code', state: 'valid-state' });
810+
const { res, getRedirectUrl } = createMockResponse();
811+
812+
await handler(req as Request, res as Response, jest.fn());
813+
814+
// Should reject login when disabled status cannot be verified
815+
// (returns account_disabled since we fail closed)
816+
expect(getRedirectUrl()).toBe('/?error=account_disabled');
817+
expect(req.login).not.toHaveBeenCalled();
818+
});
819+
772820
it('should login existing user by providerUserId when Apple omits email', async () => {
773821
const deps = createAppleMockDeps();
774822
const handler = createAppleOAuthCallbackHandler(deps);

0 commit comments

Comments
 (0)