Skip to content

Commit

Permalink
NONE: Re-enable more lint rules (#2594)
Browse files Browse the repository at this point in the history
Co-authored-by: joshkay10 <josh.kay@outlook.com>
  • Loading branch information
atraversatlassian and joshkay10 authored Dec 5, 2023
1 parent 859e331 commit 1ec2d17
Show file tree
Hide file tree
Showing 15 changed files with 65 additions and 58 deletions.
4 changes: 1 addition & 3 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,7 @@
"src/models/**",
"src/sync/**",
"src/transforms/**",
"src/util/**",
"src/sqs/**",
"src/middleware/**"
"src/util/**"
],
"rules": {
// To be removed later
Expand Down
2 changes: 1 addition & 1 deletion src/middleware/frontend-log-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe("frontend-log-middleware", () => {

it("preserves old fields", async () => {
await LogMiddleware(request, response, next);
expect(request.log?.fields?.foo).toBe(123);
expect(request.log.fields?.foo).toBe(123);
});

describe("log level FF", () => {
Expand Down
8 changes: 5 additions & 3 deletions src/middleware/frontend-log-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,14 @@ declare global {

export const LogMiddleware = async (req: Request<ParamsDictionary, unknown, { jiraHost?: string }>, res: Response, next: NextFunction): Promise<void> => {
req.log = getLogger("frontend-log-middleware", {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unnecessary-condition
fields: req.log?.fields,
level: await stringFlag(StringFlags.LOG_LEVEL, defaultLogLevel, getUnvalidatedJiraHost(req)),
filterHttpRequests: true
});

req.addLogFields = (fields: Record<string, unknown>): void => {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (req.log) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
req.log.fields = merge(req.log.fields, fields);
Expand All @@ -78,14 +79,15 @@ export const LogMiddleware = async (req: Request<ParamsDictionary, unknown, { ji
};

const getUnvalidatedJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHost?: string }>): string | undefined =>
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
req.session?.jiraHost || extractUnsafeJiraHost(req);

/**
* Checks if the URL matches any of the URL patterns defined in `moduleUrls`
*/
const checkPathValidity = (url: string) => moduleUrls.some(moduleUrl => matchRouteWithPattern(moduleUrl, url));

const extractUnsafeJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHost?: string }>): string | undefined => {
const extractUnsafeJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHost?: string } | undefined>): string | undefined => {
if (checkPathValidity(req.path) && req.method == "GET") {
// Only save xdm_e query when on the GET post install url (iframe url)
return req.query.xdm_e as string;
Expand All @@ -95,7 +97,7 @@ const extractUnsafeJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHos
return req.body?.jiraHost;
}

const cookies = req.cookies as { jiraHost?: string };
const cookies = req.cookies as { jiraHost?: string } | undefined;
if (cookies && cookies.jiraHost) {
return cookies.jiraHost;
}
Expand Down
31 changes: 17 additions & 14 deletions src/middleware/github-webhook-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ export const LOGGER_NAME = "github.webhooks";
// Returns an async function that reports errors errors to Sentry.
// This works similar to Sentry.withScope but works in an async context.
// A new Sentry hub is assigned to context.sentry and can be used later to add context to the error message.
const withSentry = function(callback: (context: WebhookContext<{ action?: string }>) => Promise<void>) {
const withSentry = function(callback: (context: WebhookContext<{ action?: string } | undefined>) => Promise<void>) {
return async (context: WebhookContext<{ action?: string }>) => {
context.sentry = new Sentry.Hub(Sentry.getCurrentHub().getClient());
context.sentry?.configureScope((scope) =>
context.sentry.configureScope((scope) =>
scope.addEventProcessor((event, hint) => AxiosErrorEventDecorator.decorate(event, hint))
);
context.sentry?.configureScope((scope) =>
context.sentry.configureScope((scope) =>
scope.addEventProcessor((event, hint) => SentryScopeProxy.processEvent(event, hint))
);

Expand All @@ -36,7 +36,7 @@ const withSentry = function(callback: (context: WebhookContext<{ action?: string
} catch (err: unknown) {
context.log.error({ err, context }, "Error while processing webhook");
emitWebhookFailedMetrics(extractWebhookEventNameFromContext(context), undefined);
context.sentry?.captureException(err);
context.sentry.captureException(err);
throw err;
}
};
Expand All @@ -60,15 +60,15 @@ const isStateChangeBranchCreateOrDeploymentAction = (action: string) =>
action
);

const extractWebhookEventNameFromContext = (context: WebhookContext<{ action?: string }>): string => {
const extractWebhookEventNameFromContext = (context: WebhookContext<{ action?: string } | undefined>): string => {
let webhookEvent = context.name;
if (context.payload?.action) {
webhookEvent = `${webhookEvent}.${context.payload.action}`;
}
return webhookEvent;
};

const moreWebhookSpecificTags = (webhookContext: WebhookContext<{ deployment_status?: { state?: string } }>): Record<string, string | undefined> => {
const moreWebhookSpecificTags = (webhookContext: WebhookContext<{ deployment_status?: { state?: string } } | undefined>): Record<string, string | undefined> => {
if (webhookContext.name === "deployment_status") {
return {
deploymentStatusState: webhookContext.payload?.deployment_status?.state
Expand All @@ -88,7 +88,7 @@ export const GithubWebhookMiddleware = (
sender?: { type?: string; id?: number; login?: string };
action?: string;
deployment_status?: { state?: string };
}>): Promise<void> => {
} | undefined>): Promise<void> => {
const webhookEvent = extractWebhookEventNameFromContext(context);

// Metrics for webhook payload size
Expand All @@ -102,7 +102,7 @@ export const GithubWebhookMiddleware = (
action: context.payload?.action,
id: context.id,
repo: context.payload?.repository ? {
owner: context.payload.repository?.owner?.login,
owner: context.payload.repository.owner?.login,
repo: context.payload.repository.name
} : undefined,
payload: context.payload,
Expand All @@ -116,7 +116,9 @@ export const GithubWebhookMiddleware = (
context.log.info("Halting further execution since no installation id found.");
return;
}
const gitHubInstallationId = Number(payload?.installation?.id);
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
const gitHubInstallationId = Number(payload.installation?.id);
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
const gitHubAppId = context.gitHubAppConfig?.gitHubAppId;

const subscriptions = await Subscription.getAllForInstallation(gitHubInstallationId, gitHubAppId);
Expand All @@ -140,7 +142,7 @@ export const GithubWebhookMiddleware = (

const gitHubProduct = getCloudOrServerFromGitHubAppId(gitHubAppId);

const action = String(payload?.action);
const action = String(payload.action);
statsd.increment(metricWebhooks.webhookEvent, {
name: "webhooks",
event: name,
Expand All @@ -152,22 +154,22 @@ export const GithubWebhookMiddleware = (
// Edit actions are not allowed because they trigger this Jira integration to write data in GitHub and can trigger events, causing an infinite loop.
// State change actions are allowed because they're one-time actions, therefore they won’t cause a loop.
if (
context.payload?.sender?.type === "Bot" &&
payload.sender?.type === "Bot" &&
!isStateChangeBranchCreateOrDeploymentAction(action) &&
!isStateChangeBranchCreateOrDeploymentAction(context.name)
) {
context.log.info(
{
noop: "bot",
botId: context.payload?.sender?.id,
botLogin: context.payload?.sender?.login
botId: payload.sender.id,
botLogin: payload.sender.login
},
"Halting further execution since the sender is a bot and action is not a state change nor a deployment"
);
return;
}

if (isFromIgnoredRepo(context.payload)) {
if (isFromIgnoredRepo(payload)) {
context.log.info(
{
installation_id: context.payload?.installation?.id,
Expand Down Expand Up @@ -232,6 +234,7 @@ export const GithubWebhookMiddleware = (
const jiraClient = await getJiraClient(
jiraHost,
gitHubInstallationId,
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
context.gitHubAppConfig?.gitHubAppId,
context.log
);
Expand Down
19 changes: 8 additions & 11 deletions src/middleware/jira-symmetric-jwt-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
authorization: `JWT ${await generateJwt()}`
});

expect(await checkGenericContainerActionUrl(
"https://test-github-app-instance.com/jira/workspaces/search"))
expect(checkGenericContainerActionUrl("https://test-github-app-instance.com/jira/workspaces/search"))
.toBeTruthy();
});

Expand All @@ -267,8 +266,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
)}`
});

expect(await checkGenericContainerActionUrl(
"https://test-github-app-instance.com/jira/workspaces/repositories/search?searchQuery=atlas"))
expect(checkGenericContainerActionUrl("https://test-github-app-instance.com/jira/workspaces/repositories/search?searchQuery=atlas"))
.toBeTruthy();
});

Expand All @@ -290,7 +288,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
authorization: `JWT ${await generateJwt()}`
});

expect(await checkGenericContainerActionUrl("https://test-github-app-instance.com/jira/workspaces/repositories/associate")).toBeTruthy();
expect(checkGenericContainerActionUrl("https://test-github-app-instance.com/jira/workspaces/repositories/associate")).toBeTruthy();
});

it("should return false for create branch", async () => {
Expand All @@ -302,8 +300,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
githubToken: "random-token"
}));

expect(await checkGenericContainerActionUrl(
"https://test-github-app-instance.com/create-branch-options"))
expect(checkGenericContainerActionUrl("https://test-github-app-instance.com/create-branch-options"))
.toBeFalsy();
});
});
Expand Down Expand Up @@ -339,7 +336,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
authorization: `JWT ${await generateJwt()}`
});

expect(await getTokenType("/jira/workspaces/search", "GET")).toEqual("normal");
expect(getTokenType("/jira/workspaces/search", "GET")).toEqual("normal");
});

it("should return normal tokenType for search repositories", async () => {
Expand All @@ -364,7 +361,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
)}`
});

expect(await getTokenType("/jira/workspaces/repositories/search?searchQuery=atlas", "GET")).toEqual("normal");
expect(getTokenType("/jira/workspaces/repositories/search?searchQuery=atlas", "GET")).toEqual("normal");
});

it("should return normal tokenType for associate repository", async () => {
Expand All @@ -385,7 +382,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
authorization: `JWT ${await generateJwt()}`
});

expect(await getTokenType("/jira/workspaces/repositories/associate", "POST")).toEqual("normal");
expect(getTokenType("/jira/workspaces/repositories/associate", "POST")).toEqual("normal");
});

it("should return context tokenType for create branch", async () => {
Expand All @@ -397,7 +394,7 @@ describe("jiraSymmetricJwtMiddleware", () => {
githubToken: "random-token"
}));

expect(await getTokenType("/create-branch-options", "GET")).toEqual("context");
expect(getTokenType("/create-branch-options", "GET")).toEqual("context");
});
});

Expand Down
19 changes: 9 additions & 10 deletions src/middleware/jira-symmetric-jwt-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@ export const jiraSymmetricJwtMiddleware = async (req: Request<ParamsDictionary,
const authHeader = req.headers["authorization"] as string;
const authHeaderPrefix = "JWT ";
const cookies = req.cookies as { jwt?: string };
const token = req.query?.jwt
|| cookies?.jwt
|| req.body?.jwt
|| authHeader?.startsWith(authHeaderPrefix) && authHeader.substring(authHeaderPrefix.length);
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
const token = req.query?.jwt || cookies?.jwt || req.body?.jwt || authHeader?.startsWith(authHeaderPrefix) && authHeader.substring(authHeaderPrefix.length);
if (token) {
let issuer: string | undefined;
try {
Expand Down Expand Up @@ -57,6 +55,7 @@ export const jiraSymmetricJwtMiddleware = async (req: Request<ParamsDictionary,
req.addLogFields({ jiraHost: installation.jiraHost });
next(); return;

// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
} else if (req.session?.jiraHost) {

const installation = await Installation.getForHost(req.session.jiraHost);
Expand Down Expand Up @@ -96,9 +95,9 @@ const getIssuer = (token: string, logger: Logger): string | undefined => {
return unverifiedClaims.iss;
};

export const getTokenType = async (url: string, method: string): Promise<TokenType> =>
export const getTokenType = (url: string, method: string): TokenType =>
checkPathValidity(url) && method == "GET"
|| await checkGenericContainerActionUrl(`${envVars.APP_URL}${url}`)
|| checkGenericContainerActionUrl(`${envVars.APP_URL}${url}`)
|| checkSecurityContainerActionUrl(`${envVars.APP_URL}${url}`) ? TokenType.normal
: TokenType.context;

Expand All @@ -108,7 +107,7 @@ const verifySymmetricJwt = async (req: Request, token: string, installation: Ins

try {
const claims = decodeSymmetric(token, secret, algorithm, false) as Record<string, string | number>;
const tokenType = await getTokenType(req.originalUrl, req.method);
const tokenType = getTokenType(req.originalUrl, req.method);

verifyJwtClaims(claims, tokenType, req);
return claims;
Expand Down Expand Up @@ -147,10 +146,10 @@ const checkPathValidity = (url: string) => {
});
};

export const checkGenericContainerActionUrl = async (url: string): Promise<boolean | undefined> => {
const genericContainerActionUrls = await getGenericContainerUrls();
export const checkGenericContainerActionUrl = (url: string): boolean | undefined => {
const genericContainerActionUrls = getGenericContainerUrls();

return genericContainerActionUrls?.some(moduleUrl => {
return genericContainerActionUrls.some(moduleUrl => {
return matchRouteWithPattern(moduleUrl, url);
});
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const SEARCH_CONNECTED_WORKSPACES_ENDPOINT = `${envVars.APP_URL}/jira/workspaces
const SEARCH_REPOSITORIES_ENDPOINT = `${envVars.APP_URL}/jira/workspaces/repositories/search`;
const ASSOCIATE_REPOSITORY_ENDPOINT = `${envVars.APP_URL}/jira/workspaces/repositories/associate`;

export const getGenericContainerUrls = async (): Promise<string[]> => {
export const getGenericContainerUrls = (): string[] => {
return [
SEARCH_CONNECTED_WORKSPACES_ENDPOINT,
SEARCH_REPOSITORIES_ENDPOINT,
Expand Down
4 changes: 2 additions & 2 deletions src/sqs/backfill-discovery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe("Discovery Queue Test - GitHub Client", () => {
const subscription = await Subscription.getSingleInstallation(jiraHost, TEST_INSTALLATION_ID, undefined);
expect(subscription).toBeTruthy();
const states = await RepoSyncState.findAllFromSubscription(subscription!, 100, 0, [["id", "ASC"]]);
expect(states?.length).toBe(2);
expect(states.length).toBe(2);
});
});

Expand All @@ -153,7 +153,7 @@ describe("Discovery Queue Test - GitHub Client", () => {
const subscription = await Subscription.getSingleInstallation(jiraHost, TEST_INSTALLATION_ID, gitHubServerApp.id);
expect(subscription).toBeTruthy();
const states = await RepoSyncState.findAllFromSubscription(subscription!, 100, 0, [["id", "ASC"]]);
expect(states?.length).toBe(2);
expect(states.length).toBe(2);
});
});
});
2 changes: 1 addition & 1 deletion src/sqs/backfill-error-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe("backfillErrorHandler", () => {
subscription = ret.subscription;
repoSyncState = ret.repoSyncState!;

task = { task: "commit", repositoryId: repoSyncState?.repoId, repository: _.cloneDeep(TEST_REPO) };
task = { task: "commit", repositoryId: repoSyncState.repoId, repository: _.cloneDeep(TEST_REPO) };
sendMessageMock = jest.fn();
});

Expand Down
2 changes: 2 additions & 0 deletions src/sqs/branch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { getCloudOrServerFromGitHubAppId } from "utils/get-cloud-or-server";

export const branchQueueMessageHandler: MessageHandler<BranchMessagePayload> = async (context: SQSMessageContext<BranchMessagePayload>) => {
const messagePayload: BranchMessagePayload = context.payload;
// Validate payload before using
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (messagePayload.webhookReceived === undefined || messagePayload.webhookPayload === undefined || messagePayload.webhookId === undefined) {
context.log.error({ messagePayload }, "Missing required fields");
return;
Expand Down
2 changes: 2 additions & 0 deletions src/sqs/deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { DeploymentMessagePayload, MessageHandler, SQSMessageContext } from "./s

export const deploymentQueueMessageHandler: MessageHandler<DeploymentMessagePayload> = async (context: SQSMessageContext<DeploymentMessagePayload>) => {
const messagePayload: DeploymentMessagePayload = context.payload;
// validate payload before using it
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (messagePayload.webhookReceived === undefined || messagePayload.webhookPayload === undefined || messagePayload.webhookId === undefined) {
context.log.error({ messagePayload }, "Missing required fields");
}
Expand Down
1 change: 1 addition & 0 deletions src/sqs/error-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export const webhookMetricWrapper = <MessagePayload extends BaseMessagePayload>(

if (errorHandlingResult.isFailure && (!errorHandlingResult.retryable || context.lastAttempt)) {
context.log.error({ error }, `${webhookName} webhook processing failed and won't be retried anymore`);
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
emitWebhookFailedMetrics(webhookName, context.payload?.jiraHost);
}

Expand Down
2 changes: 2 additions & 0 deletions src/sqs/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { booleanFlag, BooleanFlags } from "config/feature-flags";

export const pushQueueMessageHandler: MessageHandler<PushQueueMessagePayload> = async (context: SQSMessageContext<PushQueueMessagePayload>) => {
const { payload } = context;
// validate payload before using it
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (payload.repository === undefined || payload.repository.full_name === undefined || payload.shas === undefined || payload.webhookId === undefined) {
context.log.error({ payload }, "Missing required fields");
return;
Expand Down
1 change: 1 addition & 0 deletions src/sqs/sqs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ describe("SQS", () => {
});

afterEach(async () => {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (queue) {
await queue.stop();
await queue.purgeQueue();
Expand Down
Loading

0 comments on commit 1ec2d17

Please sign in to comment.