Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions backend/chainlit/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -668,29 +668,27 @@ async def oauth_callback(
return _get_oauth_redirect_error(request, error)

if not code or not state:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Missing code or state",
)
return _get_oauth_redirect_error(request, "oauthSignin")
Comment thread
dokterbob marked this conversation as resolved.

try:
validate_oauth_state_cookie(request, state)
except Exception as e:
logger.exception("Unable to validate oauth state: %s", e)

raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Unauthorized",
)
return _get_oauth_redirect_error(request, "oauthSignin")
Comment thread
dokterbob marked this conversation as resolved.

url = get_user_facing_url(request.url)
token = await provider.get_token(code, url)

(raw_user_data, default_user) = await provider.get_user_info(token)
try:
token = await provider.get_token(code, url)
(raw_user_data, default_user) = await provider.get_user_info(token)
user = await config.code.oauth_callback(
provider_id, token, raw_user_data, default_user
)
except Exception as e:
logger.exception("OAuth callback error: %s", e)
return _get_oauth_redirect_error(request, "oauthSignin")
Comment thread
Copilot marked this conversation as resolved.

user = await config.code.oauth_callback(
provider_id, token, raw_user_data, default_user
)
if not user:
return _get_oauth_redirect_error(request, "oauthSignin")

response = await _authenticate_user(request, user, redirect_to_callback=True)

Expand Down Expand Up @@ -727,19 +725,21 @@ async def oauth_azure_hf_callback(
return _get_oauth_redirect_error(request, error)

if not code:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Missing code",
)
return _get_oauth_redirect_error(request, "oauthSignin")
Comment thread
dokterbob marked this conversation as resolved.
Outdated

url = get_user_facing_url(request.url)
token = await provider.get_token(code, url)

(raw_user_data, default_user) = await provider.get_user_info(token)
try:
token = await provider.get_token(code, url)
(raw_user_data, default_user) = await provider.get_user_info(token)
user = await config.code.oauth_callback(
provider_id, token, raw_user_data, default_user, id_token
)
except Exception as e:
logger.exception("OAuth callback error: %s", e)
return _get_oauth_redirect_error(request, "oauthSignin")

user = await config.code.oauth_callback(
provider_id, token, raw_user_data, default_user, id_token
)
if not user:
return _get_oauth_redirect_error(request, "oauthSignin")

response = await _authenticate_user(request, user, redirect_to_callback=True)

Expand Down
153 changes: 153 additions & 0 deletions backend/tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -1152,3 +1152,156 @@ def test_health_check(test_client: TestClient):
response = test_client.get("/health")
assert response.status_code == 200
assert response.json() == {"status": "ok"}


# ---------------------------------------------------------------------------
# OAuth callback redirect tests (regression for #1273)
# ---------------------------------------------------------------------------


@pytest.fixture
def mock_oauth_provider(monkeypatch: pytest.MonkeyPatch):
"""Return a mock OAuthProvider and patch it into chainlit.server."""
from chainlit.user import User

provider = Mock()
provider.id = "github"
provider.get_token = AsyncMock(return_value="mock_token")
provider.get_user_info = AsyncMock(
return_value=({"login": "user"}, User(identifier="user"))
)
monkeypatch.setattr("chainlit.server.get_oauth_provider", lambda _: provider)
return provider


def test_oauth_callback_provider_error_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""Provider-returned error param already redirects — guard the existing path."""
test_config.code.oauth_callback = AsyncMock()
response = test_client.get(
"/auth/oauth/github/callback?error=access_denied",
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]


def test_oauth_callback_missing_code_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""Missing code/state → redirect instead of 400 JSON (regression for #1273)."""
test_config.code.oauth_callback = AsyncMock()
response = test_client.get(
"/auth/oauth/github/callback",
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]


def test_oauth_callback_invalid_state_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""Invalid state cookie → redirect instead of 401 JSON (regression for #1273)."""
test_config.code.oauth_callback = AsyncMock()
# No oauth_state cookie set, so state validation will fail
response = test_client.get(
"/auth/oauth/github/callback?code=fake_code&state=fake_state",
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]


def test_oauth_callback_get_token_error_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""get_token exception → redirect instead of 500 (regression for #1273)."""
test_config.code.oauth_callback = AsyncMock()
mock_oauth_provider.get_token = AsyncMock(side_effect=Exception("provider down"))
response = test_client.get(
"/auth/oauth/github/callback?code=fake_code&state=valid_state",
cookies={"oauth_state": "valid_state"},
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]


def test_oauth_callback_none_user_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""oauth_callback returns None → redirect instead of 401 JSON (regression for #1273)."""
test_config.code.oauth_callback = AsyncMock(return_value=None)
response = test_client.get(
"/auth/oauth/github/callback?code=fake_code&state=valid_state",
cookies={"oauth_state": "valid_state"},
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]


# ---------------------------------------------------------------------------
# Azure AD hybrid flow redirect tests (regression for #1273)
# ---------------------------------------------------------------------------


def test_oauth_azure_hf_callback_missing_code_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""Missing code in Azure hybrid flow → redirect instead of 400 JSON (regression for #1273)."""
test_config.code.oauth_callback = AsyncMock()
response = test_client.post(
"/auth/oauth/azure-ad-hybrid/callback",
data={},
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]
Comment thread
dokterbob marked this conversation as resolved.
Outdated


def test_oauth_azure_hf_callback_get_token_error_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""get_token exception in Azure hybrid flow → redirect instead of 500 (regression for #1273)."""
test_config.code.oauth_callback = AsyncMock()
mock_oauth_provider.get_token = AsyncMock(side_effect=Exception("provider down"))
response = test_client.post(
"/auth/oauth/azure-ad-hybrid/callback",
data={"code": "fake_code"},
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]

Comment thread
dokterbob marked this conversation as resolved.

def test_oauth_azure_hf_callback_none_user_redirects(
test_client: TestClient,
test_config,
mock_oauth_provider,
):
"""oauth_callback returns None in Azure hybrid flow → redirect (regression for #1273)."""
test_config.code.oauth_callback = AsyncMock(return_value=None)
response = test_client.post(
"/auth/oauth/azure-ad-hybrid/callback",
data={"code": "fake_code"},
follow_redirects=False,
)
assert response.status_code in (302, 307)
assert "/login?error=" in response.headers["location"]
Comment thread
dokterbob marked this conversation as resolved.
Outdated
23 changes: 23 additions & 0 deletions cypress/e2e/oauth_auth/main.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import os
from typing import Optional

os.environ["CHAINLIT_AUTH_SECRET"] = "SUPER_SECRET" # nosec B105
os.environ["OAUTH_GITHUB_CLIENT_ID"] = "fake_client_id" # nosec B105
os.environ["OAUTH_GITHUB_CLIENT_SECRET"] = "fake_client_secret" # nosec B105

import chainlit as cl # noqa: E402


@cl.oauth_callback
def oauth_callback(
provider_id: str,
token: str,
raw_user_data: dict,
default_user: cl.User,
) -> Optional[cl.User]:
return default_user


@cl.on_chat_start
async def on_chat_start():
await cl.Message("Hello").send()
37 changes: 37 additions & 0 deletions cypress/e2e/oauth_auth/spec.cy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
describe('OAuth Auth Error UX (#1273)', () => {
describe('OAuth callback failure paths redirect instead of returning raw JSON', () => {
it('redirects on provider-returned error param', () => {
cy.request({
url: '/auth/oauth/github/callback?error=access_denied',
followRedirect: false,
failOnStatusCode: false
}).then((response) => {
expect(response.status).to.be.oneOf([301, 302, 303, 307, 308]);
expect(response.headers['location']).to.include('/login?error=');
});
Comment thread
dokterbob marked this conversation as resolved.
});

it('redirects on invalid state (no raw JSON 401) — regression for #1273', () => {
cy.request({
url: '/auth/oauth/github/callback?code=fake_code&state=fake_state',
followRedirect: false,
failOnStatusCode: false
}).then((response) => {
expect(response.status).to.be.oneOf([301, 302, 303, 307, 308]);
expect(response.headers['location']).to.include('/login?error=');
expect(response.body).to.not.include('{"detail"');
Comment thread
dokterbob marked this conversation as resolved.
Outdated
});
Comment thread
dokterbob marked this conversation as resolved.
});
});

describe('login page renders friendly OAuth error message', () => {
it('shows a specific message for oauthSignin error', () => {
cy.visit('/login?error=oauthSignin');
cy.get('[role="alert"]').should(
'contain',
'Try signing in with a different account'
);
cy.get('body').should('not.contain', '{"detail"');
});
});
});
1 change: 1 addition & 0 deletions frontend/src/components/LoginForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export function LoginForm({
{errorState && (
<Alert variant="error">
{t([
`auth.login.errors.${errorState}`,
`auth.login.errors.${errorState.toLowerCase()}`,
`auth.login.errors.default`
])}
Expand Down
Loading