Repository navigation
Conversation
Update email/password authentication to mirror Skylight's current web OAuth sequence instead of the old session-based basic auth flow. This aligns managed login with the live API, uses the returned bearer token for subsequent requests, and reduces failures caused by the previous auth mechanism. Also centralize shared API constants, refresh auth documentation and error guidance, and add automated coverage for the OAuth login flow alongside live smoke validation.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors authentication from email/password direct session login to a browser-style OAuth authorization code + PKCE flow, exchanges authorization codes for bearer tokens, and introduces centralized API constants. Version bumped from 1.1.8 to 1.1.8 with supporting documentation and comprehensive test coverage for the new OAuth flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / MCP
participant OAuth as OAuth Server<br/>/oauth/authorize
participant Form as Login Form<br/>/auth/session
participant TokenAPI as Token Exchange<br/>/oauth/token
participant API as API Server<br/>/api/plus_access
Client->>OAuth: POST /oauth/authorize<br/>(client_id, state, code_challenge, redirect_uri)
OAuth-->>Client: HTML login form + authenticity_token
Client->>Form: POST /auth/session<br/>(email, password, authenticity_token)
Form-->>Client: 302 redirect + authorization code
Client->>TokenAPI: POST /oauth/token<br/>(code, code_verifier, grant_type)
TokenAPI-->>Client: access_token (bearer token)
Client->>API: GET /api/plus_access<br/>(Authorization: Bearer token)
API-->>Client: subscription status
Client-->>Client: Return { email, token, subscriptionStatus }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 4
🧹 Nitpick comments (1)
tests/auth.test.ts (1)
35-41: Assert cookie replay in the happy-path test.The first redirect sets
_skylight_cloud_session, but the/auth/session/newand/auth/sessionhandlers never check that theCookieheader comes back. A brokenCookieJarwould still pass this suite. Please assert the replayed cookie and add a multi-Set-Cookiecase so the stateful part of the flow is actually covered.Also applies to: 44-59, 94-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth.test.ts` around lines 35 - 41, The test's fake redirect response currently sets a single `_skylight_cloud_session` cookie but downstream handlers (`/auth/session/new` and `/auth/session` request handlers in tests/auth.test.ts that read capturedState) do not assert that the Cookie header is replayed; update the initial redirect response to return multiple Set-Cookie headers (e.g., an array with `_skylight_cloud_session=abc123;...` plus another cookie) and add assertions in the `/auth/session/new` and `/auth/session` handlers to check request.headers.get("cookie") includes `_skylight_cloud_session=abc123` (and any other cookie values) before proceeding, failing the test if the Cookie header is missing or malformed so the CookieJar replay is actually enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 18-19: Add a blank line between the last line of the previous
entry ("Added automated tests for the OAuth login flow and kept live smoke
validation against the real API") and the next version header ("## [1.1.7] -
2025-12-30") in CHANGELOG.md so each version entry is separated by an empty line
consistent with Keep a Changelog formatting.
In `@src/api/auth.ts`:
- Around line 350-366: Remove PII/token material from the auth logs by
eliminating or redacting the two console.error calls around the OAuth sequence
(the line logging email before calling
createState/createPkceVerifier/beginOAuthFlow and the line logging
token.substring(...) after detectSubscriptionStatus). Replace them with a
generic status log like "Starting OAuth login" and "OAuth login successful" or
gate the detailed logs behind an explicit debug flag (e.g., only include email
or token fragments when a DEBUG/verbose flag is set). Ensure you update the
logging around the flow that uses state, codeVerifier/createPkceChallenge,
beginOAuthFlow, submitLoginForm, authorizeAuthenticatedSession,
exchangeAuthorizationCode, and detectSubscriptionStatus so no sensitive email or
token fragments are emitted in normal operation.
- Around line 35-40: The fallback that builds headerValues from
response.headers.get("set-cookie") loses multiple Set-Cookie headers because
get() returns a single comma-joined string; update the code around
headerValues/response.headers.getSetCookie to correctly parse multiple cookies
by either (a) preferring response.headers.getSetCookie() when available or (b)
using a robust Set-Cookie parser (e.g. add and use a library like
set-cookie-parser) to parse the string returned by
response.headers.get("set-cookie") into separate cookie strings before the
existing parsing loop; reference the headerValues variable and the
response.headers.getSetCookie/get("set-cookie") calls so the replacement parsing
logic is applied where headerValues is constructed.
- Around line 174-179: Add response.body?.cancel() on every early-exit branch
that throws or returns without consuming the body to avoid leaking Undici
connections: in beginOAuthFlow(), call response.body?.cancel() before throwing
on the non-302/no-location branch; in loadLoginForm(), call
response.body?.cancel() before any return/throw that skips reading the body; in
submitLoginForm(), call response.body?.cancel() on the early exits; in
authorizeAuthenticatedSession() and detectSubscriptionStatus(), call
response.body?.cancel() immediately before each return/throw path that does not
read the response body. Locate these branches by searching for the functions
beginOAuthFlow, loadLoginForm, submitLoginForm, authorizeAuthenticatedSession,
and detectSubscriptionStatus and insert response.body?.cancel() just before each
early return/throw.
---
Nitpick comments:
In `@tests/auth.test.ts`:
- Around line 35-41: The test's fake redirect response currently sets a single
`_skylight_cloud_session` cookie but downstream handlers (`/auth/session/new`
and `/auth/session` request handlers in tests/auth.test.ts that read
capturedState) do not assert that the Cookie header is replayed; update the
initial redirect response to return multiple Set-Cookie headers (e.g., an array
with `_skylight_cloud_session=abc123;...` plus another cookie) and add
assertions in the `/auth/session/new` and `/auth/session` handlers to check
request.headers.get("cookie") includes `_skylight_cloud_session=abc123` (and any
other cookie values) before proceeding, failing the test if the Cookie header is
missing or malformed so the CookieJar replay is actually enforced.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb402347-a170-4cb1-a80c-1f9f2024710b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CHANGELOG.mdCLAUDE.mdREADME.mdpackage.jsonsrc/api/auth.tssrc/api/client.tssrc/api/constants.tssrc/config.tssrc/utils/errors.tstests/auth.test.ts
|
@coderabbitai review |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
Replaces PR TheEagleByte#41 (token capture required) with PR TheEagleByte#39 which keeps SKYLIGHT_EMAIL / SKYLIGHT_PASSWORD but replaces the broken /api/sessions endpoint with the browser-style OAuth PKCE flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Confirming this works against live Skylight as of June 28, 2026. GET /oauth/authorize (with the PKCE + client_id=skylight-mobile params from this branch) returns 302 → /auth/session/new, exactly as the flow expects — so the endpoint contract is current. I also confirmed this branch merges cleanly with #31 and #33 (no conflicts), so landing this doesn't block the other open chore/reward fixes. |
|
I'm going to start maintaining my own version at https://github.com/fergbrain/skylight-mcp/ since this version seems dead (at least for now) and Skylight (and the state of MCP in general) is moving quick |
|
Thanks that'd be great. Having this is super useful. |
|
Okay, it's up at @fergbrain/skylight-mcp@1.2.0. Issues and pull requests welcome over at https://github.com/fergbrain/skylight-mcp |
2 participants
Update email/password authentication to mirror Skylight's current web OAuth sequence instead of the old session-based basic auth flow. This aligns managed login with the live API, uses the returned bearer token for subsequent requests, and reduces failures caused by the previous auth mechanism.
Also centralize shared API constants, refresh auth documentation and error guidance, and add automated coverage for the OAuth login flow alongside live smoke validation.
Summary by CodeRabbit
Release Notes v1.1.8
New Features
Documentation
Tests