diff --git a/api/controllers/site-branch-config.js b/api/controllers/site-branch-config.js
index 33bac163d..8ce26c7bc 100644
--- a/api/controllers/site-branch-config.js
+++ b/api/controllers/site-branch-config.js
@@ -14,7 +14,7 @@ function generateS3Key(site, context, branch) {
return null;
}
- return `preview/${site.owner}/${site.repository}/${branch}`;
+ return `preview/${site.owner}/${site.repository}/${encodeURIComponent(branch)}`;
}
function validate({ branch, config = {}, context } = {}) {
diff --git a/api/models/site-build-task.js b/api/models/site-build-task.js
index 1a9b7bf6c..c7b482f66 100644
--- a/api/models/site-build-task.js
+++ b/api/models/site-build-task.js
@@ -1,3 +1,4 @@
+const { isEmptyOrBranch } = require('../utils/validators');
const associate = ({ BuildTask, BuildTaskType, Site, SiteBuildTask }) => {
SiteBuildTask.belongsTo(BuildTaskType, {
foreignKey: 'buildTaskTypeId',
@@ -28,6 +29,9 @@ module.exports = (sequelize, DataTypes) => {
{
branch: {
type: DataTypes.STRING,
+ validate: {
+ isEmptyOrBranch,
+ },
allowNull: true,
},
metadata: {
diff --git a/api/queue-jobs/index.js b/api/queue-jobs/index.js
index 60590e935..d7dcc9792 100644
--- a/api/queue-jobs/index.js
+++ b/api/queue-jobs/index.js
@@ -199,7 +199,8 @@ class QueueJobs {
async startSiteBuild(build, priority) {
const { branch, id: buildId, Site } = build;
const { owner, repository } = Site;
- const jobName = `${owner}/${repository}: ${truncateString(branch)}`;
+ const encodedBranch = `${truncateString(encodeURIComponent(branch))}`;
+ const jobName = `${owner}/${repository}: ${encodedBranch}`;
await this.siteBuildsQueue.waitUntilReady();
diff --git a/api/services/GithubBuildHelper.js b/api/services/GithubBuildHelper.js
index 8bd212bdd..6157bee34 100644
--- a/api/services/GithubBuildHelper.js
+++ b/api/services/GithubBuildHelper.js
@@ -105,13 +105,11 @@ const reportBuildStatus = async (build) => {
config.app.hostname,
`/sites/${site.id}/builds/${build.id}/logs`,
);
- // eslint-disable-next-line max-len
options.description =
'The build is running. Click "Details" to see the Pages build status.';
} else if (build.state === 'success') {
options.state = 'success';
options.target_url = build.url;
- // eslint-disable-next-line max-len
options.description =
'The build is complete! Click "Details" to visit the Site Preview.';
} else if (build.state === 'error') {
@@ -120,7 +118,6 @@ const reportBuildStatus = async (build) => {
config.app.hostname,
`/sites/${site.id}/builds/${build.id}/logs`,
);
- // eslint-disable-next-line max-len
options.description =
'The build has encountered an error. Click "Details" to see the Pages build logs.';
}
diff --git a/api/services/S3PublishedFileLister.js b/api/services/S3PublishedFileLister.js
index 84a40bc42..50f663596 100644
--- a/api/services/S3PublishedFileLister.js
+++ b/api/services/S3PublishedFileLister.js
@@ -90,7 +90,7 @@ function listPagedPublishedFilesForBranch(site, branch, startAtKey) {
} else if (site.demoBranch === branch) {
filepath = `demo/${site.owner}/${site.repository}`;
} else {
- filepath = `preview/${site.owner}/${site.repository}/${branch}`;
+ filepath = `preview/${site.owner}/${site.repository}/${encodeURIComponent(branch)}`;
}
return apiClient
diff --git a/api/utils/build.js b/api/utils/build.js
index dd60c43b0..1bcdd3f67 100644
--- a/api/utils/build.js
+++ b/api/utils/build.js
@@ -1,7 +1,7 @@
const config = require('../../config');
function previewPath(build, site) {
- return `/preview/${site.owner}/${site.repository}/${build.branch}`;
+ return `/preview/${site.owner}/${site.repository}/${encodeURIComponent(build.branch)}`;
}
function proxyUrl(path, site) {
diff --git a/api/utils/validators.js b/api/utils/validators.js
index 5973b8769..79cfc3e02 100644
--- a/api/utils/validators.js
+++ b/api/utils/validators.js
@@ -1,8 +1,6 @@
const yaml = require('js-yaml');
const validator = require('validator');
-// eslint-disable-next-line sonarjs/slow-regex
-const branchRegex = /^[\w.]+(?:[/-]*[\w.])*$/;
const githubUsernameRegex = /^[^-][a-zA-Z-]+$/;
const shaRegex = /^[a-f0-9]{40}$/;
const subdomainRegex = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?$/;
@@ -101,7 +99,7 @@ function isEmptyOrBranch(value) {
throw new Error('Invalid branch name — branch names are limitted to 299 characters.');
}
- if (value && value.length && !branchRegex.test(value)) {
+ if (value && value.length && !isValidBranchName(value)) {
throw new Error(
// eslint-disable-next-line max-len
'Invalid branch name — branches can only contain alphanumeric characters, underscores, and hyphens.',
@@ -109,6 +107,24 @@ function isEmptyOrBranch(value) {
}
}
+const isValidBranchName = (branchName) => {
+ if (branchName === '@') return false;
+
+ // Check control characters
+ for (const char of branchName) {
+ const code = char.charCodeAt(0);
+ if ((code >= 0 && code <= 31) || code === 127) return false;
+ }
+
+ // Combined regex for all other forbidden patterns
+ if (/^\/|[ ~^:?*[\\]|\.\..|@\{|\/\/|\.$|\/$/.test(branchName)) {
+ return false;
+ }
+
+ // Check components
+ return !branchName.split('/').some((c) => c.startsWith('.') || c.endsWith('.lock'));
+};
+
function isEmptyOrUrl(value) {
const validUrlOptions = {
require_protocol: true,
@@ -136,7 +152,6 @@ const isDelimitedFQDN = (str) => {
};
module.exports = {
- branchRegex,
CustomError,
shaRegex,
githubUsernameRegex,
@@ -149,4 +164,5 @@ module.exports = {
ValidationError,
isValidSubdomain,
isDelimitedFQDN,
+ isValidBranchName,
};
diff --git a/frontend/pages/sites/$siteId/published/$name/BranchFileRow.jsx b/frontend/pages/sites/$siteId/published/$name/BranchFileRow.jsx
index 76594f202..a3d14e04b 100644
--- a/frontend/pages/sites/$siteId/published/$name/BranchFileRow.jsx
+++ b/frontend/pages/sites/$siteId/published/$name/BranchFileRow.jsx
@@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
export default function BranchFileRow({ file }) {
let viewFileLink;
const branch = file.publishedBranch.name;
+ const encBranch = encodeURIComponent(branch);
switch (branch) {
case file.publishedBranch.site.defaultBranch:
viewFileLink = `${file.publishedBranch.site.viewLink}${file.name}`;
@@ -12,7 +13,7 @@ export default function BranchFileRow({ file }) {
viewFileLink = `${file.publishedBranch.site.demoViewLink}${file.name}`;
break;
default:
- viewFileLink = `${file.publishedBranch.site.previewLink}${branch}/${file.name}`;
+ viewFileLink = `${file.publishedBranch.site.previewLink}${encBranch}/${file.name}`;
}
return (
diff --git a/frontend/pages/sites/$siteId/published/BranchRow.jsx b/frontend/pages/sites/$siteId/published/BranchRow.jsx
index 7004ea8db..0f21de9cb 100644
--- a/frontend/pages/sites/$siteId/published/BranchRow.jsx
+++ b/frontend/pages/sites/$siteId/published/BranchRow.jsx
@@ -7,7 +7,7 @@ import BranchViewLink from './BranchViewLink';
import { SITE } from '@propTypes';
function BranchFilesLink({ branch }) {
- const href = `/sites/${branch.site.id}/published/${branch.name}`;
+ const href = `/sites/${branch.site.id}/published/${encodeURIComponent(branch.name)}`;
return View files;
}
diff --git a/frontend/pages/sites/$siteId/published/BranchViewLink.jsx b/frontend/pages/sites/$siteId/published/BranchViewLink.jsx
index 197d68dbb..a2826a7e4 100644
--- a/frontend/pages/sites/$siteId/published/BranchViewLink.jsx
+++ b/frontend/pages/sites/$siteId/published/BranchViewLink.jsx
@@ -13,7 +13,7 @@ const getViewLink = (branch, site) => {
// Checks to see if there is a site branch config
// If not it returns the preview url
if (!branchConfig) {
- return `${origin}/preview/${owner}/${repository}/${branch}`;
+ return `${origin}/preview/${owner}/${repository}/${encodeURIComponent(branch)}`;
}
const domain = domains.find((d) => d.siteBranchConfigId === branchConfig.id);
diff --git a/frontend/pages/sites/$siteId/published/BranchViewLink.test.jsx b/frontend/pages/sites/$siteId/published/BranchViewLink.test.jsx
index 30dbf0e22..af9dccf12 100644
--- a/frontend/pages/sites/$siteId/published/BranchViewLink.test.jsx
+++ b/frontend/pages/sites/$siteId/published/BranchViewLink.test.jsx
@@ -94,13 +94,14 @@ describe('', () => {
});
it('renders a preview link to the other branches', () => {
- const branchName = 'some-other-branch';
+ const branchName = 'some-other-branch-with-special_characters-###';
+ const branchEncoded = 'some-other-branch-with-special_characters-%23%23%23';
const updatedProps = { ...props, branchName };
render();
const anchor = screen.getByRole('link');
expect(anchor).toHaveAttribute(
'href',
- `${proxyOrigin}/preview/${testSite.owner}/${testSite.repository}/${branchName}`,
+ `${proxyOrigin}/preview/${testSite.owner}/${testSite.repository}/${branchEncoded}`,
);
expect(anchor).toHaveTextContent(VIEW_BUILD);
});
diff --git a/frontend/util/federalistApi.js b/frontend/util/federalistApi.js
index 4352831a6..a8ef85a41 100644
--- a/frontend/util/federalistApi.js
+++ b/frontend/util/federalistApi.js
@@ -104,7 +104,7 @@ export default {
},
fetchPublishedFiles(id, branch, startAtKey = null) {
- let path = `site/${id}/published-branch/${branch}/published-file`;
+ let path = `site/${id}/published-branch/${encodeURIComponent(branch)}/published-file`;
if (startAtKey) {
path += `?startAtKey=${startAtKey}`;
}
diff --git a/test/api/unit/utils/validators.test.js b/test/api/unit/utils/validators.test.js
index 5f6d9bb7e..6d98d1e9e 100644
--- a/test/api/unit/utils/validators.test.js
+++ b/test/api/unit/utils/validators.test.js
@@ -80,4 +80,139 @@ describe('validators', () => {
);
});
});
+
+ describe('.isValidBranchName', () => {
+ // https://git-scm.com/docs/git-check-ref-format
+ // Git imposes the following rules on how references are named:
+ //
+ // 1. They can include slash / for hierarchical (directory) grouping,
+ // but no slash-separated component can begin with a dot .
+ // or end with the sequence .lock.
+ //
+ // 2. They must contain at least one /. This enforces the presence of a category
+ // like heads/, tags/ etc. but the actual names are not restricted.
+ // If the --allow-onelevel option is used, this rule is waived.
+ //
+ // 3. They cannot have two consecutive dots .. anywhere.
+ //
+ // 4. They cannot have ASCII control characters
+ // (i.e. bytes whose values are lower than \040, or \177 DEL),
+ // space, tilde ~, caret ^, or colon : anywhere.
+ //
+ // 5. They cannot have question-mark ?, asterisk *, or open bracket [ anywhere.
+ // (--refspec-pattern option is an exception to this rule).
+ //
+ // 6. They cannot begin or end with a slash / or contain multiple consecutive slashes
+ // (--normalize option is an exception to this rule).
+ //
+ // 7. They cannot end with a dot ..
+ //
+ // 8. They cannot contain a sequence @{.
+ //
+ // 9. They cannot be the single character @.
+ //
+ // 10. They cannot contain a \.
+
+ const specialChars = [
+ // Control characters (0x00-0x1F) - ALL forbidden
+ { specialChar: '\x00', allowed: false }, // NUL
+ { specialChar: '\x01', allowed: false }, // SOH
+ { specialChar: '\x02', allowed: false }, // STX
+ { specialChar: '\x03', allowed: false }, // ETX
+ { specialChar: '\x04', allowed: false }, // EOT
+ { specialChar: '\x05', allowed: false }, // ENQ
+ { specialChar: '\x06', allowed: false }, // ACK
+ { specialChar: '\x07', allowed: false }, // BEL
+ { specialChar: '\x08', allowed: false }, // BS
+ { specialChar: '\x09', allowed: false }, // TAB
+ { specialChar: '\x0A', allowed: false }, // LF
+ { specialChar: '\x0B', allowed: false }, // VT
+ { specialChar: '\x0C', allowed: false }, // FF
+ { specialChar: '\x0D', allowed: false }, // CR
+ { specialChar: '\x0E', allowed: false }, // SO
+ { specialChar: '\x0F', allowed: false }, // SI
+ { specialChar: '\x10', allowed: false }, // DLE
+ { specialChar: '\x11', allowed: false }, // DC1
+ { specialChar: '\x12', allowed: false }, // DC2
+ { specialChar: '\x13', allowed: false }, // DC3
+ { specialChar: '\x14', allowed: false }, // DC4
+ { specialChar: '\x15', allowed: false }, // NAK
+ { specialChar: '\x16', allowed: false }, // SYN
+ { specialChar: '\x17', allowed: false }, // ETB
+ { specialChar: '\x18', allowed: false }, // CAN
+ { specialChar: '\x19', allowed: false }, // EM
+ { specialChar: '\x1A', allowed: false }, // SUB
+ { specialChar: '\x1B', allowed: false }, // ESC
+ { specialChar: '\x1C', allowed: false }, // FS
+ { specialChar: '\x1D', allowed: false }, // GS
+ { specialChar: '\x1E', allowed: false }, // RS
+ { specialChar: '\x1F', allowed: false }, // US
+
+ // Printable special characters
+ { specialChar: ' ', allowed: false }, // Space - forbidden
+ { specialChar: '!', allowed: true },
+ { specialChar: '"', allowed: true },
+ { specialChar: '#', allowed: true },
+ { specialChar: '$', allowed: true },
+ { specialChar: '%', allowed: true },
+ { specialChar: '&', allowed: true },
+ { specialChar: "'", allowed: true },
+ { specialChar: '(', allowed: true },
+ { specialChar: ')', allowed: true },
+ { specialChar: '*', allowed: false }, // Asterisk - forbidden
+ { specialChar: '+', allowed: true },
+ { specialChar: ',', allowed: true },
+ { specialChar: '-', allowed: true },
+ { specialChar: '.', allowed: true }, // Allowed but with restrictions
+ { specialChar: '/', allowed: true }, // Allowed but with restrictions
+ { specialChar: ':', allowed: false }, // Colon - forbidden
+ { specialChar: ';', allowed: true },
+ { specialChar: '<', allowed: true },
+ { specialChar: '=', allowed: true },
+ { specialChar: '>', allowed: true },
+ { specialChar: '?', allowed: false }, // Question mark - forbidden
+ { specialChar: '@', allowed: true }, // Allowed but '@{' sequence forbidden
+ { specialChar: '[', allowed: false }, // Open bracket - forbidden
+ { specialChar: '\\', allowed: false }, // Backslash - forbidden
+ { specialChar: ']', allowed: true },
+ { specialChar: '^', allowed: false }, // Caret - forbidden
+ { specialChar: '_', allowed: true },
+ { specialChar: '`', allowed: true },
+ { specialChar: '{', allowed: true }, // Allowed but '@{' sequence forbidden
+ { specialChar: '|', allowed: true },
+ { specialChar: '}', allowed: true },
+ { specialChar: '~', allowed: false }, // Tilde - forbidden
+
+ // DEL control character
+ { specialChar: '\x7F', allowed: false }, // DEL
+ ];
+
+ it("validates Git reference names according to Git's rules", () => {
+ expect(validators.isValidBranchName('feature/my-branch')).to.equal(true);
+ expect(validators.isValidBranchName('hotfix/bug-123')).to.equal(true);
+ expect(validators.isValidBranchName('/invalid')).to.equal(false);
+ expect(validators.isValidBranchName('bad..name')).to.equal(false);
+ expect(validators.isValidBranchName('branch with spaces')).to.equal(false);
+ expect(validators.isValidBranchName('main')).to.equal(true);
+ expect(validators.isValidBranchName('refs/tags/v1.0.0')).to.equal(true);
+ expect(validators.isValidBranchName('main')).to.equal(true);
+ expect(validators.isValidBranchName('.hidden')).to.equal(false);
+ expect(validators.isValidBranchName('branch.lock')).to.equal(false);
+ expect(validators.isValidBranchName('bad..name')).to.equal(false);
+ expect(validators.isValidBranchName('bad@{name}')).to.equal(false);
+ expect(validators.isValidBranchName('bad\\name')).to.equal(false);
+ expect(validators.isValidBranchName('/main')).to.equal(false);
+ expect(validators.isValidBranchName('main/')).to.equal(false);
+ expect(validators.isValidBranchName('ma//in')).to.equal(false);
+ expect(validators.isValidBranchName('main.')).to.equal(false);
+ expect(validators.isValidBranchName('@{')).to.equal(false);
+
+ specialChars.forEach((i) => {
+ expect(
+ validators.isValidBranchName(`branch${i.specialChar}branch`),
+ i.specialChar,
+ ).to.equal(i.allowed);
+ });
+ });
+ });
});