-
-
Notifications
You must be signed in to change notification settings - Fork 334
fix: add timeout to registry URL validation to prevent CLI hang #2086
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@asyncapi/cli": patch | ||
| --- | ||
|
|
||
| Add timeout to registry URL validation to prevent CLI hang when registry is unreachable. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||
| import { expect } from 'chai'; | ||||||
| import { registryURLParser, registryValidation } from '../../../src/utils/generate/registry'; | ||||||
|
|
||||||
| describe('registryURLParser()', () => { | ||||||
| it('should return undefined for empty input', () => { | ||||||
| expect(registryURLParser(undefined)).to.be.undefined; | ||||||
| expect(registryURLParser('')).to.be.undefined; | ||||||
| }); | ||||||
|
|
||||||
| it('should accept valid http URLs', () => { | ||||||
| expect(() => registryURLParser('https://registry.npmjs.org')).to.not.throw(); | ||||||
| expect(() => registryURLParser('http://localhost:4873')).to.not.throw(); | ||||||
| }); | ||||||
|
|
||||||
| it('should reject non-http URLs', () => { | ||||||
| expect(() => registryURLParser('ftp://registry.example.com')).to.throw('Invalid --registry-url'); | ||||||
| expect(() => registryURLParser('not-a-url')).to.throw('Invalid --registry-url'); | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| describe('registryValidation()', () => { | ||||||
| it('should return undefined when no URL provided', async () => { | ||||||
| const result = await registryValidation(undefined); | ||||||
| expect(result).to.be.undefined; | ||||||
| }); | ||||||
|
|
||||||
| it('should fail fast for unreachable URLs instead of hanging', async () => { | ||||||
| const start = Date.now(); | ||||||
| try { | ||||||
| // 10.255.255.1 is a non-routable IP that will trigger the timeout | ||||||
| await registryValidation('http://10.255.255.1'); | ||||||
| expect.fail('Should have thrown'); | ||||||
| } catch (error: unknown) { | ||||||
| const elapsed = Date.now() - start; | ||||||
| expect(elapsed).to.be.lessThan(15_000); // Must resolve within 15s (10s timeout + margin) | ||||||
| expect(error).to.be.instanceOf(Error); | ||||||
| const msg = (error as Error).message; | ||||||
| expect(msg).to.satisfy( | ||||||
| (m: string) => m.includes('timed out') || m.includes('Unable to reach'), | ||||||
| `Expected timeout or unreachable error, got: ${msg}` | ||||||
| ); | ||||||
| } | ||||||
| }).timeout(20_000); | ||||||
|
Comment on lines
+27
to
+43
|
||||||
|
|
||||||
| it('should throw auth error for 401 without credentials', async () => { | ||||||
|
||||||
| it('should throw auth error for 401 without credentials', async () => { | |
| it.skip('should throw auth error for 401 without credentials', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The auth error preservation currently relies on checking whether
error.messagecontains the substringregistryAuth. This is brittle (message changes will silently alter behavior) and could misclassify unrelated errors. Prefer restructuring so only thefetchcall is inside the try/catch (rethrow non-fetch errors), or use a dedicated error type/constant for the 401-without-credentials case and check against that instead of parsing the message.