Skip to content

Commit 1807f18

Browse files
ulrikandersenulrikandersen
committed
Prevent loading files from repos with incorrect names
Framna Docs builds upon loading files from repositories with a specific naming convention ("-openapi" by default). Prior to this change, the GitHub client used would allow loading files from all the repos the user has access to, including ones that do not have the correct name. This change adds a safeguard against accidentally loading files from non-eligible repositories.
1 parent 5c2d285 commit 1807f18

File tree

3 files changed

+202
-2
lines changed

3 files changed

+202
-2
lines changed

‎__test__/common/github/RepoRestrictedGitHubClient.test.ts‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
import { RepoRestrictedGitHubClient } from '@/common/github/RepoRestrictedGitHubClient';
2+
import {
3+
IGitHubClient,
4+
AddCommentToPullRequestRequest,
5+
GetPullRequestCommentsRequest,
6+
GetPullRequestFilesRequest,
7+
GetRepositoryContentRequest,
8+
GraphQLQueryRequest,
9+
UpdatePullRequestCommentRequest,
10+
GitHubClient
11+
} from "@/common";
12+
import { jest } from '@jest/globals';
13+
14+
describe('RepoRestrictedGitHubClient', () => {
15+
let client: RepoRestrictedGitHubClient;
16+
const repositoryNameSuffix = '-suffix';
17+
18+
const gitHubClient: jest.Mocked<IGitHubClient> = {
19+
graphql: jest.fn(),
20+
getRepositoryContent: jest.fn(),
21+
getPullRequestFiles: jest.fn(),
22+
getPullRequestComments: jest.fn(),
23+
addCommentToPullRequest: jest.fn(),
24+
updatePullRequestComment: jest.fn(),
25+
};
26+
27+
beforeEach(() => {
28+
client = new RepoRestrictedGitHubClient({
29+
repositoryNameSuffix,
30+
gitHubClient
31+
});
32+
});
33+
34+
it('should delegate graphql request to the underlying client', async () => {
35+
const request: GraphQLQueryRequest = { query: '' };
36+
await client.graphql(request);
37+
expect(gitHubClient.graphql).toHaveBeenCalledWith(request);
38+
});
39+
40+
it('should check suffix for getRepositoryContent', async () => {
41+
const request: GetRepositoryContentRequest = {
42+
repositoryName: 'repo-suffix', path: '',
43+
repositoryOwner: '',
44+
ref: undefined
45+
};
46+
await client.getRepositoryContent(request);
47+
expect(gitHubClient.getRepositoryContent).toHaveBeenCalledWith(request);
48+
});
49+
50+
it('should throw error if suffix is invalid for getRepositoryContent', async () => {
51+
const request: GetRepositoryContentRequest = {
52+
repositoryName: 'repo', path: '',
53+
repositoryOwner: '',
54+
ref: undefined
55+
};
56+
await expect(client.getRepositoryContent(request)).rejects.toThrow("Invalid repository name");
57+
});
58+
59+
it('should check suffix for getPullRequestFiles', async () => {
60+
const request: GetPullRequestFilesRequest = {
61+
repositoryName: 'repo-suffix', pullRequestNumber: 1,
62+
appInstallationId: 0,
63+
repositoryOwner: ''
64+
};
65+
await client.getPullRequestFiles(request);
66+
expect(gitHubClient.getPullRequestFiles).toHaveBeenCalledWith(request);
67+
});
68+
69+
it('should throw error if suffix is invalid for getPullRequestFiles', async () => {
70+
const request: GetPullRequestFilesRequest = {
71+
repositoryName: 'repo', pullRequestNumber: 1,
72+
appInstallationId: 0,
73+
repositoryOwner: ''
74+
};
75+
await expect(client.getPullRequestFiles(request)).rejects.toThrow("Invalid repository name");
76+
});
77+
78+
it('should check suffix for getPullRequestComments', async () => {
79+
const request: GetPullRequestCommentsRequest = {
80+
repositoryName: 'repo-suffix', pullRequestNumber: 1,
81+
appInstallationId: 0,
82+
repositoryOwner: ''
83+
};
84+
await client.getPullRequestComments(request);
85+
expect(gitHubClient.getPullRequestComments).toHaveBeenCalledWith(request);
86+
});
87+
88+
it('should throw error if suffix is invalid for getPullRequestComments', async () => {
89+
const request: GetPullRequestCommentsRequest = {
90+
repositoryName: 'repo', pullRequestNumber: 1,
91+
appInstallationId: 0,
92+
repositoryOwner: ''
93+
};
94+
await expect(client.getPullRequestComments(request)).rejects.toThrow("Invalid repository name");
95+
});
96+
97+
it('should check suffix for addCommentToPullRequest', async () => {
98+
const request: AddCommentToPullRequestRequest = {
99+
repositoryName: 'repo-suffix', pullRequestNumber: 1, body: '',
100+
appInstallationId: 0,
101+
repositoryOwner: ''
102+
};
103+
await client.addCommentToPullRequest(request);
104+
expect(gitHubClient.addCommentToPullRequest).toHaveBeenCalledWith(request);
105+
});
106+
107+
it('should throw error if suffix is invalid for addCommentToPullRequest', async () => {
108+
const request: AddCommentToPullRequestRequest = {
109+
repositoryName: 'repo', pullRequestNumber: 1, body: '',
110+
appInstallationId: 0,
111+
repositoryOwner: ''
112+
};
113+
await expect(client.addCommentToPullRequest(request)).rejects.toThrow("Invalid repository name");
114+
});
115+
116+
it('should check suffix for updatePullRequestComment', async () => {
117+
const request: UpdatePullRequestCommentRequest = {
118+
repositoryName: 'repo-suffix', commentId: 1, body: '',
119+
appInstallationId: 0,
120+
repositoryOwner: ''
121+
};
122+
await client.updatePullRequestComment(request);
123+
expect(gitHubClient.updatePullRequestComment).toHaveBeenCalledWith(request);
124+
});
125+
126+
it('should throw error if suffix is invalid for updatePullRequestComment', async () => {
127+
const request: UpdatePullRequestCommentRequest = {
128+
repositoryName: 'repo', commentId: 1, body: '',
129+
appInstallationId: 0,
130+
repositoryOwner: ''
131+
};
132+
await expect(client.updatePullRequestComment(request)).rejects.toThrow("Invalid repository name");
133+
});
134+
});

‎src/common/github/RepoRestrictedGitHubClient.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import {
2+
IGitHubClient,
3+
AddCommentToPullRequestRequest,
4+
GetPullRequestCommentsRequest,
5+
GetPullRequestFilesRequest,
6+
GetRepositoryContentRequest,
7+
GraphQLQueryRequest,
8+
GraphQlQueryResponse,
9+
PullRequestComment,
10+
PullRequestFile,
11+
RepositoryContent,
12+
UpdatePullRequestCommentRequest
13+
} from "@/common";
14+
15+
export class RepoRestrictedGitHubClient implements IGitHubClient {
16+
17+
private gitHubClient: IGitHubClient;
18+
private repositoryNameSuffix: string;
19+
20+
constructor(config: {
21+
repositoryNameSuffix: string;
22+
gitHubClient: IGitHubClient
23+
}) {
24+
this.gitHubClient = config.gitHubClient;
25+
this.repositoryNameSuffix = config.repositoryNameSuffix;
26+
}
27+
28+
graphql(request: GraphQLQueryRequest): Promise<GraphQlQueryResponse> {
29+
return this.gitHubClient.graphql(request);
30+
}
31+
32+
getRepositoryContent(request: GetRepositoryContentRequest): Promise<RepositoryContent> {
33+
if (!this.isRepositoryNameValid(request.repositoryName)) return Promise.reject(new Error("Invalid repository name"));
34+
return this.gitHubClient.getRepositoryContent(request);
35+
}
36+
37+
getPullRequestFiles(request: GetPullRequestFilesRequest): Promise<PullRequestFile[]> {
38+
if (!this.isRepositoryNameValid(request.repositoryName)) return Promise.reject(new Error("Invalid repository name"));
39+
return this.gitHubClient.getPullRequestFiles(request);
40+
}
41+
42+
getPullRequestComments(request: GetPullRequestCommentsRequest): Promise<PullRequestComment[]> {
43+
if (!this.isRepositoryNameValid(request.repositoryName)) return Promise.reject(new Error("Invalid repository name"));
44+
return this.gitHubClient.getPullRequestComments(request);
45+
}
46+
47+
addCommentToPullRequest(request: AddCommentToPullRequestRequest): Promise<void> {
48+
if (!this.isRepositoryNameValid(request.repositoryName)) return Promise.reject(new Error("Invalid repository name"));
49+
return this.gitHubClient.addCommentToPullRequest(request);
50+
}
51+
52+
updatePullRequestComment(request: UpdatePullRequestCommentRequest): Promise<void> {
53+
if (!this.isRepositoryNameValid(request.repositoryName)) return Promise.reject(new Error("Invalid repository name"));
54+
return this.gitHubClient.updatePullRequestComment(request);
55+
}
56+
57+
private isRepositoryNameValid(repositoryName: string): boolean {
58+
return repositoryName.endsWith(this.repositoryNameSuffix);
59+
}
60+
}

‎src/composition.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import {
5050
RepositoryNameEventFilter,
5151
PullRequestCommenter
5252
} from "@/features/hooks/domain"
53+
import { RepoRestrictedGitHubClient } from "./common/github/RepoRestrictedGitHubClient"
5354

5455
const gitHubAppCredentials = {
5556
appId: env.getOrThrow("GITHUB_APP_ID"),
@@ -135,13 +136,18 @@ const oauthTokenRefresher = new LockingOAuthTokenRefresher({
135136
})
136137
})
137138

138-
export const gitHubClient = new GitHubClient({
139+
const gitHubClient = new GitHubClient({
139140
...gitHubAppCredentials,
140141
oauthTokenDataSource
141142
})
142143

144+
const repoRestrictedGitHubClient = new RepoRestrictedGitHubClient({
145+
repositoryNameSuffix: env.getOrThrow("REPOSITORY_NAME_SUFFIX"),
146+
gitHubClient
147+
})
148+
143149
export const userGitHubClient = new OAuthTokenRefreshingGitHubClient({
144-
gitHubClient,
150+
gitHubClient: repoRestrictedGitHubClient,
145151
oauthTokenDataSource,
146152
oauthTokenRefresher
147153
})

0 commit comments

Comments
 (0)