fix: harden dashboard auth across SDK servers#408
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughDashboard authentication now separates page-load token bootstrapping from API credentials, protects readiness and metrics according to auth mode, and removes first-user OAuth admin assignment across SDKs. Documentation and tests reflect the updated behavior. ChangesDashboard authentication and authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DashboardServer
participant AuthStore
Client->>DashboardServer: Request dashboard probe
DashboardServer->>AuthStore: Check session when required
DashboardServer-->>Client: Return probe data or 401
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (2)
sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardAuthTest.java (1)
187-207: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise the bootstrapped cookie as an authenticated credential.
Assert the cookie is
HttpOnly, then replay itsname=valueon/api/statsand expect 200. The current assertion only verifies a cookie prefix, so a non-HttpOnly or unusable bootstrap cookie would pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardAuthTest.java` around lines 187 - 207, Update queryTokenOnlyBootstrapsPageLoads to capture the bootstrapped taskito_token cookie, assert its Set-Cookie value includes the HttpOnly attribute, then replay its name=value as the request Cookie on /api/stats and assert a 200 response. Preserve the existing redirect, token stripping, and wrong-token assertions.sdks/node/test/dashboard/auth.test.ts (1)
63-74: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify the bootstrap cookie’s security and usability.
Also require
HttpOnlyand send the returned cookie on an API request that succeeds. Checking only fortaskito_token=would miss a client-readable or malformed credential.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/test/dashboard/auth.test.ts` around lines 63 - 74, The page-load token bootstrap test should verify the returned taskito_token cookie includes the HttpOnly attribute, then reuse that cookie in a subsequent authenticated API request and assert the request succeeds. Update the test covering the valid ?token= flow while preserving the existing redirect and query-stripping assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/content/docs/java/guides/operations/dashboard.mdx`:
- Around line 189-193: Update the first-run public-route list in the dashboard
operations guide to remove `/readiness` and `/metrics`, leaving only routes that
remain public during session-auth setup; preserve the later authorization
behavior description and the `/health` entry.
- Around line 171-174: Update the dashboard authentication documentation near
the token redirect description to remove the claim that the redirect prevents
access-log exposure. State that it strips the token from subsequent browser
history/navigation and Referer propagation, and explicitly instruct operators to
redact query strings in server and proxy access logs.
In `@docs/content/docs/node/guides/operations/dashboard.mdx`:
- Around line 64-66: Update the dashboard authentication documentation around
the httpOnly-cookie redirect description to remove the claim that it prevents
access-log exposure. Limit the stated protection to browser history and later
Referer propagation, and advise redacting the token from query strings in access
logs.
In `@sdks/node/src/dashboard/server.ts`:
- Around line 117-121: Update the bootstrapped token-cookie flow in the GET
authentication branch to pass options.secureCookies !== false to setTokenCookie,
and extend the helper’s cookie attributes to append Secure unless that value is
explicitly disabled. Preserve plain-HTTP development through the explicit false
opt-out.
In `@sdks/node/test/dashboard/sessionAuth.test.ts`:
- Around line 240-251: Update the test around the metrics-token setup to capture
the existing TASKITO_DASHBOARD_METRICS_TOKEN value before overwriting it, then
restore that value in finally; only delete the environment variable when it was
originally unset.
---
Nitpick comments:
In
`@sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardAuthTest.java`:
- Around line 187-207: Update queryTokenOnlyBootstrapsPageLoads to capture the
bootstrapped taskito_token cookie, assert its Set-Cookie value includes the
HttpOnly attribute, then replay its name=value as the request Cookie on
/api/stats and assert a 200 response. Preserve the existing redirect, token
stripping, and wrong-token assertions.
In `@sdks/node/test/dashboard/auth.test.ts`:
- Around line 63-74: The page-load token bootstrap test should verify the
returned taskito_token cookie includes the HttpOnly attribute, then reuse that
cookie in a subsequent authenticated API request and assert the request
succeeds. Update the test covering the valid ?token= flow while preserving the
existing redirect and query-stripping assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aff26b7-429e-4155-86d0-28df152c1337
📒 Files selected for processing (27)
docs/content/docs/java/guides/operations/dashboard.mdxdocs/content/docs/java/guides/operations/sso.mdxdocs/content/docs/node/guides/operations/dashboard-api.mdxdocs/content/docs/node/guides/operations/dashboard.mdxdocs/content/docs/python/guides/dashboard/authentication.mdxdocs/content/docs/python/guides/dashboard/sso.mdxdocs/content/docs/python/guides/operations/security.mdxsdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/AuthStore.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/TokenAuth.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/oauth/OAuthFlow.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardAuthTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/auth/AuthStoreTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/oauth/OAuthFlowTest.javasdks/node/src/dashboard/auth/oauth/flow.tssdks/node/src/dashboard/auth/store.tssdks/node/src/dashboard/auth/tokenAuth.tssdks/node/src/dashboard/server.tssdks/node/test/dashboard/auth.test.tssdks/node/test/dashboard/oauth.test.tssdks/node/test/dashboard/oauthEndpoints.test.tssdks/node/test/dashboard/sessionAuth.test.tssdks/python/taskito/dashboard/auth.pysdks/python/taskito/dashboard/oauth/flow.pysdks/python/taskito/dashboard/server.pysdks/python/tests/dashboard/test_auth.py
1 participant
Summary
Audit of the dashboard servers confirmed every
/api/*route is already enforced server-side by a single pre-routing gate in each SDK (401 → session-bound CSRF → admin RBAC). This PR closes the gaps that sat outside that gate, keeping behaviour identical across all three servers.Probe endpoints no longer bypass auth
/metricsand/readinesswere reachable without credentials in every mode unlessTASKITO_DASHBOARD_METRICS_TOKENhappened to be set — exposing the Prometheus registry and worker/storage health on auth-enabled deployments. With auth enabled they now require either the metrics bearer token (scraper-friendly) or the mode's own credential (valid session, or shared token in legacy mode). Open mode is unchanged, and/healthstays public for liveness probes.Query-string token restricted to page-load bootstrap
Legacy token mode accepted
?token=on API calls, leaking the secret into access logs, browser history, and Referer headers. API requests now accept the token only via header or cookie; a valid?token=on a page load sets the httpOnly cookie once and 302-redirects with the token stripped from the URL.OAuth first-user-admin fallback removed
With no admin allowlist configured, the first verified-email OAuth login became admin — a first-login-wins race on misconfigured deployments. OAuth users now always get the viewer role unless listed in
TASKITO_DASHBOARD_OAUTH_ADMIN_EMAILS; admin access otherwise comes from the setup flow or env bootstrap. A startup warning fires when providers are configured without an allowlist.Tests
Docs
7 pages updated: probe gating semantics, scraper token guidance, OAuth role rules, token-mode auth sources.
Summary by CodeRabbit