mirror of
https://github.com/toeverything/AFFiNE.git
synced 2026-07-22 04:26:23 +08:00
2e95d91093
## 📝 Summary This PR fixes a regression that caused the following error during GitHub OAuth login: > Unable to parse JSON response from [https://github.com/login/oauth/access_token](https://github.com/login/oauth/access_token) Related issue: [https://github.com/toeverything/AFFiNE/issues/14334](https://github.com/toeverything/AFFiNE/issues/14334) Regression introduced in: [https://github.com/toeverything/AFFiNE/pull/14061](https://github.com/toeverything/AFFiNE/pull/14061) --- ## 🎯 Background GitHub’s OAuth access token endpoint returns different response formats depending on the request headers. To receive a JSON response, the request must include: ``` Accept: application/json ``` If the `Accept` header is missing, GitHub responds with: ``` application/x-www-form-urlencoded ``` The current implementation assumes a JSON response and parses it directly. When a non-JSON response is returned, JSON parsing fails, breaking the OAuth login flow. --- ## 🔍 Traffic Analysis (tcpdump) Network path: affine-graphql → (HTTPS) → envoy → (HTTP, tcpdump) → envoy → GitHub ### Observed Request ``` POST /login/oauth/access_token HTTP/1.1 host: github-proxy.com content-type: application/x-www-form-urlencoded accept: */* ... ``` ### Observed Response ``` HTTP/1.1 200 OK date: Sat, 28 Feb 2026 14:47:43 GMT content-type: application/x-www-form-urlencoded; charset=utf-8 ... ``` The `Accept` header was `*/*` instead of `application/json`, causing GitHub to return a form-urlencoded response. --- ## 🐛 Root Cause PR #14061 introduced a side effect in the request configuration. Although the `Accept` header was initially defined, the request options were later overwritten by the `init` parameter. Because `init.headers` replaced the previously defined headers object, the required header was lost. Resulting in: * Missing `Accept: application/json` * GitHub returning `application/x-www-form-urlencoded` * JSON parsing failure * OAuth login failure --- ## 🔧 Changes ### 1️⃣ Fix header overwrite order * Process the incoming `init` parameter first * Explicitly overwrite required headers afterward * Ensure `Accept: application/json` is always enforced --- ## 💥 Breaking Changes None. --- ## 🧪 How to Test 1. Configure GitHub OAuth. 2. Attempt login via GitHub. 3. Verify that: * The request contains `Accept: application/json` * The response content-type is `application/json` * No JSON parsing error occurs * OAuth login completes successfully --- ## 📌 Notes This change restores correct OAuth behavior and prevents regression caused by header overwriting introduced in #14061. The same header overwrite pattern identified in this issue was also found in the calendar module and has been corrected there as well. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved backend HTTP header handling for external integrations to avoid unintended header overrides, ensuring content-type and encoding hints are applied consistently and improving reliability of service requests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
136 lines
3.0 KiB
TypeScript
136 lines
3.0 KiB
TypeScript
import { Inject, Injectable, Logger } from '@nestjs/common';
|
|
|
|
import {
|
|
Config,
|
|
InvalidOauthCallbackCode,
|
|
InvalidOauthResponse,
|
|
OnEvent,
|
|
} from '../../../base';
|
|
import { OAuthProviderName } from '../config';
|
|
import { OAuthProviderFactory } from '../factory';
|
|
import type { OAuthState } from '../types';
|
|
|
|
export interface OAuthAccount {
|
|
id: string;
|
|
email: string;
|
|
name?: string;
|
|
avatarUrl?: string;
|
|
}
|
|
|
|
export interface Tokens {
|
|
accessToken: string;
|
|
scope?: string;
|
|
refreshToken?: string;
|
|
expiresAt?: Date;
|
|
idToken?: string;
|
|
tokenType?: string;
|
|
}
|
|
|
|
export interface AuthOptions {
|
|
client_id: string;
|
|
redirect_uri: string;
|
|
scope: string;
|
|
state: string;
|
|
}
|
|
|
|
@Injectable()
|
|
export abstract class OAuthProvider {
|
|
abstract provider: OAuthProviderName;
|
|
abstract getAuthUrl(state: string, clientNonce?: string): string;
|
|
abstract getToken(code: string, state: OAuthState): Promise<Tokens>;
|
|
abstract getUser(tokens: Tokens, state: OAuthState): Promise<OAuthAccount>;
|
|
|
|
protected readonly logger = new Logger(this.constructor.name);
|
|
@Inject() private readonly factory!: OAuthProviderFactory;
|
|
@Inject() private readonly AFFiNEConfig!: Config;
|
|
|
|
get config() {
|
|
return this.AFFiNEConfig.oauth.providers[this.provider];
|
|
}
|
|
|
|
get configured() {
|
|
return (
|
|
!!this.config && !!this.config.clientId && !!this.config.clientSecret
|
|
);
|
|
}
|
|
|
|
@OnEvent('config.init')
|
|
onConfigInit() {
|
|
this.setup();
|
|
}
|
|
|
|
@OnEvent('config.changed')
|
|
onConfigUpdated(event: Events['config.changed']) {
|
|
if ('oauth' in event.updates) {
|
|
this.setup();
|
|
}
|
|
}
|
|
|
|
protected setup() {
|
|
if (this.configured) {
|
|
this.factory.register(this);
|
|
} else {
|
|
this.factory.unregister(this);
|
|
}
|
|
}
|
|
|
|
get requiresPkce() {
|
|
return false;
|
|
}
|
|
|
|
protected async fetchJson<T>(
|
|
url: string,
|
|
init?: RequestInit,
|
|
options?: { treatServerErrorAsInvalid?: boolean }
|
|
) {
|
|
const response = await fetch(url, {
|
|
...init,
|
|
headers: { ...init?.headers, Accept: 'application/json' },
|
|
});
|
|
|
|
const body = await response.text();
|
|
if (!response.ok) {
|
|
if (response.status < 500 || options?.treatServerErrorAsInvalid) {
|
|
throw new InvalidOauthCallbackCode({ status: response.status, body });
|
|
}
|
|
throw new Error(
|
|
`Server responded with non-success status ${response.status}, body: ${body}`
|
|
);
|
|
}
|
|
|
|
if (!body) {
|
|
return {} as T;
|
|
}
|
|
|
|
try {
|
|
return JSON.parse(body) as T;
|
|
} catch {
|
|
throw new InvalidOauthResponse({
|
|
reason: `Unable to parse JSON response from ${url}`,
|
|
});
|
|
}
|
|
}
|
|
|
|
protected postFormJson<T>(
|
|
url: string,
|
|
body: string,
|
|
options?: {
|
|
headers?: Record<string, string>;
|
|
treatServerErrorAsInvalid?: boolean;
|
|
}
|
|
) {
|
|
return this.fetchJson<T>(
|
|
url,
|
|
{
|
|
method: 'POST',
|
|
body,
|
|
headers: {
|
|
'Content-Type': 'application/x-www-form-urlencoded',
|
|
...options?.headers,
|
|
},
|
|
},
|
|
options
|
|
);
|
|
}
|
|
}
|