Skip to content

Commit b4e05bb

Browse files
brendan-kellambrendan-kellam
committed
fix(web): show 404 for invalid browse paths
Fixes WEB-8
1 parent 86f54a6 commit b4e05bb

4 files changed

Lines changed: 104 additions & 63 deletions

File tree

‎packages/web/src/app/(app)/browse/[...path]/page.tsx‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { CommitsPanel } from "./components/commitHistoryPanel/commitsPanel";
77
import { Loader2 } from "lucide-react";
88
import { TreePreviewPanel } from "./components/treePreviewPanel/treePreviewPanel";
99
import { Metadata } from "next";
10+
import { notFound } from "next/navigation";
1011
import { TrackRepoVisit } from "./components/trackRepoVisit";
1112
import { auth } from "@/auth";
1213

@@ -24,6 +25,10 @@ const parsePathForTitle = (path: string[]): string => {
2425
const pathParam = path.join('/');
2526

2627
const browseProps = getBrowseParamsFromPathParam(pathParam);
28+
if (!browseProps) {
29+
return 'Browse';
30+
}
31+
2732
const { repoName, revisionName, path: filePath } = browseProps;
2833

2934
// Build the base repository and revision string.
@@ -104,6 +109,10 @@ export default async function BrowsePage(props: BrowsePageProps) {
104109

105110
const rawPath = _rawPath.join('/');
106111
const browseProps = getBrowseParamsFromPathParam(rawPath);
112+
if (!browseProps) {
113+
notFound();
114+
}
115+
107116
const { repoName, revisionName, path } = browseProps;
108117

109118
const page = Math.max(1, parseInt(searchParams.page ?? '1', 10) || 1);
@@ -166,4 +175,3 @@ export default async function BrowsePage(props: BrowsePageProps) {
166175
</div>
167176
)
168177
}
169-

‎packages/web/src/app/(app)/browse/hooks/useBrowseParams.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { usePathname } from "next/navigation";
1+
import { notFound, usePathname } from "next/navigation";
22
import { useMemo } from "react";
33
import { getBrowseParamsFromPathParam } from "./utils";
44

@@ -8,11 +8,15 @@ export const useBrowseParams = () => {
88
return useMemo(() => {
99
const startIndex = pathname.indexOf('/browse/');
1010
if (startIndex === -1) {
11-
throw new Error(`Invalid browse pathname: "${pathname}" - expected to contain "/browse/"`);
11+
notFound();
1212
}
1313

1414
const rawPath = pathname.substring(startIndex + '/browse/'.length);
15-
return getBrowseParamsFromPathParam(rawPath);
15+
const browseParams = getBrowseParamsFromPathParam(rawPath);
16+
if (!browseParams) {
17+
notFound();
18+
}
19+
20+
return browseParams;
1621
}, [pathname]);
1722
}
18-

‎packages/web/src/app/(app)/browse/hooks/utils.test.ts‎

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,29 @@ describe('getBrowseParamsFromPathParam', () => {
152152
});
153153
});
154154

155+
describe('history paths', () => {
156+
it('should parse a commits path', () => {
157+
const result = getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/commits/packages/web');
158+
expect(result).toEqual({
159+
repoName: 'github.com/sourcebot-dev/zoekt',
160+
revisionName: 'HEAD',
161+
path: 'packages/web',
162+
pathType: 'commits',
163+
});
164+
});
165+
166+
it('should parse a commit path', () => {
167+
const result = getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt/-/commit/a1b2c3d');
168+
expect(result).toEqual({
169+
repoName: 'github.com/sourcebot-dev/zoekt',
170+
revisionName: undefined,
171+
path: '',
172+
pathType: 'commit',
173+
commitSha: 'a1b2c3d',
174+
});
175+
});
176+
});
177+
155178
describe('edge cases', () => {
156179
it('should handle repo name with multiple @ symbols', () => {
157180
const result = getBrowseParamsFromPathParam('gitlab.com/user@domain/repo@main/-/tree/');
@@ -175,40 +198,44 @@ describe('getBrowseParamsFromPathParam', () => {
175198
});
176199

177200
describe('error cases', () => {
178-
it('should throw error for blob path with trailing slash and no path', () => {
179-
expect(() => {
180-
getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/blob/');
181-
}).toThrow();
201+
it('should return null for blob path with trailing slash and no path', () => {
202+
expect(getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/blob/')).toBeNull();
203+
});
204+
205+
it('should return null for blob path without trailing slash and no path', () => {
206+
expect(getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/blob')).toBeNull();
207+
});
208+
209+
it('should return null for invalid pattern - missing /-/', () => {
210+
expect(getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/tree/')).toBeNull();
211+
});
212+
213+
it('should return null for invalid pattern - missing tree/blob', () => {
214+
expect(getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/invalid/')).toBeNull();
215+
});
216+
217+
it('should return null for completely invalid format', () => {
218+
expect(getBrowseParamsFromPathParam('invalid-path')).toBeNull();
182219
});
183220

184-
it('should throw error for blob path without trailing slash and no path', () => {
185-
expect(() => {
186-
getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/blob');
187-
}).toThrow();
221+
it('should return null for empty string', () => {
222+
expect(getBrowseParamsFromPathParam('')).toBeNull();
188223
});
189224

190-
it('should throw error for invalid pattern - missing /-/', () => {
191-
expect(() => {
192-
getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/tree/');
193-
}).toThrow();
225+
it('should return null for an empty repository name', () => {
226+
expect(getBrowseParamsFromPathParam('/-/tree')).toBeNull();
194227
});
195228

196-
it('should throw error for invalid pattern - missing tree/blob', () => {
197-
expect(() => {
198-
getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/invalid/');
199-
}).toThrow();
229+
it('should return null for a commit path without a SHA', () => {
230+
expect(getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt/-/commit')).toBeNull();
200231
});
201232

202-
it('should throw error for completely invalid format', () => {
203-
expect(() => {
204-
getBrowseParamsFromPathParam('invalid-path');
205-
}).toThrow();
233+
it('should return null for invalid URL encoding', () => {
234+
expect(getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/blob/%')).toBeNull();
206235
});
207236

208-
it('should throw error for empty string', () => {
209-
expect(() => {
210-
getBrowseParamsFromPathParam('');
211-
}).toThrow();
237+
it('should return null when the browse type is only a prefix', () => {
238+
expect(getBrowseParamsFromPathParam('github.com/sourcebot-dev/zoekt@HEAD/-/treehouse')).toBeNull();
212239
});
213240
});
214-
});
241+
});

‎packages/web/src/app/(app)/browse/hooks/utils.ts‎

Lines changed: 35 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -59,65 +59,68 @@ export type BrowsePathType = BrowseProps['pathType'];
5959
// both map to the empty path.
6060
const normalizeRepoPath = (path: string): string => path.replace(/^\/+/, '');
6161

62-
export const getBrowseParamsFromPathParam = (pathParam: string): BrowseProps => {
63-
// @note: order matters — `commits` must come before `commit` so the regex
64-
// engine doesn't greedily match `commit` against `/-/commits/...`.
65-
const sentinelIndex = pathParam.search(/\/-\/(tree|blob|commits|commit)/);
66-
if (sentinelIndex === -1) {
67-
throw new Error(`Invalid browse pathname: "${pathParam}" - expected to contain "/-/(tree|blob|commits|commit)/" pattern`);
62+
const decodeBrowsePathPart = (pathPart: string): string | null => {
63+
try {
64+
return decodeURIComponent(pathPart);
65+
} catch {
66+
return null;
67+
}
68+
};
69+
70+
export const getBrowseParamsFromPathParam = (pathParam: string): BrowseProps | null => {
71+
const sentinelMatch = pathParam.match(/\/-\/(tree|blob|commits|commit)(?:\/|$)/);
72+
if (!sentinelMatch || sentinelMatch.index === undefined) {
73+
return null;
74+
}
75+
76+
const sentinelIndex = sentinelMatch.index;
77+
const repoAndRevisionPart = decodeBrowsePathPart(pathParam.substring(0, sentinelIndex));
78+
if (repoAndRevisionPart === null) {
79+
return null;
6880
}
6981

70-
const repoAndRevisionPart = decodeURIComponent(pathParam.substring(0, sentinelIndex));
7182
const lastAtIndex = repoAndRevisionPart.lastIndexOf('@');
7283

7384
const repoName = lastAtIndex === -1 ? repoAndRevisionPart : repoAndRevisionPart.substring(0, lastAtIndex);
7485
const revisionName = lastAtIndex === -1 ? undefined : repoAndRevisionPart.substring(lastAtIndex + 1);
86+
if (!repoName) {
87+
return null;
88+
}
7589

76-
const tail = pathParam.substring(sentinelIndex + '/-/'.length);
77-
const pathType = ((): BrowsePathType => {
78-
if (tail.startsWith('tree')) {
79-
return 'tree';
80-
}
81-
else if (tail.startsWith('commits')) {
82-
return 'commits';
83-
}
84-
else if (tail.startsWith('commit')) {
85-
return 'commit';
86-
}
87-
88-
return 'blob';
89-
})();
90+
const pathType = sentinelMatch[1] as BrowsePathType;
91+
const tail = pathParam.substring(sentinelIndex + '/-/'.length + pathType.length);
92+
const pathPart = tail.startsWith('/') ? tail.substring(1) : tail;
93+
const decodedPathPart = decodeBrowsePathPart(pathPart);
94+
if (decodedPathPart === null) {
95+
return null;
96+
}
9097

91-
// @note: decodeURIComponent is needed in case the path contains a space.
9298
switch (pathType) {
9399
case 'tree': {
94-
const rest = tail.startsWith('tree/') ? tail.substring('tree/'.length) : tail.substring('tree'.length);
95100
return {
96101
repoName,
97102
revisionName,
98103
pathType,
99-
path: normalizeRepoPath(decodeURIComponent(rest)),
104+
path: normalizeRepoPath(decodedPathPart),
100105
};
101106
}
102107
case 'commits': {
103-
const rest = tail.startsWith('commits/') ? tail.substring('commits/'.length) : tail.substring('commits'.length);
104108
return {
105109
repoName,
106110
revisionName,
107111
pathType,
108-
path: normalizeRepoPath(decodeURIComponent(rest)),
112+
path: normalizeRepoPath(decodedPathPart),
109113
};
110114
}
111115
case 'commit': {
112116
// Path suffix on /-/commit/<sha>/<path> is no longer used, but we
113117
// keep the slash-split here so legacy URLs still resolve to the
114118
// commit (we just ignore everything after the SHA).
115-
const rest = tail.startsWith('commit/') ? tail.substring('commit/'.length) : tail.substring('commit'.length);
116-
const firstSlash = rest.indexOf('/');
117-
const commitSha = decodeURIComponent(firstSlash === -1 ? rest : rest.substring(0, firstSlash));
119+
const firstSlash = decodedPathPart.indexOf('/');
120+
const commitSha = firstSlash === -1 ? decodedPathPart : decodedPathPart.substring(0, firstSlash);
118121

119122
if (!commitSha) {
120-
throw new Error(`Invalid browse pathname: "${pathParam}" - expected to contain a commit SHA for commit type`);
123+
return null;
121124
}
122125

123126
return {
@@ -129,11 +132,10 @@ export const getBrowseParamsFromPathParam = (pathParam: string): BrowseProps =>
129132
};
130133
}
131134
case 'blob': {
132-
const rest = tail.startsWith('blob/') ? tail.substring('blob/'.length) : tail.substring('blob'.length);
133-
const path = normalizeRepoPath(decodeURIComponent(rest));
135+
const path = normalizeRepoPath(decodedPathPart);
134136

135137
if (path === '') {
136-
throw new Error(`Invalid browse pathname: "${pathParam}" - expected to contain a path for blob type`);
138+
return null;
137139
}
138140

139141
return {

0 commit comments

Comments
 (0)