Add Gmail thread ID to queries, harden cache validation, and clean up API layer#76
Add Gmail thread ID to queries, harden cache validation, and clean up API layer#76
Conversation
welcome to the resistance! I'm taking a look at this now |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| with: | ||
| # SECURITY: Always checkout base branch (trusted code), never PR branch | ||
| # This is critical when using pull_request_target which has access to secrets | ||
| ref: ${{ github.event.pull_request.base.ref }} |
There was a problem hiding this comment.
🚨 Insufficient branch verification before secret access (high severity)
The workflow checks out the base branch but does not verify the Python script integrity before executing it with ANTHROPIC_API_KEY access. A malicious PR could modify .github/scripts/security_review.py in a prior merged commit, which would then execute with secrets. Add explicit verification that security_review.py matches a known-good hash or is signed before running with secrets.
Automated security review by Claude 4.5 Sonnet - Human review still required
| import subprocess | ||
|
|
||
| result = subprocess.run( | ||
| ["git", "diff", f"{base_sha}...{head_sha}"], |
There was a problem hiding this comment.
🚨 Command injection via git diff with untrusted SHAs (high severity)
The get_pr_diff function uses BASE_SHA and HEAD_SHA from environment variables in subprocess.run without validation. An attacker controlling these values (e.g., via workflow_run trigger manipulation) could inject shell commands. Validate SHAs match ^[a-f0-9]{40}$ pattern before passing to git diff.
Automated security review by Claude 4.5 Sonnet - Human review still required
| HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| run: | | ||
| # Run from base branch (trusted code) - PR branch is only fetched for diff | ||
| python .github/scripts/security_review.py |
There was a problem hiding this comment.
🚨 Python dependencies not pinned to hashes (high severity)
The workflow installs anthropic==0.49.0 and PyGithub==2.6.0 using pip without hash verification (--require-hashes). An attacker compromising PyPI could inject malicious code that exfiltrates ANTHROPIC_API_KEY. Pin dependencies with hashes or use a lockfile with integrity checks.
Automated security review by Claude 4.5 Sonnet - Human review still required
| bindAddr := s.cfg.Server.BindAddr | ||
| if bindAddr == "" { | ||
| bindAddr = "127.0.0.1" | ||
| } |
There was a problem hiding this comment.
🚨 API server allows unauthenticated localhost binding (high severity)
When APIKey is empty and bind_addr is 127.0.0.1, the server starts without authentication. While localhost-only, this allows any local process (malware, compromised app) to access 20+ years of email via HTTP. Require API key even for localhost, or add explicit warning that localhost binding is unauthenticated.
Automated security review by Claude 4.5 Sonnet - Human review still required
| func (s *Server) handleSearch(w http.ResponseWriter, r *http.Request) { | ||
| if s.store == nil { | ||
| writeError(w, http.StatusServiceUnavailable, "store_unavailable", "Database not available") | ||
| return |
There was a problem hiding this comment.
The searchMessagesLike function uses escapeLike but only escapes wildcard chars (%, _). If the query contains SQL syntax like ' OR 1=1 --, it could break out of the LIKE pattern. While LIKE is not as dangerous as WHERE clause injection, this could cause unexpected results or DoS. Use parameterized queries exclusively.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| // IsLoopback returns true if the bind address is a loopback address. | ||
| // Handles the full 127.0.0.0/8 range, IPv6 ::1, and "localhost". |
There was a problem hiding this comment.
The allow_insecure flag bypasses API key enforcement for non-loopback addresses. While explicitly opt-in, this is dangerous for users who misunderstand the risk. An attacker on the local network could access all email data. Remove allow_insecure or require both API key AND TLS when binding to non-loopback.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| // Validate and add the cron job | ||
| entryID, err := s.cron.AddFunc(cronExpr, func() { |
There was a problem hiding this comment.
The runSync function uses s.ctx (which is only cancelled on Stop) without per-sync timeout. A malicious or buggy sync could run indefinitely, blocking shutdown and consuming resources. Add a per-sync timeout context (e.g., 1 hour) to prevent runaway syncs from blocking Stop().
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| var serveCmd = &cobra.Command{ | ||
| Use: "serve", | ||
| Short: "Run msgvault as a daemon with scheduled sync", |
There was a problem hiding this comment.
The serve command starts an API server that can access messages, which internally uses OAuth tokens. If APIKey is not configured and bind_addr is localhost, any local process can trigger syncs or access token-protected data. Require APIKey for serve command or add prominent warning that unauthenticated local access is allowed.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| steps: | ||
| - name: Checkout base branch | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 |
There was a problem hiding this comment.
🚨 Workflow uses pull_request_target with untrusted code (high severity)
The workflow uses pull_request_target which grants GITHUB_TOKEN write access to pull requests and access to secrets, even for PRs from forks. While the workflow attempts to mitigate this by checking out the base branch only, a malicious PR could still exploit this through environment variable injection or by modifying the trusted-contributors.json file if the attacker has write access. Consider using pull_request trigger with manual approval for external contributors, or implement additional safeguards like cryptographic verification of the base branch checkout.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
🚨 Script executes in pull_request_target context with secrets (high severity)
This Python script runs in a pull_request_target workflow context where it has access to ANTHROPIC_API_KEY and GITHUB_TOKEN secrets. If an attacker can influence the execution environment (e.g., via malicious dependencies or environment variables in the PR), they could exfiltrate these secrets. The workflow should verify script integrity via checksum or run it in a more restricted context. Consider moving secret-dependent operations to a separate workflow that only triggers after human approval.
Automated security review by Claude 4.5 Sonnet - Human review still required
| { | ||
| "trusted_github_usernames": [ | ||
| "wesm", | ||
| "dependabot[bot]" |
There was a problem hiding this comment.
🚨 Trusted contributors list can be modified by attackers (high severity)
The trusted-contributors.json file is checked into the repository and read by the security-review workflow to bypass automated review. If an attacker gains write access to the repository (e.g., through a compromised maintainer account or a vulnerability in GitHub), they could add themselves to this list and bypass all security checks. CODEOWNERS protection mitigates this but is not foolproof. Consider implementing additional verification like requiring multiple approvals or using GitHub's branch protection rules with required reviews from specific teams.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| // authMiddleware validates the API key. | ||
| func (s *Server) authMiddleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
🚨 Timing-safe comparison used but insufficient for auth (high severity)
The API key authentication uses subtle.ConstantTimeCompare to prevent timing attacks, which is good practice. However, the auth middleware only checks for non-empty API keys at the server level. If cfg.Server.APIKey is accidentally set to an empty string in production, authentication is completely disabled. The ValidateSecure check in Start() helps but doesn't prevent runtime configuration changes. Add an explicit check that rejects empty API keys and consider implementing key rotation mechanisms.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| idStr := chi.URLParam(r, "id") | ||
| id, err := strconv.ParseInt(idStr, 10, 64) |
There was a problem hiding this comment.
The message ID from the URL parameter is parsed as int64 and used directly in database queries. While ParseInt provides some validation, the subsequent database query in GetMessage uses this value. Verify that all downstream database operations use parameterized queries (appears to be the case from the code, but should be confirmed in the store implementation). Consider adding explicit range validation for message IDs to prevent potential integer overflow issues.
Automated security review by Claude 4.5 Sonnet - Human review still required
| rl.mu.Unlock() | ||
| return entry.limiter.Allow() | ||
| } | ||
|
|
There was a problem hiding this comment.
The rate limiter extracts the client IP from r.RemoteAddr using net.SplitHostPort. This works for direct connections but can be bypassed when the API is behind a reverse proxy, as RemoteAddr will be the proxy's IP rather than the real client IP. If deploying behind a proxy, all external clients will share the same rate limit. Consider adding support for X-Forwarded-For or X-Real-IP headers with proper validation to prevent header spoofing.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| // escapeLike escapes SQL LIKE special characters (%, _) so they are | ||
| // matched literally. The escaped string should be used with ESCAPE '\'. | ||
| func escapeLike(s string) string { |
There was a problem hiding this comment.
The escapeLike function properly escapes LIKE wildcards to prevent SQL injection, which is good. However, searchMessagesLike performs a full table scan with LIKE operations on subject and snippet columns. For large archives (20+ years of email), this could cause performance degradation or DoS. The limit parameter helps but doesn't prevent repeated expensive queries. Consider adding query timeout constraints or requiring minimum search term length (e.g., 3 characters) to prevent abuse.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| // Load reads the configuration from the specified file. | ||
| // If path is empty, uses the default location (~/.msgvault/config.toml), |
There was a problem hiding this comment.
The Load function expands tilde (~) in paths using expandPath, which is useful for usability but could allow path traversal if the homeDir parameter is attacker-controlled (e.g., via environment variables or command-line flags). An attacker could potentially load arbitrary config files by manipulating MSGVAULT_HOME or the --home flag. While this requires local access, it could lead to information disclosure. Validate that expanded paths stay within expected directories and consider warning users when loading configs from unusual locations.
Automated security review by Claude 4.5 Sonnet - Human review still required
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
| _, exists := s.jobs[email] | ||
| return exists |
There was a problem hiding this comment.
The AddAccount function uses robfig/cron to parse and schedule sync jobs based on user-provided cron expressions. While the parser validates syntax, malicious cron expressions like '* * * * *' (every minute) could cause excessive API calls to Gmail, potentially hitting rate limits or causing account suspension. Consider restricting allowed cron patterns to reasonable intervals (e.g., minimum 5-minute intervals) or implementing additional rate limiting at the scheduler level to prevent abuse.
Automated security review by Claude 4.5 Sonnet - Human review still required
| @@ -1,27 +1,27 @@ | |||
| module github.com/wesm/msgvault | |||
There was a problem hiding this comment.
This PR updates numerous dependencies including critical ones like go-chi/chi (HTTP router), go-duckdb (database driver), and charmbracelet packages (TUI framework). While most are minor/patch updates, some are significant version bumps (e.g., go-chi/chi appears to be newly added). Each dependency change introduces potential supply chain risks. Verify that all updated dependencies have been reviewed for known vulnerabilities using govulncheck and that new dependencies (especially go-chi) are from trusted sources with good security track records.
Automated security review by Claude 4.5 Sonnet - Human review still required
| with: | ||
| # SECURITY: Always checkout base branch (trusted code), never PR branch | ||
| # This is critical when using pull_request_target which has access to secrets | ||
| ref: ${{ github.event.pull_request.base.ref }} |
There was a problem hiding this comment.
🚨 fetch-depth: 0 allows PR branch checkout via git commands (high severity)
Setting fetch-depth: 0 fetches all branches including the untrusted PR branch. While the workflow checks out the base branch, a malicious script could use 'git checkout' to switch to the PR branch and execute untrusted code with access to ANTHROPIC_API_KEY and GITHUB_TOKEN. Use fetch-depth: 1 and only fetch the base branch, or implement additional safeguards to prevent branch switching.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| import subprocess | ||
|
|
||
| result = subprocess.run( |
There was a problem hiding this comment.
🚨 Unsafe subprocess call with untrusted git diff range (high severity)
The get_pr_diff function uses subprocess.run with BASE_SHA and HEAD_SHA from environment variables (supplied by GitHub) in a git diff command without validation. While GitHub provides these SHAs, an attacker with workflow_run trigger access could potentially inject malicious content. Use subprocess with shell=False (already present) and validate SHAs match the expected format (40-char hex) before use.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| - name: Set up Python | ||
| if: steps.check-trusted.outputs.is_trusted != 'true' | ||
| uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 |
There was a problem hiding this comment.
🚨 Python package versions not pinned with hashes (high severity)
Installing anthropic==0.49.0 and PyGithub==2.6.0 without hash verification allows supply chain attacks via PyPI compromise. An attacker who compromises PyPI could inject malicious code that exfiltrates ANTHROPIC_API_KEY and GITHUB_TOKEN. Use pip install with --require-hashes and a requirements.txt file containing SHA256 hashes for all dependencies and their transitive dependencies.
Automated security review by Claude 4.5 Sonnet - Human review still required
| // authMiddleware validates the API key. | ||
| func (s *Server) authMiddleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // Skip auth if no API key configured |
There was a problem hiding this comment.
🚨 Timing attack in API key comparison (high severity)
Line 191 uses subtle.ConstantTimeCompare correctly, but the code converts strings to byte slices just before comparison. If the conversion or slice creation leaks timing information (e.g., different allocations for different lengths), it could enable timing attacks to extract the API key. Consider hashing both keys with a constant-time hash before comparison, or ensure the entire comparison path is timing-safe.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| // Load reads the configuration from the specified file. | ||
| // If path is empty, uses the default location (~/.msgvault/config.toml), |
There was a problem hiding this comment.
The Load function now accepts a homeDir parameter that overrides MSGVAULT_HOME, but when homeDir is set without an explicit path, the config file is loaded from homeDir instead of the expected location. This could allow an attacker with control over the homeDir parameter to load a malicious config.toml with attacker-controlled oauth.client_secrets pointing to a credential harvesting server. Validate homeDir against path traversal and ensure explicit config path takes precedence.
Automated security review by Claude 4.5 Sonnet - Human review still required
| info := AccountInfo{ | ||
| Email: acc.Email, | ||
| Schedule: acc.Schedule, | ||
| Enabled: acc.Enabled, |
There was a problem hiding this comment.
The handleListAccounts and handleTriggerSync handlers use account email from URL parameters without validation. While the scheduler checks if the account is scheduled, the email is used in log messages and potentially in error messages exposed to API clients, creating a vector for log injection or information disclosure. Sanitize and validate email format before using it in logs or responses.
Automated security review by Claude 4.5 Sonnet - Human review still required
| // matched literally. The escaped string should be used with ESCAPE '\'. | ||
| func escapeLike(s string) string { | ||
| s = strings.ReplaceAll(s, `\`, `\\`) | ||
| s = strings.ReplaceAll(s, `%`, `\%`) |
There was a problem hiding this comment.
The searchMessagesLike function on line 242 uses user input in a SQL LIKE query after escaping. While SQL injection is prevented by parameterized queries, the LIKE pattern matching could exhibit worst-case performance (quadratic time) on pathological inputs with many escape characters. Consider limiting query length and implementing a timeout for LIKE searches to prevent resource exhaustion.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| func runServe(cmd *cobra.Command, args []string) error { | ||
| // Validate security posture before doing any work |
There was a problem hiding this comment.
The serve command starts an HTTP API that includes scheduler control endpoints (trigger sync, list accounts) but the security validation only checks bind address and API key presence. If an attacker bypasses authentication (e.g., via SSRF or network misconfiguration), they could trigger expensive sync operations repeatedly. Ensure all administrative endpoints require authentication and implement per-client rate limiting.
Automated security review by Claude 4.5 Sonnet - Human review still required
| with: | ||
| # SECURITY: Always checkout base branch (trusted code), never PR branch | ||
| # This is critical when using pull_request_target which has access to secrets | ||
| ref: ${{ github.event.pull_request.base.ref }} |
There was a problem hiding this comment.
🚨 Untrusted PR code could be executed via fetch-depth (high severity)
Setting fetch-depth: 0 fetches the entire history including PR commits. If the PR contains malicious commits that modify .github/scripts/security_review.py, those changes are fetched and could be executed when the script runs. Use fetch-depth: 1 and only fetch the base branch to prevent this attack vector.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| import subprocess | ||
|
|
||
| result = subprocess.run( |
There was a problem hiding this comment.
🚨 Command injection via git diff with unsanitized SHAs (high severity)
The git diff command uses BASE_SHA and HEAD_SHA from environment variables without validation. A malicious actor with control over these (e.g., via workflow_dispatch or repository_dispatch) could inject shell commands. Validate SHAs are 40-char hex strings before passing to subprocess.run.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| // clientIP extracts the host IP from RemoteAddr, stripping the port. | ||
| func clientIP(r *http.Request) string { |
There was a problem hiding this comment.
The clientIP function extracts IP from RemoteAddr without checking X-Forwarded-For or X-Real-IP headers. This is actually safer than trusting those headers (which can be spoofed), but in a deployment behind a reverse proxy, the rate limiter will see all requests as coming from the proxy IP. Document this limitation and recommend setting bind_addr appropriately, or add optional trusted proxy support with explicit configuration.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| return messages, total, nil | ||
| } |
There was a problem hiding this comment.
The escapeLike function escapes %, _, and \ characters for LIKE queries with ESCAPE '', which correctly prevents wildcard injection. However, the implementation should be reviewed to ensure the escape character itself () is escaped first before other characters. The current order (, %, _) is correct, but a comment explaining why the order matters would prevent future bugs.
Automated security review by Claude 4.5 Sonnet - Human review still required
| homeDir = expandPath(homeDir) | ||
| cfg.HomeDir = homeDir | ||
| cfg.Data.DataDir = homeDir | ||
| } |
There was a problem hiding this comment.
The expandPath function processes user-controlled paths (from --config and --home flags) including tilde expansion. While it validates the path starts with ~ for tilde expansion, it doesn't prevent path traversal for absolute or relative paths. An attacker with CLI access could use --config to read arbitrary TOML files. Add validation to ensure config paths are within expected directories, or document this is expected behavior for power users.
Automated security review by Claude 4.5 Sonnet - Human review still required
| module github.com/wesm/msgvault | ||
|
|
||
| go 1.25.5 | ||
| go 1.25.7 |
There was a problem hiding this comment.
🚨 Go version upgraded from 1.25.5 to 1.25.7 (high severity)
The go.mod shows an upgrade from Go 1.25.5 to 1.25.7. While this is an upgrade (not a downgrade), it's unusual that the version numbers are in the future (Go 1.25 doesn't exist as of 2024). This could indicate a typo, a custom build, or a supply chain issue. Verify the Go version is legitimate and from the official distribution.
Automated security review by Claude 4.5 Sonnet - Human review still required
| entryID, err := s.cron.AddFunc(cronExpr, func() { | ||
| s.mu.Lock() | ||
| if s.stopped || s.running[email] { | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
In AddAccount, if cron.AddFunc returns an error after the mutex is locked, the function returns without cleaning up resources. While the current code only has in-memory state, if this is extended to include other resources (e.g., database connections), cleanup would be needed. The current implementation is safe but fragile for future changes. Consider using defer for cleanup or restructuring to validate the cron expression before acquiring locks.
Automated security review by Claude 4.5 Sonnet - Human review still required
wesm
changed the title
| - name: Run security review | ||
| if: steps.check-trusted.outputs.is_trusted != 'true' | ||
| env: | ||
| ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} |
There was a problem hiding this comment.
🚨 Untrusted PR code execution with secrets access (high severity)
The workflow uses pull_request_target trigger which grants GITHUB_TOKEN and ANTHROPIC_API_KEY access, then executes security_review.py from the BASE branch. However, the script calls get_pr_diff() which runs 'git diff' on HEAD_SHA (untrusted PR code). If an attacker can inject malicious git hooks or manipulate the git environment during the diff operation, they could potentially exfiltrate secrets. Consider using the GitHub API to fetch diffs instead of git commands, or run this in a more isolated environment.
Automated security review by Claude 4.5 Sonnet - Human review still required
| def get_pr_diff() -> str: | ||
| """Get the full diff for this PR.""" | ||
| base_sha = os.environ["BASE_SHA"] | ||
| head_sha = os.environ["HEAD_SHA"] |
There was a problem hiding this comment.
🚨 Command injection via unvalidated git SHAs (high severity)
The get_pr_diff() function passes BASE_SHA and HEAD_SHA from environment variables directly to subprocess.run(['git', 'diff', f'{base_sha}...{head_sha}']) without validation. An attacker controlling PR metadata could inject shell metacharacters. While shell=False provides some protection, git itself may interpret special characters. Validate that both SHAs match the pattern ^[a-f0-9]{40}$ before passing to subprocess.
Automated security review by Claude 4.5 Sonnet - Human review still required
| } | ||
|
|
||
| func runServe(cmd *cobra.Command, args []string) error { | ||
| // Validate security posture before doing any work |
There was a problem hiding this comment.
🚨 API server starts before security validation (high severity)
The serve command calls cfg.Server.ValidateSecure() after already initializing sensitive resources (database, OAuth manager). If validation fails, these resources remain open. Move the ValidateSecure() call to line 56, immediately after the config check and before opening any resources, to ensure the server never starts in an insecure configuration.
Automated security review by Claude 4.5 Sonnet - Human review still required
| func (s *Server) authMiddleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // Skip auth if no API key configured | ||
| if s.cfg.Server.APIKey == "" { |
There was a problem hiding this comment.
The authMiddleware uses subtle.ConstantTimeCompare for API key validation (line 206), which is correct. However, the code strips the 'Bearer ' prefix with a simple string slice (line 203) before comparison. This prefix stripping happens in non-constant time. Move the Bearer stripping before the ConstantTimeCompare to ensure the entire authentication flow is timing-safe, or apply ConstantTimeCompare to the full header value.
Automated security review by Claude 4.5 Sonnet - Human review still required
There was a problem hiding this comment.
However, the code strips the 'Bearer ' prefix with a simple string slice (line 203) before comparison. This prefix stripping happens in non-constant time.
Is the claim that the comparison of s.cfg.Server.APIKey to the empty string happens in a variable amount of time?
| } | ||
|
|
||
| // batchPopulate batch-loads recipients and labels for a slice of messages. | ||
| func (s *Store) batchPopulate(messages []APIMessage, ids []int64) error { |
There was a problem hiding this comment.
The batchGetRecipients and batchGetLabels functions build dynamic SQL IN clauses by joining placeholders with strings.Join (lines 345, 384). While the function uses parameterized queries (placeholders are '?'), the number of placeholders is controlled by user input (messageIDs length). An attacker passing millions of IDs could cause resource exhaustion. Add a maximum batch size check (e.g., len(messageIDs) <= 1000) before constructing the query.
Automated security review by Claude 4.5 Sonnet - Human review still required
| return ip != nil && ip.IsLoopback() | ||
| } | ||
|
|
||
| // ValidateSecure returns an error if the server is configured insecurely |
There was a problem hiding this comment.
The IsLoopback() function checks if addr is 'localhost' (line 58) but doesn't resolve it to verify it actually points to 127.0.0.1. An attacker could set /etc/hosts to point 'localhost' to a public IP. Use net.LookupHost to resolve the hostname and verify all returned IPs are loopback addresses, or restrict BindAddr to IP addresses only and reject hostnames.
Automated security review by Claude 4.5 Sonnet - Human review still required
| module github.com/wesm/msgvault | ||
|
|
||
| go 1.25.5 | ||
| go 1.25.7 |
There was a problem hiding this comment.
🚨 Go toolchain version downgrade from 1.25.7 to 1.25.5 (high severity)
The go.mod file shows the Go version changed from go 1.25.5 to go 1.25.7 in the diff header, but this appears to be an upgrade not a downgrade. However, verify this is intentional and that 1.25.7 has no known security issues. Always review Go toolchain updates for security patches, especially for a security-sensitive email archive application.
Automated security review by Claude 4.5 Sonnet - Human review still required
… API layer 1. Gmail Thread ID (source_conversation_id) in query layer: - Add SourceConversationID to MessageSummary and MessageDetail - DuckDB joins conversations parquet; SQLite joins conversations table - Export conversations to Parquet (including Windows CSV fallback) - MCP tools now return source_conversation_id 2. Cache validation hardening: - Shared RequiredParquetDirs for cache builder, TUI, and MCP startup - HasCompleteParquetData checks all required tables, not just messages - missingRequiredParquet detects partial caches and triggers rebuild - buildCacheMu mutex prevents concurrent cache writes - maxID > 0 gate avoids rebuild loops for zero-message accounts - +482 lines of cache edge-case tests 3. Remove dead config and eliminate DTO duplication: - Remove unused server.mcp_enabled config field - Replace duplicate API structs with type aliases to store/scheduler - Collapse ~80 lines of adapter boilerplate to pass-throughs - Add backward-compat test for old configs with mcp_enabled Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
wesm
force-pushed
the
add-thread-id-to-mcp
branch
from
41cb98b to
cff1256
Compare
|
Sorry Claude made a mess, I think I fixed it now |
|
Closing in favor of a clean PR with squashed history. |
|
I'm going to revamp the security bot which is currently a big fail, sorry about the noise |
… API layer (#104) ## Summary This started as adding Gmail thread IDs to MCP responses but grew to include cache validation hardening and API cleanup that came up along the way. ### 1. Gmail Thread ID (`source_conversation_id`) in query layer - Add `SourceConversationID` field to `MessageSummary` and `MessageDetail` models - DuckDB engine joins `conversations` parquet table; SQLite engine joins `conversations` table - Export `conversations` table to Parquet in `build-cache` (including Windows CSV fallback path) - MCP tools (`list_messages`, `get_message`, `search_messages`) now return `source_conversation_id` ### 2. Cache validation hardening - Shared `RequiredParquetDirs` list used by cache builder, TUI, and MCP startup - `HasCompleteParquetData()` checks all required tables exist (not just messages) - TUI and MCP use `HasCompleteParquetData` instead of `HasParquetData` — prevents DuckDB errors when tables like `conversations` are missing - `missingRequiredParquet()` detects partial/broken caches and triggers full rebuild (handles hive-partitioned messages layout) - `buildCacheMu` mutex prevents concurrent cache writes from parallel syncs - `maxID > 0` gate avoids infinite rebuild loops for zero-message accounts - +482 lines of test coverage for cache edge cases ### 3. Remove dead config field and eliminate DTO duplication - Remove unused `server.mcp_enabled` config field (MCP is always available via `msgvault mcp`) - Replace duplicate `APIMessage`, `APIAttachment`, `StoreStats`, `AccountStatus` structs in `internal/api` with type aliases to `store`/`scheduler` types - Collapse ~80 lines of field-by-field adapter boilerplate in `serve.go` to direct pass-throughs - Add backward-compat test for old configs that still contain `mcp_enabled` ## Test plan - [x] All existing tests pass - [x] New cache tests cover: backfill missing conversations, backfill missing messages, zero-message skip, incremental no-duplicates, hive-partitioned detection - [x] MCP tests verify `source_conversation_id` in search/get/list responses - [x] Config backward-compat test for deprecated `mcp_enabled` Originally PR #76 (from @robelkin), squashed and rebased cleanly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Rob Elkin <187335+robelkin@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
@hughdbrown if you have any concerns about changes in this PR (and d24c017) please go ahead and open up a follow up PR! |
3 participants
Summary
This started as adding Gmail thread IDs to MCP responses but grew to include cache validation hardening and API cleanup that came up along the way.
1. Gmail Thread ID (
source_conversation_id) in query layerSourceConversationIDfield toMessageSummaryandMessageDetailmodelsconversationsparquet table; SQLite engine joinsconversationstableconversationstable to Parquet inbuild-cache(including Windows CSV fallback path)list_messages,get_message,search_messages) now returnsource_conversation_id2. Cache validation hardening
RequiredParquetDirslist used by cache builder, TUI, and MCP startupHasCompleteParquetData()checks all required tables exist (not just messages)HasCompleteParquetDatainstead ofHasParquetData— prevents DuckDB errors when tables likeconversationsare missingmissingRequiredParquet()detects partial/broken caches and triggers full rebuild (handles hive-partitioned messages layout)buildCacheMumutex prevents concurrent cache writes from parallel syncsmaxID > 0gate avoids infinite rebuild loops for zero-message accounts3. Remove dead config field and eliminate DTO duplication
server.mcp_enabledconfig field (MCP is always available viamsgvault mcp)APIMessage,APIAttachment,StoreStats,AccountStatusstructs ininternal/apiwith type aliases tostore/schedulertypesserve.goto direct pass-throughsmcp_enabledTest plan
source_conversation_idin search/get/list responsesmcp_enabled🤖 Generated with Claude Code