Skip to content

Commit 6b2e0f9

Browse files
committed
Compare a written-out deeplink prefix with its case folded too
Folding only the first segment left a prefix that spans more than one segment half-matched: `Waitpoints/Tokens/wp_123` did not equal `waitpoints/tokens`, so the graft branch fired on top of it and produced `waitpoints/tokens/Tokens/wp_123`, which matches no route. The all-lowercase spelling worked, so this was a regression the previous commit introduced. Compare as many leading segments as the prefix spans, lowercased, and return the prefix in the map's own spelling with everything past it exactly as written. Driven off `prefix` rather than special-cased for waitpoints, so a second multi-segment entry is covered when it is added.
1 parent 71c6d3c commit 6b2e0f9

2 files changed

Lines changed: 50 additions & 11 deletions

File tree

apps/webapp/app/utils/deeplinkPages.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,37 @@ describe("resolveDeeplinkPage", () => {
216216
);
217217
});
218218

219+
it("recognises a written-out prefix whatever its case, however many segments it spans", () => {
220+
// `waitpoints`' prefix is two segments, so folding only the first left `Tokens` looking like a
221+
// segment of its own: the graft fired on top of it and produced waitpoints/tokens/Tokens/{id},
222+
// which matches no route. The lowercase spelling worked, so this was case-folding's own bug.
223+
expect(resolveDeeplinkPage("Waitpoints/Tokens/wp_123")).toBe("waitpoints/tokens/wp_123");
224+
expect(resolveDeeplinkPage("waitpoints/Tokens/wp_123")).toBe("waitpoints/tokens/wp_123");
225+
expect(resolveDeeplinkPage("WAITPOINTS/TOKENS/wp_123")).toBe("waitpoints/tokens/wp_123");
226+
// The bare longhand, with nothing beyond the prefix to carry.
227+
expect(resolveDeeplinkPage("Waitpoints/Tokens")).toBe("waitpoints/tokens");
228+
});
229+
230+
it("holds for every multi-segment prefix in the map, not just waitpoints", () => {
231+
// Driven off the map so a second such entry is covered the day it is added rather than the day
232+
// someone notices. Every prefix segment is upper-cased and the id is left mixed.
233+
const multiSegment = [...ENV_PAGE_TARGETS.values()].filter(({ prefix }) =>
234+
prefix.includes("/")
235+
);
236+
237+
// Guards against this passing because it iterated nothing.
238+
expect(multiSegment.length).toBeGreaterThan(0);
239+
240+
for (const { prefix } of multiSegment) {
241+
const shouted = prefix
242+
.split("/")
243+
.map((segment) => segment.toUpperCase())
244+
.join("/");
245+
expect(resolveDeeplinkPage(`${shouted}/${PROBE}`)).toBe(`${prefix}/${PROBE}`);
246+
expect(resolveDeeplinkPage(shouted)).toBe(prefix);
247+
}
248+
});
249+
219250
it("drops traversal segments, in plain and escaped spellings", () => {
220251
expect(resolveDeeplinkPage("runs/../../../etc/passwd")).toBe("runs/etc/passwd");
221252
expect(resolveDeeplinkPage("../runs")).toBe("runs");

apps/webapp/app/utils/deeplinkPages.ts

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -111,24 +111,32 @@ function isUsableSegment(segment: string): boolean {
111111
* `suffix` is expected already encoded (see `deeplinkSuffix`) and is passed through untouched — an
112112
* `encodeURIComponent` pass here would double-encode every id that contains an escape.
113113
*
114-
* Only the first segment is matched case-insensitively, to the same end as the prefix in
115-
* `deeplinkSuffix`: `/env/{env}/APIKeys` would have matched its route, so `/deeplink/APIKeys` should
116-
* reach it rather than falling through to the environment root. The name resolves to the map's own
117-
* spelling, and every segment after it is left exactly as written — folding the case of a task or
118-
* run id would break the link far more thoroughly than the miss this fixes.
114+
* The name is matched case-insensitively, to the same end as the prefix in `deeplinkSuffix`:
115+
* `/env/{env}/APIKeys` would have matched its route, so `/deeplink/APIKeys` should reach it rather
116+
* than falling through to the environment root. So is the written-out prefix, which is why the
117+
* comparison is against the lowercased path rather than the path itself — a prefix can be more than
118+
* one segment (`waitpoints/tokens`), and reading only `Tokens` as a segment of its own would graft
119+
* the prefix on top of it and produce `waitpoints/tokens/Tokens/{id}`.
120+
*
121+
* The prefix comes back in the map's own spelling and everything past it exactly as written, since
122+
* folding the case of a task or run id would break the link far more thoroughly than the miss this
123+
* fixes.
119124
*/
120125
export function resolveDeeplinkPage(suffix: string): string | undefined {
121-
const [first = "", ...rest] = suffix.split("/").filter(isUsableSegment);
126+
const segments = suffix.split("/").filter(isUsableSegment);
127+
const [first = "", ...rest] = segments;
122128

123-
const name = first.toLowerCase();
124-
const target = ENV_PAGE_TARGETS.get(name);
129+
const target = ENV_PAGE_TARGETS.get(first.toLowerCase());
125130
if (target === undefined) return undefined;
126131

127132
if (rest.length === 0) return target.landing;
128133

129-
const written = [name, ...rest].join("/");
134+
//however many segments the prefix spans, so the whole of it is compared and none of it re-grafted
135+
const prefixDepth = target.prefix.split("/").length;
136+
const writesPrefix = segments.slice(0, prefixDepth).join("/").toLowerCase() === target.prefix;
137+
130138
//already written out under the prefix, so grafting would duplicate it
131-
if (written === target.prefix || written.startsWith(`${target.prefix}/`)) return written;
139+
const beyondPrefix = writesPrefix ? segments.slice(prefixDepth) : rest;
132140

133-
return [target.prefix, ...rest].join("/");
141+
return [target.prefix, ...beyondPrefix].join("/");
134142
}

0 commit comments

Comments
 (0)