diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 9068fbdfe2..e280586a1a 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -8449,7 +8449,7 @@ paths: - picks /w/{workspace}/resources/type/hub/pick/{name}: - get: + post: summary: record a hub resource type pick operationId: pickHubResourceType tags: diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 0d29c1c369..dbc8bfb0a7 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -5516,6 +5516,9 @@ mod embed_token_tests { "GET", ), ("/api/w/test/resources/list_search", "GET"), + // The metadata allowlist matches `resources/type/` by prefix, so a route + // added under it that is not a read must be denied by its method. + ("/api/w/test/resources/type/hub/pick/slack", "POST"), // Workspace-wide job enumeration/export must NOT be reachable — an app // reads only jobs it launched, by id (blocked via the app_embed sentinel). ("/api/w/test/jobs/list", "GET"), diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index d19387ab53..3f524f0969 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -91,7 +91,7 @@ pub fn workspaced_service() -> Router { .route("/type/listnames", get(list_resource_types_names)) .route("/type/resource_counts", get(list_resource_counts_by_type)) .route("/type/hub/picked", get(list_hub_picked_resource_types)) - .route("/type/hub/pick/{name}", get(pick_hub_resource_type)) + .route("/type/hub/pick/{name}", post(pick_hub_resource_type)) .route("/type/get/{name}", get(get_resource_type)) .route("/type/exists/{name}", get(exists_resource_type)) .route("/type/update/{name}", post(update_resource_type)) @@ -2693,10 +2693,17 @@ struct PickHubResourceTypeResult { /// Never fails the caller: a hub predating the route, an unreachable one, and a type that /// is local-only all mean the same thing — not counted — and the request that reaches here /// has already saved the user's resource. +/// +/// POST, and scoped as a write, because it changes state on the hub under the instance's +/// own credentials. The sibling `/type/*` routes are metadata reads that a `resources:run` +/// app-embed token may make, and both the method and this check keep such a token — which +/// is untrusted app JavaScript — from driving hub counters through us. async fn pick_hub_resource_type( + authed: ApiAuthed, Extension(db): Extension, Path((_w_id, name)): Path<(String, String)>, ) -> JsonResult { + check_scopes(&authed, || "resources:write".to_string())?; let hub_base_url = (**windmill_common::HUB_BASE_URL.load()).clone(); let success = async { let id = hub_resource_type_ids(&db, &hub_base_url) @@ -2736,6 +2743,29 @@ struct HubPickedResourceTypes { resource_types: Vec, } +#[cfg(test)] +mod hub_picks_tests { + use super::HubPickedResourceTypes; + + /// The hub counts picks in a bigint, which postgres.js serialises as a string. Typing + /// the field as a plain i64 fails the whole response, and the ranking silently empties. + #[test] + fn picks_decode_from_a_string_or_a_number() { + let parsed: HubPickedResourceTypes = serde_json::from_str( + r#"{"resource_types":[{"name":"slack","picks":"42"},{"name":"github","picks":7}]}"#, + ) + .unwrap(); + assert_eq!( + parsed + .resource_types + .iter() + .map(|rt| (rt.name.as_str(), rt.picks)) + .collect::>(), + vec![("slack", 42), ("github", 7)] + ); + } +} + /// The hub's own popularity ranking for resource types. Empty rather than an error when /// the hub has no such endpoint, so the pickers reading this treat an older or private hub /// as "no hub signal" and fall back to what the workspace itself uses. diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index 32519b8eab..ec49be4802 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -42,6 +42,7 @@ import { sameTopDomainOrigin } from '$lib/cookies' import SyncResourceTypes from './SyncResourceTypes.svelte' import { + alphabetical, byPopularity, hubResourceTypePicks, localResourceTypeCounts, @@ -390,7 +391,7 @@ * has resources of. Both signals are fetched, so the rows render in the order the lists * arrived in and re-sort when this lands. */ - let popularity: (a: string, b: string) => number = $state(() => 0) + let popularity: (a: string, b: string) => number = $state(alphabetical) async function loadPopularity() { if (!effectiveWorkspace) return diff --git a/frontend/src/lib/components/flows/pickers/PickHubScript.svelte b/frontend/src/lib/components/flows/pickers/PickHubScript.svelte index 4b2b608f19..b6be224087 100644 --- a/frontend/src/lib/components/flows/pickers/PickHubScript.svelte +++ b/frontend/src/lib/components/flows/pickers/PickHubScript.svelte @@ -10,7 +10,11 @@ import TextInput from '$lib/components/text_input/TextInput.svelte' import { disableHubStore, workspaceStore } from '$lib/stores' import { logHubScriptPick } from '$lib/utils/featureUsage' - import { byPopularity, localResourceTypeCounts } from '$lib/components/pickerPopularity' + import { + alphabetical, + byPopularity, + localResourceTypeCounts + } from '$lib/components/pickerPopularity' interface Props { kind?: HubScriptKind & string @@ -47,7 +51,7 @@ }[] = $state([]) let allApps: string[] = $state([]) - let popularity: (a: string, b: string) => number = $state(() => 0) + let popularity: (a: string, b: string) => number = $state(alphabetical) let apps: string[] = $derived.by(() => filter.length > 0 ? Array.from(new Set(items?.map((x) => x.app) ?? [])).sort(popularity) diff --git a/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte b/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte index b6d7ac8b98..b1bf7ff11e 100644 --- a/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte +++ b/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte @@ -50,7 +50,11 @@ import { Alert } from '$lib/components/common' import type { FlowBuilderWhitelabelCustomUi } from '$lib/components/custom_ui' import { logHubScriptPick } from '$lib/utils/featureUsage' - import { byPopularity, localResourceTypeCounts } from '$lib/components/pickerPopularity' + import { + alphabetical, + byPopularity, + localResourceTypeCounts + } from '$lib/components/pickerPopularity' let customUi: undefined | FlowBuilderWhitelabelCustomUi = getContext('customUi') @@ -95,7 +99,7 @@ }: Props = $props() let allApps: string[] = $state([]) - let popularity: (a: string, b: string) => number = $state(() => 0) + let popularity: (a: string, b: string) => number = $state(alphabetical) $effect(() => { if (filter.length > 0) { apps = Array.from(new Set(items?.map((x) => x.app) ?? [])).sort(popularity) diff --git a/frontend/src/lib/components/pickerPopularity.test.ts b/frontend/src/lib/components/pickerPopularity.test.ts index 82a7c519a1..7af85ecdeb 100644 --- a/frontend/src/lib/components/pickerPopularity.test.ts +++ b/frontend/src/lib/components/pickerPopularity.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { byPopularity } from './pickerPopularity' +import { alphabetical, byPopularity } from './pickerPopularity' const order = (names: string[], hub: Record, local: Record = {}) => [...names].sort(byPopularity(hub, local)) @@ -27,4 +27,10 @@ describe('byPopularity', () => { 'github' ]) }) + + // The lists render before either signal lands, and one of them arrives in a server-side + // HashMap's iteration order, so the resting comparator has to sort rather than no-op. + it('leaves an alphabetical order with no signal at all', () => { + expect(['stripe', 'ably', 'github'].sort(alphabetical)).toEqual(['ably', 'github', 'stripe']) + }) }) diff --git a/frontend/src/lib/components/pickerPopularity.ts b/frontend/src/lib/components/pickerPopularity.ts index 9401f8a4d1..a289d09b73 100644 --- a/frontend/src/lib/components/pickerPopularity.ts +++ b/frontend/src/lib/components/pickerPopularity.ts @@ -1,4 +1,6 @@ +import { get } from 'svelte/store' import { ResourceService } from '$lib/gen' +import { disableHubStore } from '$lib/stores' import { createCache } from '$lib/utils' import { isCustomResourceTypeName } from './resourceTypeDisplay' @@ -39,8 +41,13 @@ const localCountsCached = createCache( { invalidateMs: CACHE_MS } ) -/** What the hub sees people pick, per resource type. Empty on a hub that counts nothing. */ +/** + * What the hub sees people pick, per resource type. Empty on a hub that counts nothing, + * and on an instance that has switched the hub off — a closed environment must not spend a + * request on hub.windmill.dev just to order a list. + */ export function hubResourceTypePicks(workspace: string): Promise { + if (get(disableHubStore)) return Promise.resolve({}) return hubPicksCached({ workspace }) } @@ -59,6 +66,7 @@ export function localResourceTypeCounts(workspace: string): Promise {}) } @@ -78,3 +86,13 @@ export function byPopularity( return (a, b) => (hub[b] ?? 0) - (hub[a] ?? 0) || (local[b] ?? 0) - (local[a] ?? 0) || a.localeCompare(b) } + +/** + * The ordering to hold before either signal has landed: the alphabetical floor, which is + * what `byPopularity` degrades to anyway. + * + * A list has to be sorted by *something* from its first paint — one source of these names + * is a `HashMap` on the server, so leaving them unsorted means hash order, which differs + * between processes. + */ +export const alphabetical = byPopularity({}, {})