Skip to content
This repository was archived by the owner on Jul 19, 2024. It is now read-only.

Conversation

@Perryvw
Copy link
Contributor

@Perryvw Perryvw commented Oct 17, 2023

Having running tests is crucial to maintain project quality and functioning.

I added the test steps back to the CI run and updated the test expectations of the failing tests.

I'm not entirely sure the tests are correct now, but at least they are running and showing us when things change.

@Perryvw
Copy link
Contributor Author

Perryvw commented Oct 17, 2023

Seems there are still some failures in the runner I'm not seeing locally, I'm guessing this is probably due to linux/windows differences. I'll investigate further tomorrow

@archywillhe
Copy link
Contributor

oh thanks for fixing those tests! (have always wanted to fix them but I've gotten quite occupied with my main job recently So yeah, thanks!) Let me see why it is failing in the CI

@archywillhe archywillhe merged commit cb02ab8 into ArchGPT:main Oct 18, 2023
@archywillhe
Copy link
Contributor

archywillhe commented Oct 18, 2023

look like it is because NodeJS urlParse is not returning the same thing nodejs/node#49024

I'll bump node a little bit more to resolve rhis

@Perryvw
Copy link
Contributor Author

Perryvw commented Oct 18, 2023

Indeed, it looks like url.parse is deprecated too, I'm looking into using the new URL interface, though I'm having some issues with it being much stricter in what is a valid URL and throwing otherwise. (Wildcards are NOT valid urls...)

@archywillhe
Copy link
Contributor

I think we can keep it as it is for now; updating to node LTS (18.18.2) somehow resolved the problem; I also just added a debug flag to url-matches-cert-host.ts

@archywillhe
Copy link
Contributor

archywillhe commented Oct 18, 2023

Wildcards are NOT valid urls...

yeah.. that's kind of the a bummer since urlMatchesCertHost is mostly used for filtering URLs in a list.

looks like the main problem of url.parse is that it is vulnerable to hostname spoofing: https://hackerone.com/reports/678487

url.parse('http://evil.c℀.victim.test/?') returns evil.ca/c.victim.test as hostname, so this hostname matches *.victim.test but will access evil.ca.

I think it should be fine sticking to url.parse for Insomnium's use case (as an API testing tool) where url.parse is not really used in any auth/high-risk operations

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants