Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: parse arg typing #36

Merged
merged 3 commits into from
Nov 21, 2024
Merged

fix: parse arg typing #36

merged 3 commits into from
Nov 21, 2024

Conversation

zhongliang02
Copy link
Contributor

@zhongliang02 zhongliang02 commented Nov 21, 2024

Previously parse and parsePathname required a string as the first argument.
However, since we perform type checking via Zod under the hood, we can loosen this to any.

This will simplify

const url: string | string[] | undefined = router.query['redirect']
const redirectUrl = url ? String(url) : '/home'
router.push(validator.parse(redirectUrl, '/home'))

to

router.push(validator.parse(router.query['redirect'], '/home'))

Also added a precommit script, which should catch CI issues in advance

@zhongliang02 zhongliang02 requested a review from a team November 21, 2024 08:29
#parse(url: string): URL {
const result = this.schema.safeParse(url)
#parse(url: any): URL {
const result = url instanceof URL ? this.schema.safeParse(url.href) : this.schema.safeParse(url)
Copy link

@spaceraccoon spaceraccoon Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick callout here that instanceof checks whether the class is in the entire prototype chain, so:

class MyOwnUrl extends URL {
    constructor(url) {
        super(url)
        this.href = 'https://evil.com'
    }
}

const newUrl = new MyOwnUrl('https://good.com')
console.log(newUrl instanceof URL)  // true
console.log(newUrl.href)    // https://evil.com

In any case this should get caught by safeParse, but just for awareness as there are some rare and esoteric prototype pollution scenarios, and making sure this isn't a hidden assumption.

@zhongliang02 zhongliang02 merged commit 076c546 into develop Nov 21, 2024
5 checks passed
@zhongliang02 zhongliang02 deleted the fix/parse-arg-typing branch November 21, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants