-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(web-search): assess the bridge search endpoint as a destination (#4519) #4555
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
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 | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -507,9 +507,15 @@ export function requestPacingConfigError(value: unknown): string | null { | |||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Bounds for the opt-in passthrough web-search bridge (`providers.<name>.webSearchBridge`, | ||||||||||||||||||||||||||||||||||
| * #3761). Strict for the same reason `retryOn429` is: a misspelled key here would silently | ||||||||||||||||||||||||||||||||||
| * leave the bridge disarmed while the operator believes they enabled it. `endpoint` is only | ||||||||||||||||||||||||||||||||||
| * shape-checked here; `planPassthroughWebSearchBridge` re-validates the origin before any key | ||||||||||||||||||||||||||||||||||
| * is sent to it, because config validation is not an authorization boundary. | ||||||||||||||||||||||||||||||||||
| * leave the bridge disarmed while the operator believes they enabled it. | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * `endpoint` names the destination that receives this provider's API key, so it gets the same | ||||||||||||||||||||||||||||||||||
| * literal destination assessment `baseUrl` gets (#4519) — see `providerWebSearchBridgeConfigError` | ||||||||||||||||||||||||||||||||||
| * below. This schema itself still only shape-checks: it is `.catch(undefined)` at the provider | ||||||||||||||||||||||||||||||||||
| * row, and a hand-edited config file never reaches the error function at all. The authorization | ||||||||||||||||||||||||||||||||||
| * boundary is therefore `resolveOllamaWebSearchEndpoint`, which runs the same assessment and is | ||||||||||||||||||||||||||||||||||
| * the only reader of this field in the tree; config validation is where an operator is told why, | ||||||||||||||||||||||||||||||||||
| * not what makes the value safe. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| const providerWebSearchBridgeSchema = z.object({ | ||||||||||||||||||||||||||||||||||
| enabled: z.boolean().optional(), | ||||||||||||||||||||||||||||||||||
|
|
@@ -519,7 +525,11 @@ const providerWebSearchBridgeSchema = z.object({ | |||||||||||||||||||||||||||||||||
| endpoint: z.string().min(1).optional(), | ||||||||||||||||||||||||||||||||||
| }).strict(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| export function providerWebSearchBridgeConfigError(value: unknown): string | null { | ||||||||||||||||||||||||||||||||||
| export function providerWebSearchBridgeConfigError( | ||||||||||||||||||||||||||||||||||
| value: unknown, | ||||||||||||||||||||||||||||||||||
| providerName: string, | ||||||||||||||||||||||||||||||||||
| provider: Pick<OcxProviderConfig, "allowPrivateNetwork">, | ||||||||||||||||||||||||||||||||||
| ): string | null { | ||||||||||||||||||||||||||||||||||
| if (value === undefined) return null; | ||||||||||||||||||||||||||||||||||
| if (!value || typeof value !== "object" || Array.isArray(value)) { | ||||||||||||||||||||||||||||||||||
| return "webSearchBridge must be a plain object"; | ||||||||||||||||||||||||||||||||||
|
|
@@ -541,6 +551,17 @@ export function providerWebSearchBridgeConfigError(value: unknown): string | nul | |||||||||||||||||||||||||||||||||
| if (url.protocol !== "https:" && url.protocol !== "http:") { | ||||||||||||||||||||||||||||||||||
| return "webSearchBridge.endpoint must be an absolute http(s) URL"; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| // Same classifier baseUrl uses, so a metadata address is refused outright and loopback or | ||||||||||||||||||||||||||||||||||
| // private space needs the provider's allowPrivateNetwork opt-in (or a registry entry that is | ||||||||||||||||||||||||||||||||||
| // local by definition, which is what keeps a self-hosted Ollama working). Literal-only and | ||||||||||||||||||||||||||||||||||
| // synchronous, exactly as at the baseUrl boundary: no DNS is resolved here. | ||||||||||||||||||||||||||||||||||
| const destinationError = providerDestinationConfigError(providerName, { | ||||||||||||||||||||||||||||||||||
| baseUrl: endpoint, | ||||||||||||||||||||||||||||||||||
| allowPrivateNetwork: provider.allowPrivateNetwork, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| if (destinationError) { | ||||||||||||||||||||||||||||||||||
| return destinationError.replace(/^baseUrl/, "webSearchBridge.endpoint"); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+558
to
+564
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Apply destination policy only to the Ollama backend.
The current unconditional check rejects an existing configuration such as Keep the URL shape check for all configured endpoints. Run Proposed fix- const destinationError = providerDestinationConfigError(providerName, {
- baseUrl: endpoint,
- allowPrivateNetwork: provider.allowPrivateNetwork,
- });
- if (destinationError) {
- return destinationError.replace(/^baseUrl/, "webSearchBridge.endpoint");
+ if (parsed.data.backend === "ollama") {
+ const destinationError = providerDestinationConfigError(providerName, {
+ baseUrl: endpoint,
+ allowPrivateNetwork: provider.allowPrivateNetwork,
+ });
+ if (destinationError) {
+ return destinationError.replace(/^baseUrl/, "webSearchBridge.endpoint");
+ }
}As per coding guidelines, “Preserve existing public exports and configuration compatibility unless the task explicitly changes them.” 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
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 provider reference at
docs-site/src/content/docs/reference/configuration/providers.md:204still says that namingwebSearchBridge.endpointexplicitly is sufficient for a noncanonical Ollama origin, but this new assessment rejects metadata destinations and silently disarms loopback/private endpoints unlessallowPrivateNetworkor a local-by-default registry name applies. In particular, hand-edited configurations receive no validation message, so operators following the current documentation can enable a bridge that never runs; update the provider reference and keep translated versions consistent with these destination rules.AGENTS.md reference: src/AGENTS.md:L24-L29
Useful? React with 👍 / 👎.