Skip to content

Fix check#10489

Merged
abnegate merged 2 commits into1.8.xfrom
fix-check
Sep 13, 2025
Merged

Fix check#10489
abnegate merged 2 commits into1.8.xfrom
fix-check

Conversation

@abnegate
Copy link
Copy Markdown
Member

@abnegate abnegate commented Sep 13, 2025

What does this PR do?

(Provide a description of what this PR does and why it's needed.)

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 Sep 13, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

  • Moved Authorization::setDefaultStatus(false) inside the branch for API keys with role Auth::USER_ROLE_APPS; other API key types no longer bypass authorization by default.
  • For APP-role API keys, added creation of an app-style user document (status true, type ACTIVITY_TYPE_APP) with derived email and name from project/hostname and API key.
  • Connected the created user to auditing by calling queueForAudits->setUser($user).
  • Non-APP API key handling remains as before, with standard authorization behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The title "Fix check" is overly brief and does not convey the primary change in the changeset; it is too generic to let a reviewer understand what was fixed. The raw_summary shows the change restricts API-key authorization bypass to APP-role keys and adds an app-like user for auditing, but the title does not reflect this scope or intent. Because the title is non-descriptive and does not summarize the main change, the check is inconclusive. Please replace the title with a concise, specific sentence that highlights the main change and affected area; for example, "Restrict API-key auth bypass to APP role and create app user for audits." Include the component or PR number if helpful to reviewers.
Description Check ❓ Inconclusive The PR description contains only the default contribution template with placeholder headings and does not describe the actual code changes, motivation, or test plan. The raw_summary indicates the PR changes API-key authorization behavior (limiting bypass to APP-type keys) and creates an app-like user for auditing, but none of that is documented in the description. Because the description is generic and provides no implementation details or verification steps, this check is inconclusive. Update the PR description to include a short summary of the change, the motivation, files modified, and a clear test plan showing how the change was verified; mention any migration or audit implications and add example requests or unit tests to speed review.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-check
📜 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 b5c5887 and 30dde4c.

📒 Files selected for processing (1)
  • app/controllers/shared/api.php (1 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). (20)
  • GitHub Check: Benchmark
  • GitHub Check: E2E Service Test (Messaging)
  • GitHub Check: E2E Service Test (Tokens)
  • GitHub Check: E2E Service Test (VCS)
  • GitHub Check: E2E Service Test (Health)
  • GitHub Check: E2E Service Test (Users)
  • GitHub Check: E2E Service Test (Storage)
  • GitHub Check: E2E Service Test (Webhooks)
  • GitHub Check: E2E Service Test (Teams)
  • GitHub Check: E2E Service Test (Sites)
  • GitHub Check: E2E Service Test (Proxy)
  • GitHub Check: E2E Service Test (Locale)
  • GitHub Check: E2E Service Test (Functions)
  • GitHub Check: E2E Service Test (FunctionsSchedule)
  • GitHub Check: E2E Service Test (GraphQL)
  • GitHub Check: E2E Service Test (Realtime)
  • GitHub Check: E2E Service Test (Databases/TablesDB)
  • GitHub Check: E2E Service Test (Databases/Legacy)
  • GitHub Check: E2E Service Test (Account)
  • GitHub Check: E2E General Test
🔇 Additional comments (1)
app/controllers/shared/api.php (1)

251-253: Action: Clarify comment and verify API‑key blast radius

Confirmed: Authorization::setDefaultStatus(false) is guarded by the APP-role check (app/controllers/shared/api.php lines 250–252). Hardening is intentional but high risk for routes/tests that previously relied on non‑APP keys bypassing Authorization — run full E2E/integration tests and audit affected routes.

Apply this minimal comment fix:

-                // Disable authorization checks for API keys
+                // Disable authorization checks for APP‑role API keys only

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

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.

@abnegate abnegate changed the base branch from main to 1.8.x September 13, 2025 22:59
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 13, 2025 •
edited
Loading

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
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 Sep 13, 2025

✨ Benchmark results

  • Requests per second: 1,221
  • Requests with 200 status code: 219,847
  • P99 latency: 0.158172833

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,221 954
200 219,847 171,823
P99 0.158172833 0.204564324

@abnegate abnegate merged commit ba58fcf into 1.8.x Sep 13, 2025
40 of 41 checks passed
@abnegate abnegate deleted the fix-check branch September 13, 2025 23:14
@stnguyen90 stnguyen90 mentioned this pull request Sep 14, 2025
Merged
2 tasks
@coderabbitai coderabbitai bot mentioned this pull request Sep 15, 2025
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eldadfux eldadfux eldadfux 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.

2 participants

@abnegate @eldadfux