-
Notifications
You must be signed in to change notification settings - Fork 7
RU-T50 Bug and Security fixes, permission fix #256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| const mockRequestInterceptorUse = jest.fn(); | ||
| const mockResponseInterceptorUse = jest.fn(); | ||
| const mockLoggerWarn = jest.fn(); | ||
| const mockGetAuthState = jest.fn(); | ||
| const mockAxiosInstance = Object.assign(jest.fn(), { | ||
| defaults: { | ||
| headers: { | ||
| common: {}, | ||
| }, | ||
| }, | ||
| interceptors: { | ||
| request: { | ||
| use: mockRequestInterceptorUse, | ||
| }, | ||
| response: { | ||
| use: mockResponseInterceptorUse, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| jest.mock('axios', () => ({ | ||
| __esModule: true, | ||
| default: { | ||
| create: jest.fn(() => mockAxiosInstance), | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('@/lib/logging', () => ({ | ||
| logger: { | ||
| warn: mockLoggerWarn, | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('@/lib/storage/app', () => ({ | ||
| getBaseApiUrl: jest.fn(() => 'https://example.test'), | ||
| })); | ||
|
|
||
| jest.mock('@/stores/auth/store', () => ({ | ||
| __esModule: true, | ||
| default: { | ||
| getState: mockGetAuthState, | ||
| }, | ||
| })); | ||
|
|
||
| let rejectResponse: (error: unknown) => Promise<unknown>; | ||
|
|
||
| describe('API client token refresh logging', () => { | ||
| beforeAll(() => { | ||
| jest.isolateModules(() => { | ||
| require('@/api/common/client'); | ||
| }); | ||
| rejectResponse = mockResponseInterceptorUse.mock.calls[0]?.[1] as (error: unknown) => Promise<unknown>; | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('logs the token refresh operation and original request trace ID when refresh fails', async () => { | ||
| const refreshError = new Error('Token refresh failed'); | ||
| const refreshAccessToken = jest.fn().mockRejectedValue(refreshError); | ||
| const getHeader = jest.fn((name: string) => (name === 'x-trace-id' ? 'trace-123' : undefined)); | ||
|
|
||
| mockGetAuthState.mockReturnValue({ | ||
| refreshAccessToken, | ||
| refreshToken: 'refresh-token', | ||
| }); | ||
|
|
||
| const requestError = { | ||
| config: { | ||
| headers: { | ||
| get: getHeader, | ||
| }, | ||
| }, | ||
| response: { | ||
| status: 401, | ||
| }, | ||
| }; | ||
|
|
||
| await expect(rejectResponse(requestError)).rejects.toBe(refreshError); | ||
|
|
||
| expect(mockLoggerWarn).toHaveBeenCalledWith({ | ||
| message: 'Request failed after token refresh attempt', | ||
| operation: 'token_refresh', | ||
| trace_id: 'trace-123', | ||
| context: { error: refreshError }, | ||
| }); | ||
| expect(getHeader).toHaveBeenCalledWith('x-trace-id'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,5 @@ | ||
| import axios, { type AxiosError, type AxiosInstance, type InternalAxiosRequestConfig } from 'axios'; | ||
|
|
||
| import { refreshTokenRequest } from '@/lib/auth/api'; | ||
| import { isRefreshCredentialRejection } from '@/lib/auth/token-refresh'; | ||
| import { logger } from '@/lib/logging'; | ||
| import { getBaseApiUrl } from '@/lib/storage/app'; | ||
| import useAuthStore from '@/stores/auth/store'; | ||
|
|
@@ -61,6 +59,10 @@ axiosInstance.interceptors.response.use( | |
| } | ||
| // Handle 401 errors | ||
| if (error.response?.status === 401 && !(originalRequest as InternalAxiosRequestConfig & { _retry?: boolean })._retry) { | ||
| // Mark as retried immediately — also covers requests queued while a | ||
| // refresh is in flight, so a second 401 never triggers another refresh. | ||
| (originalRequest as InternalAxiosRequestConfig & { _retry: boolean })._retry = true; | ||
|
|
||
| if (isRefreshing) { | ||
| // If refreshing, queue the request | ||
| return new Promise((resolve, reject) => { | ||
|
|
@@ -74,26 +76,28 @@ axiosInstance.interceptors.response.use( | |
| }); | ||
| } | ||
|
|
||
| // Add _retry property to request config type | ||
| (originalRequest as InternalAxiosRequestConfig & { _retry: boolean })._retry = true; | ||
| isRefreshing = true; | ||
|
|
||
| try { | ||
| const refreshToken = useAuthStore.getState().refreshToken; | ||
| if (!refreshToken) { | ||
| if (!useAuthStore.getState().refreshToken) { | ||
| throw new Error('No refresh token available'); | ||
| } | ||
|
|
||
| const response = await refreshTokenRequest(refreshToken); | ||
| const { access_token, refresh_token: newRefreshToken } = response; | ||
| // Delegate to the auth store's single-flight refresh. Concurrent 401s, | ||
| // the proactive refresh timer and SignalR reconnects all share one | ||
| // request, so server-side refresh-token rotation never invalidates a | ||
| // parallel caller. The store also reschedules the proactive timer and | ||
| // performs the full logout + data wipe when the server rejects the | ||
| // refresh token. | ||
| const refreshed = await useAuthStore.getState().refreshAccessToken(); | ||
| if (!refreshed) { | ||
| throw new Error('Token refresh failed'); | ||
| } | ||
|
|
||
| // Update tokens in store | ||
| useAuthStore.setState({ | ||
| accessToken: access_token, | ||
| refreshToken: newRefreshToken, | ||
| status: 'signedIn', | ||
| error: null, | ||
| }); | ||
| const access_token = useAuthStore.getState().accessToken; | ||
| if (!access_token) { | ||
| throw new Error('No access token available after refresh'); | ||
| } | ||
|
|
||
| // Update Authorization header | ||
| axiosInstance.defaults.headers.common.Authorization = `Bearer ${access_token}`; | ||
|
|
@@ -104,22 +108,15 @@ axiosInstance.interceptors.response.use( | |
| } catch (refreshError) { | ||
| processQueue(refreshError as Error); | ||
|
|
||
| // Only log the user out when the token endpoint explicitly rejected the | ||
| // refresh token (400 invalid_grant / 401). Transient failures — no response | ||
| // (offline/timeout), 5xx server errors, or 429 — must preserve the session | ||
| // and retry, otherwise a backend incident logs out every active responder. | ||
| if (isRefreshCredentialRejection(refreshError)) { | ||
| logger.warn({ | ||
| message: 'Token refresh rejected by server (invalid/expired credentials), logging out user', | ||
| context: { error: refreshError }, | ||
| }); | ||
| useAuthStore.getState().logout(); | ||
| } else { | ||
| logger.warn({ | ||
| message: 'Token refresh failed transiently, preserving session for retry', | ||
| context: { error: refreshError }, | ||
| }); | ||
| } | ||
| // The store has already classified the failure: a credential rejection | ||
| // (400/401 from the token endpoint) triggered logout there; anything | ||
| // else is transient and the session is preserved for a later retry. | ||
| logger.warn({ | ||
| message: 'Request failed after token refresh attempt', | ||
| operation: 'token_refresh', | ||
| trace_id: originalRequest.headers.get('x-trace-id')?.toString(), | ||
| context: { error: refreshError }, | ||
| }); | ||
|
Comment on lines
+114
to
+119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Token-refresh failure log lacks the operation name and correlation identifiers required by Rule [3] for cross-log/SIEM correlation. Add structured fields for the operation and a correlation identifier, e.g., Kody rule violation: Include error context in structured logs Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| return Promise.reject(refreshError); | ||
| } finally { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { cacheManager } from '@/lib/cache/cache-manager'; | ||
| import { type NoteCategoryResult } from '@/models/v4/notes/noteCategoryResult'; | ||
| import { type NoteResult } from '@/models/v4/notes/noteResult'; | ||
| import { type NotesResult } from '@/models/v4/notes/notesResult'; | ||
|
|
@@ -62,5 +63,8 @@ export const saveNote = async (data: SaveNoteInput) => { | |
| const response = await saveNoteApi.post<SaveNoteResult>({ | ||
| ...data, | ||
| }); | ||
| // The notes list is cached for 2 days — invalidate so the new/updated note | ||
| // is visible immediately instead of after cache expiry. | ||
| cacheManager.remove('/Notes/GetAllNotes'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inline cache key literal — '/Notes/GetAllNotes' is duplicated knowledge shared with the cache write site, so a typo in either location silently breaks cache invalidation. Define a shared constant (e.g., Kody rule violation: Centralize string constants Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| return response.data; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unguarded external OS call — shell.openExternal(url) is invoked without a try/catch wrapper, leaving missing-browser and permission-denied failures unhandled. Wrap the call in try/catch with context and log the url and err on failure (e.g.,
try { await require('electron').shell.openExternal(url); } catch (err) { console.error('openExternal failed', { url, err }); }).Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.