Skip to content

Skip auth check to delete vcs lock document#10691

Merged
Meldiron merged 1 commit into1.8.xfrom
chore-skip-auth-to-delete-vcs-lock
Oct 23, 2025
Merged

Skip auth check to delete vcs lock document#10691
Meldiron merged 1 commit into1.8.xfrom
chore-skip-auth-to-delete-vcs-lock

Conversation

@vermakhushboo
Copy link
Copy Markdown
Contributor

@vermakhushboo vermakhushboo commented Oct 23, 2025 •
edited by stnguyen90
Loading

What does this PR do?

Skip auth check to delete vcs lock document

Fixes: #10781

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)

Related PRs and Issues

  • (Related PR or issue)

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 23, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The changes modify app/controllers/api/vcs.php to wrap deleteDocument() calls with an authorization-skip pattern. Specifically, direct deletion calls for vcsCommentLocks entries in finally blocks and installations in webhook event handling are replaced with Authorization::skip(fn () => $dbForPlatform->deleteDocument(...)) constructs. This same pattern is applied consistently across multiple cleanup operations related to VCS comment handling and deployment logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes apply a repetitive, consistent pattern across a single file. The authorization-skip wrapper is straightforward to verify, though reviewers should confirm that skipping authorization is appropriate for each cleanup operation targeted. The homogeneous nature of the edits (same wrapper pattern repeated) and single-file scope keep complexity low.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Skip auth check to delete vcs lock document" directly and accurately describes the main change in the pull request. The raw summary confirms that the changeset wraps deleteDocument calls with Authorization::skip() for VCS lock cleanup operations and installation deletion in webhook handling. The title is concise, clear, and specific enough that a teammate scanning history would understand the primary technical modification.
Description Check ✅ Passed The pull request description states "Skip auth check to delete vcs lock document," which is directly related to the changeset. While the description is minimal and lacks detailed explanation of testing or related issues, it accurately describes what the PR accomplishes and is not off-topic or unrelated to the actual code changes shown in the summary. The lenient evaluation criteria for this check requires only that the description be related to the changeset in some way, regardless of level of detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore-skip-auth-to-delete-vcs-lock
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c80b40c and a6af0cc.

📒 Files selected for processing (1)
  • app/controllers/api/vcs.php (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
  • GitHub Check: E2E Service Test (Messaging)
  • GitHub Check: E2E Service Test (Storage)
  • GitHub Check: E2E Service Test (Console)
  • GitHub Check: E2E Service Test (FunctionsSchedule)
  • GitHub Check: E2E Service Test (Health)
  • GitHub Check: E2E Service Test (GraphQL)
  • GitHub Check: E2E Service Test (Avatars)
  • GitHub Check: E2E Service Test (Databases/Legacy)
  • GitHub Check: E2E Service Test (Site Screenshots)
  • GitHub Check: E2E Service Test (Account)
  • GitHub Check: Unit Test
  • GitHub Check: E2E General Test
  • GitHub Check: E2E Service Test (Dev Keys)
  • GitHub Check: Setup & Build Appwrite Image
  • GitHub Check: scan
🔇 Additional comments (2)
app/controllers/api/vcs.php (2)

161-170: LGTM! Authorization skip is appropriate for lock cleanup.

The Authorization::skip() wrapper ensures the lock document is always deleted in the finally block, regardless of the current authorization context. This is the correct approach for cleanup of temporary coordination locks.

1377-1377: LGTM! Authorization skip is necessary for webhook-triggered deletion.

The Authorization::skip() wrapper is required here because the webhook handler runs in a public scope context. This allows the installation to be deleted when GitHub notifies that the app installation was removed.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 23, 2025

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
binutils 2.44-r2 CVE-2025-5244 HIGH
binutils 2.44-r2 CVE-2025-5245 HIGH
libxml2 2.13.8-r0 CVE-2025-49794 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49796 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49795 HIGH
libxml2 2.13.8-r0 CVE-2025-6021 HIGH
pcre2 10.43-r1 CVE-2025-58050 CRITICAL
golang.org/x/crypto v0.31.0 CVE-2025-22869 HIGH
golang.org/x/oauth2 v0.24.0 CVE-2025-22868 HIGH
stdlib 1.22.10 CVE-2025-47907 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 23, 2025

✨ Benchmark results

  • Requests per second: 1,142
  • Requests with 200 status code: 205,613
  • P99 latency: 0.169376043

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,142 944
200 205,613 169,873
P99 0.169376043 0.203341633

@Meldiron Meldiron merged commit 046d119 into 1.8.x Oct 23, 2025
41 checks passed
@stnguyen90 stnguyen90 deleted the chore-skip-auth-to-delete-vcs-lock branch November 12, 2025 18:14
@stnguyen90 stnguyen90 moved this to Done in 1.8 Release Nov 12, 2025
@stnguyen90 stnguyen90 removed this from 1.8 Release Nov 12, 2025
@stnguyen90 stnguyen90 mentioned this pull request Nov 12, 2025
Closed
2 tasks
@stnguyen90 stnguyen90 linked an issue Nov 12, 2025 that may be closed by this pull request
VCS webhook handler fails with 'No permissions provided for action delete' when processing GitHub events #10781
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Meldiron Meldiron Meldiron approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

VCS webhook handler fails with 'No permissions provided for action delete' when processing GitHub events

3 participants

@vermakhushboo @Meldiron @stnguyen90