Skip to content

Commit ead55ca

Browse files
fix(ui): reject malformed owner/repo strings in splitRepoFullName (#7818)
splitRepoFullName gated the free-text Repository input across 8 panels by destructuring `[owner, repo, extra]` and rejecting only when `extra` was truthy. That inspected just the 3rd `/`-segment, so any input whose 3rd segment was empty slipped through as a truncated pair -- "owner/repo/", "owner/repo//x", and pastes like "owner/repo//stale-copy" all resolved to {owner, repo} and were silently queried instead of being rejected with the usual "owner/repo" validation message. Validate the segment count explicitly (`parts.length !== 2`) so anything with more or fewer than two non-empty segments is rejected. Return shape and all callers are unchanged. Adds maintainer-settings-preview.test.ts (the file had no direct tests): splitRepoFullName's valid/trailing-slash/double-slash/empty-segment cases plus baseline coverage for the file's other exported helpers. Closes #7783 Co-authored-by: jaytbarimbao-collab <300663773+jaytbarimbao-collab@users.noreply.github.com>
1 parent 3fd145b commit ead55ca

2 files changed

Lines changed: 159 additions & 2 deletions

File tree

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
import { describe, expect, it } from "vitest";
2+
3+
import {
4+
buildSettingsPreviewRequest,
5+
extractPreviewRepoOptions,
6+
findPreviewScenario,
7+
parseLinkedIssues,
8+
parsePreviewLabels,
9+
PREVIEW_SCENARIOS,
10+
splitRepoFullName,
11+
splitReviewabilityPr,
12+
type PreviewFormState,
13+
} from "@/lib/maintainer-settings-preview";
14+
15+
describe("splitRepoFullName", () => {
16+
it("accepts a well-formed owner/repo pair", () => {
17+
expect(splitRepoFullName("acme/repo")).toEqual({ owner: "acme", repo: "repo" });
18+
});
19+
20+
it("trims surrounding whitespace before splitting", () => {
21+
expect(splitRepoFullName(" acme/repo ")).toEqual({ owner: "acme", repo: "repo" });
22+
});
23+
24+
// Regression (#7783): the old `[owner, repo, extra]` destructuring only looked at the 3rd segment,
25+
// so any input whose 3rd `/`-segment was empty slipped through as a truncated owner/repo pair.
26+
it("rejects a trailing slash instead of silently truncating", () => {
27+
expect(splitRepoFullName("acme/repo/")).toBeNull();
28+
});
29+
30+
it("rejects a double slash with a trailing segment", () => {
31+
expect(splitRepoFullName("acme/repo//x")).toBeNull();
32+
});
33+
34+
it("rejects a pasted stale-copy suffix that the old guard accepted", () => {
35+
expect(splitRepoFullName("owner/repo//stale-copy")).toBeNull();
36+
});
37+
38+
it("rejects any input with more than two segments", () => {
39+
expect(splitRepoFullName("a/b/c")).toBeNull();
40+
expect(splitRepoFullName("a/b/c/d")).toBeNull();
41+
});
42+
43+
it("rejects fewer than two segments", () => {
44+
expect(splitRepoFullName("acme")).toBeNull();
45+
expect(splitRepoFullName("")).toBeNull();
46+
expect(splitRepoFullName(" ")).toBeNull();
47+
});
48+
49+
it("rejects an empty owner or empty repo segment", () => {
50+
expect(splitRepoFullName("/repo")).toBeNull();
51+
expect(splitRepoFullName("acme/")).toBeNull();
52+
expect(splitRepoFullName("/")).toBeNull();
53+
});
54+
});
55+
56+
describe("splitReviewabilityPr", () => {
57+
it("parses owner/repo#number into its parts", () => {
58+
expect(splitReviewabilityPr("acme/repo#123")).toEqual({
59+
owner: "acme",
60+
repo: "repo",
61+
number: 123,
62+
});
63+
});
64+
65+
it("inherits splitRepoFullName's stricter validation for the repo half", () => {
66+
expect(splitReviewabilityPr("acme/repo//stale#123")).toBeNull();
67+
});
68+
69+
it("rejects a missing, zero, negative, or non-integer issue number", () => {
70+
expect(splitReviewabilityPr("acme/repo")).toBeNull();
71+
expect(splitReviewabilityPr("acme/repo#0")).toBeNull();
72+
expect(splitReviewabilityPr("acme/repo#-1")).toBeNull();
73+
expect(splitReviewabilityPr("acme/repo#1.5")).toBeNull();
74+
});
75+
});
76+
77+
describe("extractPreviewRepoOptions", () => {
78+
it("returns unique, sorted, well-formed repos and drops malformed rows", () => {
79+
expect(
80+
extractPreviewRepoOptions([
81+
{ pr: "beta/two#5" },
82+
{ pr: "alpha/one#1" },
83+
{ pr: "alpha/one#2" },
84+
{ pr: "not-a-repo#3" },
85+
{ pr: "has space/repo#4" },
86+
]),
87+
).toEqual(["alpha/one", "beta/two"]);
88+
});
89+
});
90+
91+
describe("parsePreviewLabels", () => {
92+
it("splits on commas, trims, drops blanks, and de-duplicates case-insensitively", () => {
93+
expect(parsePreviewLabels(" bug , Bug ,, feature ")).toEqual(["bug", "feature"]);
94+
});
95+
96+
it("caps the result at 50 labels", () => {
97+
const many = Array.from({ length: 60 }, (_, index) => `label-${index}`).join(",");
98+
expect(parsePreviewLabels(many)).toHaveLength(50);
99+
});
100+
});
101+
102+
describe("parseLinkedIssues", () => {
103+
it("parses hash-prefixed and whitespace/comma-separated positive integers, de-duplicated", () => {
104+
expect(parseLinkedIssues("#12, 34 12 56")).toEqual([12, 34, 56]);
105+
});
106+
107+
it("drops zero, negative, and non-numeric tokens", () => {
108+
expect(parseLinkedIssues("#0, -3, abc, 7")).toEqual([7]);
109+
});
110+
});
111+
112+
describe("findPreviewScenario", () => {
113+
it("returns the matching scenario", () => {
114+
expect(findPreviewScenario("bot-author").id).toBe("bot-author");
115+
});
116+
117+
it("falls back to the first scenario for an unknown id", () => {
118+
expect(findPreviewScenario("nope" as never)).toBe(PREVIEW_SCENARIOS[0]);
119+
});
120+
});
121+
122+
describe("buildSettingsPreviewRequest", () => {
123+
const baseForm: PreviewFormState = {
124+
repoFullName: "acme/repo",
125+
scenarioId: "confirmed-miner",
126+
title: " My PR ",
127+
labels: "bug, feature",
128+
linkedIssues: "#1 2",
129+
body: " hello ",
130+
};
131+
132+
it("assembles the sample from the scenario, trimmed title/body, and parsed labels/issues", () => {
133+
expect(buildSettingsPreviewRequest(baseForm)).toEqual({
134+
sample: {
135+
authorLogin: "sample-miner",
136+
authorType: "User",
137+
authorAssociation: "CONTRIBUTOR",
138+
minerStatus: "confirmed",
139+
title: "My PR",
140+
labels: ["bug", "feature"],
141+
linkedIssues: [1, 2],
142+
body: "hello",
143+
},
144+
});
145+
});
146+
147+
it("defaults a blank title and omits an empty body", () => {
148+
const result = buildSettingsPreviewRequest({ ...baseForm, title: " ", body: " " });
149+
expect(result.sample.title).toBe("Sample pull request");
150+
expect(result.sample).not.toHaveProperty("body");
151+
});
152+
});

apps/loopover-ui/src/lib/maintainer-settings-preview.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,13 @@ export function extractPreviewRepoOptions(reviewability: Array<{ pr: string }>):
116116
}
117117

118118
export function splitRepoFullName(repoFullName: string): { owner: string; repo: string } | null {
119-
const [owner, repo, extra] = repoFullName.trim().split("/");
120-
if (!owner || !repo || extra) return null;
119+
// Split into exactly two non-empty segments. The old `[owner, repo, extra]` destructuring only
120+
// inspected the 3rd segment, so any input whose 3rd `/`-segment was empty ("owner/repo/",
121+
// "owner/repo//stale") slipped through as a truncated owner/repo pair (#7783).
122+
const parts = repoFullName.trim().split("/");
123+
if (parts.length !== 2) return null;
124+
const [owner, repo] = parts;
125+
if (!owner || !repo) return null;
121126
return { owner, repo };
122127
}
123128

0 commit comments

Comments
 (0)