Add OWASP Slack invitation link expiration check#4417
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds Slack invite-link monitoring: new GitHub utility to find the invite commit, a Django management command to check member thresholds and post Slack alerts, model fields and migrations to track invite metadata and alerting state, Makefile/cron entries to run the check, utility exports, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 11 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: the only flagged item is a moderate, localized null-handling gap rather than a broad behavioral regression.
- In
backend/apps/slack/management/commands/slack_check_invite_link.py, calling.strip()on a NULLinvite_link_commit_shacan raise anAttributeError, which could cause the management command to fail for records missing that field. - Severity/confidence (4/10, 7/10) suggests a real but limited impact area, so this is a small robustness fix rather than a merge-blocking defect.
- Pay close attention to
backend/apps/slack/management/commands/slack_check_invite_link.py- guardinvite_link_commit_shabefore.strip()to prevent runtime errors on NULL values.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/slack/management/commands/slack_check_invite_link.py">
<violation number="1" location="backend/apps/slack/management/commands/slack_check_invite_link.py:61">
P2: Handle a missing invite_link_commit_sha before calling .strip() to avoid an AttributeError when the field is NULL.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/apps/slack/management/commands/slack_check_invite_link.py
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cron/production (1)
6-6: Consider a lock (flock) for this new cron chain.Line 6 can overlap with the next run if a job stalls, which may duplicate alerts/log churn. Wrapping the chain with a lockfile would harden this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cron/production` at line 6, Wrap the existing cron command chain that runs "make slack-sync-data" and "make slack-check-invite-link" in a flock-based lock to prevent overlapping runs: acquire a named lockfile (e.g. slack-sync-data.lock) and run the cd && make ... && make ... sequence under that lock (use non-blocking or a reasonable timeout as desired) while preserving the existing log redirections so stalled jobs cannot start a second concurrent run.backend/tests/apps/slack/commands/slack_check_invite_link_test.py (2)
384-386: Consider adding a test case for mixed valid and blank IDs.The tests cover empty/blank-only lists, but a case with mixed valid and blank IDs (e.g.,
["U123", "", "U456"]) would verify filtering works correctly when some IDs are valid.🧪 Suggested additional test
def test_filters_blank_ids_from_valid_list(self): cmd = SlackCheckInviteLinkCommand() result = cmd.invite_link_alert_cc_line(["U123", "", "U456"]) assert result == "\n\ncc: <@U123> <@U456>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/commands/slack_check_invite_link_test.py` around lines 384 - 386, Add a unit test that verifies invite_link_alert_cc_line filters out blank IDs when presented with a mixed list; specifically call SlackCheckInviteLinkCommand().invite_link_alert_cc_line with a list like ["U123", "", "U456"] and assert the output contains only the valid mentions (e.g., "\n\ncc: <@U123> <@U456>"), ensuring blank strings are ignored.
52-56: Add comments explaining intentionally empty stub methods.SonarCloud flagged these empty methods. Since they're intentional no-op stubs for the in-memory test double, add brief comments to document this.
📝 Proposed fix
def save(self, update_fields=None): - pass + pass # No-op: in-memory stub, no DB persistence needed. def refresh_from_db(self): - pass + pass # No-op: state is already in-memory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/slack/commands/slack_check_invite_link_test.py` around lines 52 - 56, The empty methods save and refresh_from_db in the test double are intentional no-op stubs and should be documented to silence SonarCloud and help future readers; update the save(self, update_fields=None) and refresh_from_db(self) definitions to include brief inline comments explaining they are intentionally left blank because this is an in-memory test double and persistence/refresh are not required in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/github/utils.py`:
- Around line 174-176: Replace use of django.utils.timezone.utc with the stdlib
UTC object: in the block where timezone.is_naive(dt) and timezone.make_aware(dt,
timezone.utc) are used (around the dt handling in utils.py), change the second
argument to datetime.timezone.utc and ensure datetime is imported at the module
top if not already; keep the existing calls to timezone.is_naive and
timezone.make_aware unchanged aside from swapping timezone.utc to
datetime.timezone.utc to restore Django 6 compatibility.
---
Nitpick comments:
In `@backend/tests/apps/slack/commands/slack_check_invite_link_test.py`:
- Around line 384-386: Add a unit test that verifies invite_link_alert_cc_line
filters out blank IDs when presented with a mixed list; specifically call
SlackCheckInviteLinkCommand().invite_link_alert_cc_line with a list like
["U123", "", "U456"] and assert the output contains only the valid mentions
(e.g., "\n\ncc: <@U123> <@U456>"), ensuring blank strings are ignored.
- Around line 52-56: The empty methods save and refresh_from_db in the test
double are intentional no-op stubs and should be documented to silence
SonarCloud and help future readers; update the save(self, update_fields=None)
and refresh_from_db(self) definitions to include brief inline comments
explaining they are intentionally left blank because this is an in-memory test
double and persistence/refresh are not required in tests.
In `@cron/production`:
- Line 6: Wrap the existing cron command chain that runs "make slack-sync-data"
and "make slack-check-invite-link" in a flock-based lock to prevent overlapping
runs: acquire a named lockfile (e.g. slack-sync-data.lock) and run the cd &&
make ... && make ... sequence under that lock (use non-blocking or a reasonable
timeout as desired) while preserving the existing log redirections so stalled
jobs cannot start a second concurrent run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f84ce200-b858-4c64-86a9-376136b60152
📒 Files selected for processing (11)
backend/apps/github/utils.pybackend/apps/slack/Makefilebackend/apps/slack/__init__.pybackend/apps/slack/management/commands/slack_check_invite_link.pybackend/apps/slack/migrations/0020_workspace_invite_link_fields.pybackend/apps/slack/models/workspace.pybackend/apps/slack/utils/__init__.pybackend/apps/slack/utils/format.pybackend/tests/apps/slack/commands/slack_check_invite_link_test.pycron/productioncspell/custom-dict.txt
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4417 +/- ##
==========================================
- Coverage 99.28% 99.17% -0.11%
==========================================
Files 521 522 +1
Lines 16507 16648 +141
Branches 2262 2279 +17
==========================================
+ Hits 16389 16511 +122
- Misses 68 87 +19
Partials 50 50
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
arkid15r
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/slack/models/workspace.py (1)
43-101: Deduplicate the350offset default to a single constant.Line 47 and Line 99 repeat the same default value. Centralizing it avoids drift between field default and runtime fallback.
Proposed refactor
class Workspace(TimestampedModel): """Slack Workspace model.""" + INVITE_LINK_ALERT_MEMBER_OFFSET_DEFAULT = 350 @@ invite_link_alert_member_offset = models.PositiveSmallIntegerField( @@ - default=350, + default=INVITE_LINK_ALERT_MEMBER_OFFSET_DEFAULT, @@ def invite_link_alert_threshold(self) -> int | None: @@ offset = ( self.invite_link_alert_member_offset if self.invite_link_alert_member_offset is not None - else 350 + else self.INVITE_LINK_ALERT_MEMBER_OFFSET_DEFAULT ) return self.invite_link_member_count + offset🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/slack/models/workspace.py` around lines 43 - 101, Create a single module-level constant (e.g. INVITE_LINK_ALERT_OFFSET_DEFAULT = 350) and use it everywhere the literal 350 appears so the field default and runtime fallback stay in sync; update the PositiveSmallIntegerField default for invite_link_alert_member_offset and replace the hardcoded 350 in the invite_link_alert_threshold property fallback with that constant (ensure the constant name is exported/placed above the Workspace model so invite_link_alert_member_offset and method invite_link_alert_threshold reference it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/apps/slack/commands/slack_check_invite_link_test.py`:
- Around line 57-61: The test-double methods save and refresh_from_db currently
have empty bodies which trips static analysis; update the methods in the test
double to include explicit no-op docstrings or comments (e.g., """No-op in
tests: intentionally does not persist/refresh state.""") so intent is clear to
linters and readers, leaving the pass statement in place; reference the save and
refresh_from_db method definitions in the test class and add the
docstrings/comments inside those methods.
---
Nitpick comments:
In `@backend/apps/slack/models/workspace.py`:
- Around line 43-101: Create a single module-level constant (e.g.
INVITE_LINK_ALERT_OFFSET_DEFAULT = 350) and use it everywhere the literal 350
appears so the field default and runtime fallback stay in sync; update the
PositiveSmallIntegerField default for invite_link_alert_member_offset and
replace the hardcoded 350 in the invite_link_alert_threshold property fallback
with that constant (ensure the constant name is exported/placed above the
Workspace model so invite_link_alert_member_offset and method
invite_link_alert_threshold reference it).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 486a8d14-5971-49d1-85fd-6b7d78904f4e
📒 Files selected for processing (4)
backend/apps/slack/management/commands/slack_check_invite_link.pybackend/apps/slack/migrations/0021_alter_workspace_invite_link_alert_channel_id_and_more.pybackend/apps/slack/models/workspace.pybackend/tests/apps/slack/commands/slack_check_invite_link_test.py
✅ Files skipped from review due to trivial changes (1)
- backend/apps/slack/management/commands/slack_check_invite_link.py
backend/tests/apps/slack/commands/slack_check_invite_link_test.py
Outdated
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Add OWASP Slack invitation link basic monitorin/alerting via NestBot.
Checklist
make check-testlocally: all warnings addressed, tests passed