mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-05 08:02:18 +00:00
fix: reject hub integration slugs that would re-target the proxied request
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
bc0bef14c2
commit
7983bdf5a7
File diff suppressed because it is too large
Load Diff
@@ -15,7 +15,7 @@
|
||||
],
|
||||
"validation": {
|
||||
"status": "live_validated",
|
||||
"method": "smoke test via `bun --env-file=.env.test -e` (+ `bun test` with the windmill-client fake for the trigger) against a free Confluence Cloud site (edwind-wm-confluence.atlassian.net) with an API token. All 13 actions + the trigger returned 2xx: full page lifecycle (create -> get -> update -> list -> delete), blog post create/update/delete, list_spaces, search_content (CQL), get_current_user, and the new_or_updated_page polling trigger.",
|
||||
"method": "smoke test via `bun --env-file=.env.test -e` (+ `bun test` with the windmill-client fake for the trigger) against a free Confluence Cloud site (an Atlassian Cloud site) with an API token. All 13 actions + the trigger returned 2xx: full page lifecycle (create -> get -> update -> list -> delete), blog post create/update/delete, list_spaces, search_content (CQL), get_current_user, and the new_or_updated_page polling trigger.",
|
||||
"sources": {
|
||||
"pages": "HIGH \u2014 Pipedream actions + official v2 OpenAPI; create/get/update/list/delete all run live (2xx)",
|
||||
"blogposts": "HIGH \u2014 Pipedream actions + official v2 OpenAPI; create/update/delete all run live (2xx)",
|
||||
@@ -101,7 +101,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"baseUrl": {
|
||||
"description": "Base URL of your Confluence Cloud site, e.g. https://your-domain.atlassian.net (no trailing slash, no /wiki).",
|
||||
"description": "Base URL of your Confluence Cloud site, e.g. https://an Atlassian Cloud site (no trailing slash, no /wiki).",
|
||||
"type": "string"
|
||||
},
|
||||
"email": {
|
||||
|
||||
@@ -15,7 +15,7 @@
|
||||
],
|
||||
"validation": {
|
||||
"status": "live_validated",
|
||||
"method": "All surfaces run live against a ServiceNow PDI (dev209312, Zurich) on 2026-06-04 with Basic auth: Table CRUD (create / get / update PATCH+PUT / delete on incident, delete verified by 404), list_records (encoded query + fields + pagination), aggregate (Stats group-by count), get_current_user, list_tables, and the table/fields/group_by dynselect resolvers. Attachment upload (binary /file) -> list -> download -> delete round-trip with the downloaded bytes byte-for-byte matching the upload. insert_import_set + get_import_set_result against imp_user (transform status surfaced). Trigger e2e-tested via bun test with the windmill-client fake. All created records cleaned up.",
|
||||
"method": "All surfaces run live against a ServiceNow PDI (a developer instance, Zurich) on 2026-06-04 with Basic auth: Table CRUD (create / get / update PATCH+PUT / delete on incident, delete verified by 404), list_records (encoded query + fields + pagination), aggregate (Stats group-by count), get_current_user, list_tables, and the table/fields/group_by dynselect resolvers. Attachment upload (binary /file) -> list -> download -> delete round-trip with the downloaded bytes byte-for-byte matching the upload. insert_import_set + get_import_set_result against imp_user (transform status surfaced). Trigger e2e-tested via bun test with the windmill-client fake. All created records cleaned up.",
|
||||
"sources": {
|
||||
"table_crud": "HIGH - run live (create/get/update PATCH & PUT/delete on incident)",
|
||||
"aggregate_stats": "HIGH - run live (/api/now/stats group-by count on incident)",
|
||||
@@ -95,8 +95,8 @@
|
||||
"properties": {
|
||||
"instance_url": {
|
||||
"default": "",
|
||||
"description": "Instance base URL, e.g. https://dev12345.service-now.com (no trailing slash). Every REST call is made against this host.",
|
||||
"placeholder": "https://dev12345.service-now.com",
|
||||
"description": "Instance base URL, e.g. https://a developer a ServiceNow instance (no trailing slash). Every REST call is made against this host.",
|
||||
"placeholder": "https://a developer a ServiceNow instance",
|
||||
"type": "string"
|
||||
},
|
||||
"password": {
|
||||
|
||||
@@ -720,6 +720,31 @@ pub fn global_service() -> Router {
|
||||
mod tests {
|
||||
use super::trim_to_top_score;
|
||||
|
||||
// The blob carries an explicit `"description": null` for roughly a fifth of hub
|
||||
// scripts, and an older hub omits the key entirely. A bare String here fails the
|
||||
// whole 155 MB array and takes hub search down with it.
|
||||
#[test]
|
||||
fn reads_a_hub_script_whether_or_not_it_has_a_description() {
|
||||
let present = r#"{"ask_id":1,"id":2,"version_id":3,"summary":"s","description":"d","app":"a","kind":"script","embedding":[]}"#;
|
||||
let null = r#"{"ask_id":1,"id":2,"version_id":3,"summary":"s","description":null,"app":"a","kind":"script","embedding":[]}"#;
|
||||
let missing = r#"{"ask_id":1,"id":2,"version_id":3,"summary":"s","app":"a","kind":"script","embedding":[]}"#;
|
||||
|
||||
assert_eq!(
|
||||
serde_json::from_str::<super::HubScript>(present)
|
||||
.unwrap()
|
||||
.description,
|
||||
Some("d".to_string())
|
||||
);
|
||||
for without in [null, missing] {
|
||||
assert_eq!(
|
||||
serde_json::from_str::<super::HubScript>(without)
|
||||
.unwrap()
|
||||
.description,
|
||||
None
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trims_scores_more_than_5pct_below_top() {
|
||||
// top=1.0, cutoff at 0.95: 0.96 stays (0.04 drop), 0.93 is the first
|
||||
|
||||
@@ -8819,6 +8819,7 @@ paths:
|
||||
type: string
|
||||
description:
|
||||
type: string
|
||||
nullable: true
|
||||
app:
|
||||
type: string
|
||||
version_id:
|
||||
|
||||
@@ -37,6 +37,17 @@ async fn list_hub_integrations(
|
||||
Ok::<_, Error>((status_code, headers, response))
|
||||
}
|
||||
|
||||
/// Axum percent-decodes a path parameter, so an interpolated slug carrying `..`, `?`
|
||||
/// or `#` re-targets the proxied GET at another path on the hub origin — with the
|
||||
/// instance's hub credentials attached. Slugs are `[A-Za-z0-9_-]`; reject the rest.
|
||||
fn is_hub_integration_slug(app: &str) -> bool {
|
||||
!app.is_empty()
|
||||
&& app.len() <= 64
|
||||
&& app
|
||||
.chars()
|
||||
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
|
||||
}
|
||||
|
||||
/// Everything a caller needs to write code against one integration: its resource
|
||||
/// types, the provider knowledge the content repo authored, and facts derived from
|
||||
/// the shipped scripts. A hub older than the endpoint answers 404, which passes
|
||||
@@ -45,6 +56,11 @@ async fn get_hub_integration_meta(
|
||||
Path(app): Path<String>,
|
||||
Extension(db): Extension<DB>,
|
||||
) -> impl IntoResponse {
|
||||
if !is_hub_integration_slug(&app) {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"Not a valid integration name: {app}"
|
||||
)));
|
||||
}
|
||||
let (status_code, headers, response) = query_elems_from_hub(
|
||||
&HTTP_CLIENT,
|
||||
&format!("{}/integrations/{}/meta", **HUB_BASE_URL.load(), app),
|
||||
@@ -54,3 +70,26 @@ async fn get_hub_integration_meta(
|
||||
.await?;
|
||||
Ok::<_, Error>((status_code, headers, response))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::is_hub_integration_slug;
|
||||
|
||||
#[test]
|
||||
fn rejects_slugs_that_would_re_target_the_proxied_request() {
|
||||
assert!(is_hub_integration_slug("confluence"));
|
||||
assert!(is_hub_integration_slug("aws-ses"));
|
||||
assert!(is_hub_integration_slug("bamboo_hr"));
|
||||
assert!(is_hub_integration_slug("RSS"));
|
||||
|
||||
for escape in [
|
||||
"../../scripts/top",
|
||||
"confluence?foo=bar",
|
||||
"confluence#frag",
|
||||
"confluence/meta",
|
||||
"",
|
||||
] {
|
||||
assert!(!is_hub_integration_slug(escape), "accepted {escape}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -70,10 +70,11 @@ export interface ChatLoopConfig {
|
||||
* lets the caller recover partial output if the loop throws or is aborted.
|
||||
*/
|
||||
addedMessages?: ChatCompletionMessageParam[]
|
||||
/** Called before each request (e.g. to refresh tool schemas, or to record which
|
||||
* model it is about to use), including again mid-iteration when a fallback
|
||||
* changes `webSearch`. That argument is the effective value, and the system
|
||||
* message is read after this returns, so a caller can resync its prompt in time. */
|
||||
/** Called before each iteration (e.g. to refresh tool schemas, or to record which
|
||||
* model it is about to use), and again when the Completions fallback drops
|
||||
* `webSearch`. That argument is the effective value, and the system message is read
|
||||
* after this returns, so a caller can resync its prompt in time. A same-iteration
|
||||
* retry does not re-enter this; `onWebSearchUnavailable` covers that path. */
|
||||
onBeforeIteration?: (
|
||||
tools: Tool<any>[],
|
||||
helpers: any,
|
||||
|
||||
@@ -1671,13 +1671,38 @@ describe('getHubIntegrationTool', () => {
|
||||
expect(!!parsed.scripts_note).toBe(expected)
|
||||
})
|
||||
|
||||
// A hub that times out has said nothing about whether the integration exists, and
|
||||
// reporting it as absent would stick for the rest of the conversation.
|
||||
it('does not report a transient hub failure as a missing integration', async () => {
|
||||
const { IntegrationService } = await import('$lib/gen')
|
||||
Object.assign(IntegrationService, {
|
||||
getHubIntegrationMeta: vi.fn(async () => {
|
||||
throw Object.assign(new Error('Service Unavailable'), { status: 503 })
|
||||
}),
|
||||
listHubIntegrations: vi.fn(async () => [{ name: 'confluence' }])
|
||||
})
|
||||
|
||||
const { getHubIntegrationTool, clearHubIntegrationsCache } = await import('./shared')
|
||||
clearHubIntegrationsCache()
|
||||
const parsed = JSON.parse(
|
||||
await getHubIntegrationTool.fn({
|
||||
args: { integration: 'confluence' },
|
||||
toolId: 't1',
|
||||
toolCallbacks: { setToolStatus: vi.fn() }
|
||||
} as any)
|
||||
)
|
||||
|
||||
expect(parsed.error).toContain('Could not reach the hub')
|
||||
expect(parsed.error).not.toContain('No hub metadata')
|
||||
})
|
||||
|
||||
// A hub with no such integration and one too old to serve the endpoint both 404;
|
||||
// neither may surface as a tool error, since the model can still read scripts.
|
||||
it('suggests real slugs instead of failing when the integration is unknown', async () => {
|
||||
const { IntegrationService } = await import('$lib/gen')
|
||||
Object.assign(IntegrationService, {
|
||||
getHubIntegrationMeta: vi.fn(async () => {
|
||||
throw new Error('Not Found')
|
||||
throw Object.assign(new Error('Not Found'), { status: 404 })
|
||||
}),
|
||||
listHubIntegrations: vi.fn(async () => [{ name: 'stripe' }, { name: 'slack' }])
|
||||
})
|
||||
|
||||
@@ -1247,9 +1247,15 @@ export function isHubPath(path: string): boolean {
|
||||
const MAX_BROWSED_HUB_SCRIPTS = 20
|
||||
const MAX_SUGGESTED_INTEGRATIONS = 5
|
||||
|
||||
/** Common shape of the two hub listings. Both carry a description, but the hub
|
||||
* has none for roughly a fifth of its scripts. */
|
||||
type HubScriptHit = { version_id: number; app: string; summary: string; description?: string }
|
||||
/** Common shape of the two hub listings. Both carry a description, but the hub has
|
||||
* none for roughly a fifth of its scripts, and says so as a null rather than by
|
||||
* omitting the key. */
|
||||
type HubScriptHit = {
|
||||
version_id: number
|
||||
app: string
|
||||
summary: string
|
||||
description?: string | null
|
||||
}
|
||||
|
||||
/** The integration slugs are a large but static list, so one fetch per session
|
||||
* is enough. Only matched slugs ever reach the model, never the whole list. */
|
||||
@@ -1281,12 +1287,10 @@ async function suggestHubIntegrations(query: string): Promise<string[]> {
|
||||
.slice(0, MAX_SUGGESTED_INTEGRATIONS)
|
||||
}
|
||||
|
||||
/** Matches a query word against a slug on word boundaries rather than by bare
|
||||
* substring. A substring test reads every three-letter English word as a hit —
|
||||
* `for` in sales*for*ce, `the` in basis_*the*ory — so a request that names no
|
||||
* integration still came back with five confident-looking ones. Short tokens must
|
||||
* equal a slug or one of its parts, which is also what reaches the two-character
|
||||
* slugs (`s3`, `wiz`) that a length filter alone hides. */
|
||||
/** Matches a query word against a slug on word boundaries. A bare substring test
|
||||
* makes every three-letter English word a hit — `for` in sales*for*ce, `the` in
|
||||
* basis_*the*ory. Short tokens must equal a slug or a part, which is also what
|
||||
* reaches the two-character slugs (`s3`, `wiz`) a length floor would hide. */
|
||||
function tokenMatchesSlug(token: string, slug: string): boolean {
|
||||
const parts = slug.split(/[_-]/).filter(Boolean)
|
||||
if (slug === token || parts.includes(token)) {
|
||||
@@ -1307,12 +1311,9 @@ function tokenMatchesSlug(token: string, slug: string): boolean {
|
||||
}
|
||||
|
||||
/** The slug of an integration the query names outright, when it names exactly one.
|
||||
* Semantic search ranks on the whole of a script's text, so a named vendor is weak
|
||||
* signal against the task words: "create a jira ticket" ranks netlify, zendesk and
|
||||
* intercom above every Jira script, and "look up an account in salesforce" puts
|
||||
* Pinterest first, because its summaries say Salesforce. Narrowing to the named
|
||||
* integration fixes both. The test is deliberately exact — a fuzzy one reads `send`
|
||||
* in "send an invoice" as sendgrid and quietly searches the wrong integration. */
|
||||
* Semantic search ranks a named vendor weakly against the task words — "create a jira
|
||||
* ticket" puts netlify and zendesk above every Jira script — so narrowing to it wins.
|
||||
* Matching must stay exact: fuzzily, `send` in "send an invoice" is sendgrid. */
|
||||
async function integrationNamedIn(query: string): Promise<string | undefined> {
|
||||
const list = await loadHubIntegrations()
|
||||
const words = new Set(
|
||||
@@ -1373,14 +1374,20 @@ export const getHubIntegrationTool = {
|
||||
let doc: Awaited<ReturnType<typeof IntegrationService.getHubIntegrationMeta>>
|
||||
try {
|
||||
doc = await IntegrationService.getHubIntegrationMeta({ app: integration })
|
||||
} catch {
|
||||
// An unknown slug and a hub predating the endpoint both answer 404, and the
|
||||
// response is the same either way: hand back real slugs so the model can
|
||||
// retry or fall back to reading scripts.
|
||||
toolCallbacks.setToolStatus(toolId, { content: `No hub integration named ${integration}` })
|
||||
} catch (err) {
|
||||
// Only a 404 means the integration is absent — an unknown slug, or a hub
|
||||
// predating the endpoint. Reporting a timeout the same way would teach the
|
||||
// model that a real integration does not exist for the rest of the chat.
|
||||
const absent = (err as { status?: number } | undefined)?.status === 404
|
||||
const label = absent
|
||||
? `No hub integration named ${integration}`
|
||||
: `Could not reach the hub for ${integration}`
|
||||
toolCallbacks.setToolStatus(toolId, { content: label })
|
||||
const suggested = await suggestHubIntegrations(integration)
|
||||
return JSON.stringify({
|
||||
error: `No hub metadata for "${integration}".`,
|
||||
error: absent
|
||||
? `No hub metadata for "${integration}".`
|
||||
: `Could not reach the hub for "${integration}"; it may still exist. Read its scripts instead, or try again.`,
|
||||
suggested_integrations: suggested
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user