Enabled slack connector to ingest files and support OAuth

[mateuszkuprowski]

No description provided.

[cubic-dev-ai]

2 issues found across 2 files

<sub>Shadow auto-approve: would not auto-approve because issues were found.

Fix all with cubic | Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 4 files (changes from recent commits).

<sub>Shadow auto-approve: would auto-approve. This PR adds file attachment indexing and OAuth refresh token support to the Slack connector, with thorough test coverage and no changes to core business logic, infrastructure, or security-sensitive paths, and the AI review found no issues.

Re-trigger cubic</sub>

[potter-potter]

@claude please review

[claude]

Claude finished @potter-potter's task —— View job

---

Code Review Complete ✅

Reviewing PR: "Enabled slack connector to ingest files and support OAuth"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Review main connector implementation changes
• Analyze new test file for coverage and quality
• Check changelog and version updates
• Provide comprehensive review feedback

---

Review Summary

This PR successfully adds file attachment ingestion and OAuth refresh token support to the Slack connector. The implementation is well-structured, secure, and thoroughly tested.

✅ Strengths

Security (Excellent)

• _validate_private_download_url() function provides robust URL validation for Slack file downloads
• Proper validation prevents SSRF attacks by restricting to files.slack.com with HTTPS
• Credentials validation prevents URL-embedded auth tokens
• Timeout protection with PRIVATE_FILE_DOWNLOAD_TIMEOUT_SECONDS (60s) prevents hanging requests
• Bearer token properly isolated from URL construction

Code Quality (Very Good)

• Clean separation between conversation and file download logic
• Proper async/await patterns throughout SlackDownloader
• Good error handling with descriptive error messages
• Constants appropriately defined at module level
• Filename sanitization with _safe_slack_filename() prevents path traversal

Testing (Excellent)

• Comprehensive test coverage for new functionality
• Tests verify both positive and negative security scenarios
• Mock usage is appropriate and tests actual behavior
• Async test patterns properly implemented with pytest.mark.asyncio

📋 Detailed Analysis

File Attachment Support (slack.py:181-217)

• _message_files_to_file_data() properly extracts file metadata from messages
• Robust identifier generation using SHA256 of channel-timestamp-file_id
• Good fallback logic for missing file names/titles

Download Implementation (slack.py:307-341)

• _download_file() method is well-structured with proper error handling
• Proper use of asyncio.to_thread() for blocking urllib operations
• File validation occurs before any network requests
• Bearer authentication correctly implemented

OAuth Support (slack.py:79)

• Simple, clean addition of refresh_token field to SlackAccessConfig
• Proper typing with Optional[str] and field description
• Maintains backward compatibility

Test Coverage (test_slack.py)

• Tests cover the critical security validation scenario
• Timeout verification ensures network protection
• File attachment indexing thoroughly tested
• Mock patterns are appropriate and maintainable

🔍 Minor Observations

Documentation

• Changelog entries are clear and accurate (CHANGELOG.md:5-9)
• Version bump is appropriate for the feature scope
• Function docstrings could be added for public methods, but existing code style doesn't require them

Error Handling

• Good use of custom ValueError from unstructured_ingest.error
• Error messages are descriptive and actionable

Constants

• Well-defined constants (PAGINATION_LIMIT, PRIVATE_FILE_DOWNLOAD_TIMEOUT_SECONDS, SLACK_PRIVATE_FILE_HOST)
• Good separation of concerns

✅ Security Review

The security improvements are particularly noteworthy:

• Addresses previous security findings with proper URL validation
• Prevents token leakage through URL validation
• SSRF protection via hostname/scheme/port validation
• Timeout protection prevents resource exhaustion
• No credentials embedded in URLs

🎯 Recommendation

APPROVE - This is a well-implemented feature addition with excellent security practices and comprehensive testing. The code follows the project's patterns, includes appropriate error handling, and addresses the security concerns raised in previous reviews.

The implementation demonstrates good understanding of:

• Async patterns in Python
• Security best practices for external API integrations
• Proper test coverage including edge cases
• Clean separation of concerns

---

[cubic-dev-ai]

0 issues found across 2 files (changes from recent commits).

<sub>Shadow auto-approve: would require human review. This PR adds significant new functionality to the Slack connector—file attachment ingestion, private file download with bearer authentication, and OAuth refresh token support—all of which have a moderate blast radius and risk of breakage, so human review is warranted to ensure correctness and...

Re-trigger cubic</sub>

[potter-potter]

LG