Skip to content

feat!: OIDC device flow 인증으로 API key 인증 대체#1

Open
dlddu wants to merge 11 commits into
mainfrom
feat/oidc-pkce-browser-auth
Open

feat!: OIDC device flow 인증으로 API key 인증 대체#1
dlddu wants to merge 11 commits into
mainfrom
feat/oidc-pkce-browser-auth

Conversation

@dlddu

@dlddu dlddu commented May 27, 2026

Copy link
Copy Markdown

Summary

기존 REDASH_API_KEY 기반 인증을 제거하고, 외부 OIDC IdP가 RFC 8628 device flow로 발급한 짧은 액세스 토큰으로 Redash API에 접근하도록 변경합니다.

  • REDASH_API_KEY 환경변수 제거
  • ✅ Device authorization grant — Authentik이 verification_uri_complete + user_code를 발급, 사용자가 본인 브라우저 한 번 클릭으로 인증
  • 모든 API 호출에 Authorization: Bearer <jwt> 헤더 자동 주입
  • 신규 wait_for_oidc_login MCP tool — 어시스턴트가 device flow 완료를 await 후 원래 tool을 자동 재시도 (resume-after-auth UX)
  • 401 + refresh 실패 케이스도 동일 URL surfacing 경로로 자동 흡수 (mid-session 토큰 revoke 대응)

서버 측 토큰 검증은 Buzzvil/redash-custom#18, IdP 측 OAuth2 client + device code flow는 Buzzvil/buzz-k8s-resources#1802. datavil 플러그인 연동은 Buzzvil/buzzvil-plugins#101.

설계 결정 — 왜 device flow인가

MCP 서버는 Claude Desktop / Claude Code의 stdio 서브프로세스로 동작하고 종종 컨테이너 안에서 돕니다.

  • PKCE+loopback redirect는 컨테이너에서 동작 불가 — 서버의 127.0.0.1과 사용자 브라우저의 127.0.0.1이 다른 network namespace, 별도 port-forward 없이는 도달 불가
  • xdg-open도 컨테이너에서 미지원 — 서버측 브라우저 spawn 실패

Device flow는 outbound HTTPS만 필요하고, 사용자의 브라우저는 어떤 장비에 있어도 됩니다. Authentik의 verification_uri_complete이 user_code를 URL에 자동 임베드해주므로 single-click UX이고, PKCE 대비 UX 비용 차이도 거의 없습니다. 초기엔 PKCE/device 환경 분기를 시도했지만 (v0.1.0-pr.5~7), 복잡도 대비 이득이 작아 v0.1.0-pr.8에서 device-only로 단순화.

런타임 흐름

  1. MCP 서버 부팅 — 인증 체크 없이 stdio만 바인딩 (이전에는 부팅 시 인증 시도가 stdio 서브프로세스에서 invisible하게 일어나서 혼란 야기)
  2. 어시스턴트가 redash tool (예: list_queries) 호출
  3. dispatcher의 ensureValidTokens 게이트 → 캐시 미스 → device flow 시작 → verification_uri_complete + user_code 담은 AuthError throw
  4. tool response가 URL을 어시스턴트에게 surfacing (## For the user / ## For the assistant 두 섹션 명시)
  5. 어시스턴트가 같은 turn에: 사용자에게 URL 노출 + wait_for_oidc_login MCP tool 호출 (블로킹)
  6. 사용자가 브라우저에서 device authorization 완료 → 백그라운드 polling이 token 수령 → 캐시 저장
  7. wait_for_oidc_login resolve → 어시스턴트가 원래 tool 자동 재시도 → 결과 반환

사용자는 한 번 prompt하고 URL을 한 번 클릭하면 됩니다 — 별도 재요청 불필요.

CLI redash-mcp login 서브커맨드도 동일한 device flow를 사용하며, 추가로 open / start / xdg-open로 브라우저 자동 spawn 시도 (실패 시 stderr에 URL + user_code 출력).

변경 사항

파일 내용
신규 src/auth.ts OIDC 디스커버리, RFC 8628 device flow (requestDeviceCode + pollForDeviceToken + beginDeviceFlow), 토큰 캐시 atomic write + 0600, refresh_token grant, AuthError, makeLoginRequiredError 공유 헬퍼 (cold cache miss와 warm 401 retry 양쪽이 동일한 user/assistant 지시 메시지 사용)
src/cli.ts 서브커맨드 dispatcher (serve/login/logout/status/help). REDASH_OIDC_ISSUER+CLIENT_ID 필수. serve는 부팅 시 인증 체크 없음
src/redashClient.ts 생성자에서 Key <apiKey> 헤더 제거. axios request interceptor가 매 요청마다 getValidTokens()Bearer 주입. response interceptor가 401 1회 재시도 (forceRefresh), refresh 실패 시 fresh device flow URL 담은 AuthError throw. 모든 메서드 catch에 AuthError instance preserve 가드 (57 sites) — 이전엔 generic error로 wrap되어 URL 메시지 손실
src/index.ts CallTool dispatcher가 ensureValidTokens 게이트. 신규 wait_for_oidc_login MCP tool — 어시스턴트가 device flow URL을 사용자에게 노출한 직후 호출하면 polling resolve까지 블로킹 → 동일 turn에서 원래 tool 자동 재시도 가능
신규 src/__tests__/auth.test.ts loadOidcConfig, tokenCachePath, getValidTokens (캐시 적중/만료 갱신/issuer 불일치/refresh_token 부재), forceRefresh, performLogout (idempotent), readStatus — 13 케이스
src/__tests__/redashClient.test.ts API key 케이스 제거 → URL만 필수, interceptor 검증 (Bearer 주입, 401 재시도, 비-401 retry 안 함, Authorization extra-header override 거부)
src/__tests__/integration.test.ts env 요구사항 갱신
playwright.config.ts e2e가 REDASH_OIDC_* env / 캐시 경로 사용
README.md 인증 모델·환경 변수·서브커맨드·Claude Desktop 설정·마이그레이션 가이드 재작성. 호스트명 .dev 수정
package.json 0.0.13 → 0.1.0 (Breaking)

새 환경 변수

변수 필수 설명
REDASH_URL yes Redash 인스턴스 URL
REDASH_OIDC_ISSUER yes OIDC 발급자 URL (예: https://authentik.buzzvil.com/application/o/redash-cli/)
REDASH_OIDC_CLIENT_ID yes public OIDC client ID (예: redash-cli)
REDASH_OIDC_AUDIENCE no 기본값 = CLIENT_ID
REDASH_OIDC_SCOPES no 기본값 openid email offline_access
REDASH_OIDC_TOKEN_CACHE_PATH no 캐시 파일 경로 override
REDASH_API_KEY 제거됨

보안 모델

  • 토큰 캐시는 600 권한 + atomic write (refresh token이 민감)
  • 캐시에 issuer/client_id를 함께 저장 → 설정 변경 시 무효화하고 재로그인 요구
  • REDASH_EXTRA_HEADERSAuthorization을 덮어쓰지 못하도록 차단
  • 401 응답 시 forceRefresh 1회 시도, 실패 시 stale 캐시 clear + 새 device flow URL surfacing (무한 루프 방지)
  • device code 발급/소비는 단일 flight (pendingLogin slot) — 동시 호출이 같은 URL 공유

토큰 만료 처리

케이스 처리
캐시 access token 만료 + refresh_token valid getValidTokens이 refresh_token grant로 자동 갱신 (투명)
캐시 없음 / refresh_token 없음 ensureValidTokensmakeLoginRequiredError → device flow URL surfacing
Redash가 401 + refresh 가능 response interceptor → forceRefresh → 새 토큰으로 1회 재시도
Redash가 401 + refresh도 실패 response interceptor → performLogout + makeLoginRequiredError → 동일 URL surfacing

Test plan

  • npx tsc --noEmit clean
  • npm test 77/77 통과
  • CLI 서브커맨드 (help/status/logout/login) 동작 확인
  • 실 IdP (Buzzvil Authentik) 연동 redash-mcp login → 토큰 캐시 + refreshToken 발급 확인 (offline_access scope 부여 후)
  • MCP list_queries E2E (dev Redash에서 5개 쿼리 반환 성공)
  • wait_for_oidc_login resume-after-auth 흐름 동작 확인
  • 401 + refresh 실패 시 fresh device flow URL이 tool response로 surfacing 되는지 확인
  • e2e (npm run e2e:test) — 유효 토큰 캐시 준비 후
  • Authentik device_flow.token_validity 늘린 후 60초 timeout 회피 확인 (현재 default 60초)

GitHub Release prerelease 시퀀스 (anonymous npx 설치 가능)

GitHub Packages npm은 visibility public이어도 anonymous read 차단 → GitHub Release tarball URL을 npx 인자로 직접 박는 방식으로 우회. v0.1.0-pr.1 ~ v0.1.0-pr.10 단계별 UX 폴리싱, 최신 v0.1.0-pr.10이 production-ready 상태:

npx -y https://github.com/Buzzvil/redash-mcp/releases/download/v0.1.0-pr.10/buzzvil-redash-mcp-0.1.0-pr.10.tgz <subcommand>

마이그레이션 가이드

기존 사용자 (.env / Claude Desktop config):

- REDASH_API_KEY=xxx
+ REDASH_OIDC_ISSUER=https://authentik.buzzvil.com/application/o/redash-cli/
+ REDASH_OIDC_CLIENT_ID=redash-cli

CLI에서 미리 인증해두려면 (선택):

npx -y https://github.com/Buzzvil/redash-mcp/releases/download/v0.1.0-pr.10/buzzvil-redash-mcp-0.1.0-pr.10.tgz login

MCP 서버 안에서 첫 tool 호출 시 자동으로 device flow가 시작되므로, 별도 명시 호출은 필수 아닙니다.

관련 PR

🤖 Generated with Claude Code

장기 API 키 대신 외부 OIDC IdP (Authentik/Auth0/Keycloak 등) 에서 PKCE 로 발급받은
짧은 액세스 토큰으로 Redash API 를 호출하도록 전환.

설계 결정:
- MCP 서버는 Claude Desktop 의 stdio 서브프로세스로 동작하므로 직접 브라우저를
  열 수 없음. 대신 `redash-mcp login` 서브커맨드를 새로 만들고, 그 결과로 생성된
  토큰 캐시를 MCP 서버가 읽어 사용 (gcloud auth login / gh auth login 패턴).
- HS256 등 대칭 알고리즘은 redash-custom 측에서 이미 거부하므로 클라이언트는
  토큰 형식을 신경 쓰지 않음 — Bearer 헤더만 붙임.
- 만료 임박 시 silent refresh, 401 응답 시 한 번에 한해 강제 refresh 후 재시도.

변경:
- 신규 src/auth.ts: OIDC 디스커버리, PKCE 코드 verifier/challenge, loopback HTTP
  서버, 브라우저 자동 오픈 (외부 dep 없이 platform 별 spawn), 토큰 캐시 (0600,
  $XDG_STATE_HOME/redash-mcp/tokens.json), refresh 로직, AuthError.
- src/cli.ts: 서브커맨드 dispatcher (serve / login / logout / status / help).
  REDASH_API_KEY 환경변수 체크 제거, REDASH_OIDC_ISSUER + CLIENT_ID 필수화.
  serve 시작 전에 getValidTokens() 로 토큰 존재 확인 → 없으면 명확한 안내 후 종료.
- src/redashClient.ts: 생성자에서 Authorization 헤더 베이크 제거. axios request
  interceptor 가 매 요청마다 getValidTokens().accessToken 으로 Bearer 헤더 주입.
  response interceptor 가 401 1회 재시도 (forceRefresh).
- src/__tests__/auth.test.ts (신규): loadOidcConfig, tokenCachePath,
  getValidTokens (캐시 적중/만료 갱신/issuer 불일치/refresh_token 부재),
  forceRefresh, performLogout (idempotent), readStatus.
- src/__tests__/redashClient.test.ts: API key 관련 케이스 제거 → URL 만 필수,
  request/response interceptor 동작 검증 (Bearer 주입, 401 재시도).
- src/__tests__/integration.test.ts: env 요구사항 갱신.
- playwright.config.ts: e2e 가 API key 대신 OIDC env / 캐시 경로 사용하도록.
- README.md: 인증 모델·환경 변수·서브커맨드·Claude Desktop 설정·마이그레이션 가이드 재작성.
- package.json: 0.0.13 → 0.1.0 (Breaking).

마이그레이션:
- REDASH_API_KEY 제거
- REDASH_OIDC_ISSUER, REDASH_OIDC_CLIENT_ID 추가
- `redash-mcp login` 한 번 실행

77/77 테스트 통과, tsc --noEmit clean.

Refs: Buzzvil/redash-custom#18, Buzzvil/buzz-k8s-resources#1802, DO-2616

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

이 PR은 Redash MCP 서버의 인증 메커니즘을 API key 정적 기반에서 OIDC Device Authorization Grant 기반 단기 토큰으로 완전히 전환합니다. 새로운 auth.ts 모듈에서 환경변수 로딩, 토큰 캐시 관리, 디바이스 플로우, 토큰 갱신을 구현하고, RedashClient의 요청 인터셉터를 통해 매 요청마다 OIDC 액세스 토큰을 Bearer Authorization으로 주입합니다. CLI는 login, logout, status, serve 서브커맨드를 지원하도록 재구성되며, MCP 서버에는 wait_for_oidc_login 도구가 추가되어 브라우저 기반 인증 완료를 대기합니다. 모든 테스트 환경과 문서가 OIDC 구성으로 갱신되고 버전이 0.1.0으로 업데이트됩니다.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed PR 제목이 주요 변경사항인 OIDC device flow 인증으로의 전환을 명확하게 설명하고 있으며, API key 인증 대체라는 핵심 내용을 잘 전달합니다.
Description check ✅ Passed PR 설명이 변경사항의 모든 주요 측면(OIDC device flow, 환경변수, CLI 서브커맨드, 보안 모델, 토큰 만료 처리 등)을 상세히 설명하고 있으며, 변경사항 표와 마이그레이션 가이드도 포함되어 있습니다.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/oidc-pkce-browser-auth

Comment @coderabbitai help to get the list of available commands and usage tips.

dlddu and others added 10 commits May 27, 2026 03:07
README 예제의 REDASH_URL=redash-dev.buzzvil.com 은 오타. 실제는 .dev.
silent refresh 동작은 Authentik blueprint 측 offline_access scope 추가 후
정상화됨 (Buzzvil/buzz-k8s-resources#1802 의 fix 커밋 e5585c2e).

Refs: DO-2616, Buzzvil/buzz-k8s-resources#1802

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`redash-mcp serve` no longer requires a separate `redash-mcp login` step.
On cache miss / unrecoverable AuthError (no refresh_token, expired
refresh_token, etc.) the server transparently launches the OIDC PKCE
browser flow on the first tool call and blocks until the user completes
it.

- new `ensureValidTokens()` wraps `getValidTokens` and triggers
  `performLogin` on AuthError; concurrent callers share one in-flight
  login so the browser is only opened once.
- `runServer()` uses `ensureValidTokens` and exits only on non-auth
  fatal errors.
- 401 retry path in `redashClient.ts`: when `forceRefresh` fails with
  AuthError (revoked / no refresh_token), clear the stale cache via
  `performLogout` and call `ensureValidTokens` so the user is prompted
  to log in again instead of getting a generic 401 error.

This keeps the "no explicit login step" UX for plugin users at the cost
of a one-time browser popup on first use. Manual `redash-mcp login` is
still supported for headless / pre-warmed setups.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MCP server runs as a stdio subprocess of Claude Desktop / IDE, so any
browser popup launched during boot is invisible — the user has no way to
know what's happening, and the host UI just sits there spinning while
PKCE waits 5 minutes for a callback.

Drop the pre-flight `ensureValidTokens()` from runServer entirely. Auth
is already deferred to the request interceptor in redashClient.ts, which
runs in response to an explicit tool call — that's when the user is
already attending to the action and can sensibly interpret a browser
popup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rowser

The MCP server runs as a stdio subprocess of Claude Desktop / Claude Code
and is frequently deployed in containers (no xdg-open / no DISPLAY). The
previous "auto-launch browser on auth failure" path is invisible to the
user and broken in container environments.

New flow:
- `ensureValidTokens()` no longer spawns a browser. On cache miss it calls
  `startPendingLogin()` (loopback callback server only) and throws an
  AuthError whose message embeds the auth URL.
- The MCP CallTool dispatcher in index.ts now gates every tool call on
  `ensureValidTokens()`. When it throws AuthError, the dispatcher returns
  a tool response (isError: true) carrying the URL. The host UI renders
  this text, the user clicks the URL, completes PKCE in their browser,
  and the loopback server picks up the callback. The next tool call
  succeeds with the freshly cached token.
- `performLogin()` (CLI `redash-mcp login`) still auto-launches the
  browser for terminal users — factored through the shared
  `beginLoginFlow()` helper.
- redashClient interceptors simplified: request interceptor uses
  `getValidTokens` (cache-only), 401 retry path no longer tries to
  trigger interactive login.

Concurrent tool calls during a pending login share the same URL via a
module-level `pendingLogin` slot with a 5-minute TTL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PKCE+loopback flow assumes the user's browser can reach
http://127.0.0.1:<port>/callback on this process. That works on regular
desktops but breaks in containers (different network namespace, no port
forward) and headless Linux (no DISPLAY).

Add OAuth 2.0 Device Authorization Grant as an alternative flow that
needs only outbound HTTPS to the IdP — the user's browser can be on any
device. Both flows now coexist; selection rule:

  1. REDASH_OIDC_FLOW env var override (`pkce` | `device`)
  2. Auto: device flow if running in a container or headless Linux
     (KUBERNETES_SERVICE_HOST, `container`, /.dockerenv,
     /run/.containerenv, /proc/1/cgroup heuristic, or linux without
     DISPLAY/WAYLAND_DISPLAY). PKCE otherwise.

Implementation:
- `selectAuthFlow()` + `detectContainerLike()` decide per-process
- `beginDeviceFlow()` calls device_authorization_endpoint, returns
  verification_uri_complete + user_code + poll-driven completion promise
- `beginPkceFlow()` (renamed from `beginLoginFlow`) unchanged
- Unified `LoginFlowHandle` shape; `beginLoginFlow()` dispatches
- `performLogin` and `startPendingLogin` work with either flow
- `ensureValidTokens` includes the user_code in its tool-response message
  when the device flow is used (verification_uri_complete pre-fills it,
  but the fallback line covers IdPs that don't honor pre-fill)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When `selectAuthFlow()` picks PKCE (i.e. running on a non-container
desktop where the user's own machine has a reachable browser),
startPendingLogin now also spawns `open` / `start` / `xdg-open` so the
browser pops up in response to the tool call without forcing the user
to click the URL in the chat surface.

- Device flow path unchanged — the IdP page is meant to be opened on a
  separate device, and the server may not have a usable browser at all.
- Browser is launched only on the first attempt of a pending login;
  subsequent tool calls during the same in-flight flow reuse the
  existing pending state without spawning more browser windows.
- Tool response still includes the URL as a clickable fallback in case
  the auto-launch silently failed (e.g. xdg-open missing on Linux).
- Wording adjusts to "your browser should have opened automatically"
  vs "open this URL" depending on whether launch was attempted.
- Opt out via `REDASH_OIDC_AUTO_LAUNCH_BROWSER=false`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add `canLaunchBrowser()` pre-flight check that synchronously verifies the
platform-specific browser-opening command is reachable (`open` on macOS,
`cmd /c start` on Windows, `xdg-open` + display server on Linux/*BSD).
Wire it into `selectAuthFlow`: even on a non-container desktop, if we
can't actually launch a browser we switch to device flow instead of
serving the user a PKCE loopback URL their browser would have no path
to reach.

This covers the previously broken case:
  PKCE selected on desktop → browser launch fails (xdg-open missing /
  DISPLAY torn down / etc.) → user given a useless 127.0.0.1 redirect URL.

After this change, that case is handled by surfacing the device flow's
verification_uri_complete + user_code, both of which are reachable from
any browser on any device.

DISPLAY/WAYLAND_DISPLAY presence moved from detectContainerLike to
canLaunchBrowser — they're a browser-availability signal, not a container
signal — so the two functions express orthogonal concerns.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After iterating through PKCE+loopback, URL-via-tool-response, container
detection, and PKCE-with-browser-fallback, settle on the simpler model:
always use OAuth 2.0 Device Authorization Grant (RFC 8628). It works
uniformly across containers, headless setups, and regular desktops, and
the verification_uri_complete + user_code UX is short enough that the
"single click" advantage of PKCE doesn't justify the extra code path or
runtime branching.

Removed:
- PKCE primitives: generatePkcePair, base64UrlEncode, exchangeCodeForTokens
- Loopback callback server: awaitCallback, CallbackResult
- Flow selection: selectAuthFlow, detectContainerLike, canLaunchBrowser,
  AuthFlow type, REDASH_OIDC_FLOW env var, REDASH_OIDC_AUTO_LAUNCH_BROWSER
- Per-flow branches in performLogin / startPendingLogin / ensureValidTokens

Kept:
- Device flow: requestDeviceCode, pollForDeviceToken, beginDeviceFlow
- Token cache I/O, discovery, refresh_token grant
- openBrowser is still called from `redash-mcp login` CLI as a convenience
  (silently no-ops in containers — the user already has the URL printed)
- ensureValidTokens surfaces URL + user_code via tool response for the
  MCP serve path

Net: auth.ts goes from ~800 to ~450 lines, single flow, no env-var
behavior matrix to reason about.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a new MCP tool the assistant can call right after surfacing the
device flow URL to the user. The tool blocks on the in-flight pending
login's polling promise and returns once tokens are cached, so the
assistant can immediately retry the original Redash tool without
prompting the user a second time ("you authorized, now ask me again").

Changes:
- auth.ts: export `waitForPendingLogin()` — checks cache first, starts
  or attaches to a pending login, races on `pending.completion` with the
  device code's own expiry. Also `getPendingLogin()` for read-only
  introspection.
- index.ts: register `wait_for_oidc_login` as the first tool. Tool
  dispatcher bypasses `ensureValidTokens` for this tool (it IS the auth
  step). Returns the authenticated email on success.
- auth.ts: `ensureValidTokens` error message now contains explicit
  "## For the user" / "## For the assistant" sections instructing the
  assistant to surface the URL AND call `wait_for_oidc_login` in the
  same turn, then retry the original tool.

End-to-end UX:
1. User: "list redash queries"
2. Assistant calls `list_queries` → gets auth-required error with URL
3. Assistant prints URL to user + calls `wait_for_oidc_login` (blocks)
4. User clicks URL, completes device flow in browser
5. Device-flow polling resolves, `wait_for_oidc_login` returns success
6. Assistant retries `list_queries`, returns results
All in a single turn from the user's POV.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, a Redash 401 followed by a failed `forceRefresh` (revoked /
rotated refresh_token, no refresh_token at all) raised an AuthError
inside the axios response interceptor, but every RedashClient method's
catch block wrapped it as `throw new Error('Failed to fetch queries
from Redash')` — discarding the auth context. The assistant saw a
generic failure and didn't know to prompt the user to re-authorize.

Two fixes:

1. Extract `makeLoginRequiredError(reason, opts)` in auth.ts that starts
   a pending device flow and returns an AuthError with the standard
   For-the-user / For-the-assistant message pointing at
   `wait_for_oidc_login`. Used by both `ensureValidTokens` (cold cache)
   and the 401 retry path (warm-but-revoked).

2. In redashClient.ts: when `forceRefresh` fails with AuthError on the
   401 retry path, wipe the stale cache via `performLogout` and throw
   `makeLoginRequiredError(...)` so the failure surfaces the same way
   as a cold cache miss.

3. Add `if (error instanceof AuthError) throw error` at the top of every
   RedashClient method's catch block (57 sites) so the auth-context
   error is preserved verbatim through to the MCP tool response instead
   of being replaced with a generic message.

End-to-end effect: when the session-cached token gets rejected by
Redash mid-conversation, the assistant gets back the same URL-and-
wait_for_oidc_login instructions it would on a cold start, and the
"resume after auth" UX (pr.9) continues to work transparently.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dlddu dlddu marked this pull request as ready for review May 27, 2026 09:29

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/__tests__/integration.test.ts (1)

37-65: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

싱글턴(redashClient) 생성 전에 axios.create mockReturnValue를 주입하세요.

src/redashClient.tsexport const redashClient = new RedashClient();로 eager singleton을 만들고, 생성자에서 곧바로 this.client = axios.create(axiosConfig)를 저장합니다. 그런데 src/__tests__/integration.test.tsredashClient를 먼저 import한 뒤(라인 37~38) 테스트 본문에서 mockedAxios.create.mockReturnValue(...)(라인 64)를 설정하고 있어, 싱글턴 내부 this.client가 의도한 mockInstance로 고정되지 않을 수 있습니다(가드 로직은 주로 interceptors 주입 누락만 다룹니다). 게다가 현재 테스트는 logger.error 호출 여부만 확인하고 있어(인자/에러 원인 검증 없음) 잘못된 실패 경로로도 통과할 위험이 큽니다.

  • mockReturnValue 설정을 redashClient import 이전으로 옮기거나, jest.mock('axios') 팩토리에서 axios.create의 구현을 제공하세요.
🤖 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 `@src/__tests__/integration.test.ts` around lines 37 - 65, The test imports the
eager singleton redashClient (constructed by new RedashClient()) before stubbing
axios.create, so the internal this.client may be created with the real axios
instead of your mock; move the axios.create mockReturnValue setup so it runs
before importing redashClient (or replace the import with a jest.mock('axios',
() => { create: ... }) factory that returns the mockInstance), ensure the
factory/mock provides create and the interceptors/defaults shape used by
RedashClient, and update the test assertion to verify logger.error was called
with the expected error/arguments rather than only checking that it was called.
src/redashClient.ts (1)

277-282: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Authorization 차단이 대소문자 변형을 놓칩니다.

여기서는 Authorizationauthorization만 지우는데, AUTHORIZATION 같은 키는 그대로 남습니다. HTTP 헤더명은 대소문자를 구분하지 않아서 REDASH_EXTRA_HEADERS로 들어온 값이 같은 헤더로 정규화될 때 Bearer 토큰 주입을 덮어쓸 수 있습니다. 키를 normalize해서 비교하거나, request interceptor에서 authorization 계열 키를 전부 제거한 뒤 설정해 주세요.

🤖 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 `@src/redashClient.ts` around lines 277 - 282, The current check only deletes
'Authorization' and 'authorization' but misses other casings (e.g.,
'AUTHORIZATION'); update the logic around extraHeaders in src/redashClient.ts
(the extraHeaders handling block and any request interceptor that applies
REDASH_EXTRA_HEADERS) to normalize header names before comparing or to iterate
all keys and delete any whose lowercased form === 'authorization' so all case
variants are removed prior to injecting the OIDC Authorization header.
🧹 Nitpick comments (3)
src/__tests__/redashClient.test.ts (2)

129-141: ⚡ Quick win

401 단일 재시도 보장을 음수 케이스로도 고정하세요.

Line 129~141은 “첫 401에서 재시도 성공”만 검증합니다. _retried가 이미 true인 401에서 즉시 중단되는 경로를 추가로 고정해야 무한 재시도 회귀를 막을 수 있습니다.

테스트 추가 예시
+    it('should not retry when request is already marked as retried', async () => {
+      const originalConfig: any = { _retried: true, headers: {}, url: '/api/queries', method: 'get' };
+      const err: any = { response: { status: 401 }, config: originalConfig };
+
+      await expect(responseErrorInterceptor!(err)).rejects.toBe(err);
+      expect(mockAxiosInstance.request).not.toHaveBeenCalled();
+    });
🤖 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 `@src/__tests__/redashClient.test.ts` around lines 129 - 141, Add a negative
test verifying that responseErrorInterceptor does not retry when a 401 arrives
and the request already has _retried === true: invoke responseErrorInterceptor
with an error object whose response.status is 401 and config._retried is true,
assert that mockAxiosInstance.request is not called,
originalConfig.headers.Authorization is not changed to the refreshed token, and
the interceptor rethrows or returns the original rejection (e.g., rejects with
the same error). This targets the responseErrorInterceptor behavior to prevent
infinite retry loops.

182-191: ⚡ Quick win

Authorization 우회 방지 테스트를 헤더 대소문자 변형까지 확장하세요.

Line 182~191은 Authorization 키만 확인합니다. authorization/AUTHORIZATION 변형을 추가해도 기본 헤더/요청 헤더에 반영되지 않는지 검증해야 보안 계약을 더 견고하게 고정할 수 있습니다.

테스트 확장 예시
+    it('should refuse lowercase authorization header from extra headers', () => {
+      process.env.REDASH_EXTRA_HEADERS = '{"authorization":"malicious-key"}';
+
+      mockedAxios.create.mockClear();
+      new RedashClient();
+
+      const callArgs = mockedAxios.create.mock.calls[0]?.[0];
+      expect((callArgs?.headers as any)?.Authorization).toBeUndefined();
+      expect((callArgs?.headers as any)?.authorization).toBeUndefined();
+    });
🤖 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 `@src/__tests__/redashClient.test.ts` around lines 182 - 191, Update the test
'should refuse to let extra headers override Authorization' to assert that extra
headers with different casing cannot inject an Authorization header: when
setting process.env.REDASH_EXTRA_HEADERS include keys like "authorization" and
"AUTHORIZATION" (in addition to "Authorization"), instantiate new
RedashClient(), read the axios.create call config
(mockedAxios.create.mock.calls[0]?.[0]) and assert that none of callArgs.headers
contain Authorization in any casing (e.g., check that
callArgs.headers.Authorization, callArgs.headers.authorization, and
callArgs.headers.AUTHORIZATION are all undefined), ensuring the interceptor
remains the sole place that sets the Authorization header.
README.md (1)

103-109: ⚡ Quick win

코드 블록에 언어 식별자 추가

Fenced code block에 언어 식별자가 누락되어 markdownlint 경고가 발생합니다.

수정 제안
-```
+```text
 redash-mcp [serve]    Start the MCP server (default). Requires a prior `login`.
 redash-mcp login      Run the OIDC PKCE browser flow and cache tokens.
 redash-mcp logout     Clear the cached tokens.
 redash-mcp status     Show cached token info (email, expiry).
 redash-mcp help       Show help.

</details>

As per coding guidelines: markdownlint-cli2 규칙 MD040은 모든 fenced code block에 언어 식별자를 요구합니다.

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @README.md around lines 103 - 109, Add a language identifier to the fenced
code block that lists the CLI commands (the block starting with the three
backticks followed by lines beginning "redash-mcp [serve]" through "redash-mcp
help"); change the opening fence from totext so markdownlint MD040 is
satisfied and the block is treated as plain text.


</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

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 @playwright.config.ts:

  • Around line 37-44: The comment clarifies that DANGEROUSLY_OMIT_AUTH used in
    the command array is only disabling Inspector proxy auth and does not bypass
    OIDC in server code (see ensureValidTokens and src/index.ts), and that
    e2e/inspector-basic.spec.ts relies on a valid pre-populated
    REDASH_OIDC_TOKEN_CACHE_PATH; update the playground docs or the playbook comment
    to state this explicitly and ensure CI injects a valid
    REDASH_OIDC_TOKEN_CACHE_PATH token cache (or modify the test to run
    wait_for_oidc_login device flow), referencing the symbols DANGEROUSLY_OMIT_AUTH,
    REDASH_OIDC_TOKEN_CACHE_PATH, ensureValidTokens, src/index.ts, and
    e2e/inspector-basic.spec.ts so readers know where the behavior originates and
    what to change.

In @README.md:

  • Line 105: Update the "redash-mcp login" subcommand description to accurately
    reflect the implemented auth flow: if the code implements the OIDC Device Flow,
    replace "OIDC PKCE browser flow" with "OIDC Device Flow (device code flow) and
    token caching"; otherwise remove or generalize the PKCE wording to avoid
    claiming PKCE when not implemented. Locate the descriptive text for the
    subcommand named "redash-mcp login" in the README and edit that single-line
    description to match the actual implementation.
  • Line 11: The README currently describes an OIDC/PKCE browser flow but the code
    in src/auth.ts implements the RFC 8628 Device Authorization Grant
    (device_code/user_code, verification_uri) and does not use redirect URIs or
    PKCE; update the README text that mentions "OIDC + PKCE browser login", any
    "PKCE required" / loopback redirect URI language, and the REDASH_OIDC_AUDIENCE
    description to state that the implementation sends audience as a parameter in
    the device code request body (not a PKCE audience parameter), add a language
    identifier to the code fence that lacks one, and reword the "v1.0.0: Initial
    release (legacy numbering)" line to something clearer; search the README for the
    phrases "OIDC + PKCE browser login", "PKCE required",
    "http://127.0.0.1:*/callback", "REDASH_OIDC_AUDIENCE", the unnamed code fence,
    and "v1.0.0: Initial release (legacy numbering)" to make these edits so wording
    matches src/auth.ts behavior.

In @src/auth.ts:

  • Around line 345-357: startPendingLogin creates a race: multiple concurrent
    callers can each call beginDeviceFlow and overwrite the global pendingLogin; to
    fix, serialize by introducing an in-flight promise (e.g., pendingLoginPromise)
    that is checked/returned before calling beginDeviceFlow, set pendingLogin only
    once when that promise resolves, and ensure you clear the in-flight promise on
    error or after resolution; update startPendingLogin to return the resolved
    pendingLogin (with url, userCode, expiresAt) for all concurrent callers and keep
    the existing expiresAt calculation and pendingLogin shape.
  • Around line 411-468: Both getValidTokens and forceRefresh perform read →
    refresh → write concurrently; serialize refreshes by adding a shared in-memory
    inflight lock so only one refresh for a given cachePath runs at a time.
    Implement a module-level Map<string, Promise> inflightRefreshes
    keyed by cache (tokenCachePath) and factor the refresh logic into a helper
    (e.g., performRefreshWithLock(cache, cfg, doRefresh)) that: 1) checks
    readTokenCache(cache) and returns cached if still valid (REFRESH_LEEWAY_MS)
    before starting a refresh, 2) if a refresh is needed uses an existing inflight
    promise from the map or creates one that runs discover(...) → refreshTokens(...)
    → tokenResponseToCached(...) → writeTokenCache(...), 3) awaits that promise,
    removes it from the map on completion, and returns the updated tokens. Call this
    helper from both getValidTokens (when cached expired) and forceRefresh so
    parallel callers share the same inflight refresh and avoid simultaneous /token
    calls.

In @src/index.ts:

  • Around line 2514-2533: The auth pre-check is gating public-dashboard calls;
    update the logic around ensureValidTokens so it only runs for tools that require
    authentication (e.g., skip when the tool name is "get_public_dashboard" or when
    a tool metadata flag like requiresAuth === false). Concretely, modify the block
    that imports and calls ensureValidTokens (and still handles AuthError) to first
    check the tool identity or a per-tool requiresAuth property and only invoke
    ensureValidTokens for authenticated tools; keep the same AuthError handling for
    those cases so behavior and messages remain unchanged for protected tools.

Outside diff comments:
In @src/__tests__/integration.test.ts:

  • Around line 37-65: The test imports the eager singleton redashClient
    (constructed by new RedashClient()) before stubbing axios.create, so the
    internal this.client may be created with the real axios instead of your mock;
    move the axios.create mockReturnValue setup so it runs before importing
    redashClient (or replace the import with a jest.mock('axios', () => { create:
    ... }) factory that returns the mockInstance), ensure the factory/mock provides
    create and the interceptors/defaults shape used by RedashClient, and update the
    test assertion to verify logger.error was called with the expected
    error/arguments rather than only checking that it was called.

In @src/redashClient.ts:

  • Around line 277-282: The current check only deletes 'Authorization' and
    'authorization' but misses other casings (e.g., 'AUTHORIZATION'); update the
    logic around extraHeaders in src/redashClient.ts (the extraHeaders handling
    block and any request interceptor that applies REDASH_EXTRA_HEADERS) to
    normalize header names before comparing or to iterate all keys and delete any
    whose lowercased form === 'authorization' so all case variants are removed prior
    to injecting the OIDC Authorization header.

Nitpick comments:
In @README.md:

  • Around line 103-109: Add a language identifier to the fenced code block that
    lists the CLI commands (the block starting with the three backticks followed by
    lines beginning "redash-mcp [serve]" through "redash-mcp help"); change the
    opening fence from totext so markdownlint MD040 is satisfied and the
    block is treated as plain text.

In @src/__tests__/redashClient.test.ts:

  • Around line 129-141: Add a negative test verifying that
    responseErrorInterceptor does not retry when a 401 arrives and the request
    already has _retried === true: invoke responseErrorInterceptor with an error
    object whose response.status is 401 and config._retried is true, assert that
    mockAxiosInstance.request is not called, originalConfig.headers.Authorization is
    not changed to the refreshed token, and the interceptor rethrows or returns the
    original rejection (e.g., rejects with the same error). This targets the
    responseErrorInterceptor behavior to prevent infinite retry loops.
  • Around line 182-191: Update the test 'should refuse to let extra headers
    override Authorization' to assert that extra headers with different casing
    cannot inject an Authorization header: when setting
    process.env.REDASH_EXTRA_HEADERS include keys like "authorization" and
    "AUTHORIZATION" (in addition to "Authorization"), instantiate new
    RedashClient(), read the axios.create call config
    (mockedAxios.create.mock.calls[0]?.[0]) and assert that none of callArgs.headers
    contain Authorization in any casing (e.g., check that
    callArgs.headers.Authorization, callArgs.headers.authorization, and
    callArgs.headers.AUTHORIZATION are all undefined), ensuring the interceptor
    remains the sole place that sets the Authorization header.

</details>

<details>
<summary>🪄 Autofix (Beta)</summary>

Fix all unresolved CodeRabbit comments on this PR:

- [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended)
- [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: Organization UI

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `c0418360-f962-4a76-b6bb-547813cbbfe0`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between f73f39f896239bdd0bcb096592a17ac3717d384c and ebc429346981143b7c3549a3e1fb34f85f610054.

</details>

<details>
<summary>⛔ Files ignored due to path filters (1)</summary>

* `package-lock.json` is excluded by `!**/package-lock.json`

</details>

<details>
<summary>📒 Files selected for processing (10)</summary>

* `README.md`
* `package.json`
* `playwright.config.ts`
* `src/__tests__/auth.test.ts`
* `src/__tests__/integration.test.ts`
* `src/__tests__/redashClient.test.ts`
* `src/auth.ts`
* `src/cli.ts`
* `src/index.ts`
* `src/redashClient.ts`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Comment thread playwright.config.ts
Comment on lines +37 to +44
command: [
`REDASH_URL=${TEST_REDASH_URL}`,
`REDASH_OIDC_ISSUER=${TEST_OIDC_ISSUER}`,
`REDASH_OIDC_CLIENT_ID=${TEST_OIDC_CLIENT_ID}`,
TEST_OIDC_TOKEN_CACHE_PATH ? `REDASH_OIDC_TOKEN_CACHE_PATH=${TEST_OIDC_TOKEN_CACHE_PATH}` : '',
'DANGEROUSLY_OMIT_AUTH=true',
'npm run inspector',
].filter(Boolean).join(' '),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: DANGEROUSLY_OMIT_AUTH 플래그가 어떻게 사용되는지 확인

# auth.ts나 index.ts에서 이 플래그가 어떻게 처리되는지 검색
rg -nC5 'DANGEROUSLY_OMIT_AUTH' src/

# E2E 테스트에서 실제 인증이 수행되는지 확인
rg -nC3 'login|getValidTokens|performLogin' e2e/

Repository: Buzzvil/redash-mcp

Length of output: 44


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) playwright.config.ts 해당 구간 확인
if [ -f playwright.config.ts ]; then
  echo "=== playwright.config.ts (lines around 37-44) ==="
  nl -ba playwright.config.ts | sed -n '25,60p'
else
  echo "playwright.config.ts not found at repo root"
fi

# 2) DANGEROUSLY_OMIT_AUTH 사용처를 repo 전체에서 탐색
echo
echo "=== occurrences: DANGEROUSLY_OMIT_AUTH (repo-wide) ==="
rg -n --hidden --no-ignore -S 'DANGEROUSLY_OMIT_AUTH' .

# 3) OIDC 관련 env가 어디서 참조되는지 확인
echo
echo "=== occurrences: REDASH_OIDC_* (repo-wide) ==="
rg -n --hidden --no-ignore -S 'REDASH_OIDC_' .

# 4) e2e에서 로그인/토큰 관련 호출이 있는지 확인 (폴더 존재 여부와 함께)
echo
echo "=== e2e folder scan for login/token keywords ==="
ls -la | head
if [ -d e2e ]; then
  rg -n -S --hidden --no-ignore 'login|getValidTokens|performLogin|token|OIDC' e2e || true
else
  echo "No e2e directory at repo root"
fi

# 5) Playwright setup/테스트 러너가 어떤 환경을 사용해 호출하는지(있다면) 확인
echo
echo "=== search for playwright config usage / auth setup in tests ==="
rg -n -S --hidden --no-ignore 'playwright|globalSetup|globalTeardown|storageState' . || true

Repository: Buzzvil/redash-mcp

Length of output: 173


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) playwright.config.ts 해당 구간 확인
if [ -f playwright.config.ts ]; then
  echo "=== playwright.config.ts (lines around 25-60) ==="
  awk 'NR>=25 && NR<=60 {printf "%d:%s\n", NR, $0}' playwright.config.ts
else
  echo "playwright.config.ts not found at repo root"
  ls -la | head
fi

# 2) DANGEROUSLY_OMIT_AUTH 사용처를 repo 전체에서 탐색
echo
echo "=== occurrences: DANGEROUSLY_OMIT_AUTH (repo-wide) ==="
rg -n --hidden --no-ignore -S 'DANGEROUSLY_OMIT_AUTH' . || true

# 3) OIDC 관련 env가 어디서 참조되는지 확인
echo
echo "=== occurrences: REDASH_OIDC_* (repo-wide) ==="
rg -n --hidden --no-ignore -S 'REDASH_OIDC_' . || true

# 4) e2e에서 로그인/토큰 관련 호출이 있는지 확인 (폴더 존재 여부와 함께)
echo
echo "=== e2e folder scan for login/token keywords ==="
if [ -d e2e ]; then
  rg -n -S --hidden --no-ignore 'login|getValidTokens|performLogin|token|OIDC' e2e || true
else
  echo "No e2e directory at repo root"
fi

# 5) Playwright 관련 setup/teardown 및 storageState 사용 여부 확인
echo
echo "=== search for playwright setup/teardown usage ==="
rg -n -S --hidden --no-ignore 'globalSetup|globalTeardown|storageState|playwright\.config' . || true

Repository: Buzzvil/redash-mcp

Length of output: 6327


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) playwright.config.ts 전체 상단/관련 주석 확인
echo "=== playwright.config.ts (head) ==="
head -n 120 playwright.config.ts | nl -ba | sed -n '1,120p'

# 2) package.json의 inspector 스크립트 확인
if [ -f package.json ]; then
  echo
  echo "=== package.json scripts ==="
  cat package.json | sed -n '1,200p' | rg -n '"scripts"' -n -C 2 || true
  # Print full scripts block more directly
  node -e "const p=require('./package.json'); console.log(JSON.stringify(p.scripts,null,2))"
else
  echo "package.json not found"
fi

# 3) inspector 엔트리/실행 파일(예: dist/src) 확인
echo
echo "=== search for 'inspector' implementation ==="
rg -n --hidden --no-ignore -S "inspector" . || true

# 4) README에 DANGEROUSLY_OMIT_AUTH 언급이 있는지 확인
echo
echo "=== README.md: DANGEROUSLY_OMIT_AUTH mentions ==="
rg -n --hidden --no-ignore -S "DANGEROUSLY_OMIT_AUTH" README.md || true

# 5) Playwright 테스트/폴더 존재 여부 확인
echo
echo "=== search for playwright usage ==="
rg -n --hidden --no-ignore -S "playwright" . || true

Repository: Buzzvil/redash-mcp

Length of output: 159


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) playwright.config.ts 상단/관련 주석 확인 (nl 미사용)
echo "=== playwright.config.ts (head 1-120) ==="
awk 'NR<=120 {printf "%d:%s\n", NR, $0}' playwright.config.ts

# 2) package.json의 scripts 확인
if [ -f package.json ]; then
  echo
  echo "=== package.json scripts ==="
  node -e "const p=require('./package.json'); console.log(p.scripts||{})"
else
  echo "package.json not found"
fi

# 3) 'npm run inspector'가 가리키는 실제 구현 확인
echo
echo "=== inspector-related references (repo-wide) ==="
rg -n --hidden --no-ignore -S 'inspector' . || true

# 4) README에 DANGEROUSLY_OMIT_AUTH 언급 있는지 확인
echo
echo "=== README.md: DANGEROUSLY_OMIT_AUTH mentions ==="
rg -n --hidden --no-ignore -S "DANGEROUSLY_OMIT_AUTH" README.md || true

# 5) Playwright/E2E 폴더/테스트 존재 여부 확인
echo
echo "=== directories check ==="
ls -la | head -n 50
echo
echo "=== search for playwright test usage (repo-wide) ==="
rg -n --hidden --no-ignore -S "test\(|`@playwright/test`|playwright/test" . || true

Repository: Buzzvil/redash-mcp

Length of output: 5821


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) DANGEROUSLY/OMIT_AUTH 관련 키워드 추가 탐색 (repo-wide)
echo "=== occurrences: OMIT_AUTH / omitAuth / DANGEROUSLY ==="
rg -n --hidden --no-ignore -S '(OMIT_AUTH|omitAuth|DANGEROUSLY_OMIT_AUTH)' . || true

# 2) dist가 있으면 문자열이 컴파일 결과에도 존재하는지 확인
echo
echo "=== dist directory check ==="
ls -la dist 2>/dev/null || echo "no dist/ directory"

echo
echo "=== occurrences in dist (if any) ==="
if [ -d dist ]; then
  rg -n --hidden --no-ignore -S '(OMIT_AUTH|DANGEROUSLY_OMIT_AUTH)' dist || true
fi

# 3) src/auth.ts에서 환경변수 기반 auth 우회 로직 있는지 확인(파일 일부 출력)
echo
echo "=== src/auth.ts: search for OIDC/auth env handling ==="
if [ -f src/auth.ts ]; then
  rg -n --hidden --no-ignore -S 'process\.env|env\.|REDASH_OIDC|tokenCachePath|cache' src/auth.ts
  echo
  awk 'NR>=1 && NR<=220 {print}' src/auth.ts | sed -n '1,220p'
fi

# 4) e2e 테스트에서 실제로 로그인/인증 플로우를 타는지 코드 확인
echo
echo "=== e2e/inspector-basic.spec.ts (head to 220) ==="
if [ -f e2e/inspector-basic.spec.ts ]; then
  awk 'NR>=1 && NR<=220 {printf "%d:%s\n", NR, $0}' e2e/inspector-basic.spec.ts
fi

# 5) e2e 테스트가 무엇을 검증하는지 확인: 폴더 내용/다른 spec 유무
echo
echo "=== e2e directory listing ==="
ls -ლა e2e

Repository: Buzzvil/redash-mcp

Length of output: 19591


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# src 트리 대략 확인
echo "=== src/ tree (depth 2) ==="
find src -maxdepth 2 -type f | sed -n '1,200p'

# OIDC/auth 로직 호출 지점 탐색
echo
echo "=== occurrences in src of auth/token-related functions ==="
rg -n --hidden --no-ignore -S 'ensureValidTokens|getValidTokens|performLogin|startPendingLogin|waitForPendingLogin|performLogout|loadOidcConfig|tokenCachePath|readTokenCache|clearTokenCache' src || true

echo
echo "=== occurrences in src of REDASH_OIDC / OIDC env usage (beyond auth.ts/cli.ts) ==="
rg -n --hidden --no-ignore -S 'process\.env\.REDASH_OIDC_|REDASH_OIDC_' src || true

echo
echo "=== occurrences in src of token cache / headers / auth middleware keywords ==="
rg -n --hidden --no-ignore -S '(Authorization|Bearer|access_token|id_token|refresh_token|auth(?!enticated)|oidc)' src || true

Repository: Buzzvil/redash-mcp

Length of output: 10089


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# src/index.ts에서 ensureValidTokens 관련 라인 및 주변 문맥
echo "=== src/index.ts: ensureValidTokens / waitForPendingLogin occurrences ==="
rg -n --hidden --no-ignore -S 'ensureValidTokens|waitForPendingLogin' src/index.ts || true

echo
echo "=== src/index.ts: print around ensureValidTokens matches ==="
# Print small context blocks by line number
python3 - <<'PY'
import re, subprocess, json, os, sys, textwrap
path="src/index.ts"
with open(path,'r',encoding='utf-8') as f:
    lines=f.readlines()
text=''.join(lines)
# find line numbers for key phrases
keywords=["ensureValidTokens","waitForPendingLogin"]
line_nums=set()
for i,l in enumerate(lines, start=1):
    if any(k in l for k in keywords):
        line_nums.add(i)
for ln in sorted(line_nums):
    start=max(1, ln-20)
    end=min(len(lines), ln+20)
    print(f"\n--- {path}:{ln} ---")
    for j in range(start,end+1):
        print(f"{j}:{lines[j-1].rstrip()}")
PY

# src/index.ts에 DANGEROUSLY_OMIT_AUTH가 아예 없는지 재확인(정확 문자열)
echo
echo "=== src/index.ts: DANGEROUSLY_OMIT_AUTH exact string check ==="
rg -n --hidden --no-ignore -S 'DANGEROUSLY_OMIT_AUTH' src/index.ts || true

# auth 우회에 쓰일 수 있는 다른 env 키워드 탐색
echo
echo "=== src/: possible auth-skip env keywords ==="
rg -n --hidden --no-ignore -S '(OMIT_AUTH|SKIP_AUTH|SKIP_AUTHORIZATION|BYPASS_AUTH|DANGEROUSLY)' src || true

Repository: Buzzvil/redash-mcp

Length of output: 8128


🌐 Web query:

DANGEROUSLY_OMIT_AUTH @modelcontextprotocol/inspector env var

💡 Result:

The DANGEROUSLY_OMIT_AUTH environment variable is used in the @modelcontextprotocol/inspector to disable the default authentication requirement [1][2]. By default, the MCP Inspector proxy server generates a random session token and requires it to be included as a Bearer token in the Authorization header for all requests to ensure secure communication [2][3]. Setting DANGEROUSLY_OMIT_AUTH=true bypasses this security measure [1][4]. Security Warnings: - This practice is strongly discouraged and officially labeled as incredibly dangerous [1]. - Disabling authentication exposes your machine to potential attacks not only from the public internet but also via your web browser [1]. Visiting a malicious website or viewing a malicious advertisement while the inspector is running could allow an attacker to remotely compromise your computer [1]. - It should not be used in enterprise or production environments [5]. The primary intended use case for this variable is strictly for local development, where a user may need to frequently restart the server and wants to avoid manually re-entering or pre-filling authentication tokens [4]. Top results: [1], [5], [2], [3]

Citations:


DANGEROUSLY_OMIT_AUTH는 OIDC 우회 플래그가 아니라 Inspector 프록시 인증 비활성화 용도

  • DANGEROUSLY_OMIT_AUTH=trueplaywright.config.tsnpm run inspector 실행에만 존재하고, src/에서는 OIDC 처리(예: ensureValidTokens)를 우회하는 용도로 사용되지 않습니다(서버 OIDC 게이트는 src/index.ts에서 여전히 걸립니다).
  • 다만 E2E(e2e/inspector-basic.spec.ts)에서 wait_for_oidc_login 디바이스 플로우 완료를 직접 수행하지 않으므로, 현재 구성은 주석대로 사전 토큰 캐시(REDASH_OIDC_TOKEN_CACHE_PATH)가 유효함을 전제로 동작할 가능성이 큽니다. CI에서 해당 값이 실제로 주입되는지(유효 토큰인지) 문서/주석으로 명확히 해주세요.
🤖 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 `@playwright.config.ts` around lines 37 - 44, The comment clarifies that
DANGEROUSLY_OMIT_AUTH used in the command array is only disabling Inspector
proxy auth and does not bypass OIDC in server code (see ensureValidTokens and
src/index.ts), and that e2e/inspector-basic.spec.ts relies on a valid
pre-populated REDASH_OIDC_TOKEN_CACHE_PATH; update the playground docs or the
playbook comment to state this explicitly and ensure CI injects a valid
REDASH_OIDC_TOKEN_CACHE_PATH token cache (or modify the test to run
wait_for_oidc_login device flow), referencing the symbols DANGEROUSLY_OMIT_AUTH,
REDASH_OIDC_TOKEN_CACHE_PATH, ensureValidTokens, src/index.ts, and
e2e/inspector-basic.spec.ts so readers know where the behavior originates and
what to change.

Comment thread README.md
## Features

- Connect to Redash instances via the Redash API
- Connect to Redash instances using **OIDC + PKCE browser login** (no long-lived API keys)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: auth.ts에서 실제 사용된 OAuth 플로우 확인

# Device Flow 관련 키워드 검색 (device_code, user_code 등)
rg -nC3 'device.*code|user.*code|device.*authorization' src/auth.ts

# PKCE/Authorization Code 관련 키워드 검색 (code_challenge, redirect_uri 등)
rg -nC3 'code_challenge|code_verifier|redirect_uri' src/auth.ts

Repository: Buzzvil/redash-mcp

Length of output: 6176


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== README.md (head to ~140) with line numbers ==="
nl -ba README.md | sed -n '1,160p'

echo "=== README.md (around line ~220-260) with line numbers ==="
nl -ba README.md | sed -n '200,260p'

echo "=== README.md OAuth flow keyword check ==="
rg -n --context 2 "PKCE|device flow|Device Flow|RFC 8628|device_authorization|verification_uri|redirect URI|127\.0\.0\.1" README.md || true

echo "=== auth.ts OAuth flow keyword check ==="
# Locate auth.ts
AUTH_FILE="$(fd -tf auth.ts . | head -n 1 || true)"
if [[ -z "${AUTH_FILE}" ]]; then
  AUTH_FILE="$(fd -tf '*auth.ts' . | head -n 1 || true)"
fi
echo "Found auth file: ${AUTH_FILE:-<none>}"

rg -n --context 2 "RFC 8628|device_authorization_endpoint|urn:ietf:params:oauth:grant-type:device_code|verification_uri|user_code|code_challenge|redirect_uri" "${AUTH_FILE:-README.md}" || true

Repository: Buzzvil/redash-mcp

Length of output: 174


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== README.md (1-200) ==="
awk 'NR<=200{printf "%6d\t%s\n", NR, $0}' README.md

echo "=== README.md (200-280) ==="
awk 'NR>=200 && NR<=280{printf "%6d\t%s\n", NR, $0}' README.md

echo "=== README.md OAuth flow keyword check ==="
rg -n --context 1 "PKCE|device flow|Device Flow|RFC 8628|device_authorization|verification_uri|redirect URI|127\.0\.0\.1|loopback|audience.*PKCE|Authorization Code \\+ PKCE" README.md || true

echo "=== auth.ts path lookup ==="
AUTH_FILE="$(fd -tf auth.ts . | head -n 5 || true)"
if [[ -z "${AUTH_FILE}" ]]; then
  AUTH_FILE="$(fd -tf '*auth.ts' . | head -n 5 || true)"
fi
echo "Found auth.ts candidates:"
echo "${AUTH_FILE:-<none>}"

# Use first candidate if multiple
AUTH_FILE_ONE="$(echo "${AUTH_FILE}" | head -n 1)"

if [[ -n "${AUTH_FILE_ONE}" ]]; then
  echo "=== auth.ts (flow keyword check) ==="
  rg -n --context 2 "RFC 8628|device_authorization_endpoint|urn:ietf:params:oauth:grant-type:device_code|device_code|user_code|verification_uri|code_challenge|code_verifier|redirect_uri|loopback|PKCE|authorization_code|grant_type" "${AUTH_FILE_ONE}"
fi

Repository: Buzzvil/redash-mcp

Length of output: 16453


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Search for PKCE/Auth Code usage in src ==="
rg -n "code_challenge|code_verifier|redirect_uri|authorization_code|grant_type['\"]?\s*:\s*['\"]authorization_code|urn:ietf:params:oauth:grant-type:authorization_code" src || true

echo "=== Search for Device Flow usage in src ==="
rg -n "RFC 8628|device_authorization_endpoint|urn:ietf:params:oauth:grant-type:device_code|device_code|user_code|verification_uri" src || true

Repository: Buzzvil/redash-mcp

Length of output: 1675


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== auth.ts: find audience usage ==="
rg -n "audience" src/auth.ts || true

echo "=== auth.ts: show audience-related block (around matches) ==="
rg -n --context 5 "audience" src/auth.ts || true

Repository: Buzzvil/redash-mcp

Length of output: 3071


README.md OAuth 플로우 문구가 실제 구현(RFC 8628 Device Flow)과 불일치

  • README.md 11, 26-27: 구현은 src/auth.ts에서 RFC 8628 Device Authorization Grant(device_code/user_code, urn:ietf:params:oauth:grant-type:device_code, verification_uri 기반)인데, README는 Authorization Code + PKCE / OIDC + PKCE browser login을 안내합니다.
  • README.md 49-50: 구현은 redirect URI/PKCE 챌린지에 의존하지 않는데, README는 PKCE requiredhttp://127.0.0.1:*/callback(loopback redirect URI)을 요구사항으로 적고 있습니다.
  • README.md 59: REDASH_OIDC_AUDIENCE 설명이 “audience PKCE parameter”라고 되어 있으나, 실제 구현에서는 device code 요청 바디의 일반 audience로만 사용됩니다.
  • README.md 103: 코드펜스에 언어 식별자 누락(`````, markdownlint 이슈).
  • README.md 235: v1.0.0: Initial release (legacy numbering) 문구는 오해 소지가 있어 더 명확히 정리 필요(소폭).
문구 교체 제안

Line 11:

-- Connect to Redash instances using **OIDC + PKCE browser login** (no long-lived API keys)
+- Connect to Redash instances using **OIDC Device Flow login** (no long-lived API keys)

Lines 26-27:

-authenticated with a short-lived OIDC access token obtained from your IdP
-(Authentik, Auth0, Keycloak, Okta, …) using the **Authorization Code + PKCE**
-flow.
+authenticated with a short-lived OIDC access token obtained from your IdP
+(Authentik, Auth0, Keycloak, Okta, …) using the **Device Authorization Grant**
+flow (RFC 8628).

Line 50:

-- An OIDC client registered as **public** with **PKCE required** and a
-  loopback redirect URI like `http://127.0.0.1:*/callback`
+- An OIDC client registered as **public** with **Device Flow enabled**

Line 59:

-| `REDASH_OIDC_AUDIENCE` | no | Override the `audience` PKCE parameter. Defaults to `REDASH_OIDC_CLIENT_ID`. |
+| `REDASH_OIDC_AUDIENCE` | no | Override the `audience` parameter. Defaults to `REDASH_OIDC_CLIENT_ID`. |
🤖 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 `@README.md` at line 11, The README currently describes an OIDC/PKCE browser
flow but the code in src/auth.ts implements the RFC 8628 Device Authorization
Grant (device_code/user_code, verification_uri) and does not use redirect URIs
or PKCE; update the README text that mentions "OIDC + PKCE browser login", any
"PKCE required" / loopback redirect URI language, and the REDASH_OIDC_AUDIENCE
description to state that the implementation sends audience as a parameter in
the device code request body (not a PKCE audience parameter), add a language
identifier to the code fence that lacks one, and reword the "v1.0.0: Initial
release (legacy numbering)" line to something clearer; search the README for the
phrases "OIDC + PKCE browser login", "PKCE required",
"http://127.0.0.1:*/callback", "REDASH_OIDC_AUDIENCE", the unnamed code fence,
and "v1.0.0: Initial release (legacy numbering)" to make these edits so wording
matches src/auth.ts behavior.

Comment thread README.md

```
redash-mcp [serve] Start the MCP server (default). Requires a prior `login`.
redash-mcp login Run the OIDC PKCE browser flow and cache tokens.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Subcommand 설명의 PKCE 언급 제거 또는 수정 필요

Line 105의 login 서브커맨드 설명에서 "OIDC PKCE browser flow"라고 명시되어 있는데, 실제 구현이 Device Flow라면 이 부분도 수정이 필요합니다.

Device Flow인 경우 수정 제안
-redash-mcp login      Run the OIDC PKCE browser flow and cache tokens.
+redash-mcp login      Run the OIDC Device Flow and cache tokens.
📝 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.

Suggested change
redash-mcp login Run the OIDC PKCE browser flow and cache tokens.
redash-mcp login Run the OIDC Device Flow and cache tokens.
🤖 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 `@README.md` at line 105, Update the "redash-mcp login" subcommand description
to accurately reflect the implemented auth flow: if the code implements the OIDC
Device Flow, replace "OIDC PKCE browser flow" with "OIDC Device Flow (device
code flow) and token caching"; otherwise remove or generalize the PKCE wording
to avoid claiming PKCE when not implemented. Locate the descriptive text for the
subcommand named "redash-mcp login" in the README and edit that single-line
description to match the actual implementation.

Comment thread src/auth.ts
Comment on lines +345 to +357
export async function startPendingLogin(opts: { cfg?: OidcConfig; cachePath?: string } = {}): Promise<{ url: string; userCode: string; expiresAt: number }> {
if (pendingLogin && pendingLogin.expiresAt > Date.now()) {
return { url: pendingLogin.url, userCode: pendingLogin.userCode, expiresAt: pendingLogin.expiresAt };
}

const handle = await beginDeviceFlow(opts);
const entry: PendingLogin = {
url: handle.url,
userCode: handle.userCode,
completion: handle.completion,
expiresAt: Date.now() + handle.expiresInSec * 1000,
};
pendingLogin = entry;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

startPendingLogin()가 동시 호출에서 서로 다른 device flow를 만들 수 있습니다.

Line 346에서 상태를 확인한 뒤 Line 350의 await beginDeviceFlow()로 넘어가는 동안 다른 호출도 같은 경로를 탈 수 있습니다. 그러면 각 호출이 서로 다른 URL/user_code를 반환하지만 전역 pendingLogin에는 마지막 flow만 남아서, 먼저 받은 URL로 인증한 사용자는 wait_for_oidc_login이 다른 flow를 기다리게 됩니다. cold start도 하나의 in-flight promise로 먼저 등록해서 직렬화해야 합니다.

🤖 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 `@src/auth.ts` around lines 345 - 357, startPendingLogin creates a race:
multiple concurrent callers can each call beginDeviceFlow and overwrite the
global pendingLogin; to fix, serialize by introducing an in-flight promise
(e.g., pendingLoginPromise) that is checked/returned before calling
beginDeviceFlow, set pendingLogin only once when that promise resolves, and
ensure you clear the in-flight promise on error or after resolution; update
startPendingLogin to return the resolved pendingLogin (with url, userCode,
expiresAt) for all concurrent callers and keep the existing expiresAt
calculation and pendingLogin shape.

Comment thread src/auth.ts
Comment on lines +411 to +468
export async function getValidTokens(opts: { cfg?: OidcConfig; cachePath?: string; now?: () => number } = {}): Promise<CachedTokens> {
const cfg = opts.cfg ?? loadOidcConfig();
const cache = opts.cachePath ?? tokenCachePath();
const now = opts.now ?? Date.now;

const cached = await readTokenCache(cache);
if (!cached) {
throw new AuthError(
`No cached OIDC tokens at ${cache}. Run \`redash-mcp login\` once in your terminal.`,
);
}
if (cached.issuer !== cfg.issuer || cached.clientId !== cfg.clientId) {
throw new AuthError(
`Cached tokens belong to a different OIDC client (issuer=${cached.issuer}, client_id=${cached.clientId}). Run \`redash-mcp login\` again.`,
);
}
if (cached.expiresAt - REFRESH_LEEWAY_MS > now()) {
return cached;
}
if (!cached.refreshToken) {
throw new AuthError(
'Cached access token is expired and no refresh_token is available. Run `redash-mcp login` again.',
);
}

const discovery = await discover(cfg.issuer);
const refreshed = await refreshTokens(discovery.token_endpoint, {
refreshToken: cached.refreshToken,
clientId: cfg.clientId,
scopes: cfg.scopes,
});
const updated = tokenResponseToCached(refreshed, cfg, cached.refreshToken);
await writeTokenCache(cache, updated);
return updated;
}

/**
* Force a refresh and persist new tokens. Used by the API client when a
* request fails with 401 — the cached token may have been revoked or rotated
* by the IdP earlier than its stated expiry.
*/
export async function forceRefresh(opts: { cfg?: OidcConfig; cachePath?: string } = {}): Promise<CachedTokens> {
const cfg = opts.cfg ?? loadOidcConfig();
const cache = opts.cachePath ?? tokenCachePath();

const cached = await readTokenCache(cache);
if (!cached?.refreshToken) {
throw new AuthError('No refresh_token available; run `redash-mcp login` again.');
}
const discovery = await discover(cfg.issuer);
const refreshed = await refreshTokens(discovery.token_endpoint, {
refreshToken: cached.refreshToken,
clientId: cfg.clientId,
scopes: cfg.scopes,
});
const updated = tokenResponseToCached(refreshed, cfg, cached.refreshToken);
await writeTokenCache(cache, updated);
return updated;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

refresh 토큰 갱신을 단일 비행으로 묶어야 합니다.

getValidTokens()forceRefresh()가 같은 캐시를 독립적으로 갱신해서, 만료 직전 병렬 요청이 동일한 refresh token으로 동시에 /token을 칠 수 있습니다. refresh token rotation을 쓰는 IdP면 한 요청이 새 refresh token을 받은 직후 다른 요청이 invalid_grant로 실패하고, 정상 갱신이 끝났어도 일부 요청이 재로그인 경로로 떨어집니다. cache read → refresh → write 전체를 공유 promise/lock으로 직렬화해 주세요.

🤖 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 `@src/auth.ts` around lines 411 - 468, Both getValidTokens and forceRefresh
perform read → refresh → write concurrently; serialize refreshes by adding a
shared in-memory inflight lock so only one refresh for a given cachePath runs at
a time. Implement a module-level Map<string, Promise<CachedTokens>>
inflightRefreshes keyed by cache (tokenCachePath) and factor the refresh logic
into a helper (e.g., performRefreshWithLock(cache, cfg, doRefresh)) that: 1)
checks readTokenCache(cache) and returns cached if still valid
(REFRESH_LEEWAY_MS) before starting a refresh, 2) if a refresh is needed uses an
existing inflight promise from the map or creates one that runs discover(...) →
refreshTokens(...) → tokenResponseToCached(...) → writeTokenCache(...), 3)
awaits that promise, removes it from the map on completion, and returns the
updated tokens. Call this helper from both getValidTokens (when cached expired)
and forceRefresh so parallel callers share the same inflight refresh and avoid
simultaneous /token calls.

Comment thread src/index.ts
Comment on lines +2514 to +2533
// Gate every other tool call on a valid OIDC token. On cache miss
// ensureValidTokens starts a device flow in the background and throws an
// AuthError whose message tells both the user and the assistant what to
// do next (open URL + call `wait_for_oidc_login`).
try {
const { ensureValidTokens } = await import('./auth.js');
await ensureValidTokens();
} catch (authErr) {
if (authErr instanceof AuthError) {
logger.info(`Auth required for ${name}; surfacing login URL via tool response`);
return {
isError: true,
content: [{
type: "text",
text: authErr.message,
}],
};
}
throw authErr;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

공개 대시보드 조회까지 인증 게이트에 걸립니다.

이 pre-check는 모든 도구에 적용돼서 get_public_dashboard도 먼저 ensureValidTokens()를 통과해야 합니다. 하지만 그 도구는 공유 토큰으로 호출하는 공개 엔드포인트(/api/dashboards/public/${token})를 사용하므로, 지금 상태면 공개 링크 조회 시나리오가 로그인 요구로 깨집니다. 인증이 필요한 도구만 게이트하거나 public endpoint는 예외 처리해야 합니다.

🤖 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 `@src/index.ts` around lines 2514 - 2533, The auth pre-check is gating
public-dashboard calls; update the logic around ensureValidTokens so it only
runs for tools that require authentication (e.g., skip when the tool name is
"get_public_dashboard" or when a tool metadata flag like requiresAuth ===
false). Concretely, modify the block that imports and calls ensureValidTokens
(and still handles AuthError) to first check the tool identity or a per-tool
requiresAuth property and only invoke ensureValidTokens for authenticated tools;
keep the same AuthError handling for those cases so behavior and messages remain
unchanged for protected tools.

@dlddu dlddu changed the title feat!: OIDC PKCE 브라우저 로그인으로 API key 인증 대체 feat!: OIDC device flow 인증으로 API key 인증 대체 May 28, 2026
@luis-hong luis-hong self-requested a review May 28, 2026 04:56

@luis-hong luis-hong left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 github-review
verdict: CHANGES_REQUESTED · confidence: LOW

Summary

API key 인증을 RFC 8628 device flow OIDC 인증으로 전환하는 대규모 auth refactor. 전반적인 구조 (0600 perms, atomic write, issuer 바인딩 캐시, 401 single-retry, AuthError 보존 가드) 는 견고하나 동시성/문서 일관성/테스트 커버리지 면에서 사람 확인이 필요한 항목들이 있음. Auth 코드 + breaking change 라 LOW 로 분류.

Findings

  • [Warning] src/auth.ts:402 (getValidTokens 의 refresh 분기) 와 redashClient.ts:316 (401 interceptor) 양쪽에서 refresh 동시성 보호가 없음. MCP 도구 dispatcher 가 여러 Redash 호출을 동시에 fan-out 하면 (예: dashboard widget 들 병렬 조회) 토큰이 만료 직전일 때 N 개의 동시 refresh 가 IdP 를 때리고, refresh_token rotation 을 켠 IdP (Authentik 포함 가능) 에서는 첫 번째만 성공하고 나머지는 invalid_grant 로 실패해 사용자가 갑자기 재로그인 요구를 받게 됨. in-flight refresh promise 를 module-level 변수로 캐시 (singleflight) 하는 게 안전.

  • [Warning] src/auth.ts:355 startPendingLogin 도 같은 race 가능성. 첫 캐시 미스 시 두 도구 호출이 거의 동시에 들어오면 if (pendingLogin && ...) 둘 다 통과 → beginDeviceFlow 두 번 호출 → IdP 에 device code 2 개 발급, 그중 하나만 pendingLogin 에 기록되고 다른 하나는 고아. 사용자는 어쨌든 한 번의 verification 만 하므로 결과적으론 한쪽만 성공. 영향은 작지만 PR 본문의 "단일 flight (pendingLogin slot)" 보장이 깨짐. pendingLoginPromise<PendingLogin> 으로 두고 동시 호출이 같은 promise 를 await 하게 바꿔야 진짜 single-flight.

  • [Warning] README.md 가 device flow 로 전환됐는데도 "OIDC + PKCE browser login" (line 11), "PKCE required" prerequisites, login 서브커맨드 도움말의 "Run the OIDC PKCE browser flow", migration 가이드의 "PKCE-required" 등 PKCE 용어를 계속 사용. PR description 에는 v0.1.0-pr.8 에서 device-only 로 단순화했다고 명시되어 있으나 README 가 따라가지 않음. 사용자가 IdP 에 PKCE-only client 를 설정하면 device flow grant_type 이 거부될 수 있음 — 호환성 혼동 가능.

  • [Warning] src/__tests__/redashClient.test.ts 가 -901/+103 으로 대폭 축소. API key 인증 케이스 제거는 정당하나 getQueries(... q='test'), executeQuery async polling, executeAdhocQuery, updateQuery field-pruning, dashboard/alert/widget/snippet/destination 의 method-level 테스트들이 모두 사라졌음. 이들은 auth 와 직교한 endpoint 동작 검증이라 같이 지울 이유가 없음. interceptor 검증을 추가한 가치는 인정하나 endpoint 커버리지 회귀를 동반.

  • [Suggestion] src/auth.ts:18 import * as fs from 'fs'; 가 미사용 (fsp from fs/promises 만 사용). 제거해도 무방.

  • [Suggestion] src/redashClient.ts:319,328(config.headers as any).set?.(...) ?? ((config.headers as any).Authorization = ...) 패턴은 의도 (AxiosHeaders 신규 API 와 plain object 양쪽 지원) 는 알겠으나 가독성이 낮음. if (typeof (h as any).set === 'function') (h as any).set('Authorization', token) else (h as any).Authorization = token; 같이 풀어 쓰는 게 리뷰어 친화적.

  • [Suggestion] src/auth.ts:484pl.completion.finally(...).catch(() => {}) 가 에러를 조용히 삼킴. pendingLogin 슬롯을 비우는 목적 자체는 맞지만 디버깅 시 device flow 실패 원인을 로그에서 추적하기 어려움 — .catch((err) => logger.debug(...)) 정도면 충분.


generated by github-review · elapsed: 137.3s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants