Repository navigation
fix(api): require admin auth on /api/v1/logs (GHSA-m3qr-fvp4-78xj) - #506
Conversation
GET /api/v1/logs returned buffered application logs without authentication even when auth was enabled (the default). It was registered among the public base routes in Server.RegisterRoutes(), which runs before the global auth middleware is installed; Fiber matches routes in registration order and returns on first match before reaching a later app.Use middleware, so the route bypassed auth. Fix: register /api/v1/logs via Server.RegisterLogsRoute() AFTER the auth middleware, wrapped with auth.RequireAdmin when auth is enabled (unguarded when disabled, preserving no-auth deployments). Also whitelist the /api/v1/metrics prefix explicitly: those JSON metrics endpoints were likewise registered pre-middleware, but they expose only aggregate counters and runtime stats (equivalent to the public Prometheus /metrics surface) and are intentionally public — codifying that makes it an auditable decision rather than an accident of order. Exposure was operational log metadata only (component names, source locations, messages, request outcomes); Arc does not log query text, database/measurement names, or credentials to this buffer, and the first-run admin-token banner uses stderr, not the logged buffer. Regression test covers unauth (401), read-token (403), admin (200), public routes (200), and the auth-disabled path (200). Verified on the running binary. Reported by @sondt99.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request secures the GET /api/v1/logs endpoint by ensuring it requires an admin token when authentication is enabled, addressing a vulnerability where it was previously exposed publicly due to route registration order. The logs route registration is moved after the global authentication middleware, and a new test suite is added to verify the authentication requirements. The review feedback correctly identifies that the http.Response bodies in the new tests should be closed to prevent potential resource leaks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request secures the GET /api/v1/logs endpoint by requiring an admin token when authentication is enabled, resolving a vulnerability where logs were exposed publicly due to route registration order. It also explicitly whitelists /api/v1/metrics as public and adds corresponding regression tests. Feedback suggests utilizing the existing withAdminAuth helper function in RegisterLogsRoute to eliminate manual conditional checks and maintain consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…(Gemini round 1) - RegisterLogsRoute now uses the existing withAdminAuth(authManager) helper (RequireAdmin when non-nil, passthrough when nil) instead of hand-rolled if/else — same semantics, consistent with every other authed route registration. - Defer resp.Body.Close() on the app.Test responses in the logs-auth tests to avoid fd/memory leaks.
|
@gemini-code-assist Round-1 findings addressed: |
There was a problem hiding this comment.
Code Review
This pull request addresses a security vulnerability (GHSA-m3qr-fvp4-78xj) by ensuring that the /api/v1/logs endpoint is registered after the global authentication middleware, thereby requiring an admin token when authentication is enabled. It also explicitly whitelists /api/v1/metrics as public and adds comprehensive integration tests. The review feedback suggests removing the -1 timeout parameter from app.Test calls in the new tests to prevent potential infinite hangs in CI/CD pipelines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ound 2) app.Test(req, -1) disables the timeout; a hung handler/middleware would block CI indefinitely. Use the default 1s timeout (app.Test(req)) — the tests complete in milliseconds, so the timeout is pure safety net.
|
@gemini-code-assist Round-2 finding addressed: both |
There was a problem hiding this comment.
Code Review
This pull request secures the GET /api/v1/logs endpoint by requiring an admin token when authentication is enabled. Previously, the endpoint was registered among public base routes before the global authentication middleware, leaving it exposed. It is now registered after the authentication middleware, while /api/v1/metrics is explicitly whitelisted as public. Additionally, regression tests have been added to verify this behavior. There are no review comments to address, and we have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Addresses GHSA-m3qr-fvp4-78xj (private advisory). Ships in 26.06.2.
Summary
GET /api/v1/logsreturned buffered application logs without authentication even with auth enabled (the default). The route was registered inServer.RegisterRoutes()among the public base routes — which runs before the global auth middleware (app.Use) is installed. Fiber v2 matches routes in registration order and returns on first match before reaching a later middleware, so the route bypassed auth. Reproduced on a running binary (unauth 200 → after fix 401).Fix
/api/v1/logsfromRegisterRoutes; addedServer.RegisterLogsRoute(authManager), called frommain.goafterapp.Use(auth.NewMiddleware(...)). With auth enabled it wraps the handler inauth.RequireAdmin; with auth disabled (authManager == nil) it registers unguarded, preserving no-auth deployments./api/v1/metricsprefix. Those JSON metrics endpoints were also registered pre-middleware, but they expose only aggregate counters + Go runtime/host-arch stats (equivalent to the public Prometheus/metricssurface — no queries, DB/measurement names, paths, or credentials) and are intentionally public. Codifying it makes their public status an auditable decision rather than an accident of route ordering.Scope / severity
Exposure was operational log metadata only — component names, source locations, log messages, request outcomes. Arc does not write query text, database/measurement names, or credentials to this buffer, and the first-run admin-token banner uses stderr (not the logged buffer). Reconnaissance-grade info disclosure, not user-data or secret exposure.
Known limitation (recorded for the advisory, not changed here): the log buffer is global and
adminis a global permission, so in a multi-tenant deployment an admin sees all orgs' log lines. Also pre-existing: logs access isn't captured by the audit middleware (registered later in the Fiber stack).Test plan
go build -tags=duckdb_arrow ./cmd/... ./internal/...,go vet,gofmt -lcleango test ./internal/api/...— all passTestLogsEndpoint_RequiresAdmin(unauth 401 / read 403 / admin 200; health + metrics stay public) +TestLogsEndpoint_AuthDisabledStaysOpen(no-auth deploys unaffected); kill-switch verified/health+/api/v1/metrics→200, auth-disabled logs→200docs/logs-endpoint-admin-auth)Reported by @sondt99.
🤖 Generated with Claude Code