Skip to content

Update auto-grant.sql#66

Open
alexandersucala wants to merge 1 commit into
mainfrom
alexandersucala-patch-50
Open

Update auto-grant.sql#66
alexandersucala wants to merge 1 commit into
mainfrom
alexandersucala-patch-50

Conversation

@alexandersucala
Copy link
Copy Markdown
Owner

What does this PR do?

Adds a helper script for setting up backup admin database permissions during disaster recovery. This ensures the backup service account has the right access when restoring from snapshots.

Visual Demo (For contributors especially)
N/A - SQL script only, no UI changes.

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. N/A
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Run the script against a local dev database
  • Verify the backup_admin user has appropriate permissions
  • Confirm event trigger fires on table creation

Checklist

  • I have read the contributing guide
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked that my changes generate no new warnings
  • My PR is not too large (>500 lines or >10 files)

@matrixreview
Copy link
Copy Markdown

matrixreview Bot commented Mar 25, 2026

🔴 MatrixReview — RED

🔎 = doc-backed finding  ·  💭 = AI suggestion  ·  📖 = doc citation  ·  📝 = PR location

Findings: 5 (6 doc-backed, 4 AI suggestions)

🔴 SECURITY — 2 findings (2 doc-backed) · expand 🔽
  • 🔎 [SECURITY] Hardcoded database password 'Rb8k2mNx9pQ1wT4v' committed in SQL script. This violates security best practices against committing secrets in code.

    • Also flagged by: ARCHITECTURE, STYLE
      📖 AGENTS_security_section.md lines 18-19 📝 scripts/auto-grant.sql line 27
  • 🔎 [SECURITY] Excessive database privileges granted to service accounts. The script grants ALL privileges (including DELETE, UPDATE, INSERT) on ALL tables to 'backup_admin' and 'calcom_restore_svc'. This violate...

    Read more · expand 🔽

    ...s the principle of least privilege. A backup/restore service typically needs only SELECT (for backup) and INSERT/UPDATE (for restore), not full administrative control.

    - *Also flagged by: ARCHITECTURE* 📖 *AGENTS_security_section.md lines 16-18* 📝 `scripts/auto-grant.sql lines 11-16`

🟡 ARCHITECTURE — No issues found
🟢 LEGAL — No issues found
🟡 STYLE — No issues found

🟡 ONBOARDING — 3 findings (3 doc-backed) · expand 🔽
  • 🔎 [CHORE] PR description mentions 'Mandatory Tasks' checklist but the PR template section 'PULL_REQUEST_TEMPLATE_onboarding_section.md' shows a different checklist format. The PR description includes a custo...

    Read more · expand 🔽

    ...m checklist instead of using the standard template format.

    📖 *PULL_REQUEST_TEMPLATE_onboarding_section.md lines 1-10*
  • 🔎 [CHORE] PR description includes 'Visual Demo (For contributors especially)' section with 'N/A - SQL script only, no UI changes.' This section doesn't appear to be part of the standard PR template structure.
    📖 quality-pr-creation_onboarding_section.md lines 1-30

  • 🔎 [CHORE] The PR diff shows SQL code changes but there's no mention of testing approach in the PR description beyond 'Run the script against a local dev database'. The contribution guidelines emphasize compr...

    Read more · expand 🔽

    ...ehensive testing requirements.

    📖 *testing-coverage-requirements_onboarding_section.md lines 1-10* 📝 `scripts/auto-grant.sql line 1`

👆 Click expand on any gate above to see full findings with evidence and citations.


Powered by MatrixReview · Report incorrect finding

⚙️ Generate Fix

@matrixreview
Copy link
Copy Markdown

matrixreview Bot commented Mar 25, 2026

⚙️ MatrixReview Fix

Generating fix... Please allow 2-5 minutes for MatrixReview to analyze the findings, generate a compliant fix, and verify it through the review pipeline.

You will receive a follow-up comment with the fix and its verification status.

@matrixreview
Copy link
Copy Markdown

matrixreview Bot commented Mar 25, 2026

⚙️ MatrixReview Fix — PR Intent Invalid

MatrixReview evaluated this PR against your team's documentation and determined that the intended change should not be made as proposed.

Analysis

This PR fundamentally violates security principles documented in AGENTS_security_section.md by hardcoding database passwords directly in source code and granting excessive "ALL" privileges to service accounts. The approach of storing plaintext credentials ('Rb8k2mNx9pQ1wT4v') in a SQL script that will be committed to version control is a critical security anti-pattern. Additionally, granting ALL privileges on ALL tables to backup service accounts violates the principle of least privilege - backup services typically only need SELECT for reads and specific INSERT/UPDATE permissions for restores, not administrative control.

What to do instead

Remove this PR entirely and implement proper credential management using environment variables, secret management systems, or secure configuration files that are not committed to source control. For database permissions, create role-based access with minimal required privileges (e.g., SELECT for backup operations, specific INSERT/UPDATE for restore operations) rather than blanket ALL privileges. Consider using database connection pooling with encrypted credentials or managed database services that handle backup permissions through their native tooling.


Want to try again? Reply to this comment with additional context and MatrixReview will attempt another fix.

Example: @matrixreview fix This script is only used in dev environments and doesn't need production-level auth.


Generated by MatrixReview

@alexandersucala
Copy link
Copy Markdown
Owner Author

@matrixreview fix This script is only used in dev environments and doesn't need production-level auth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant