mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-30 00:16:53 +00:00
fix: check the move's token against the target by re-authenticating it
The transfer endpoint looked the caller's token up in the `token` table, so an external-JWT login, which has no row there, could never move a session. It now resolves the same token against the target workspace, which refuses a token bound to the source whatever its kind, and still refuses job tokens. Also stop calling the move target a fork in the model note, the menu item, the editor placeholders and the moving notices: it can be the root or a sibling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
7b7b985e1b
commit
d363da770d
-23
@@ -1,23 +0,0 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT EXISTS(SELECT 1 FROM token WHERE token_hash = $1\n AND (expiration > NOW() OR expiration IS NULL)\n AND (workspace_id IS NULL OR workspace_id = $2))",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "exists",
|
||||
"type_info": "Bool"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "104a1aac6325f763f4ae8c42ad008f824419f2d2c161b1fd44844f0c6ff07d8c"
|
||||
}
|
||||
@@ -378,6 +378,43 @@ async fn test_transfer_drafts_to_fork(db: Pool<Postgres>) -> anyhow::Result<()>
|
||||
assert_eq!(r.status(), 403);
|
||||
assert_eq!(fork_count("u/test-user/b").await?, 0);
|
||||
|
||||
// A JWT has no `token` row: its own claims decide which workspaces it reaches.
|
||||
set_jwt_secret().await;
|
||||
let jwt = |workspace_ids: Vec<&str>, job_id: Option<&str>| {
|
||||
windmill_common::jwt::encode_with_internal_secret(windmill_common::auth::JWTAuthClaims {
|
||||
email: me.to_string(),
|
||||
username: "test-user".to_string(),
|
||||
is_admin: true,
|
||||
is_operator: false,
|
||||
groups: vec![],
|
||||
folders: vec![],
|
||||
label: None,
|
||||
workspace_id: None,
|
||||
workspace_ids: Some(workspace_ids.into_iter().map(String::from).collect()),
|
||||
exp: std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.unwrap()
|
||||
.as_secs() as usize
|
||||
+ 3600,
|
||||
job_id: job_id.map(String::from),
|
||||
scopes: None,
|
||||
audit_span: None,
|
||||
})
|
||||
};
|
||||
let job_id = uuid::Uuid::new_v4().to_string();
|
||||
for (claims, status) in [
|
||||
(jwt(vec![WS], None).await?, 403),
|
||||
(jwt(vec![WS, fork], Some(&job_id)).await?, 403),
|
||||
(jwt(vec![WS, fork], None).await?, 200),
|
||||
] {
|
||||
let r = client_for(&format!("jwt_{claims}"))
|
||||
.post(&transfer_url)
|
||||
.json(&json!({ "target_workspace": fork, "items": [{ "kind": "script", "path": "u/test-user/a" }] }))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(r.status(), status);
|
||||
}
|
||||
|
||||
// `a` was cloned into the fork: the whole transfer is refused and nothing moves.
|
||||
let body: serde_json::Value = c
|
||||
.post(&transfer_url)
|
||||
|
||||
@@ -1045,6 +1045,7 @@ pub struct TransferDraftsResponse {
|
||||
async fn transfer_drafts(
|
||||
authed: ApiAuthed,
|
||||
tokened: crate::auth::Tokened,
|
||||
Extension(auth_cache): Extension<std::sync::Arc<crate::auth::AuthCache>>,
|
||||
Extension(db): Extension<DB>,
|
||||
Extension(user_db): Extension<UserDB>,
|
||||
Path(w_id): Path<String>,
|
||||
@@ -1088,18 +1089,13 @@ async fn transfer_drafts(
|
||||
"{target} is not in the same workspace family as {w_id}"
|
||||
)));
|
||||
}
|
||||
// Authentication checked the token against `w_id` only. A token bound to that
|
||||
// workspace, or a job token (not in `token`), must not reach into the target.
|
||||
let token_valid_for_target = sqlx::query_scalar!(
|
||||
"SELECT EXISTS(SELECT 1 FROM token WHERE token_hash = $1
|
||||
AND (expiration > NOW() OR expiration IS NULL)
|
||||
AND (workspace_id IS NULL OR workspace_id = $2))",
|
||||
windmill_common::auth::hash_token(&tokened.token),
|
||||
target,
|
||||
)
|
||||
.fetch_one(&db)
|
||||
.await?
|
||||
.unwrap_or(false);
|
||||
// Authentication checked the token against `w_id` only. Re-resolving it against the
|
||||
// target refuses a token bound to `w_id`, whatever its kind (an external JWT has no
|
||||
// `token` row). A job token stays refused even where its claims list both workspaces.
|
||||
let token_valid_for_target = auth_cache
|
||||
.get_authed(Some(target.to_string()), &tokened.token)
|
||||
.await
|
||||
.is_some_and(|a| a.job_id.is_none());
|
||||
if !token_valid_for_target {
|
||||
return Err(Error::PermissionDenied(format!(
|
||||
"this token cannot act on {target}"
|
||||
|
||||
@@ -3675,7 +3675,7 @@ export class AIChatManager implements ChatViewHost {
|
||||
}
|
||||
sendUserToast(
|
||||
this.moveStarted
|
||||
? 'This session is moving to a fork. Your message was kept.'
|
||||
? 'This session is moving to another workspace. Your message was kept.'
|
||||
: 'This session is running in another tab. Your message was kept.',
|
||||
true
|
||||
)
|
||||
@@ -4859,7 +4859,7 @@ export class AIChatManager implements ChatViewHost {
|
||||
// "Text", not "message": chip edits are the part that does not survive.
|
||||
sendUserToast(
|
||||
this.moveStarted
|
||||
? 'This session is moving to a fork. Your text was kept.'
|
||||
? 'This session is moving to another workspace. Your text was kept.'
|
||||
: 'This session is running in another tab. Your text was kept.',
|
||||
true
|
||||
)
|
||||
|
||||
@@ -145,10 +145,10 @@
|
||||
<Button variant="default" unifiedSize="sm" onClick={closeTab}>Close tab</Button>
|
||||
</div>
|
||||
{:else if permissions.moving}
|
||||
<!-- Its edits are drafts, kept and shown again once the editor reopens in the fork. -->
|
||||
<!-- Its edits are drafts, kept and shown again once the editor reopens in the target. -->
|
||||
<p class="p-4 text-sm text-secondary">
|
||||
{pageItemKindLabel(item)} <span class="font-mono">{item.path}</span> reopens once the session has
|
||||
moved to its fork.
|
||||
moved.
|
||||
</p>
|
||||
{:else if permissions.readOnly}
|
||||
<div class="flex flex-col items-start gap-3 p-4 text-sm text-secondary">
|
||||
|
||||
@@ -403,7 +403,7 @@
|
||||
</div>
|
||||
{:else if permissions.moving && slot.kind !== 'viewer'}
|
||||
<div class="flex-1 flex items-center justify-center p-8 text-center text-sm text-secondary">
|
||||
The editor reopens once the session has moved to its fork.
|
||||
The editor reopens once the session has moved.
|
||||
</div>
|
||||
{:else if itemKind !== 'pipeline'}
|
||||
<div class="absolute inset-0 flex flex-col min-h-0 bg-surface">
|
||||
|
||||
@@ -532,7 +532,10 @@
|
||||
// Another tab moved the session while this dialog was open.
|
||||
if (actingWorkspaceId !== req.parent) {
|
||||
moveRequest = undefined
|
||||
sendUserToast('This session moved to another workspace meanwhile. Pick the fork again.', true)
|
||||
sendUserToast(
|
||||
'This session moved to another workspace meanwhile. Pick the workspace again.',
|
||||
true
|
||||
)
|
||||
return
|
||||
}
|
||||
// A turn started while the dialog was open (a job's auto-resume) owns the cross-tab hold
|
||||
@@ -612,7 +615,7 @@
|
||||
moveRequest = undefined
|
||||
await runtime?.manager
|
||||
.queueModelNote(
|
||||
`This session moved from workspace "${req.parent}" to its fork "${res.forkId}": every further tool call acts on "${res.forkId}". ` +
|
||||
`This session moved from workspace "${req.parent}" to workspace "${res.forkId}": every further tool call acts on "${res.forkId}". ` +
|
||||
(res.copied > 0
|
||||
? `${res.copied} draft(s) of this session were copied there` +
|
||||
(res.removed > 0 ? ` and removed from "${req.parent}". ` : '. ')
|
||||
@@ -756,7 +759,7 @@
|
||||
action: () => summaryInput?.edit()
|
||||
},
|
||||
{
|
||||
displayName: 'Move to a fork…',
|
||||
displayName: 'Move to another workspace…',
|
||||
icon: GitFork,
|
||||
// Before the first message, the "Run in" picker sets the workspace instead.
|
||||
hide: !hasFirstUserMessage || isUnavailable || session.archived,
|
||||
|
||||
Reference in New Issue
Block a user