fix: SSE auth and stale data when switching remote nodes (#1708)#1714
fix: SSE auth and stale data when switching remote nodes (#1708)#1714
Conversation
Move SSE routes behind the standard auth middleware instead of maintaining parallel inline auth logic. Add QueryTokenMiddleware to bridge browser EventSource (which cannot set custom headers) with the Authorization header-based auth middleware. Stop forwarding the local JWT token in proxy URLs to remote nodes since node.ApplyAuth() already sets the correct remote credentials via the Authorization header.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request refactors authentication handling from per-handler logic to middleware, introducing QueryTokenMiddleware to convert token query parameters to Authorization headers. Concurrently, it introduces a new data caching pattern on the frontend via useLastValidData hook and adjusts polling logic across multiple components to account for SSE data availability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The review requires understanding interconnected architectural changes: migration of authentication from handler to middleware layer, removal of token from URL in favor of headers, introduction of a new frontend data caching pattern, and polling logic adjustments across multiple components. The heterogeneity of backend refactoring and frontend pattern introduction, combined with multiple touch points across components, demands careful verification of control flow changes and state management implications. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/service/frontend/auth/middleware.go (1)
47-53: Striptokenfrom the cloned query after promoting it toAuthorization.Right now the token is copied into the header but still left in
RawQuery. Clearing it reduces accidental propagation in downstream logs/middleware/proxies.Suggested patch
token := r.URL.Query().Get("token") if token != "" { r2 := r.Clone(r.Context()) + q := r2.URL.Query() + q.Del("token") + r2.URL.RawQuery = q.Encode() r2.Header.Set("Authorization", "Bearer "+token) next.ServeHTTP(w, r2) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/auth/middleware.go` around lines 47 - 53, The middleware copies the URL token into the Authorization header (token, r.Clone, r2.Header.Set("Authorization", ...)) but leaves it in the cloned request's RawQuery, risking accidental leakage; after promoting the token set Authorization on r2 and remove it from r2.URL.RawQuery (or clear the token param from r2.URL.Query and re-encode) before calling next.ServeHTTP so downstream handlers/middleware/proxies/logs don't see the token in the query string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/frontend/server.go`:
- Around line 1039-1043: The current branch in server.go that checks
authCfg.Mode == config.AuthModeBasic returns auth.Options{Realm: realm} which
leaves no auth methods enabled so middleware (auth.Options) treats requests as
unauthenticated; update that return to enable Basic auth by populating
auth.Options with the Basic auth method/validator (e.g., set the Methods/Basic
field or attach the Basic credential validator used elsewhere) so the middleware
enforces Basic authentication for authCfg.Mode == config.AuthModeBasic instead
of allowing a pass-through.
In `@ui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsModal.tsx`:
- Around line 41-45: The code references remoteNode when initializing
prevRemoteNodeRef (useRef(remoteNode)) before remoteNode is declared, causing a
TDZ crash; move the remoteNode declaration so it appears immediately after the
isVisible state initialization (so remoteNode exists before calling useRef) and
remove the later duplicate declaration around line 71, keeping the single
remoteNode definition and leaving prevRemoteNodeRef and previousDataRef logic
intact.
In `@ui/src/pages/dags/index.tsx`:
- Around line 314-315: The DAGTable is being fed from dagFiles (derived directly
from data) which can go empty during transient refetches even though the cached
displayData from useLastValidData(remoteNode) is available; update the DAGTable
prop so it uses displayData (or dagFiles derived from displayData) instead of
dagFiles derived from raw data, ensuring components like DAGTable render stale
data while fetching; look for useLastValidData, displayData, dagFiles, data,
remoteNode and change the source passed into DAGTable accordingly so stale
cached displayData is used during refetch.
---
Nitpick comments:
In `@internal/service/frontend/auth/middleware.go`:
- Around line 47-53: The middleware copies the URL token into the Authorization
header (token, r.Clone, r2.Header.Set("Authorization", ...)) but leaves it in
the cloned request's RawQuery, risking accidental leakage; after promoting the
token set Authorization on r2 and remove it from r2.URL.RawQuery (or clear the
token param from r2.URL.Query and re-encode) before calling next.ServeHTTP so
downstream handlers/middleware/proxies/logs don't see the token in the query
string.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
internal/service/frontend/auth/middleware.gointernal/service/frontend/auth/middleware_test.gointernal/service/frontend/server.gointernal/service/frontend/sse/handler.gointernal/service/frontend/sse/handler_test.gointernal/service/frontend/sse/proxy.gointernal/service/frontend/sse/proxy_test.goui/src/App.tsxui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsModal.tsxui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsPanel.tsxui/src/features/dags/components/dag-details/DAGDetailsModal.tsxui/src/features/dags/components/dag-details/DAGDetailsPanel.tsxui/src/features/dags/components/dag-editor/DAGSpec.tsxui/src/features/dags/components/dag-execution/DAGExecutionHistory.tsxui/src/hooks/useLastValidData.tsui/src/hooks/useSSE.tsui/src/pages/dag-runs/index.tsxui/src/pages/dags/dag/index.tsxui/src/pages/dags/index.tsxui/src/pages/docs/components/DocEditor.tsxui/src/pages/index.tsx
| // Basic auth mode: EventSource API cannot send Basic auth headers. | ||
| // Skip auth to match current behavior (pre-existing security gap). | ||
| if authCfg.Mode == config.AuthModeBasic { | ||
| return auth.Options{Realm: realm} | ||
| } |
There was a problem hiding this comment.
Basic mode currently bypasses auth for stream endpoints.
On Line 1042, returning only Realm means no auth methods are enabled. In internal/service/frontend/auth/middleware.go (Line 120), that causes a full pass-through.
Result: /api/v1/events/* and the agent stream route can be accessed without auth in basic mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/server.go` around lines 1039 - 1043, The current
branch in server.go that checks authCfg.Mode == config.AuthModeBasic returns
auth.Options{Realm: realm} which leaves no auth methods enabled so middleware
(auth.Options) treats requests as unauthenticated; update that return to enable
Basic auth by populating auth.Options with the Basic auth method/validator
(e.g., set the Methods/Basic field or attach the Basic credential validator used
elsewhere) so the middleware enforces Basic authentication for authCfg.Mode ==
config.AuthModeBasic instead of allowing a pass-through.
| const prevRemoteNodeRef = useRef(remoteNode); | ||
| if (prevRemoteNodeRef.current !== remoteNode) { | ||
| prevRemoteNodeRef.current = remoteNode; | ||
| previousDataRef.current = null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd '^DAGRunDetailsModal\.tsx$'Repository: dagu-org/dagu
Length of output: 131
🏁 Script executed:
cat -n ui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsModal.tsx | head -90Repository: dagu-org/dagu
Length of output: 3467
Fix TDZ crash: remoteNode must be declared before first use.
remoteNode is referenced on line 41 (via useRef(remoteNode)) before its declaration on line 71. This will throw a ReferenceError at runtime. Move the declaration to line 38, immediately after isVisible state initialization:
const [shouldRender, setShouldRender] = useState(isOpen);
const [isVisible, setIsVisible] = useState(false);
+ const remoteNode = appBarContext.selectedRemoteNode || 'local';
const previousDataRef = useRef<PreviousData | null>(null);
const prevRemoteNodeRef = useRef(remoteNode);
if (prevRemoteNodeRef.current !== remoteNode) {
prevRemoteNodeRef.current = remoteNode;
previousDataRef.current = null;
}Then remove the duplicate declaration from line 71.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const prevRemoteNodeRef = useRef(remoteNode); | |
| if (prevRemoteNodeRef.current !== remoteNode) { | |
| prevRemoteNodeRef.current = remoteNode; | |
| previousDataRef.current = null; | |
| } | |
| const [shouldRender, setShouldRender] = useState(isOpen); | |
| const [isVisible, setIsVisible] = useState(false); | |
| const remoteNode = appBarContext.selectedRemoteNode || 'local'; | |
| const previousDataRef = useRef<PreviousData | null>(null); | |
| const prevRemoteNodeRef = useRef(remoteNode); | |
| if (prevRemoteNodeRef.current !== remoteNode) { | |
| prevRemoteNodeRef.current = remoteNode; | |
| previousDataRef.current = null; | |
| } |
🧰 Tools
🪛 Biome (2.4.4)
[error] 41-41: This variable is used before its declaration.
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 42-42: This variable is used before its declaration.
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 43-43: This variable is used before its declaration.
(lint/correctness/noInvalidUseBeforeDeclaration)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsModal.tsx`
around lines 41 - 45, The code references remoteNode when initializing
prevRemoteNodeRef (useRef(remoteNode)) before remoteNode is declared, causing a
TDZ crash; move the remoteNode declaration so it appears immediately after the
isVisible state initialization (so remoteNode exists before calling useRef) and
remove the later duplicate declaration around line 71, keeping the single
remoteNode definition and leaving prevRemoteNodeRef and previousDataRef logic
intact.
| const displayData = useLastValidData(data ?? null, remoteNode); | ||
|
|
There was a problem hiding this comment.
Use cached displayData for table rows; current prop can still blank the list.
Line 329 still feeds DAGTable from dagFiles (derived from non-cached data), so during transient refetch states the table can render empty even when displayData is available.
💡 Proposed fix
- <DAGTable
- dags={isLoading && !displayData ? [] : dagFiles}
+ <DAGTable
+ dags={displayData.dags || []}
group={group}
refreshFn={refreshFn}
searchText={searchText}Based on learnings: Applies to ui/**/*.{ts,tsx} : NEVER use full-page loading overlays or LoadingIndicator components that hide content; instead show stale data while fetching updates.
Also applies to: 329-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/dags/index.tsx` around lines 314 - 315, The DAGTable is being
fed from dagFiles (derived directly from data) which can go empty during
transient refetches even though the cached displayData from
useLastValidData(remoteNode) is available; update the DAGTable prop so it uses
displayData (or dagFiles derived from displayData) instead of dagFiles derived
from raw data, ensuring components like DAGTable render stale data while
fetching; look for useLastValidData, displayData, dagFiles, data, remoteNode and
change the source passed into DAGTable accordingly so stale cached displayData
is used during refetch.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1714 +/- ##
==========================================
+ Coverage 69.38% 69.43% +0.04%
==========================================
Files 396 396
Lines 43713 43713
==========================================
+ Hits 30332 30350 +18
+ Misses 10924 10908 -16
+ Partials 2457 2455 -2 see 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Part of #1708
Summary by CodeRabbit
New Features
Bug Fixes
Improvements