Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance(api): OAuth bearer token authentication #9021

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
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
13 changes: 9 additions & 4 deletions packages/backend/src/server/api/api-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ export default (endpoint: IEndpoint, ctx: Koa.Context) => new Promise<void>((res
};

// Authentication
authenticate(body['i']).then(([user, app]) => {
// for GET requests, do not even pass on the body parameter as it is considered unsafe
authenticate(ctx.headers.authorization, ctx.method === 'GET' ? null : body['i']).then(([user, app]) => {
// API invoking
call(endpoint.name, user, app, body, ctx).then((res: any) => {
if (ctx.method === 'GET' && endpoint.meta.cacheSec && !body['i'] && !user) {
Copy link
Contributor

Choose a reason for hiding this comment

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

appがGETでBearer認証した時にキャッシュしないために
&& !ctx.headers.authorization && !app とかが必要?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, requests that have an Authorization field are not cacheable. See also RFC 7234 § 3.2 and RFC 7235 § 4.2.

Copy link
Contributor

Choose a reason for hiding this comment

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

通常キャッシュはしないのはわかるけど、あえてキャッシュ可能とサーバーから返しちゃうのは違う感だわ

Copy link
Contributor

Choose a reason for hiding this comment

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

まあ、本来ここで考慮しといた方が良かった問題ではあるけどだわ
58e83f8

Copy link
Contributor

@mei23 mei23 Jul 25, 2022

Choose a reason for hiding this comment

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

現状の実装は間違っていないし今の実装を否定するつもりもないけど、
不適切な実装や、不適切なユーザーでの挙動の上書き をある程度考慮してあげた方が親切と思うのだわ。

Copy link
Contributor

Choose a reason for hiding this comment

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

I found additional specs.

From my understanding, requests that have an Authorization field are not cacheable. See also RFC 7234 § 3.2 and RFC 7235 § 4.2.

This behavior seems to be overridden by the Cache-Control: public directive.
https://datatracker.ietf.org/doc/html/rfc7234#section-3
https://datatracker.ietf.org/doc/html/rfc7234#section-5.2.2.5

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#directives

public
:
Responses for requests with Authorization header fields must not be stored in a shared cache;
however, the public directive will cause such responses to be stored in a shared cache.

So, it seems that if you use the Authorization header, you should not make it public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but this was already the case. (Of course I cannot say if it is still the case.)

For example here, the code first checks that there is no authenticated users before setting public:

if (request.method === 'GET' && endpoint.meta.cacheSec && !body?.['i'] && !user) {
reply.header('Cache-Control', `public, max-age=${endpoint.meta.cacheSec}`);
}

I think even the check !body?.['i'] is not really necessary too? Because if the token is valid, the check for !user will be equivalent, and if it is not valid, it will always be an error.

Copy link
Contributor

@mei23 mei23 Mar 21, 2023

Choose a reason for hiding this comment

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

Yes, user always exists on successful authentication (even with appToken authentication). So I think you should be fine without !body?.['i'].

Expand Down Expand Up @@ -80,11 +81,15 @@ export default (endpoint: IEndpoint, ctx: Koa.Context) => new Promise<void>((res
}
}).catch(e => {
if (e instanceof AuthenticationError) {
reply(403, new ApiError({
message: 'Authentication failed. Please ensure your token is correct.',
ctx.response.status = 403;
ctx.response.set('WWW-Authenticate', 'Bearer');
ctx.response.body = {
message: 'Authentication failed: ' + e.message,
code: 'AUTHENTICATION_FAILED',
id: 'b0a7f5f8-dc2f-4171-b91f-de88ad238e14',
}));
kind: 'client',
};
res();
} else {
reply(500, new ApiError());
}
Expand Down
25 changes: 21 additions & 4 deletions packages/backend/src/server/api/authenticate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,25 @@ export class AuthenticationError extends Error {
}
}

export default async (token: string | null): Promise<[CacheableLocalUser | null | undefined, AccessToken | null | undefined]> => {
if (token == null) {
export default async (authorization: string | null | undefined, bodyToken: string | null): Promise<[CacheableLocalUser | null | undefined, AccessToken | null | undefined]> => {
let token: string | null = null;

// check if there is an authorization header set
if (authorization != null) {
if (bodyToken != null) {
throw new AuthenticationError('using multiple authorization schemes');
}

// check if OAuth 2.0 Bearer tokens are being used
// Authorization schemes are case insensitive
if (authorization.substring(0, 7).toLowerCase() === 'bearer ') {
token = authorization.substring(7);
} else {
throw new AuthenticationError('unsupported authentication scheme');
}
} else if (bodyToken != null) {
token = bodyToken;
} else {
return [null, null];
}

Expand All @@ -25,7 +42,7 @@ export default async (token: string | null): Promise<[CacheableLocalUser | null
() => Users.findOneBy({ token }) as Promise<ILocalUser | null>);

if (user == null) {
throw new AuthenticationError('user not found');
throw new AuthenticationError('unknown token');
}

return [user, null];
Expand All @@ -39,7 +56,7 @@ export default async (token: string | null): Promise<[CacheableLocalUser | null
});

if (accessToken == null) {
throw new AuthenticationError('invalid signature');
throw new AuthenticationError('unknown token');
}

AccessTokens.update(accessToken.id, {
Expand Down
37 changes: 28 additions & 9 deletions packages/backend/src/server/api/openapi/gen-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ export function genOpenapiSpec() {
in: 'body',
name: 'i',
},
// TODO: change this to oauth2 when the remaining oauth stuff is set up
Bearer: {
type: 'http',
scheme: 'bearer',
}
},
},
};
Expand Down Expand Up @@ -71,6 +76,19 @@ export function genOpenapiSpec() {
schema.required.push('file');
}

const security = [
{
ApiKeyAuth: [],
},
{
Bearer: [],
},
];
if (!endpoint.meta.requireCredential) {
// add this to make authentication optional
security.push({});
}

const info = {
operationId: endpoint.name,
summary: endpoint.name,
Expand All @@ -79,14 +97,8 @@ export function genOpenapiSpec() {
description: 'Source code',
url: `https://github.com/misskey-dev/misskey/blob/develop/packages/backend/src/server/api/endpoints/${endpoint.name}.ts`,
},
...(endpoint.meta.tags ? {
tags: [endpoint.meta.tags[0]],
} : {}),
...(endpoint.meta.requireCredential ? {
security: [{
ApiKeyAuth: [],
}],
} : {}),
tags: endpoint.meta.tags || undefined,
security,
requestBody: {
required: true,
content: {
Expand Down Expand Up @@ -181,9 +193,16 @@ export function genOpenapiSpec() {
},
};

spec.paths['/' + endpoint.name] = {
const path = {
post: info,
};
if (endpoint.meta.allowGet) {
path.get = { ...info };
// API Key authentication is not permitted for GET requests
path.get.security = path.get.security.filter(elem => !Object.prototype.hasOwnProperty.call(elem, 'ApiKeyAuth'));
}

spec.paths['/' + endpoint.name] = path;
}

return spec;
Expand Down
12 changes: 8 additions & 4 deletions packages/backend/src/server/api/streaming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@ export const initializeStreamingServer = (server: http.Server) => {
ws.on('request', async (request) => {
const q = request.resourceURL.query as ParsedUrlQuery;

// TODO: トークンが間違ってるなどしてauthenticateに失敗したら
// コネクション切断するなりエラーメッセージ返すなりする
// (現状はエラーがキャッチされておらずサーバーのログに流れて邪魔なので)
const [user, app] = await authenticate(q.i as string);
const [user, app] = await authenticate(request.httpRequest.headers.authorization, q.i)
.catch(err => {
request.reject(403, err.message);
return [];
});
if (typeof user === 'undefined') {
return;
}

if (user?.isSuspended) {
request.reject(400);
Expand Down
4 changes: 3 additions & 1 deletion packages/client/src/components/cropper-dialog.vue
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,16 @@ const ok = async () => {
croppedCanvas.toBlob(blob => {
const formData = new FormData();
formData.append('file', blob);
formData.append('i', $i.token);
if (defaultStore.state.uploadFolder) {
formData.append('folderId', defaultStore.state.uploadFolder);
}

fetch(apiUrl + '/drive/files/create', {
method: 'POST',
body: formData,
headers: {
authorization: `Bearer ${$i.token}`,
},
})
.then(response => response.json())
.then(f => {
Expand Down
4 changes: 3 additions & 1 deletion packages/client/src/components/page/page.post.vue
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,16 @@ export default defineComponent({
canvas.toBlob(blob => {
const formData = new FormData();
formData.append('file', blob);
formData.append('i', this.$i.token);
if (this.$store.state.uploadFolder) {
formData.append('folderId', this.$store.state.uploadFolder);
}

fetch(apiUrl + '/drive/files/create', {
method: 'POST',
body: formData,
headers: {
authorization: `Bearer ${this.$i.token}`,
},
})
.then(response => response.json())
.then(f => {
Expand Down
15 changes: 9 additions & 6 deletions packages/client/src/os.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,16 @@ export const api = ((endpoint: string, data: Record<string, any> = {}, token?: s
pendingApiRequestsCount.value--;
};

const promise = new Promise((resolve, reject) => {
// Append a credential
if ($i) (data as any).i = $i.token;
if (token !== undefined) (data as any).i = token;
const authorizationToken = token ?? $i?.token ?? undefined;
const authorization = authorizationToken ? `Bearer ${authorizationToken}` : undefined;

// Send request
const promise = new Promise((resolve, reject) => {
fetch(endpoint.indexOf('://') > -1 ? endpoint : `${apiUrl}/${endpoint}`, {
method: 'POST',
body: JSON.stringify(data),
credentials: 'omit',
cache: 'no-cache',
headers: authorization ? { authorization } : {},
}).then(async (res) => {
const body = res.status === 204 ? null : await res.json();

Expand All @@ -52,7 +51,7 @@ export const api = ((endpoint: string, data: Record<string, any> = {}, token?: s
return promise;
}) as typeof apiClient.request;

export const apiGet = ((endpoint: string, data: Record<string, any> = {}) => {
export const apiGet = ((endpoint: string, data: Record<string, any> = {}, token?: string | null | undefined) => {
pendingApiRequestsCount.value++;

const onFinally = () => {
Expand All @@ -61,12 +60,16 @@ export const apiGet = ((endpoint: string, data: Record<string, any> = {}) => {

const query = new URLSearchParams(data);

const authorizationToken = token ?? $i?.token ?? undefined;
const authorization = authorizationToken ? `Bearer ${authorizationToken}` : undefined;

const promise = new Promise((resolve, reject) => {
// Send request
fetch(`${apiUrl}/${endpoint}?${query}`, {
method: 'GET',
credentials: 'omit',
cache: 'default',
headers: authorization ? { authorization } : {},
}).then(async (res) => {
const body = res.status === 204 ? null : await res.json();

Expand Down
2 changes: 1 addition & 1 deletion packages/client/src/scripts/upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,14 @@ export function uploadFile(
}

const formData = new FormData();
formData.append('i', $i.token);
formData.append('force', 'true');
formData.append('file', resizedImage || file);
formData.append('name', ctx.name);
if (folder) formData.append('folderId', folder);

const xhr = new XMLHttpRequest();
xhr.open('POST', apiUrl + '/drive/files/create', true);
xhr.setRequestHeader('Authorization', `Bearer ${$i.token}`);
xhr.onload = (ev) => {
if (xhr.status !== 200 || ev.target == null || ev.target.response == null) {
// TODO: 消すのではなくて(ネットワーク的なエラーなら)再送できるようにしたい
Expand Down