feat(cli): add /model list subcommand for dynamic model discovery#3797
feat(cli): add /model list subcommand for dynamic model discovery#3797B-A-M-N wants to merge 7 commits into
Conversation
Adds a scriptable /model list subcommand that queries the
configured OpenAI-compatible /models endpoint and prints available
model IDs (one per line) for CLI automation and scripting.
Behavior:
- Uses the active content generator config (baseUrl + apiKey)
- Calls {baseUrl}/models with proper trailing-slash normalization
- Sends Authorization header when an API key is configured
- Prints one model ID per line (pipe-friendly output)
- Preserves existing /model, /model <id>, and /model --fast behavior
Testing notes:
- Verified with Groq API (free tier) - /model list returns model list
- Verified with Hugging Face Inference API - /model list works
- Note: Paid API testing (OpenAI, OpenRouter) was not performed
due to account limitations; the implementation follows the same OpenAI-
compatible pattern and is expected to work with any standards-compliant
/models endpoint.
Build/lint/typecheck: all pass.
Closes QwenLM#2412
There was a problem hiding this comment.
Critical: No tests for the new /model list subcommand and fetchModels function. The existing modelCommand.test.ts has zero coverage for the new feature. fetchModels is not exported, preventing direct unit testing. At minimum, tests should cover: success path, network failure, empty model list, missing config, and malformed response. Mock global fetch to avoid real HTTP calls in tests.
Suggestion: Tab completion does not present the list subcommand. The parent completion function (line 25) only handles the --fast argument, which prevents the completion system from fuzzy-matching the list subcommand. Typing /model (with a trailing space) does not suggest list. Consider updating the parent completion to also return subcommand names.
— deepseek-v4-pro via Qwen Code /review
| headers['Authorization'] = `Bearer ${apiKey}`; | ||
| } | ||
|
|
||
| const response = await fetch(url, { method: 'GET', headers }); |
There was a problem hiding this comment.
[Critical] fetch call has no timeout and bypasses the project HTTP infrastructure
fetchModels uses raw fetch for requests, with two issues:
- No timeout/AbortSignal: An unresponsive endpoint will hang the CLI indefinitely. The codebase already has
fetchWithTimeout()(packages/core/src/utils/fetch.ts) andAbortSignal.timeout()available. - Bypasses HTTP infrastructure: The project has a full
undici-based HTTP layer (buildRuntimeFetchOptions, shared Dispatcher, proxy support,customHeaders). Using rawfetchmeans enterprise proxy users will see failed requests,customHeadersare dropped, and the connection pool is bypassed.
| const response = await fetch(url, { method: 'GET', headers }); | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 30_000); | |
| try { | |
| const response = await fetch(url, { | |
| method: 'GET', | |
| headers, | |
| signal: controller.signal, | |
| }); | |
| // ... | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } |
Suggest referencing the web-fetch tool to use the unified HTTP infrastructure.
— deepseek-v4-pro via Qwen Code /review
| ): Promise<string[]> { | ||
| // Normalize baseUrl to avoid double slash (e.g., "https://api.openai.com/v1/") | ||
| const normalizedUrl = baseUrl.replace(/\/+$/, ''); | ||
| const url = `${normalizedUrl}/models`; |
There was a problem hiding this comment.
[Critical] SSRF attack surface via configurable baseUrl
baseUrl comes from user config (config.getContentGeneratorConfig()) and is passed directly to fetch without any validation:
- No HTTPS enforcement: API keys may be transmitted in plaintext over HTTP
- No internal IP filtering: SSRF requests to internal services can be constructed (e.g.,
http://169.254.169.254/,http://localhost:6379/) - Combined with the error message disclosure issue below, attackers could extract internal service responses via error messages
The codebase already has isPrivateIp() function (packages/core/src/utils/fetch.ts) going unused.
| const url = `${normalizedUrl}/models`; | |
| let parsed: URL; | |
| try { | |
| parsed = new URL(baseUrl); | |
| } catch { | |
| throw new Error('Invalid baseUrl: must be a valid URL'); | |
| } | |
| if (parsed.protocol !== 'https:') { | |
| throw new Error('baseUrl must use HTTPS'); | |
| } | |
| const modelsUrl = new URL('/models', parsed); |
— deepseek-v4-pro via Qwen Code /review
| const response = await fetch(url, { method: 'GET', headers }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); |
There was a problem hiding this comment.
[Critical] API key may leak in error messages
When the API returns a non-2xx response, the full response.text() body is concatenated into the error message and returned to the user terminal. If the upstream server echoes request headers in error pages (e.g., nginx 502, Cloudflare WAF block page), the apiKey will be leaked in plaintext in terminal output.
| const errorText = await response.text(); | |
| const errorText = await response.text(); | |
| const truncated = errorText.slice(0, 500); | |
| const sanitized = apiKey | |
| ? truncated.replaceAll(apiKey, '[REDACTED]') | |
| : truncated; | |
| throw new Error( | |
| `Request failed (${response.status}): ${sanitized}`, | |
| ); |
— deepseek-v4-pro via Qwen Code /review
| subCommands: [ | ||
| { | ||
| name: 'list', | ||
| description: 'List available models from the configured API endpoint', |
There was a problem hiding this comment.
[Critical] Hardcoded English strings, not internationalized
All user-visible strings in the new subcommand are not wrapped with t() for i18n. The main action in the same file uses t() (e.g., t('Configuration not available.')), and all other commands with subCommands (exportCommand, statsCommand, agentsCommand) use the get description() { return t(...) } pattern.
Affected:
description(line 154): should beget description() { return t('...') }- All
contentstrings inaction(lines 165, 174, 183, 204): should uset()
| description: 'List available models from the configured API endpoint', | |
| get description() { | |
| return t('List available models from the configured API endpoint'); | |
| }, |
— deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| try { | ||
| const models = await fetchModels(baseUrl, apiKey); |
There was a problem hiding this comment.
[Suggestion] Empty model list produces blank output
When the API returns zero models or format mismatch filters out all IDs, models.join('\n') produces an empty string. The user sees a blank line and cannot distinguish between "success but no models" and "format error".
| const models = await fetchModels(baseUrl, apiKey); | |
| const models = await fetchModels(baseUrl, apiKey); | |
| if (models.length === 0) { | |
| return { | |
| type: 'message', | |
| messageType: 'info', | |
| content: 'No models found from the configured endpoint.', | |
| }; | |
| } | |
| const output = models.join('\n'); |
— deepseek-v4-pro via Qwen Code /review
| * Fetch available models from the OpenAI-compatible /models endpoint. | ||
| * Returns an array of model ID strings. | ||
| */ | ||
| async function fetchModels( |
There was a problem hiding this comment.
[Suggestion] Missing debug logging
The fetchModels function does not log the request URL, response status code, or elapsed time. When troubleshooting at 3 AM there is no trace — impossible to determine whether the request was sent, what the target URL was, or what status the server returned.
The project already has debugLogger infrastructure. Consider adding before/after logging:
debugLogger.debug('Fetching models from', url);
const response = await fetch(url, ...);
debugLogger.debug('Models response', { status: response.status, duration: Date.now() - start });— deepseek-v4-pro via Qwen Code /review
- Export fetchModels for unit testing - Add comprehensive tests (31 total): success, network failure, empty list, missing config, malformed response, API key sanitization, SSRF protection, timeout - Fix tab completion to suggest 'list' subcommand - Add SSRF protection: enforce HTTPS, block private IPs + localhost - Add timeout using fetchWithTimeout (30s) - Sanitize API key from error messages to prevent leakage - Add debug logging for troubleshooting - Internationalize all user-visible strings with t() - Handle empty model list with proper message Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks @wenshao for the thorough review and detailed feedback — I appreciate you taking the time to dig into the implementation and flag these issues. I've pushed fixes (884c1d4) addressing all review comments: Critical fixes:
Suggestions addressed:
Additional fix:
All 31 tests pass, build and lint are clean. Let me know if there's anything else you'd like changed. |
There was a problem hiding this comment.
[Critical] /model list subcommand lacks success path tests — The subcommand action tests cover scenarios like config null, contentGeneratorConfig null, baseUrl undefined, and empty model list, but do not verify the happy-path output when fetchModels returns a model list. Consider adding a test that mocks fetch to return multiple models and asserts { type: 'message', messageType: 'info', content: 'model-a\\nmodel-b' }.
[Critical] /model list subcommand lacks error path tests — The subcommand action has a catch branch that calls formatFetchErrorForUser, but no test verifies the behavior when fetchModels throws an exception. Consider adding a test that mocks fetch as rejected and asserts the result matches expect.stringContaining('Failed to fetch models').
— deepseek-v4-pro via Qwen Code /review
|
|
||
| debugLogger.debug('Fetching models from', url); | ||
|
|
||
| const startTime = Date.now(); |
There was a problem hiding this comment.
[Critical] SSRF protection bypassable via HTTP redirect
fetchModels performs hostname-level SSRF validation on baseUrl before calling fetchWithTimeout, but fetch() inside fetchWithTimeout uses the default redirect: 'follow'. An attacker-controlled public HTTPS server can return a 3xx redirect to an internal IP (e.g., http://169.254.169.254/), completely bypassing the SSRF check.
| const startTime = Date.now(); | |
| response = await fetch(url, { | |
| signal: controller.signal, | |
| headers, | |
| redirect: 'error', | |
| }); |
Note: either fetchWithTimeout needs to support passing RequestInit, or fetch should be called directly in fetchModels with redirects disabled.
— deepseek-v4-pro via Qwen Code /review
| messageType: 'error', | ||
| content: t('Content generator configuration not available.'), | ||
| }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] New i18n strings lack translations
The following 4 new t() keys have no translation entries in any locale file, so non-English users will see mixed-language output:
'No baseUrl configured...''No models found from the configured endpoint.''Failed to fetch models: ''List available models from the configured API endpoint'
t() will fall back to the English original, but at minimum the English keys should be explicitly added to packages/cli/src/i18n/locales/en.js.
— deepseek-v4-pro via Qwen Code /review
| }; | ||
|
|
||
| if (apiKey) { | ||
| headers['Authorization'] = `Bearer ${apiKey}`; |
There was a problem hiding this comment.
[Suggestion] Debug logging leaks the full URL
debugLogger.debug('Fetching models from', url) and the later debugLogger.debug('Models request failed', { error, url }) write the full URL to debug logs. If the user's baseUrl embeds an API key as a query parameter, the key will be persisted in plaintext to disk.
| headers['Authorization'] = `Bearer ${apiKey}`; | |
| const logSafeUrl = url.replace(/\?.*/, '?[REDACTED]'); | |
| debugLogger.debug('Fetching models from', logSafeUrl); |
— deepseek-v4-pro via Qwen Code /review
| content: t('No models found from the configured endpoint.'), | ||
| }; | ||
| } | ||
| const output = models.join('\n'); |
There was a problem hiding this comment.
[Suggestion] Troubleshooting hints point to the wrong URL
formatFetchErrorForUser(error, { url: baseUrl }) passes baseUrl, but the actual request endpoint is ${normalizedUrl}/models. Troubleshooting hints will suggest users check connectivity to baseUrl, but the issue may actually be on the /models subpath, which misdirects investigation.
| const output = models.join('\n'); | |
| const errorMessage = formatFetchErrorForUser(error, { url: `${normalizedUrl}/models` }); |
— deepseek-v4-pro via Qwen Code /review
| */ | ||
| export async function fetchModels( | ||
| baseUrl: string, | ||
| apiKey?: string, |
There was a problem hiding this comment.
[Suggestion] IPv4-mapped IPv6 addresses bypass SSRF hostname check
Neither isPrivateIp() nor the explicit hostname checks cover IPv4-mapped IPv6 addresses (e.g., ::ffff:192.168.1.1, ::ffff:10.0.0.1). Providing https://[::ffff:10.0.0.1]/api/ as baseUrl will resolve to an actual internal IPv4 address in the OS network stack. Consider adding ::ffff: prefixed matching patterns to PRIVATE_IP_RANGES in packages/core/src/utils/fetch.ts.
— deepseek-v4-pro via Qwen Code /review
| debugLogger.debug('Models response', { | ||
| status: response.status, | ||
| duration: Date.now() - startTime, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] response.json() body read has no timeout protection
fetchWithTimeout clears the timeout timer immediately after fetch() resolves the response headers. If the server returns headers normally but streams the JSON body slowly, response.json() will wait indefinitely — no interrupt signal and no deadline. Consider extending the AbortController lifecycle to cover the body read.
— deepseek-v4-pro via Qwen Code /review
- Add test: /model list returns model list on success path - Add test: /model list handles fetchModels throwing error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] fetchWithTimeout discards the original network error code when constructing FetchError for non-abort errors (new FetchError(getErrorMessage(error)) without code). This breaks formatFetchErrorForUser's code-based troubleshooting hints (e.g., TLS_ERROR_CODES matching). Since this PR introduces the first end-to-end consumer path that exercises this pipeline, consider preserving the original error code: new FetchError(getErrorMessage(error), getErrorCode(error)).
— deepseek-v4-pro via Qwen Code /review
| return { | ||
| type: 'message', | ||
| messageType: 'error', | ||
| content: t('Failed to fetch models: ') + errorMessage, |
There was a problem hiding this comment.
[Suggestion] Two issues with this error message:
-
i18n concatenation anti-pattern: String concatenation hardcodes word order. Other commands use parameterized templates like
t('Failed to compress: {{error}}', { error })which allow translators to reorder. Switch tot('Failed to fetch models: {{error}}', { error: errorMessage }). -
Misleading prefix for config errors: "Failed to fetch models" implies an HTTP request was attempted, but
fetchModelscan throw before any request (invalid URL, non-HTTPS, SSRF rejections). Users see confusing messages like "Failed to fetch models: baseUrl must use HTTPS". Consider distinguishing validation errors from actual fetch failures.
— deepseek-v4-pro via Qwen Code /review
Preserve original error code (e.g., TLS_ERROR_CODES, network errors) so formatFetchErrorForUser can provide proper troubleshooting hints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Review comments status:
All review items are addressed in the latest push to |
| ) { | ||
| throw new Error('baseUrl resolves to a private IP address (SSRF check)'); | ||
| } | ||
| if (isPrivateIp(baseUrl)) { |
There was a problem hiding this comment.
[Suggestion] isPrivateIp() PRIVATE_IP_RANGES is missing several critical address blocks
fetchModels relies on isPrivateIp() (packages/core/src/utils/fetch.ts), which is missing:
169.254.0.0/16— the range containing AWS IMDS (169.254.169.254). Another comment already noted HTTP redirects can bypass HTTPS enforcement; combined with this gap, it forms a full attack chain.0.0.0.0— on Linux, connecting to0.0.0.0can reach locally listening services.100.64.0.0/10— RFC 6598 shared address space (CGNAT), not publicly routable, same nature as10.0.0.0/8.
| if (isPrivateIp(baseUrl)) { | |
| const PRIVATE_IP_RANGES = [ | |
| /^0\.0\.0\.0$/, | |
| /^10\./, | |
| /^100\.(6[4-9]|[7-9][0-9]|1[01][0-9]|12[0-7])\./, | |
| /^127\./, | |
| /^169\.254\./, | |
| /^172\.(1[6-9]|2[0-9]|3[0-1])\./, | |
| /^192\.168\./, | |
| /^::1$/, | |
| /^fc00:/, | |
| /^fe80:/, | |
| ]; |
— deepseek-v4-pro via Qwen Code /review
| // Sanitize API key from error messages to prevent leakage | ||
| const truncated = errorText.slice(0, 500); | ||
| const sanitized = apiKey | ||
| ? truncated.replaceAll(apiKey, '[REDACTED]') |
There was a problem hiding this comment.
[Suggestion] API key sanitization gives a false sense of security
truncated.replaceAll(apiKey, '[REDACTED]') attempts to search for the API key in the server error response body. In reality, the API key is sent via the Authorization request header, and servers will not echo it in error bodies. This code is only effective in artificially constructed test scenarios (where the test deliberately has the server return a body containing the key).
The real leak risk is in debugLogger.debug('Models request failed', { error, url }) — if baseUrl contains embedded credentials (e.g., https://key@host), they will be logged verbatim.
Consider sanitizing credentials in the URL before the debugLogger call:
| ? truncated.replaceAll(apiKey, '[REDACTED]') | |
| const sanitizedUrl = url.replace(/\/\/[^@]+@/, '//[REDACTED]@'); | |
| debugLogger.debug('Models request failed', { error, url: sanitizedUrl }); |
— deepseek-v4-pro via Qwen Code /review
Move 3 new i18n keys to correct position in all locale files. Previous insertion placed keys between a key and its value, causing JavaScript syntax errors. Keys are now correctly placed after the value for 'No models available for the current authentication type'. Fixes CI i18n check failure on PR QwenLM#3797. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Additional findings (not mappable to a single diff line)
[Suggestion] 3 new i18n keys are missing translations in all non-English locales
packages/cli/src/i18n/locales/{ca,de,fr,ja,pt,ru,zh-TW,zh}.js — all non-English locale files have the 3 new keys as untranslated English text:
'List available models from the configured API endpoint''No baseUrl configured. Please configure modelProviders or set the API endpoint.''Failed to fetch models: '
Non-English users will see English text when using /model list, inconsistent with the translated experience elsewhere in the UI.
[Suggestion] i18n key 'Failed to fetch models: ' has a fragile trailing space
This key value ends with a space, used as t('Failed to fetch models: ') + errorMessage. The trailing space is easily lost during translation, causing concatenation without spacing in certain locales. Consider changing to ${t('Failed to fetch models')}: ${errorMessage}.
— deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| // SSRF protection: block private IPs and localhost | ||
| const hostname = parsed.hostname; |
There was a problem hiding this comment.
[Critical] IPv6 bracket addresses bypass SSRF check
In Node.js 20, new URL('https://[::1]/api/').hostname returns [::1] (including brackets), but the SSRF check's hostname === '::1' and isPrivateIp()'s /^::1$/ regex both expect the unbracketed format. IPv6 addresses like https://[::1]/, https://[fe80::1]/ completely bypass SSRF protection.
| const hostname = parsed.hostname; | |
| const hostname = parsed.hostname.replace(/^\[|\]$/g, ''); |
— deepseek-v4-pro via Qwen Code /review
| return { | ||
| type: 'message', | ||
| messageType: 'info', | ||
| content: t('No models found from the configured endpoint.'), |
There was a problem hiding this comment.
[Suggestion] Missing i18n key 'No models found from the configured endpoint.'
The code uses t('No models found from the configured endpoint.') but this key is not added to any locale file. While t() will fall back to the key itself, non-English users will see a mixed-language experience. Please add this key to all 9 locale files.
— deepseek-v4-pro via Qwen Code /review
| type: 'message', | ||
| messageType: 'info', | ||
| content: output, | ||
| }; |
There was a problem hiding this comment.
[Suggestion] Model IDs not sanitized for ANSI escape sequences
Model IDs returned by the API are output to the terminal via models.join('\n') without calling the project's existing escapeAnsiCtrlCodes() (packages/cli/src/ui/utils/textUtils.ts:169). A malicious or compromised API endpoint could return model IDs containing terminal control codes to manipulate the terminal.
| }; | |
| const output = escapeAnsiCtrlCodes(models.join('\n')); |
— deepseek-v4-pro via Qwen Code /review
|
Thank you for your time, patience, and energy in reviewing this PR. I have implemented all the suggested changes in commit 941a5d4, including:
Tests have been updated and verified, and error handling logic has been refined to ensure robust timeout classification. Please let me know if there's anything else! |
wenshao
left a comment
There was a problem hiding this comment.
Additional findings (not mappable to a single diff line)
[Suggestion] Tests mock globalThis.fetch, bypassing fetchWithTimeout timeout/abort logic — The fetchModels tests directly mock globalThis.fetch, completely bypassing fetchWithTimeout's 30-second timeout, AbortController, and ETIMEDOUT error code path. Consider adding integration tests that mock fetchWithTimeout instead, verifying that timeout errors and network error codes propagate correctly to the subcommand output.
[Suggestion] fetchWithTimeout error code preservation change lacks tests — The change in packages/core/src/utils/fetch.ts:77 (passing getErrorCode(error) to FetchError) has no dedicated test coverage. Consider adding a test that mocks global.fetch rejecting with { code: 'ECONNREFUSED', message: '...' } and verifies the resulting FetchError carries the correct code.
| const startTime = Date.now(); | ||
| let response: Response; | ||
| try { | ||
| response = await fetchWithTimeout(url, FETCH_TIMEOUT_MS, headers); |
There was a problem hiding this comment.
[Critical] SSRF protection bypassable via HTTP redirect
fetchWithTimeout uses Node.js fetch() with default redirect: 'follow'. An attacker-controlled baseUrl (https://attacker.com/redirect) passes the hostname-level SSRF check, but the server can 302-redirect to a private IP (http://169.254.169.254/latest/meta-data/), which Node.js fetch follows automatically — bypassing the SSRF check entirely.
This issue was flagged in a previous review round and persists in the current code.
| response = await fetchWithTimeout(url, FETCH_TIMEOUT_MS, headers); | |
| // In fetchModels, use redirect: 'manual' or extend fetchWithTimeout to support redirect option | |
| response = await fetchWithTimeout(url, FETCH_TIMEOUT_MS, headers, { redirect: 'manual' }); |
— deepseek-v4-pro via Qwen Code /review
| const output = models.join('\n'); | ||
| return { | ||
| type: 'message', | ||
| messageType: 'info', |
There was a problem hiding this comment.
[Critical] Missing i18n key — 'No models found from the configured endpoint.'
t('No models found from the configured endpoint.') is called at this line but the key is not present in any locale file. The other 3 new i18n keys were correctly added to all 9 locales, but this one was missed. Non-English users will see the raw English key string.
This issue was flagged in a previous review round and persists in the current code.
| messageType: 'info', | |
| // Add to all 9 locale files (en.js already has it implicitly as fallback, but add explicitly for consistency): | |
| 'No models found from the configured endpoint.': | |
| 'No models found from the configured endpoint.', |
— deepseek-v4-pro via Qwen Code /review
| hostname === '::1' | ||
| ) { | ||
| throw new Error('baseUrl resolves to a private IP address (SSRF check)'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Misleading SSRF error message — "resolves to" implies DNS lookup
isPrivateIp only performs regex matching on the hostname string — it does NOT resolve DNS. The error message "baseUrl resolves to a private IP address (SSRF check)" is misleading. A hostname like internal-api.corp.local that DNS-resolves to 10.0.0.1 passes this check silently.
| } | |
| throw new Error('baseUrl points to a private or reserved IP address (SSRF check)'); |
— deepseek-v4-pro via Qwen Code /review
|
|
||
| // SSRF protection: block private IPs and localhost | ||
| const hostname = parsed.hostname; | ||
| if ( |
There was a problem hiding this comment.
[Suggestion] Redundant 127.0.0.1 and ::1 checks
isPrivateIp() already covers 127.0.0.1 (via /^127\./) and ::1 (via /^::1$/) in its regex patterns. Only localhost needs the explicit check since it's a hostname string, not an IP pattern. The redundant checks add cognitive burden and create a fragile split defense.
| if ( | |
| if (hostname === 'localhost') { | |
| throw new Error('baseUrl points to a private or reserved IP address (SSRF check)'); | |
| } | |
| if (isPrivateIp(baseUrl)) { | |
| throw new Error('baseUrl points to a private or reserved IP address (SSRF check)'); | |
| } |
— deepseek-v4-pro via Qwen Code /review
|
|
||
| debugLogger.debug('Fetching models from', url); | ||
|
|
||
| const startTime = Date.now(); |
There was a problem hiding this comment.
[Suggestion] HTTP error responses throw plain Error, losing troubleshooting hints
When the API returns a non-2xx status, fetchModels throws new Error(...) (plain Error, not FetchError). formatFetchErrorForUser checks for FetchError.code to show TLS/proxy troubleshooting hints, so HTTP errors (502 from a corporate proxy, 503 from a gateway) never trigger those hints.
| const startTime = Date.now(); | |
| throw new FetchError( | |
| `Request failed (${response.status}): ${sanitized}`, | |
| `HTTP_${response.status}` | |
| ); |
Import FetchError from @qwen-code/qwen-code-core to use it here.
— deepseek-v4-pro via Qwen Code /review
| Accept: 'application/json', | ||
| }; | ||
|
|
||
| if (apiKey) { |
There was a problem hiding this comment.
[Suggestion] Debug log leaks full URL — potential credential exposure
debugLogger.debug('Fetching models from', url) and the error log at line 288 log the complete constructed URL. If baseUrl contains embedded credentials (https://user:pass@api.example.com/v1/), they would be written to debug logs in plaintext.
| if (apiKey) { | |
| const safeUrl = new URL(url); | |
| safeUrl.username = ''; | |
| safeUrl.password = ''; | |
| debugLogger.debug('Fetching models from', safeUrl.toString()); |
— deepseek-v4-pro via Qwen Code /review
ReviewCode routes correctly (verified via 🔴 Blocker 1 — HTTPS-only + localhost/private-IP block makes
|
SSRF hardening:
- Fix IPv6 bracket bypass: strip brackets from hostname before SSRF checks
(Node 20+ returns '[::1]' from URL.hostname, but checks expected '::1')
- Add missing private IP ranges: 169.254.0.0/16 (AWS IMDS), 0.0.0.0/8,
100.64.0.0/10 (CGNAT)
- Add IPv4-mapped IPv6 detection: ::ffff:x.x.x.x addresses that Node
normalizes to hex form (e.g. ::ffff:c0a8:101) are now caught
- Pass redirect:'error' to fetchWithTimeout to prevent 302-redirect SSRF
bypass (attacker-controlled server redirecting to private IP)
- Change error message from "resolves to" to "points to" (isPrivateIp
checks hostname string, doesn't do DNS resolution)
Security/sanitization:
- Sanitize debug log URLs: strip embedded credentials before logging
- Sanitize model IDs: pass through escapeAnsiCtrlCodes() to prevent
terminal escape sequence injection from malicious API responses
- Non-2xx errors now include HTTP_ prefix in status code for better
troubleshooting with formatFetchErrorForUser
i18n:
- Switch from trailing-space concatenation to parameterized template:
t('Failed to fetch models: {{error}}', { error: errorMessage })
- Add missing i18n keys to en.js
Tests (39 total):
- Add SSRF tests: IPv6 bracket, IPv4-mapped IPv6, AWS IMDS, CGNAT
- Add test for parameterized i18n error template
- Add test for ANSI escape sequence sanitization
- Update existing tests for new error message format
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Security hardening: - Restore default fetch timeout from 15s to 30s (matches QwenLM#3797) - Add redirect:'error' to fetchWithTimeout to prevent SSRF via 302 redirects - Refactor isPrivateIp() to accept hostname string instead of URL — fixes a bug where passing a bare hostname (e.g. "192.168.1.1") to the old URL-parsing version would silently return false, allowing private IP access. Call sites (modelCommand.ts, web-fetch.ts) now parse URL first. - Add IPv4-mapped IPv6 detection to isPrivateIp (::ffff:x.x.x.x form) - Change SSRF error message from "resolves to" to "points to" (isPrivateIp checks hostname string, doesn't do DNS resolution) Error message improvements: - Non-2xx error messages now include the endpoint URL - List subcommand error uses parameterized i18n template with URL and error - Eliminates trailing-space bug when error message is empty URL normalization: - Use new URL() object instead of string replace to correctly handle query strings (e.g. ?version=2 no longer breaks /models append) Debug logging: - Log proxy status alongside URL, response code, and duration at all three debug points in fetchModels (request start, failure, response) - Log request failures with proxy context Core fetchWithTimeout: - Non-positive timeout (<=0) throws FetchError immediately instead of leaking raw AbortError through AbortController - Added clearTimeout in finally block to prevent timer leaks - Accept optional { redirect } parameter i18n: - Update 3 keys per locale (9 files): Failed to fetch models, baseUrl points to, Request to {{url}} failed Tests (48 total, +8 new): - data field present but not an array - baseUrl with query string preserves params - URL in non-2xx error message - "points to" wording in SSRF error - Subcommand metadata (name, kind, supportedModes) - Zero/negative timeout throws FetchError (not raw AbortError) - SSRF tests assert fetch was never called (short-circuits before network) - URL in list subcommand error message Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Closing this PR. All functionality from #3797 has been superseded by #3799 (commit `5266e5e2a`), which includes all security controls from this PR plus additional hardening (IPv4-mapped IPv6 detection, redirect:'error', hostname-based isPrivateIp, comprehensive test coverage). Thank you for the thorough review that shaped both PRs. |
Security hardening: - Restore default fetch timeout from 15s to 30s (matches QwenLM#3797) - Add redirect:'error' to fetchWithTimeout to prevent SSRF via 302 redirects - Refactor isPrivateIp() to accept hostname string instead of URL — fixes a bug where passing a bare hostname (e.g. "192.168.1.1") to the old URL-parsing version would silently return false, allowing private IP access. Call sites (modelCommand.ts, web-fetch.ts) now parse URL first. - Add IPv4-mapped IPv6 detection to isPrivateIp (::ffff:x.x.x.x form) - Change SSRF error message from "resolves to" to "points to" (isPrivateIp checks hostname string, doesn't do DNS resolution) Error message improvements: - Non-2xx error messages now include the endpoint URL - List subcommand error uses parameterized i18n template with URL and error - Eliminates trailing-space bug when error message is empty URL normalization: - Use new URL() object instead of string replace to correctly handle query strings (e.g. ?version=2 no longer breaks /models append) Debug logging: - Log proxy status alongside URL, response code, and duration at all three debug points in fetchModels (request start, failure, response) - Log request failures with proxy context Core fetchWithTimeout: - Non-positive timeout (<=0) throws FetchError immediately instead of leaking raw AbortError through AbortController - Added clearTimeout in finally block to prevent timer leaks - Accept optional { redirect } parameter i18n: - Update 3 keys per locale (9 files): Failed to fetch models, baseUrl points to, Request to {{url}} failed Tests (48 total, +8 new): - data field present but not an array - baseUrl with query string preserves params - URL in non-2xx error message - "points to" wording in SSRF error - Subcommand metadata (name, kind, supportedModes) - Zero/negative timeout throws FetchError (not raw AbortError) - SSRF tests assert fetch was never called (short-circuits before network) - URL in list subcommand error message Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Abhi <43648792+abhipatel12@users.noreply.github.com>
Summary
Adds
/model listsubcommand that queries the configured OpenAI-compatible/modelsendpoint and prints available model IDs in a scriptable format.Behavior
baseUrl+apiKey){baseUrl}/modelswith proper trailing-slash normalizationAuthorization: Bearer <apiKey>when an API key is configured/model,/model <id>, and/model --fastbehaviorScope
This PR intentionally does NOT add:
Validation
npm run build— passednpm run lint— passed (no new errors)npx tsc --noEmit(CLI package) — passedqwen -p "/model list"with Groq API (free tier) — worksqwen -p "/model list"with Hugging Face Inference API — worksqwen -p "/model"— existing behavior preservedqwen -p "/model --fast"— existing behavior preservedTesting Notes
The implementation was verified with free-tier APIs (Groq, Hugging Face). The pattern is standard OpenAI-compatible and expected to work with any compliant
/modelsendpoint.Risk
Low. The change is isolated to a new
/model listsubcommand and does not modify the existing model selection path.Closes #2412