diff --git a/.changeset/fix-branch-name-extra-slash.md b/.changeset/fix-branch-name-extra-slash.md new file mode 100644 index 0000000000..c1205bd4fa --- /dev/null +++ b/.changeset/fix-branch-name-extra-slash.md @@ -0,0 +1,5 @@ +--- +"tinacms": patch +--- + +Fix "Save to new branch" failing with "Branch operation failed" when the derived branch name is not a valid Git ref, e.g. when a collection's `path` has a trailing slash, producing `content/articles//foo.mdx` and the invalid ref `tina/articles//foo`. The default branch name derived from the file path, and any user-typed name, are now normalised to a valid ref: repeated and leading/trailing slashes collapse, characters Git forbids in refs (whitespace, control characters, `~ ^ : ? * [ \` and the `@{` sequence) become hyphens, `..` runs collapse, and leading dots and trailing `.` / `.lock` are stripped per path component. Saving is disabled while the name normalises to an empty string. The same normalisation now runs when creating a branch from the branch switcher and from the deleted-branch recovery modal, and the duplicated `formatBranchName` helpers are unified into a single util (the legacy branch switcher previously deleted invalid characters; it now replaces them with hyphens like the main switcher). diff --git a/.github/scripts/check-changeset.mjs b/.github/scripts/check-changeset.mjs new file mode 100644 index 0000000000..e456a7c545 --- /dev/null +++ b/.github/scripts/check-changeset.mjs @@ -0,0 +1,107 @@ +// Flags a PR that changes a published package's dependencies with no changeset: it would +// merge with no version bump and no release, so the change never reaches npm. Fails open. +import { execSync } from 'node:child_process'; +import { readFileSync, existsSync, readdirSync, statSync } from 'node:fs'; +import { join } from 'node:path'; + +const base = process.argv[2] ?? 'origin/main'; + +const git = (cmd) => execSync(cmd, { encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'] }); + +const skip = (why) => { + console.log(`Changeset check skipped: ${why}`); + process.exit(0); +}; + +let changed; +try { + changed = git(`git diff --name-only ${base}...HEAD`).split('\n').filter(Boolean); +} catch { + skip(`cannot diff against "${base}". A shallow clone needs fetch-depth: 0.`); +} + +if (changed.some((f) => /^\.changeset\/.+\.md$/.test(f))) { + console.log('A changeset is present. Nothing to check.'); + process.exit(0); +} + +let ignore = []; +try { + ignore = JSON.parse(readFileSync('.changeset/config.json', 'utf8')).ignore ?? []; +} catch { + skip('.changeset/config.json is missing or unreadable.'); +} +const ignored = (name) => + ignore.some((p) => (p.endsWith('/*') ? name.startsWith(p.slice(0, -1)) : p === name)); + +// Walked rather than a fixed directory list, so a new nesting level cannot drop out silently. +const published = new Map(); +const walk = (dir, depth = 0) => { + if (depth > 4 || !existsSync(dir)) return; + const manifest = join(dir, 'package.json'); + if (existsSync(manifest)) { + try { + const pkg = JSON.parse(readFileSync(manifest, 'utf8')); + if (!pkg.private && pkg.name && !ignored(pkg.name)) { + published.set(pkg.name, { manifest, pkg }); + } + return; // a package root is a leaf; do not descend into its own subpackages + } catch { + /* unparseable manifest: ignore rather than fail the PR */ + } + } + for (const entry of readdirSync(dir)) { + if (entry === 'node_modules' || entry.startsWith('.')) continue; + const next = join(dir, entry); + if (statSync(next).isDirectory()) walk(next, depth + 1); + } +}; +walk('packages'); + +const affected = new Set(); + +for (const [name, { manifest }] of published) { + if (changed.includes(manifest)) affected.add(name); +} + +if (changed.includes('pnpm-workspace.yaml')) { + const catalogKeys = (text) => { + const out = new Map(); + let inCatalog = false; + for (const line of text.split('\n')) { + if (/^catalog:\s*$/.test(line)) { inCatalog = true; continue; } + if (inCatalog && /^\S/.test(line)) break; + const m = inCatalog && line.match(/^\s+(\S+):\s*(.+?)\s*$/); + if (m) out.set(m[1].replace(/^['"]|['"]$/g, ''), m[2]); + } + return out; + }; + + let before; + try { + before = catalogKeys(git(`git show ${base}:pnpm-workspace.yaml`)); + } catch { + before = new Map(); // absent at base: treat every current entry as new + } + const after = catalogKeys(readFileSync('pnpm-workspace.yaml', 'utf8')); + const moved = [...after].filter(([k, v]) => before.get(k) !== v).map(([k]) => k); + + for (const [name, { pkg }] of published) { + const deps = { ...pkg.dependencies, ...pkg.devDependencies, ...pkg.peerDependencies }; + if (moved.some((dep) => deps[dep]?.startsWith('catalog:'))) affected.add(name); + } +} + +if (affected.size === 0) { + console.log('No published package is affected. No changeset required.'); + process.exit(0); +} + +console.error( + `No changeset found, but this PR changes dependencies of published packages:\n` + + [...affected].sort().map((n) => ` - ${n}`).join('\n') + + `\n\nWithout a changeset these packages get no version bump and no release, so the\n` + + `change never reaches npm users. Run "pnpm changeset" and commit the result.\n` + + `If the change genuinely needs no release, add an empty changeset ("pnpm changeset --empty").`, +); +process.exit(1); diff --git a/.github/workflows/require-changeset.yml b/.github/workflows/require-changeset.yml new file mode 100644 index 0000000000..fb9d99ea4a --- /dev/null +++ b/.github/workflows/require-changeset.yml @@ -0,0 +1,29 @@ +name: Require Changeset + +on: + pull_request: + types: [opened, synchronize, reopened, labeled] + +permissions: + contents: read + +jobs: + require-changeset: + runs-on: ubuntu-latest + timeout-minutes: 5 + # Escape hatch for a change that genuinely ships nothing. Dependabot PRs are NOT + # exempt: they are the ones that keep merging unreleased (tinacms/tinacms#7435). + if: "!contains(github.event.pull_request.labels.*.name, 'skip-changeset')" + steps: + - name: Check out code + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + fetch-depth: 0 + + - name: Set up Node + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version-file: .nvmrc + + - name: Require a changeset for published packages + run: node .github/scripts/check-changeset.mjs origin/${{ github.base_ref }} diff --git a/packages/tinacms/src/toolkit/components/media/media-workflow-overlay.tsx b/packages/tinacms/src/toolkit/components/media/media-workflow-overlay.tsx index 034d6aeccc..cd190a1e3a 100644 --- a/packages/tinacms/src/toolkit/components/media/media-workflow-overlay.tsx +++ b/packages/tinacms/src/toolkit/components/media/media-workflow-overlay.tsx @@ -14,6 +14,7 @@ import { ModalHeader, PopupModal, } from '@toolkit/react-modals'; +import { normalizeBranchName } from '@utils/branch-name'; import { CircleAlert } from 'lucide-react'; import * as React from 'react'; @@ -116,7 +117,7 @@ export const MediaWorkflowOverlay = () => { const confirmState = state; const branchName = confirmState.branchName; - const targetBranch = `tina/${branchName}`; + const targetBranch = `tina/${normalizeBranchName(branchName)}`; abortPreflight(); const abortController = new AbortController(); preflightAbortRef.current = abortController; @@ -190,7 +191,9 @@ export const MediaWorkflowOverlay = () => { state.onCancel(); setState({ phase: 'idle' }); }} - disabled={state.branchName === '' || state.isChecking} + disabled={ + normalizeBranchName(state.branchName) === '' || state.isChecking + } errorMessage={state.errorMessage} onBranchNameChange={(branchName) => { abortPreflight(); diff --git a/packages/tinacms/src/toolkit/core/media-store.default.ts b/packages/tinacms/src/toolkit/core/media-store.default.ts index b8271d29d8..4a92dc8f61 100644 --- a/packages/tinacms/src/toolkit/core/media-store.default.ts +++ b/packages/tinacms/src/toolkit/core/media-store.default.ts @@ -10,8 +10,8 @@ import { type MediaWorkflowConfirmBranchEvent, getEditorialWorkflowPrTitle, } from '@toolkit/form-builder/editorial-workflow-utils'; -import { formatBranchName } from '@toolkit/plugin-branch-switcher/format-branch-name'; import type { TinaCMS } from '@toolkit/tina-cms'; +import { formatBranchName } from '@utils/branch-name'; import type { Client } from '../../internalClient'; import { E_BAD_ROUTE, diff --git a/packages/tinacms/src/toolkit/form-builder/branch-deleted-modal.tsx b/packages/tinacms/src/toolkit/form-builder/branch-deleted-modal.tsx index 018c0c59ce..6a96a372c8 100644 --- a/packages/tinacms/src/toolkit/form-builder/branch-deleted-modal.tsx +++ b/packages/tinacms/src/toolkit/form-builder/branch-deleted-modal.tsx @@ -1,6 +1,6 @@ import { Form } from '@toolkit/forms'; -import { formatBranchName } from '@toolkit/plugin-branch-switcher'; import { Button } from '@toolkit/styles'; +import { formatBranchName, normalizeBranchName } from '@utils/branch-name'; import { CircleAlert, GitBranchIcon } from 'lucide-react'; import * as React from 'react'; import { useCMS } from '../react-core'; @@ -33,6 +33,7 @@ export const BranchDeletedModal = ({ const cms = useCMS(); const tinaApi = cms.api.tina; const [newBranchName, setNewBranchName] = React.useState(''); + const normalizedBranchName = normalizeBranchName(newBranchName); const baseBranch = tinaApi.protectedBranches[0] || @@ -50,7 +51,7 @@ export const BranchDeletedModal = ({ const handleCreate = async () => { const { success } = await executeWorkflow({ - branchName: `tina/${newBranchName}`, + branchName: `tina/${normalizedBranchName}`, baseBranch, path, values, @@ -130,7 +131,7 @@ export const BranchDeletedModal = ({ diff --git a/packages/tinacms/src/toolkit/plugin-branch-switcher/branch-switcher.tsx b/packages/tinacms/src/toolkit/plugin-branch-switcher/branch-switcher.tsx index b224b78da9..93a5b5b1c8 100644 --- a/packages/tinacms/src/toolkit/plugin-branch-switcher/branch-switcher.tsx +++ b/packages/tinacms/src/toolkit/plugin-branch-switcher/branch-switcher.tsx @@ -31,6 +31,7 @@ import { useBranchData } from './branch-data'; import { BranchSwitcherLegacy } from './branch-switcher-legacy'; import { Branch, BranchSwitcherProps } from './types'; +import { formatBranchName, normalizeBranchName } from '@utils/branch-name'; import BranchSelectorTable from './branch-selector-table'; type ListState = 'loading' | 'ready' | 'error'; @@ -38,8 +39,7 @@ type ListState = 'loading' | 'ready' | 'error'; export const tableHeadingStyle = 'px-3 py-3 text-left text-xs font-bold text-gray-700 tracking-wider sticky top-0 bg-gray-100 z-20 border-b-2 border-gray-200 '; -import { formatBranchName } from './format-branch-name'; -export { formatBranchName }; +export { formatBranchName } from '@utils/branch-name'; export const BranchSwitcher = (props: BranchSwitcherProps) => { const cms = useCMS(); @@ -83,7 +83,7 @@ export const EditoralBranchSwitcher = ({ const handleCreateBranch = React.useCallback((value) => { setListState('loading'); createBranch({ - branchName: formatBranchName(value), + branchName: normalizeBranchName(formatBranchName(value)), baseBranch: currentBranch, }).then(async (createdBranchName) => { cms.alerts.success('Branch created.'); @@ -292,11 +292,22 @@ export const sortBranchListFn = (sortValue: 'default' | 'updated' | 'name') => { }; }; -const BranchCreator = ({ setViewState, handleCreateBranch, currentBranch }) => { +export const BranchCreator = ({ + setViewState, + handleCreateBranch, + currentBranch, +}) => { const [branchName, setBranchName] = React.useState(''); + // Guard on the value actually sent: a slashes-only name would otherwise + // normalise away and create a bare `tina` ref, blocking every `tina/*` branch + const normalizedBranchName = normalizeBranchName( + formatBranchName(branchName) + ); return ( -
+ // Submit is handled by the button below; without this the implicit + // submission from pressing Enter in the field reloads the page + e.preventDefault()}>

Create a new branch from {currentBranch}. @@ -334,9 +345,12 @@ const BranchCreator = ({ setViewState, handleCreateBranch, currentBranch }) => { variant='primary' type='submit' style={{ flexGrow: 2 }} - disabled={branchName === ''} + disabled={normalizedBranchName === ''} onClick={() => { - handleCreateBranch('tina/' + branchName); + // Button renders `disabled` as styling only, so keyboard activation + // still reaches this handler + if (!normalizedBranchName) return; + handleCreateBranch('tina/' + normalizedBranchName); }} > Create Branch diff --git a/packages/tinacms/src/toolkit/plugin-branch-switcher/format-branch-name.ts b/packages/tinacms/src/toolkit/plugin-branch-switcher/format-branch-name.ts deleted file mode 100644 index 9944e66553..0000000000 --- a/packages/tinacms/src/toolkit/plugin-branch-switcher/format-branch-name.ts +++ /dev/null @@ -1,31 +0,0 @@ -/** - * Normalizes user-supplied strings into git-ref-safe branch segments. - * Kept in its own file (no React or workspace-internal imports) so - * non-React callers like `core/media-store.default.ts` can import it - * without dragging in the rest of `plugin-branch-switcher` and creating - * a cycle through `@toolkit/core`. - */ -export function formatBranchName(str: string): string { - let result = ''; - let replacingInvalidChars = false; - - for (const char of str.toLowerCase()) { - const code = char.charCodeAt(0); - const isValid = - char === '/' || - char === '-' || - char === '_' || - (code >= 48 && code <= 57) || - (code >= 97 && code <= 122); - - if (isValid) { - result += char; - replacingInvalidChars = false; - } else if (!replacingInvalidChars) { - result += '-'; - replacingInvalidChars = true; - } - } - - return result; -} diff --git a/packages/tinacms/src/utils/branch-name.test.ts b/packages/tinacms/src/utils/branch-name.test.ts new file mode 100644 index 0000000000..b0d482975c --- /dev/null +++ b/packages/tinacms/src/utils/branch-name.test.ts @@ -0,0 +1,101 @@ +import { describe, expect, it } from 'vitest'; +import { + formatBranchName, + formatDefaultBranchName, + normalizeBranchName, +} from './branch-name'; + +describe('formatDefaultBranchName', () => { + it('strips the content/ prefix and file extension', () => { + expect(formatDefaultBranchName('content/articles/foo.mdx', 'update')).toBe( + 'articles/foo' + ); + }); + + it('collapses the doubled slash from a trailing-slash collection path', () => { + expect(formatDefaultBranchName('content/articles//foo.mdx', 'create')).toBe( + 'articles/foo' + ); + }); + + it('keeps paths without the content/ prefix', () => { + expect(formatDefaultBranchName('pages/about.md', 'update')).toBe( + 'pages/about' + ); + }); + + it('adds the deletion indicator after normalisation', () => { + expect(formatDefaultBranchName('content/articles//foo.mdx', 'delete')).toBe( + '❌-articles/foo' + ); + }); +}); + +describe('normalizeBranchName', () => { + it('collapses repeated and leading/trailing slashes', () => { + expect(normalizeBranchName('//foo//bar//')).toBe('foo/bar'); + }); + + it('returns an empty string for slash-only input', () => { + expect(normalizeBranchName('///')).toBe(''); + }); + + it('replaces whitespace with hyphens', () => { + expect(normalizeBranchName('my branch name')).toBe('my-branch-name'); + expect(normalizeBranchName('a\tb\nc')).toBe('a-b-c'); + }); + + it('replaces Git-forbidden punctuation with hyphens', () => { + expect(normalizeBranchName('a~b^c:d?e*f[g')).toBe('a-b-c-d-e-f-g'); + expect(normalizeBranchName('a\\b')).toBe('a-b'); + }); + + it('collapses runs of forbidden characters into a single hyphen', () => { + expect(normalizeBranchName('a ~ b')).toBe('a-b'); + }); + + it('replaces the @{ sequence but keeps a plain @', () => { + expect(normalizeBranchName('a@{b')).toBe('a-b'); + expect(normalizeBranchName('v@2')).toBe('v@2'); + }); + + it('collapses dot runs so no component contains ..', () => { + expect(normalizeBranchName('a..b')).toBe('a.b'); + expect(normalizeBranchName('a...b')).toBe('a.b'); + }); + + it('strips leading dots from each component', () => { + expect(normalizeBranchName('.hidden/.foo')).toBe('hidden/foo'); + }); + + it('strips trailing dots and .lock suffixes', () => { + expect(normalizeBranchName('foo.')).toBe('foo'); + expect(normalizeBranchName('foo.lock')).toBe('foo'); + expect(normalizeBranchName('foo.lock.lock')).toBe('foo'); + expect(normalizeBranchName('foo.block')).toBe('foo.block'); + }); + + it('drops components that normalise to empty', () => { + expect(normalizeBranchName('a/./b')).toBe('a/b'); + }); + + it('leaves already-valid names untouched', () => { + expect(normalizeBranchName('articles/foo-bar_baz.v2')).toBe( + 'articles/foo-bar_baz.v2' + ); + }); +}); + +describe('formatBranchName', () => { + it('replaces invalid special characters with -', () => { + expect(formatBranchName('foo bar@@--')).toBe('foo-bar---'); + }); + + it('preserves valid special character(s)', () => { + expect(formatBranchName('my/company-branch')).toBe('my/company-branch'); + }); + + it('returns as lowerCase', () => { + expect(formatBranchName('mYbRaNcH')).toBe('mybranch'); + }); +}); diff --git a/packages/tinacms/src/utils/branch-name.ts b/packages/tinacms/src/utils/branch-name.ts new file mode 100644 index 0000000000..fa891be280 --- /dev/null +++ b/packages/tinacms/src/utils/branch-name.ts @@ -0,0 +1,86 @@ +// Format the default branch name by removing content/ prefix and file extension +export const formatDefaultBranchName = ( + filePath: string, + crudType: string +): string => { + let result = filePath; + + const contentPrefix = 'content/'; + // Remove "content/" prefix if present + if (result.startsWith(contentPrefix)) { + result = result.substring(contentPrefix.length); + } + + // Remove file extension + const lastDot = result.lastIndexOf('.'); + const lastSlash = Math.max(result.lastIndexOf('/'), result.lastIndexOf('\\')); + if (lastDot > lastSlash && lastDot > 0) { + result = result.slice(0, lastDot); + } + + result = normalizeBranchName(result); + + // Add deletion indicator for delete operations + if (crudType === 'delete') { + result = `❌-${result}`; + } + + return result; +}; + +// Loop-based trims instead of anchored regexes, which CodeQL flags on this PR +const trimRefComponent = (part: string): string => { + let result = part.replace(/\.{2,}/g, '.'); + while (result.startsWith('.')) { + result = result.slice(1); + } + let previous = ''; + while (previous !== result) { + previous = result; + if (result.endsWith('.lock')) { + result = result.slice(0, -'.lock'.length); + } else if (result.endsWith('.')) { + result = result.slice(0, -1); + } + } + return result; +}; + +// Sanitise to a valid Git ref (check-ref-format rules): forbidden characters +// become hyphens; per path component, ".." runs collapse, leading dots and +// trailing "."/".lock" are stripped; empty components (and their slashes) drop. +export const normalizeBranchName = (name: string): string => + name + .replace(/[\x00-\x20\x7f~^:?*\[\\]+/g, '-') + .replace(/@\{/g, '-') + .split('/') + .map(trimRefComponent) + .filter(Boolean) + .join('/'); + +// Live-input slugifier: invalid-char runs collapse to one hyphen, lowercased; slashes +// pass through so nested names stay typable. Loop, not regex, to stay off CodeQL's radar. +export function formatBranchName(str: string): string { + let result = ''; + let replacingInvalidChars = false; + + for (const char of str.toLowerCase()) { + const code = char.charCodeAt(0); + const isValid = + char === '/' || + char === '-' || + char === '_' || + (code >= 48 && code <= 57) || + (code >= 97 && code <= 122); + + if (isValid) { + result += char; + replacingInvalidChars = false; + } else if (!replacingInvalidChars) { + result += '-'; + replacingInvalidChars = true; + } + } + + return result; +}