Integrated branch protection rules by Ramjat19 · Pull Request #1 · Ramjat19/collab-code-review

Ramjat19 · 2025-10-30T10:57:48Z

customized branch protection rules are integrated with properly handling merge and all actions for the branch

vercel · 2025-10-30T10:57:52Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
collab-code-review	Ready	Preview	Comment	Oct 30, 2025 11:45am

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Ramjat19

this can be improved in further versions

Copilot

Pull Request Overview

This PR implements a comprehensive branch protection rules feature for the collaborative code review platform. The feature enforces code quality through review requirements, CI checks, and conversation resolution before allowing pull request merges.

Key changes:

Added branch protection middleware and API endpoints for validation and enforcement
Created UI components for displaying protection status and managing merge operations
Implemented a settings page for configuring branch protection rules per project

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
frontend/frontend/src/pages/SettingsPage.tsx	New settings page component for accessing branch protection configuration
frontend/frontend/src/pages/PullRequestDetail.tsx	Integrated branch protection status display and enhanced merge button
frontend/frontend/src/components/EnhancedMergeButton.tsx	Smart merge button with protection awareness and force merge capability
frontend/frontend/src/components/BranchProtectionStatus.tsx	Real-time display of branch protection requirements and violations
frontend/frontend/src/components/BranchProtectionSettings.tsx	Configuration interface for branch protection rules
frontend/frontend/src/api/index.ts	Added API methods for branch protection endpoints
frontend/frontend/src/App.tsx	Added route for settings page
backend/src/routes/branchProtection.ts	API endpoints for branch protection operations
backend/src/models/BranchProtectionRule.ts	Database model for storing branch protection configurations
backend/src/middleware/branchProtection.ts	Middleware for validating and enforcing protection rules
backend/src/routes/pullRequest.ts	Auto-update PR status based on review decisions
backend/src/app.ts	Registered branch protection routes
docs/branch-protection.md	Comprehensive documentation for branch protection feature
docs/README.md	Updated main documentation with branch protection overview

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-30T11:05:40Z

+            <label className="flex items-center space-x-3">
+              <input
+                type="checkbox"
+                checked={config.rules.requireReviews}
+                onChange={(e) => updateRule('requireReviews', e.target.checked)}
+                className="rounded border-gray-300"
+              />
+              <span>Require review from code owners</span>
+            </label>


Duplicate checkbox options with identical labels 'Require review from code owners' at lines 255-263 and 287-295. The first checkbox controls requireReviews while the second controls requireCodeOwnerReviews. Either remove the duplication or update the labels to clearly distinguish between the two settings (e.g., 'Require reviews' vs 'Require code owner reviews specifically').

Copilot · 2025-10-30T11:05:41Z

+            <label className="flex items-center space-x-3">
+              <input
+                type="checkbox"
+                checked={config.rules.requireCodeOwnerReviews}
+                onChange={(e) => updateRule('requireCodeOwnerReviews', e.target.checked)}
+                className="rounded border-gray-300"
+              />
+              <span>Require review from code owners</span>
+            </label>


Duplicate checkbox options with identical labels 'Require review from code owners' at lines 255-263 and 287-295. The first checkbox controls requireReviews while the second controls requireCodeOwnerReviews. Either remove the duplication or update the labels to clearly distinguish between the two settings (e.g., 'Require reviews' vs 'Require code owner reviews specifically').

Copilot · 2025-10-30T11:05:41Z

+  // Mock implementation - replace with actual Git API calls
+  return {
+    satisfied: Math.random() > 0.3, // 70% chance of being up to date
+    behindBy: Math.random() > 0.3 ? 0 : Math.floor(Math.random() * 5) + 1


Non-deterministic mock implementation uses Math.random() which will produce inconsistent results across multiple calls for the same PR. This could cause the protection status to change randomly between page refreshes, confusing users. For development/demo purposes, use a deterministic mock (e.g., always return satisfied: true) or base the result on the PR ID for consistency.

Suggested change

// Mock implementation - replace with actual Git API calls

return {

satisfied: Math.random() > 0.3, // 70% chance of being up to date

behindBy: Math.random() > 0.3 ? 0 : Math.floor(Math.random() * 5) + 1

// Mock implementation - always return up-to-date for demo purposes

return {

satisfied: true,

behindBy: 0

Copilot · 2025-10-30T11:05:41Z

+    }
+
+    // Admin bypass - log for audit
+    console.log(`Admin bypass: ${user?.username} bypassed branch protection for PR ${req.params.pullRequestId}`);


References req.params.pullRequestId but the route parameter is named id (as seen in line 109 of branchProtection.ts route). This will log undefined. Should be req.params.id.

Suggested change

console.log(`Admin bypass: ${user?.username} bypassed branch protection for PR ${req.params.pullRequestId}`);

console.log(`Admin bypass: ${user?.username} bypassed branch protection for PR ${req.params.id}`);

Copilot · 2025-10-30T11:05:42Z

+              onMergeSuccess={() => {
+                fetchPullRequest();
+                // Show success message
+                alert('Pull request merged successfully!');


Using browser alert() for success messages provides poor UX. Consider using a toast notification or inline success message component consistent with the rest of the application's notification patterns.

Copilot · 2025-10-30T11:05:42Z

+    // If there are any rejections, keep as 'open'
+    if (rejectedReviews.length > 0) {
+      pullRequest.status = 'open';
+    } 
+    // If we have at least 1 approval (you can adjust this number based on your requirements)
+    else if (approvedReviews.length >= 1) {


Hard-coded approval threshold of 1 reviewer with a comment suggesting it can be adjusted. This should reference the branch protection configuration's requiredReviewers setting rather than using a hard-coded value, to maintain consistency with the configurable branch protection rules.

Suggested change

// If there are any rejections, keep as 'open'

if (rejectedReviews.length > 0) {

pullRequest.status = 'open';

}

// If we have at least 1 approval (you can adjust this number based on your requirements)

else if (approvedReviews.length >= 1) {

// Fetch branch protection configuration for the target branch

let requiredReviewers = 1; // Default fallback

if (pullRequest.repository && pullRequest.targetBranch) {

// Assuming repository is populated and has a branchProtection field

const repo = await Project.findById(pullRequest.repository);

if (repo && repo.branchProtection && repo.branchProtection[pullRequest.targetBranch]) {

requiredReviewers = repo.branchProtection[pullRequest.targetBranch].requiredReviewers || 1;

}

}

// If there are any rejections, keep as 'open'

if (rejectedReviews.length > 0) {

pullRequest.status = 'open';

}

// If we have at least the required number of approvals

else if (approvedReviews.length >= requiredReviewers) {

Copilot · 2025-10-30T11:05:42Z

+  const handleRequestReviews = async () => {
+    // This would open a modal to select additional reviewers
+    // For now, just refresh the status
+    await fetchProtectionStatus();
+  };


The 'Request Reviews' button (line 199-203) calls this function but it only refreshes status without actually requesting reviews. Either implement the functionality or remove the button to avoid misleading users.

Copilot · 2025-10-30T11:05:43Z

+            className="ml-2 p-1 text-gray-400 hover:text-gray-600 transition-colors"
+            title="Refresh protection status"
+          >
+            <RefreshCw className={`w-4 h-4 ${loading ? 'animate-spin' : ''}`} />


This use of variable 'loading' always evaluates to false.

… and other frontend and bakend bugs

+    const branchProtectionStatus = (req as any).branchProtectionStatus;
+    const userId = (req as any).user?.id || 'unknown';
+    const safeId = String(id).replace(/[^\w-]/g, ''); // Sanitize ID
+    console.log(`PR ${safeId} merged by user ${userId}`, {


To fix this issue, do not interpolate user-derived or externally-controlled values directly into the format string argument to console.log. Instead, use a fixed format string with a %s substitution, and provide the user-influenced value as an argument. Change the logging line so that it reads:

console.log("PR %s merged by user %s", safeId, userId, { ... });

This ensures the values are treated as data, never as part of the format string.
This change should be made on line 103 in backend/src/routes/branchProtection.ts. No new imports or methods are needed.

+    const userId = (req as any).user?.id || 'unknown';
+    const safeId = String(id).replace(/[^\w-]/g, ''); // Sanitize ID
+    const safeReason = String(reason || '').replace(/[^\w\s.-]/g, ''); // Sanitize reason
+    console.log(`FORCE MERGE: PR ${safeId} force-merged by user ${userId}`, {


The safest fix is to avoid passing any user-controlled value directly within the format string (the first argument to console.log). Instead, the log message should use a format specifier (%s) in the format string, and the sanitized user-controlled value should be passed as a subsequent argument. This avoids any possibility that a future change in sanitization or logging behavior could allow formatting characters to be interpreted from user input.

How:
On line 146 of backend/src/routes/branchProtection.ts, change:

console.log(`FORCE MERGE: PR ${safeId} force-merged by user ${userId}`, { ... });

to:

console.log('FORCE MERGE: PR %s force-merged by user %s', safeId, userId, { ... });

This change keeps the message and data structure as before, but ensures the externally-controlled data cannot affect the format string.

Required:

Only the code block containing line 146 in backend/src/routes/branchProtection.ts needs to be updated. No imports or other code changes are required.

+ * POST /api/branch-protection/request-review/:id
+ * Request additional reviews for a pull request
+ */
+router.post('/request-review/:id', authMiddleware, async (req, res) => {


To fix the missing rate limiting on the POST /request-review/:id route, the best approach is to attach a suitable rate limiting middleware to the route, just like other protected endpoints in this file. The fix should:

Use the existing createLimiter utility to define a limiter for this route.

Decide on appropriate rate limiting configuration (e.g., similar to the mergeLimiter on other post operations, or a custom one if suitable).

Define the limiter near other limiters for clarity.

Add the limiter as middleware for the route at line 198.

To implement this fix, the following changes are required in backend/src/routes/branchProtection.ts:

Define a new reviewRequestLimiter after the other limiters.

Add the new limiter in the middleware chain of the router.post('/request-review/:id', ...) declaration.

+router.post('/request-review/:id', authMiddleware, async (req, res) => {
+  try {
+    const { id } = req.params;
+    const { reviewerIds } = req.body;
+
+    const pr = await PullRequestModel.findById(id);
+
+    if (!pr) {
+      return res.status(404).json({ error: 'Pull request not found' });
+    }
+
+    // Add reviewers to the PR
+    const newReviewers = reviewerIds.filter((reviewerId: string) => 
+      !pr.assignedReviewers.some(reviewer => reviewer.toString() === reviewerId)
+    );
+
+    pr.assignedReviewers.push(...newReviewers);
+    await pr.save();
+
+    // In a real implementation, you would send notifications to reviewers
+    console.log(`Additional reviewers requested for PR ${id}:`, newReviewers);
+
+    res.json({
+      message: 'Review requests sent successfully',
+      addedReviewers: newReviewers.length,
+      totalReviewers: pr.assignedReviewers.length
+    });
+
+  } catch (error) {
+    console.error('Request review error:', error);
+    res.status(500).json({ error: 'Failed to request reviews' });
+  }
+});


How to, in general terms, fix the problem
All route handlers that access the database or perform expensive operations should be protected by a rate-limiting middleware to mitigate the risk of DoS attacks.

Best way to fix the problem without changing existing functionality

Identify the appropriate rate limiter middleware for this route, matching project conventions (e.g., mergeLimiter, readLimiter). If an appropriate limiter (such as writeLimiter or another specific one) is already defined (though not shown in the snippet), use it.

If not, define a new reviewRequestLimiter (e.g., allowing 10 requests per minute per IP) near the top of the file, matching the pattern for existing limiters.

Insert the rate limiter as the first middleware for the /request-review/:id route, so that the route definition becomes router.post('/request-review/:id', reviewRequestLimiter, authMiddleware, ...).

Files/regions/lines to change

In backend/src/routes/branchProtection.ts:

Add a reviewRequestLimiter definition alongside other limiter definitions near the top (after line 23 or wherever the other limiters are defined).

Update line 198 to insert reviewRequestLimiter as the first middleware.

What is needed

Import is already present for rateLimit (import rateLimit from 'express-rate-limit').

Middleware definition for the new limiter.

No new dependencies required.

+    await pr.save();
+
+    // In a real implementation, you would send notifications to reviewers
+    console.log(`Additional reviewers requested for PR ${id}:`, newReviewers);


To fix this issue, we should never allow externally-controlled or interpolated values to be used as the format string (i.e. the first argument) in console.log or similar logging functions if additional, possibly attacker-controlled parameters are passed as subsequent arguments. Instead, use a fixed string with format specifiers like %s, and supply the untrusted value (id) as a parameter. Therefore, the log line on 218 should be modified from:

console.log(`Additional reviewers requested for PR ${id}:`, newReviewers);

to:

console.log('Additional reviewers requested for PR %s:', id, newReviewers);

No extra imports or helpers are needed.

+router.post('/rules', async (req, res) => {
+  try {
+    const { projectId, rules, branchPattern, createdBy } = req.body;
+
+    if (!projectId || !rules) {
+      return res.status(400).json({ error: 'Project ID and rules are required' });
+    }
+
+    // Check if rules already exist for this project
+    const existingRule = await BranchProtectionRule.findOne({ 
+      projectId, 
+      branchPattern: branchPattern || 'main',
+      isActive: true 
+    });
+
+    if (existingRule) {
+      return res.status(409).json({ 
+        error: 'Branch protection rule already exists for this project and branch pattern' 
+      });
+    }
+
+    // Create new rule
+    const newRule = new BranchProtectionRule({
+      projectId,
+      branchPattern: branchPattern || 'main',
+      rules,
+      createdBy
+    });
+
+    await newRule.save();
+    console.log('New branch protection rule created:', newRule._id);
+
+    res.status(201).json({ 
+      success: true, 
+      data: newRule, 
+      message: 'Rule created successfully' 
+    });
+  } catch (error) {
+    console.error('Create rule error:', error);
+    res.status(500).json({ error: 'Failed to create branch protection rule' });
+  }
+});


To fix the missing rate limiting issue, we should apply suitable rate limiting middleware directly to the POST /api/branch-protection/rules route handler (line 342). In keeping with the existing code, which defines writeLimiter for operations that change data, we should use the writeLimiter middleware for this route as well.

This requires editing the route definition to include writeLimiter as a middleware argument before the handler:

Change: router.post('/rules', async (req, res) => {...

To: router.post('/rules', writeLimiter, async (req, res) => {...

No new imports/methods/definitions are required—the necessary rate limiting configuration is already present in the file and used in other endpoints.

+    const existingRule = await BranchProtectionRule.findOne({ 
+      projectId, 
+      branchPattern: branchPattern || 'main',
+      isActive: true 
+    });


To fix this issue, we need to ensure that the user-controlled value projectId is treated as a literal in the MongoDB query. This can be safely accomplished by wrapping it in the $eq operator within the query filter. Optionally, we should also validate its type (e.g., ensure it’s a string or a valid ObjectId format, or accept only certain allowed values like "global" or "default").

Specifically:

Edit the query object in the POST /rules route so that instead of { projectId, ... }, it becomes { projectId: { $eq: projectId }, ... }.

Optionally, add format/type validation for projectId matching what is done in the PUT route, to ensure it’s a string and matches allowed values (ObjectId, "global", or "default").

No new dependencies are required for the $eq operator, and the code can be updated in place.

+router.delete('/rules/:id', async (req, res) => {
+  try {
+    const { id } = req.params;
+
+    // Soft delete by setting isActive to false
+    const rule = await BranchProtectionRule.findByIdAndUpdate(
+      id,
+      { 
+        isActive: false,
+        updatedAt: new Date()
+      },
+      { new: true }
+    );
+
+    if (!rule) {
+      return res.status(404).json({ error: 'Branch protection rule not found' });
+    }
+
+    console.log('Branch protection rule deleted:', id);
+    res.json({ success: true, message: 'Rule deleted successfully' });
+  } catch (error) {
+    console.error('Delete rule error:', error);
+    res.status(500).json({ error: 'Failed to delete branch protection rule' });
+  }
+});


To fix the missing rate limiting on the DELETE /api/branch-protection/rules/:id endpoint, we should wrap this route with a suitable rate limiting middleware. For consistency and least privilege, we should use the same or similar limiter as for other write operations (e.g., writeLimiter, if that is used for similar operations like PUT /rules). This will protect the database from potential abuse via repeated delete requests.

Detailed steps:

Identify which rate limiter middleware is being used for similar "write" operations. In this case, writeLimiter is used for the PUT /rules endpoint.

Apply the writeLimiter middleware to the DELETE /rules/:id route.

No new rate limiter setup is needed; just add it to the middleware chain.

Only the route definition on line 386 needs to be updated to include writeLimiter as middleware.

To fix this problem, we need to protect the /pullRequest/:id/review POST endpoint by applying rate limiting middleware. The optimal approach in an Express-based app is to use a well-known library such as express-rate-limit. We can create a limiter instance (with reasonable settings, such as 10 requests per 15 minutes per user/IP), and apply it as middleware specifically to this route. This ensures review submissions can't be spammed to exhaust database or server resources.

Steps:

Import express-rate-limit in backend/src/routes/pullRequest.ts.

Define a rate limiter for review submission (e.g., 10 per 15 minutes).

Add this limiter as middleware to the .post("/:id/review", ...) route (between authMiddleware and the async handler).

No behavioral changes will occur besides limiting the rate of review submissions to a reasonable value.

Integrated branch protection rules

6cdf24e

github-advanced-security AI found potential problems Oct 30, 2025

View reviewed changes

Ramjat19 requested a review from Copilot October 30, 2025 11:02

Ramjat19 commented Oct 30, 2025

View reviewed changes

Copilot AI reviewed Oct 30, 2025

View reviewed changes

fixed rate limiting, Input Validation & Sanitization, Authentication,…

04f03b5

… and other frontend and bakend bugs

vercel Bot deployed to Preview October 30, 2025 11:45 View deployment

github-advanced-security AI found potential problems Oct 30, 2025

View reviewed changes

Ramjat19 merged commit b33cf44 into main Oct 30, 2025
16 of 17 checks passed

@@ -1,10 +1,16 @@
             import express, { Response } from 'express';
+            import rateLimit from 'express-rate-limit';
             import authMiddleware, { AuthRequest } from '../middleware/auth';
             import PullRequest from '../models/PullRequest';
             import Project from '../models/Project';
             import { NotificationService } from '../services/NotificationService';
-            const router = express.Router();
+            // Rate limiter for reviewing PRs: max 10 review submissions per 15 minutes per IP
+            const reviewRateLimiter = rateLimit({
+              windowMs: 15 * 60 * 1000, // 15 minutes
+              max: 10, // limit each IP to 10 review requests per window
+              message: { message: "Too many review submissions from this IP, please try again later." }
+            });
             // Create a new pull request
             router.post('/', authMiddleware, async (req: AuthRequest, res: Response): Promise<void> => {
@@ -276,7 +277,7 @@
             });
             // Submit review decision
-            router.post("/:id/review", authMiddleware, async (req: AuthRequest, res: Response): Promise<void> => {
+            router.post("/:id/review", authMiddleware, reviewRateLimiter, async (req: AuthRequest, res: Response): Promise<void> => {
               try {
                 const { decision, comment } = req.body;
                 const pullRequest = await PullRequest.findById(req.params.id);

	console.log(`Admin bypass: ${user?.username} bypassed branch protection for PR ${req.params.pullRequestId}`);
	console.log(`Admin bypass: ${user?.username} bypassed branch protection for PR ${req.params.id}`);

-    // If there are any rejections, keep as 'open'
-    if (rejectedReviews.length > 0) {
-      pullRequest.status = 'open';
-    }
-    // If we have at least 1 approval (you can adjust this number based on your requirements)
-    else if (approvedReviews.length >= 1) {
+    // Fetch branch protection configuration for the target branch
+    let requiredReviewers = 1; // Default fallback
+    if (pullRequest.repository && pullRequest.targetBranch) {
+      // Assuming repository is populated and has a branchProtection field
+      const repo = await Project.findById(pullRequest.repository);
+      if (repo && repo.branchProtection && repo.branchProtection[pullRequest.targetBranch]) {
+        requiredReviewers = repo.branchProtection[pullRequest.targetBranch].requiredReviewers || 1;
+      }
+    }
+    // If there are any rejections, keep as 'open'
+    if (rejectedReviews.length > 0) {
+      pullRequest.status = 'open';
+    }
+    // If we have at least the required number of approvals
+    else if (approvedReviews.length >= requiredReviewers) {

@@ -100,7 +100,7 @@
                 const branchProtectionStatus = (req as any).branchProtectionStatus;
                 const userId = (req as any).user?.id || 'unknown';
                 const safeId = String(id).replace(/[^\w-]/g, ''); // Sanitize ID
-                console.log(`PR ${safeId} merged by user ${userId}`, {
+                console.log("PR %s merged by user %s", safeId, userId, {
                   method: mergeMethod,
                   branchProtection: branchProtectionStatus,
                   sourceBranch: pr.sourceBranch,

@@ -339,7 +339,7 @@
             });
             // POST /api/branch-protection/rules - Create new branch protection rule
-            router.post('/rules', async (req, res) => {
+            router.post('/rules', writeLimiter, async (req, res) => {
               try {
                 const { projectId, rules, branchPattern, createdBy } = req.body;

@@ -343,13 +343,21 @@
               try {
                 const { projectId, rules, branchPattern, createdBy } = req.body;
-                if (!projectId || !rules) {
-                  return res.status(400).json({ error: 'Project ID and rules are required' });
+                // Validate and sanitize input
+                if (!projectId || typeof projectId !== 'string') {
+                  return res.status(400).json({ error: 'Valid Project ID is required' });
                 }
+                // Validate projectId format
+                if (!/^[a-f0-9]{24}|global|default$/.test(projectId)) {
+                  return res.status(400).json({ error: 'Invalid Project ID format' });
+                }
+                if (!rules) {
+                  return res.status(400).json({ error: 'Rules are required' });
+                }
                 // Check if rules already exist for this project
                 const existingRule = await BranchProtectionRule.findOne({
-                  projectId,
+                  projectId: { $eq: projectId },
                   branchPattern: branchPattern || 'main',
                   isActive: true
                 });

@@ -383,7 +383,7 @@
             });
             // DELETE /api/branch-protection/rules/:id - Delete branch protection rule
-            router.delete('/rules/:id', async (req, res) => {
+            router.delete('/rules/:id', writeLimiter, async (req, res) => {
               try {
                 const { id } = req.params;

Conversation

Ramjat19 commented Oct 30, 2025

Uh oh!

vercel Bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-advanced-security AI left a comment

Choose a reason for hiding this comment

Uh oh!

Ramjat19 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot Autofix

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel Bot commented Oct 30, 2025 •

edited

Loading