feat: URL validator #1
Conversation
| try { | ||
| router.push(validator.parse(redirectUrl)) | ||
| } catch (error) { | ||
| router.push('/home') | ||
| } |
There was a problem hiding this comment.
Most validators use more of an if ... else pattern based on a boolean return value; is there a strong reason to use the try catch pattern instead? I guess one advantage is more meaningful error messages.
There was a problem hiding this comment.
I think I also wanted to return a parsed version of the URL, which might be safer than saying OK to the string URL and letting them re-use it in e.g. router.push() - in the Isomer case, it was something like if isRelativeUrl(url) then router.push(url), but the Next.js weirdness could potentially have been avoided if parse returns a sane URL and it was router.push(parse(url)).
If we want to make it a parser + validator in that it returns the parsed result, I don't really like the "returning both the result and status" pattern because it doesn't force devs to handle errors, but throwing exceptions do. Maybe one option would be to do something like zod's parse and safeParse and let people choose?
There was a problem hiding this comment.
Makes sense! I think stick to a single implementation i.e. the try catch pattern.
| import { UrlValidationError } from '@/url/errors' | ||
| import { Whitelist } from '@/url/options' | ||
|
|
||
| const DYNAMIC_ROUTE_SEGMENT_REGEX = /\[\[?([^\]]+)\]?\]/g |
There was a problem hiding this comment.
Where is this from? Might be more true-to-implementation by using Next.js' regex exactly https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/utils/route-regex.ts#L29C27-L29C48
There was a problem hiding this comment.
This regex is weird because it results in a greedy match of e.g.
[test][test]
<-------->
I think they're mostly the same functionally except I've moved the capturing group inside and used [^\]] to match all but ] to avoid the greedy matching. Maybe best way is still to block weird chars in pathname or urlencode it
packages/validators/src/url/utils.ts
Outdated
| let normalizedUrl: URL | ||
|
|
||
| try { | ||
| normalizedUrl = new URL(url, baseUrl) |
There was a problem hiding this comment.
Is there a potential footgun here in which a user expects that they can provide a baseUrl with a path e.g.
> new URL('/123', 'https://example.com/user/')
< URL {href: "https://example.com/123", origin: "https://example.com", protocol: "https:", username: "", password: "", …}
This also opens it up to traversals.
> new URL('./123', 'https://example.com/user/test')
URL {
href: 'https://example.com/user/123',
origin: 'https://example.com',
protocol: 'https:',
username: '',
password: '',
host: 'example.com',
hostname: 'example.com',
port: '',
pathname: '/user/123',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
> new URL('../123', 'https://example.com/user/test')
URL {
href: 'https://example.com/123',
origin: 'https://example.com',
protocol: 'https:',
username: '',
password: '',
host: 'example.com',
hostname: 'example.com',
port: '',
pathname: '/123',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
There was a problem hiding this comment.
Enforcing url.pathname === '/' for baseOrigin
| return normalizedUrl | ||
| } | ||
| /* As of Next.js 14.2.5, router.push() resolves dynamic routes using query parameters. */ | ||
| const resolveNextDynamicRoute = (url: URL): URL => { |
There was a problem hiding this comment.
Assuming this is a lightweight version of https://github.com/vercel/next.js/blob/cbdf9897b0fb381b50d6a13f397eb9b7cd21dd7d/packages/next/src/shared/lib/router/utils/interpolate-as.ts#L6
|
|
||
| Validates URLs against a whitelist of allowed protocols and hostnames, preventing open redirects, XSS, SSRF, and other security vulnerabilities. | ||
|
|
||
| ### new UrlValidator(options) |
There was a problem hiding this comment.
Can we add this as TSDoc comments?
UrlValidatorclass