1a8be234de
## Summary Follow-up to #18267. Hardens the OAuth implementation with security fixes identified during audit: **P0 — Critical:** - Bind authorization codes to `client_id` in context to prevent auth code injection (RFC 6749 §4.1.3) - Store PKCE `code_challenge` directly in auth code context instead of a separate `CodeChallenge` token — cryptographically binds the challenge to its code - Enforce `code_verifier` when `code_challenge` was used during authorization - Hash authorization codes (SHA-256) before storage to prevent exposure if DB is compromised - Add `Cache-Control: no-store` + `Pragma: no-cache` headers on token responses (RFC 6749 §5.1) - Add rate limiting on `/oauth/token` endpoint (20 req/min per client via existing `ThrottlerService`) **P1 — High:** - Return HTTP 401 for `invalid_client` errors instead of 400 (RFC 6749 §5.2) - Verify refresh tokens belong to the presenting client (cross-client token theft prevention) - Limit fields exposed by public `findApplicationRegistrationByClientId` query to only what the frontend needs (`id`, `name`, `logoUrl`, `websiteUrl`, `oAuthScopes`) - Require `API_KEYS_AND_WEBHOOKS` permission for `createApplicationRegistration` mutation **P2/P3 — Medium/Low:** - Add error handling and loading states to frontend Authorize page - Rename redirect URL param from `authorizationCode` to `code` (RFC standard) - Add unit tests for `validateRedirectUri` utility (8 test cases) ## Test plan - [ ] Existing OAuth integration tests updated for all changes (hashed codes, context-based PKCE, client binding, 401 status codes, cache headers) - [ ] New test: auth code rejected when presented by a different client - [ ] New test: refresh token rejected when presented by a different client - [ ] New test: `code_verifier` required when PKCE was used in authorization - [ ] New test: `Cache-Control: no-store` header present on responses - [ ] New unit tests for `validateRedirectUri` (HTTPS, localhost, fragments, invalid URIs) - [ ] Verify frontend authorize page shows errors gracefully Made with [Cursor](https://cursor.com)
155 lines
4.3 KiB
TypeScript
155 lines
4.3 KiB
TypeScript
import {
|
|
createApplicationRegistrationVariableMutationFactory,
|
|
deleteApplicationRegistrationVariableMutationFactory,
|
|
findApplicationRegistrationVariablesQueryFactory,
|
|
updateApplicationRegistrationVariableMutationFactory,
|
|
} from 'test/integration/metadata/suites/application-registration-variable/utils/application-registration-variable-query-factories.util';
|
|
import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util';
|
|
import { type CommonResponseBody } from 'test/integration/metadata/types/common-response-body.type';
|
|
import { warnIfErrorButNotExpectedToFail } from 'test/integration/metadata/utils/warn-if-error-but-not-expected-to-fail.util';
|
|
import { warnIfNoErrorButExpectedToFail } from 'test/integration/metadata/utils/warn-if-no-error-but-expected-to-fail.util';
|
|
|
|
import { type ApplicationRegistrationVariableEntity } from 'src/engine/core-modules/application-registration/application-registration-variable.entity';
|
|
|
|
type VariableFields = Pick<
|
|
ApplicationRegistrationVariableEntity,
|
|
| 'id'
|
|
| 'key'
|
|
| 'description'
|
|
| 'isSecret'
|
|
| 'isRequired'
|
|
| 'isFilled'
|
|
| 'createdAt'
|
|
| 'updatedAt'
|
|
>;
|
|
|
|
const handleExpectation = (
|
|
response: { body: { errors?: unknown[]; data?: unknown } },
|
|
expectToFail: boolean | undefined,
|
|
operationName: string,
|
|
) => {
|
|
if (expectToFail === true) {
|
|
warnIfNoErrorButExpectedToFail({
|
|
response: response as never,
|
|
errorMessage: `${operationName} should have failed but did not`,
|
|
});
|
|
}
|
|
|
|
if (expectToFail === false) {
|
|
warnIfErrorButNotExpectedToFail({
|
|
response: response as never,
|
|
errorMessage: `${operationName} has failed but should not`,
|
|
});
|
|
}
|
|
};
|
|
|
|
export const findApplicationRegistrationVariables = async ({
|
|
applicationRegistrationId,
|
|
expectToFail,
|
|
token,
|
|
}: {
|
|
applicationRegistrationId: string;
|
|
expectToFail?: boolean;
|
|
token?: string;
|
|
}): CommonResponseBody<{
|
|
findApplicationRegistrationVariables: VariableFields[];
|
|
}> => {
|
|
const graphqlOperation = findApplicationRegistrationVariablesQueryFactory({
|
|
applicationRegistrationId,
|
|
});
|
|
|
|
const response = await makeMetadataAPIRequest(graphqlOperation, token);
|
|
|
|
handleExpectation(response, expectToFail, 'Find variables');
|
|
|
|
return { data: response.body.data, errors: response.body.errors };
|
|
};
|
|
|
|
export const createApplicationRegistrationVariable = async ({
|
|
applicationRegistrationId,
|
|
key,
|
|
value,
|
|
description,
|
|
isSecret,
|
|
expectToFail,
|
|
token,
|
|
}: {
|
|
applicationRegistrationId: string;
|
|
key: string;
|
|
value: string;
|
|
description?: string;
|
|
isSecret?: boolean;
|
|
expectToFail?: boolean;
|
|
token?: string;
|
|
}): CommonResponseBody<{
|
|
createApplicationRegistrationVariable: VariableFields;
|
|
}> => {
|
|
const graphqlOperation = createApplicationRegistrationVariableMutationFactory(
|
|
{
|
|
applicationRegistrationId,
|
|
key,
|
|
value,
|
|
description,
|
|
isSecret,
|
|
},
|
|
);
|
|
|
|
const response = await makeMetadataAPIRequest(graphqlOperation, token);
|
|
|
|
handleExpectation(response, expectToFail, 'Create variable');
|
|
|
|
return { data: response.body.data, errors: response.body.errors };
|
|
};
|
|
|
|
export const updateApplicationRegistrationVariable = async ({
|
|
id,
|
|
value,
|
|
description,
|
|
expectToFail,
|
|
token,
|
|
}: {
|
|
id: string;
|
|
value?: string;
|
|
description?: string;
|
|
expectToFail?: boolean;
|
|
token?: string;
|
|
}): CommonResponseBody<{
|
|
updateApplicationRegistrationVariable: VariableFields;
|
|
}> => {
|
|
const graphqlOperation = updateApplicationRegistrationVariableMutationFactory(
|
|
{
|
|
id,
|
|
value,
|
|
description,
|
|
},
|
|
);
|
|
|
|
const response = await makeMetadataAPIRequest(graphqlOperation, token);
|
|
|
|
handleExpectation(response, expectToFail, 'Update variable');
|
|
|
|
return { data: response.body.data, errors: response.body.errors };
|
|
};
|
|
|
|
export const deleteApplicationRegistrationVariable = async ({
|
|
id,
|
|
expectToFail,
|
|
token,
|
|
}: {
|
|
id: string;
|
|
expectToFail?: boolean;
|
|
token?: string;
|
|
}): CommonResponseBody<{
|
|
deleteApplicationRegistrationVariable: boolean;
|
|
}> => {
|
|
const graphqlOperation = deleteApplicationRegistrationVariableMutationFactory(
|
|
{ id },
|
|
);
|
|
|
|
const response = await makeMetadataAPIRequest(graphqlOperation, token);
|
|
|
|
handleExpectation(response, expectToFail, 'Delete variable');
|
|
|
|
return { data: response.body.data, errors: response.body.errors };
|
|
};
|