Skip to content

Commit 63b9b15

Browse files
chrisbobbegnprice
chrisbobbe
authored and
gnprice
committed
internalLinks [nfc]: Have isNarrowLink take a URL object
1 parent eb204c4 commit 63b9b15

File tree

2 files changed

+68
-126
lines changed

2 files changed

+68
-126
lines changed

‎src/utils/__tests__/internalLinks-test.js

Lines changed: 60 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
/* @flow strict-local */
2+
import invariant from 'invariant';
23

34
import type { Stream } from '../../types';
45
import { streamNarrow, topicNarrow, pmNarrowFromUsersUnsafe, STARRED_NARROW } from '../narrow';
@@ -10,121 +11,80 @@ import {
1011
decodeHashComponent,
1112
} from '../internalLinks';
1213
import * as eg from '../../__tests__/lib/exampleData';
14+
import { isUrlRelative } from '../url';
1315

1416
const realm = new URL('https://example.com');
17+
const urlOnRealm = relativeUrl => {
18+
invariant(
19+
isUrlRelative(relativeUrl),
20+
'For absolute URLs, just use the one-argument `new URL(…)`.',
21+
);
22+
return new URL(relativeUrl, realm);
23+
};
1524

1625
describe('isNarrowLink', () => {
17-
const cases: $ReadOnlyArray<[boolean, string, string] | [boolean, string, string, URL]> = [
18-
[true, 'fragment-only, to a narrow', '#narrow/stream/jest/topic/topic1'],
19-
[false, 'fragment-only, wrong fragment', '#nope'],
20-
[true, 'path-absolute, to a narrow', '/#narrow/stream/jest'],
21-
[false, 'path-absolute, wrong fragment', '/#nope'],
22-
[false, 'path-absolute, wrong path', '/user_uploads/#narrow/stream/jest'],
23-
[true, 'same domain, to a narrow', 'https://example.com/#narrow/stream/jest'],
24-
[false, 'same domain, wrong fragment', 'https://example.com/#nope'],
25-
[false, 'same domain, wrong path', 'https://example.com/user_uploads/#narrow/stream/jest'],
26-
[false, 'wrong domain', 'https://another.com/#narrow/stream/jest'],
27-
28-
[true, 'fragment-only, with numeric IDs', '#narrow/stream/123-jest/topic/topic1'],
29-
[true, 'path-absolute, with numeric IDs', '/#narrow/stream/123-jest'],
30-
[true, 'path-absolute, with numeric IDs', '/#narrow/pm-with/123-mark'],
31-
32-
[false, 'fragment-only, #narrowly', '#narrowly/stream/jest'],
33-
[false, 'path-absolute, #narrowly', '/#narrowly/stream/jest'],
34-
[false, 'same domain, #narrowly', 'https://example.com/#narrowly/stream/jest'],
35-
36-
[false, 'same domain, double slash', 'https://example.com//#narrow/stream/jest'],
37-
[false, 'same domain, triple slash', 'https://example.com///#narrow/stream/jest'],
38-
39-
// These examples may be odd-looking URLs, but are nevertheless valid.
40-
[true, 'scheme-relative', '//example.com/#narrow/stream/jest'],
41-
[true, 'path-relative, nop path', '.#narrow/stream/jest'],
42-
[true, 'path-relative, nop path', './#narrow/stream/jest'],
43-
[true, 'path-relative, nop path', '..#narrow/stream/jest'],
44-
[true, 'path-relative, nop path', '../#narrow/stream/jest'],
45-
[true, 'path-relative, nop path', 'foo/..#narrow/stream/jest'],
46-
[true, 'path-relative, nop path', 'foo/../#narrow/stream/jest'],
47-
[true, 'path-relative, nop path', 'foo/bar/../../../baz/.././#narrow/stream/jest'],
48-
[true, 'path-absolute, nop path', '/.#narrow/stream/jest'],
49-
[true, 'path-absolute, nop path', '/./#narrow/stream/jest'],
50-
[true, 'path-absolute, nop path', '/..#narrow/stream/jest'],
51-
[true, 'path-absolute, nop path', '/../#narrow/stream/jest'],
52-
[true, 'path-absolute, nop path', '/foo/..#narrow/stream/jest'],
53-
[true, 'path-absolute, nop path', '/foo/../#narrow/stream/jest'],
54-
[true, 'path-absolute, nop path', '/foo/bar/../../../baz/.././#narrow/stream/jest'],
55-
[true, 'same domain, nop path', 'https://example.com/.#narrow/stream/jest'],
56-
[true, 'same domain, nop path', 'https://example.com/./#narrow/stream/jest'],
57-
[true, 'same domain, nop path', 'https://example.com/..#narrow/stream/jest'],
58-
[true, 'same domain, nop path', 'https://example.com/../#narrow/stream/jest'],
59-
[true, 'same domain, nop path', 'https://example.com/foo/..#narrow/stream/jest'],
60-
[true, 'same domain, nop path', 'https://example.com/foo/../#narrow/stream/jest'],
26+
const cases: $ReadOnlyArray<[boolean, string, URL] | [boolean, string, URL, URL]> = [
27+
[true, 'legacy: stream name, no ID', urlOnRealm('#narrow/stream/jest')],
28+
[true, 'legacy: stream name, no ID, topic', urlOnRealm('#narrow/stream/jest/topic/topic1')],
29+
30+
[true, 'with numeric stream ID', urlOnRealm('#narrow/stream/123-jest')],
31+
[true, 'with numeric stream ID and topic', urlOnRealm('#narrow/stream/123-jest/topic/topic1')],
32+
33+
[true, 'with numeric pm user IDs', urlOnRealm('#narrow/pm-with/123-mark')],
34+
35+
[false, 'wrong fragment', urlOnRealm('#nope')],
36+
[false, 'wrong path', urlOnRealm('/user_uploads/#narrow/stream/jest')],
37+
[false, 'wrong domain', new URL('https://another.com/#narrow/stream/jest')],
38+
39+
[false, '#narrowly', urlOnRealm('#narrowly/stream/jest')],
40+
41+
[false, 'double slash', new URL(`${realm.origin}//#narrow/stream/jest`)],
42+
[false, 'triple slash', new URL(`${realm.origin}///#narrow/stream/jest`)],
43+
6144
[
6245
true,
63-
'same domain, nop path',
64-
'https://example.com/foo/bar/../../../baz/.././#narrow/stream/jest',
46+
'with port',
47+
new URL('#narrow/stream/jest', 'https://example.com:444/'),
48+
new URL('https://example.com:444/'),
6549
],
66-
[true, 'same domain, %-encoded host', 'https://%65xample%2ecom/#narrow/stream/jest'],
50+
6751
// This one fails because our polyfilled URL implementation has IDNA stripped out.
68-
// [true, 'same domain, punycoded host', 'https://example.xn--h2brj9c/#narrow/stream/jest', new URL('https://example.भारत/'),], // FAILS
52+
// [
53+
// true,
54+
// 'same domain, punycoded host',
55+
// new URL('https://example.xn--h2brj9c/#narrow/stream/jest'),
56+
// new URL('https://example.भारत/'),
57+
// ], // FAILS
6958
[
7059
true,
71-
'same domain, punycodable host',
72-
'https://example.भारत/#narrow/stream/jest',
60+
'punycodable host',
61+
new URL('#narrow/stream/jest', 'https://example.भारत/'),
7362
new URL('https://example.भारत/'),
7463
],
64+
7565
// This one fails because our polyfilled URL implementation has IDNA stripped out.
76-
// [true, 'same domain, IDNA-mappable', 'https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/stream/jest'], // FAILS
66+
// [
67+
// true,
68+
// 'same domain, IDNA-mappable',
69+
// new URL('https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/stream/jest'),
70+
// new URL('https://example.com'),
71+
// ], // FAILS
72+
7773
[
7874
true,
79-
'same IPv4 address, %-encoded',
80-
'http://%31%39%32%2e168%2e0%2e1/#narrow/stream/jest',
75+
'ipv4 address',
76+
new URL('#narrow/stream/jest', 'http://192.168.0.1/'),
8177
new URL('http://192.168.0.1/'),
8278
],
8379
// This one fails because our polyfilled URL implementation has IDNA stripped out.
84-
// [true, 'same IPv4 address, IDNA-mappable', 'http://１𝟗𝟚。①⁶🯸．₀｡𝟭/#narrow/stream/jest', new URL('http://192.168.0.1/'),], // FAILS
85-
[
86-
true,
87-
'same IPv4 address, suppressed zero octet',
88-
'http://192.168.1/#narrow/stream/jest',
89-
new URL('http://192.168.0.1/'),
90-
],
91-
// TODO: Add tests for IPv6.
92-
[true, 'same domain, empty port', 'https://example.com:/#narrow/stream/jest'],
93-
[true, 'same domain, redundant port', 'https://example.com:443/#narrow/stream/jest'],
94-
[
95-
true,
96-
'same domain, padded port',
97-
'https://example.com:00000000444/#narrow/stream/jest',
98-
new URL('https://example.com:444/'),
99-
],
80+
// [
81+
// true,
82+
// 'same IPv4 address, IDNA-mappable',
83+
// new URL('http://１𝟗𝟚。①⁶🯸．₀｡𝟭/#narrow/stream/jest'),
84+
// new URL('http://192.168.0.1/'),
85+
// ], // FAILS
10086

101-
// These examples are not "valid URL strings", but are nevertheless
102-
// accepted by the URL parser.
103-
[true, 'fragment-only, with whitespace', ' #\tnar\rr\now/stream/jest '],
104-
[true, 'path-absolute, with whitespace', ' /\t#\nnar\rrow/stream/jest '],
105-
[true, 'same domain, with whitespace', ' ht\ttp\ns\r://ex\nample.com/#\nnarrow/stream/jest'],
106-
[true, 'scheme but path-relative', 'https:#narrow/stream/jest'],
107-
[true, 'scheme but path-relative, nop path', 'https:./foo/../#narrow/stream/jest'],
108-
[true, 'scheme but path-absolute', 'https:/#narrow/stream/jest'],
109-
[true, 'scheme but path-absolute, nop path', 'https:/./foo/../#narrow/stream/jest'],
110-
[
111-
true,
112-
'same IPv4 address, in hex and octal',
113-
'http://0xc0.0250.0.1/#narrow/stream/jest',
114-
new URL('http://192.168.0.1/'),
115-
],
116-
[
117-
true,
118-
'same IPv4 address, with joined octets',
119-
'http://192.11010049/#narrow/stream/jest',
120-
new URL('http://192.168.0.1/'),
121-
],
122-
[
123-
true,
124-
'same IPv4 address, with trailing dot',
125-
'http://192.168.0.1./#narrow/stream/jest',
126-
new URL('http://192.168.0.1/'),
127-
],
87+
// TODO: Add tests for IPv6.
12888

12989
// These examples may seem weird, but a previous version accepted most of them.
13090
[
@@ -133,34 +93,24 @@ describe('isNarrowLink', () => {
13393
// This one, except possibly the fragment, is a 100% realistic link
13494
// for innocent normal use. The buggy old version narrowly avoided
13595
// accepting it... but would accept all the variations below.
136-
'https://web.archive.org/web/*/https://example.com/#narrow/stream/jest',
96+
new URL(`https://web.archive.org/web/*/${urlOnRealm('#narrow/stream/jest').toString()}`),
13797
],
13898
[
13999
false,
140100
'odd scheme, wrong domain, realm-like path, narrow-like fragment',
141-
'ftp://web.archive.org/web/*/https://example.com/#narrow/stream/jest',
101+
new URL(`ftp://web.archive.org/web/*/${urlOnRealm('#narrow/stream/jest').toString()}`),
142102
],
143103
[
144104
false,
145105
'same domain, realm-like path, narrow-like fragment',
146-
'https://example.com/web/*/https://example.com/#narrow/stream/jest',
147-
],
148-
[
149-
false,
150-
'path-absolute, realm-like path, narrow-like fragment',
151-
'/web/*/https://example.com/#narrow/stream/jest',
152-
],
153-
[
154-
false,
155-
'path-relative, realm-like path, narrow-like fragment',
156-
'web/*/https://example.com/#narrow/stream/jest',
106+
urlOnRealm(`web/*/${urlOnRealm('#narrow/stream/jest').toString()}`),
157107
],
158108
];
159109

160110
/* $FlowFixMe[invalid-tuple-index]:
161111
realm_ is URL | void, but complains of out-of-bounds access */
162112
for (const [expected, description, url, realm_] of cases) {
163-
test(`${expected ? 'accept' : 'reject'} ${description}: ${url}`, () => {
113+
test(`${expected ? 'accept' : 'reject'} ${description}: ${url.toString()}`, () => {
164114
expect(isNarrowLink(url, realm_ ?? realm)).toBe(expected);
165115
});
166116
}

‎src/utils/internalLinks.js

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -45,22 +45,14 @@ const getHashSegmentsFromNarrowLink = (url: string, realm: URL) => {
4545
*
4646
* Test for a link to a Zulip narrow on the given realm.
4747
*
48-
* True just if the given URL string appears to be a link, either absolute
49-
* or relative, to a Zulip narrow on the given realm.
50-
*
51-
* This performs a call to `new URL` and therefore may take a fraction of a
52-
* millisecond. Avoid using in a context where it might be called more than
53-
* 10 or 100 times per user action.
48+
* True just if the given URL appears to be a link to a Zulip narrow on the
49+
* given realm.
5450
*/
55-
export const isNarrowLink = (url: string, realm: URL): boolean => {
56-
const resolved = new URL(url, realm);
57-
return (
58-
isUrlOnRealm(resolved, realm)
59-
&& resolved.pathname === '/'
60-
&& resolved.search === ''
61-
&& /^#narrow\//i.test(resolved.hash)
62-
);
63-
};
51+
export const isNarrowLink = (url: URL, realm: URL): boolean =>
52+
isUrlOnRealm(url, realm)
53+
&& url.pathname === '/'
54+
&& url.search === ''
55+
&& /^#narrow\//i.test(url.hash);
6456

6557
type LinkType = 'non-narrow' | 'home' | 'pm' | 'topic' | 'stream' | 'special';
6658

@@ -74,7 +66,7 @@ type LinkType = 'non-narrow' | 'home' | 'pm' | 'topic' | 'stream' | 'special';
7466
// TODO: Work out what this does, write a jsdoc for its interface, and
7567
// reimplement using URL object (not just for the realm)
7668
export const getLinkType = (url: string, realm: URL): LinkType => {
77-
if (!isNarrowLink(url, realm)) {
69+
if (!isNarrowLink(new URL(url, realm), realm)) {
7870
return 'non-narrow';
7971
}
8072

0 commit comments

Comments
 (0)