fix(workspaces): validate fork id as a git branch name component#9049
Conversation
|
Claude finished @rubenfiszel's task in 3m 31s —— View job Mergeable, but should ideally address nits: validator accepts trailing SummaryThe fix is in the right place: validating the fork id synchronously in FindingsP2 — Validator allows trailing Git's rule 6 ( So for component in id.split('/') {
if component.is_empty() {
return reject("a path component cannot be empty");
}
if component.starts_with('.') { ... }
if component.ends_with(".lock") { ... }
}P2 — Validator accepts the bare prefix The prefix check is AGENTS.md complianceNo issues. The validator lives in Out-of-scope coverage gap (not introduced by this PR, just noting)
Test coverage
|
Deploying windmill with
|
| Latest commit: |
0c69202
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f83f07e1.windmill.pages.dev |
| Branch Preview URL: | https://create-branch-api-issue.windmill.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/windmill-common/src/workspaces.rs">
<violation number="1" location="backend/windmill-common/src/workspaces.rs:199">
P2: Reject ids that end with '/' as git check-ref-format forbids refnames ending with a slash; currently `wm-fork-foo/` passes validation and still fails at git ref creation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| if id.contains("@{") { | ||
| return reject("cannot contain '@{'"); | ||
| } | ||
| if id.contains("//") { |
There was a problem hiding this comment.
P2: Reject ids that end with '/' as git check-ref-format forbids refnames ending with a slash; currently wm-fork-foo/ passes validation and still fails at git ref creation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/windmill-common/src/workspaces.rs, line 199:
<comment>Reject ids that end with '/' as git check-ref-format forbids refnames ending with a slash; currently `wm-fork-foo/` passes validation and still fails at git ref creation.</comment>
<file context>
@@ -163,6 +163,65 @@ pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28217/sync-script-to-git-repo
+ if id.contains("@{") {
+ return reject("cannot contain '@{'");
+ }
+ if id.contains("//") {
+ return reject("cannot contain '//'");
+ }
</file context>
| if id.contains("//") { | |
| if id.ends_with('/') { | |
| return reject("cannot end with '/'"); | |
| } | |
| if id.contains("//") { |
Codex ReviewShould address issues before merging: trailing slash fork ids still pass validation Findings[P1] Test coverageBackend unit tests were added for the validator and cover several forbidden characters and sequences, but they miss the trailing slash case above. I did not run the Rust test suite; I validated the missed case directly with Manual verification needed before merge: call |
Pi ReviewGood to merge. SummaryThis PR adds synchronous validation of fork workspace IDs against What I verified
Test coverageBackend — four new Rust unit tests in Manual verification still needed: Run the git-sync worker after creating a fork with an edge-case-but-valid ID (e.g., |
1 participant
Summary
POST /w/{workspace}/workspaces/create_workspace_fork_branchwas returning 200 even when the id contained characters that make the resulting git branch name invalid (e.g.wm-fork-test:allow→wm-fork/master/test:allow). The endpoint queues a deferred git-sync worker job, so the actual branch creation failure never made it back to the API caller. Reported by Matthew Parry.This PR validates the fork id synchronously at the API layer against
git check-ref-formatrules, rejecting unsafe inputs with HTTP 400 before a job is queued. The same validator is also applied toPOST /w/{workspace}/workspaces/create_fork, where the id likewise becomes a git branch suffix.Changes
validate_fork_workspace_id()inwindmill-common/src/workspaces.rs. Enforces the existingwm-fork-prefix check plus git ref rules: rejects:,~,^,?,*,[,\, control chars,..,@{,//, trailing.,.locksuffix, and components starting with..create_workspace_fork_branchandcreate_workspace_forkinwindmill-api-workspaces/src/workspaces.rs. The previous inlinestarts_with(WM_FORK_PREFIX)block increate_workspace_forkis folded into the helper so both endpoints share the same error contract.Test plan
cargo test -p windmill-common --lib workspaces::tests— all four new tests passPOST /api/w/admins/workspaces/create_workspace_fork_branchwith{"id":"wm-fork-test-allow","name":"test-allow"}→ 200{"id":"wm-fork-test:allow",...}→ 400 withFork workspace id 'wm-fork-test:allow' is invalid: contains forbidden character ':' (must be a valid git branch name component){"id":"not-a-fork",...}→ 400 with prefix errorPOST /api/w/admins/workspaces/create_forkwith the same colon id → 400 (validation also applies to fork creation)Summary by cubic
Validate fork workspace ids as safe git branch components to prevent invalid refs. The API now returns 400 for bad ids and avoids enqueueing failing git-sync jobs.
validate_fork_workspace_id()inwindmill-commonenforcingWM_FORK_PREFIXandgit check-ref-formatrules.create_workspace_fork_branchandcreate_workspace_forkinwindmill-api-workspacesfor consistent early rejection.Written for commit 0c69202. Summary will update on new commits.