fix(security): remediation bundle — AES-256-GCM, DOMPurify, WebSocket auth, 4 CVEs, SonarCloud - #198
Conversation
… auth, 4 CVEs, SonarCloud CRITICAL-1 (#169): AES-256-GCM encryption with OS keyring, per-encryption random nonce, deterministic test key fallback. Removes zero-key/zero-IV. HIGH-4 (#177): DOMPurify sanitization via useSanitize() composable with explicit allowlist. Applied to 9 Vue v-html components. HIGH-8 (#181): Remove dead verify_client_certificate() — zero callers. MEDIUM-1 (#182): WebSocket per-message auth handler validates token on each message (defense-in-depth). Rust advisories: rand 0.8.5→0.8.7 via cargo update in both workspaces. quick-xml CVE (#165): git patch to 0.41.0 in CLI only (desktop uses trusted plist chain, not untrusted XML). SonarCloud: Fix S8786 regex ReDoS, S2137 globalThis cast, S6506 shell quoting, S6471 Docker USER ordering. Document S7637/S6505 false positives. MEDIUM-4 (#185): Replace 4 @ts-ignore with @ts-expect-error + rationale. Replace 7 as any with proper types (3 justified exceptions kept with eslint-disable for TS conditional-generic limitation). Verification: pnpm typecheck pass, pnpm lint 0 errors, pnpm test 225/226, cargo check pass (database, desktop, CLI), cargo test 9/9 (database).
- ws.get.ts: wire up socketSessions + notificationSystem after message auth - steam.ts: restore while loop for HTML comment sanitization (CodeQL) - nginx.ts: fix healthcheck return type (boolean vs Response) - db.rs: keyring panic -> ephemeral key fallback; test key panics on malformed hex - interface.rs: legacy AES-128-CTR migration fallback; preserve AEAD error details - useSanitize.ts: hoist constants, DOMPurify link hardening hook, null guard - Cargo.toml: remove unused getrandom, add aes+ctr for legacy migration - ssl.rs: blank line after deleted function - Cargo.lock: sync with new dependencies
#193: remove duplicate isomorphic-dompurify + @types/dompurify from root package.json #194: harden keyring error handling — distinguish NoEntry vs PlatformFailure, validate secret length, surface set_secret errors, bail on init failure #195: add database format magic bytes (DMS1/DMS2) for safe migration dispatch #196: document DOMPurify security rationale — allowed tags, img privacy, XSS exclusions #197: extract shared authenticatePeer helper in ws.get.ts
Both branches executed identical opts.run(message, callbacks as any). Runtime guard on line above already handles type mismatch via early return.
- package.json: remove duplicate scripts key (Biome error) - interface.rs: fix legacy decryption — use full encrypted buffer for pre-PR databases (no magic prefix on genuine legacy files) - ws.get.ts: add try-catch to open handler, log auth failures, skip re-auth on already-authenticated peers, log message errors - useSanitize.ts: hoist ALLOWED_TARGETS Set to module scope
…rdening - db.rs: zero heap-allocated secret after copy_to_slice to prevent key material leakage in memory - ws.get.ts: close connection after sending unauthenticated; clean up old listener before re-registering (identity switch leak) - useSanitize.ts: add ALLOWED_URI_REGEXP for defense-in-depth against javascript: URI scheme bypass
- useSanitize.ts: fix ALLOWED_URI_REGEXP — previous pattern broke relative markdown links (/page). Use simpler /^(?:(?:https?|ftp|mailto):|\/)/i - ws.get.ts: remove peer.close() from open handler so message handler can still receive token-based auth; restructure message handler to ignore non-token messages from authenticated peers instead of disconnecting them
The SonarQube scan uploads findings before checking quality gate. When new_coverage dropped to 0% (QC failure), the downstream sonar-pr-comment job was skipped because needs.sonar.result == 'success' was false. The API still has the scan data — just the gate failed. Changed condition to !cancelled() so the comment job runs whenever the scan job completed (success or failure), not just on success.
- database/Cargo.toml: aes-gcm '0.10' -> '0.10.3' (three-part semver) - ws.get.ts: log error.message instead of raw error object to avoid leaking stack traces in production logs
useSanitize.ts hooksRegistered flag blocks re-registration of DOMPurify hooks in HMR and test scenarios. Expose resetHooks() function to allow cleanup/reset when composable is disposed.
…e log - ws.get.ts: add 10s timeout before closing unauthenticated connections to prevent resource exhaustion DoS; add warn log for non-token messages from unauthenticated peers - useSanitize.ts: remove ftp: from ALLOWED_URI_REGEXP (unused in user-generated Markdown)
- ws.get.ts: extract AUTH_GRACE_PERIOD_MS constant (#191) - ws.get.ts: store auth timeout ref, clear on re-auth and close (#191) - useSanitize.ts: export resetHooks() — fixes unused-vars lint error - risk-register.yaml: add RISK-014/RISK-015 for quick-xml RUSTSEC-2026-0194/-0195 (transitive deps via opendal/plist, no untrusted XML input)
Runs ocr review comparing HEAD against origin/rebuild before push. Non-blocking if ocr CLI is unavailable. Lockfiles excluded from review. Requires fetchable remote base branch.
- pre-push: dynamic base branch detection via @{upstream}, mktemp log,
else branch for missing remote
- ws.get.ts: close peer on catch, env-configurable AUTH_GRACE_PERIOD_MS,
fix message error label
- db.rs: better keyring panic message explaining LazyLock behavior
- interface.rs: document legacy zero-key AES-128-CTR path
- package.json: remove redundant dompurify dep
- droplet-interface.ts: narrow as any to type field only
Remove nohup background — push now blocks until OCR finishes. Exits with OCR exit code on findings.
…abbit) - R1: Remove deprecated @types/dompurify from devDependencies - R2: Add eslint-disable-next-line for v-html with DOMPurify rationale - R3: Add logger.warn on WebSocket token auth failure - R4: Extract AES-256-GCM encrypt/decrypt helpers, deduplicate tests - R5: Memoize formatExcerpt via computed excerptCache in News.vue - R6: Replace object as never with key-narrowed typed assignment - R7: Add USER directives + hadolint disable in Docker build stages
- useSanitize.ts: removeAllHooks() before addHook to prevent HMR hook accumulation (DOMPurify hooks are additive) - news/[id]/index.vue: use block eslint-disable for v-html (disable-next-line was targeting wrong line)
Reviewer's GuideImplements AES-256-GCM database encryption with an OS-managed key, introduces DOMPurify-based HTML sanitization for all v-html usage, hardens WebSocket authentication, removes dead SSL verification code, documents accepted quick-xml risks, adjusts CI/workflows and Docker user handling, and replaces unsafe TypeScript patterns while addressing SonarCloud findings. Sequence diagram for AES-256-GCM database read/writesequenceDiagram
participant App
participant DatabaseInterface
participant ENCRYPTION_KEY
participant KeyringEntry
participant Filesystem
App->>DatabaseInterface: create_at_path(db_path, database)
DatabaseInterface->>ENCRYPTION_KEY: encryption_key_impl()
ENCRYPTION_KEY->>KeyringEntry: get_secret()
alt no existing key
KeyringEntry->>KeyringEntry: set_secret(buffer)
end
ENCRYPTION_KEY-->>DatabaseInterface: u8_32_byte_array
DatabaseInterface->>DatabaseInterface: encrypt_database(ENCRYPTION_KEY, plaintext)
DatabaseInterface->>Filesystem: write(db_path, encrypted)
App->>DatabaseInterface: get_from_path(db_path)
DatabaseInterface->>Filesystem: read(db_path)
Filesystem-->>DatabaseInterface: encrypted
DatabaseInterface->>DatabaseInterface: [check MAGIC_V2 or MAGIC_V1]
alt MAGIC_V2
DatabaseInterface->>DatabaseInterface: decrypt_database(ENCRYPTION_KEY, payload)
else legacy format
DatabaseInterface->>DatabaseInterface: Aes128Ctr64LE::new(legacy_key, legacy_iv)
DatabaseInterface->>DatabaseInterface: apply_keystream(&mut legacy_data)
end
DatabaseInterface-->>App: DatabaseInterface instance
Sequence diagram for WebSocket authentication with per-message tokensequenceDiagram
actor Client
participant WebSocketHandler
participant authenticatePeer
participant aclManager
participant notificationSystem
Client->>WebSocketHandler: open(peer)
WebSocketHandler->>WebSocketHandler: pendingAuth.add(peer.id)
WebSocketHandler->>authenticatePeer: authenticatePeer(peer, headers)
authenticatePeer->>aclManager: getUserIdACL(h3, [notifications:listen])
aclManager-->>authenticatePeer: userId or null
alt userId and ACLs
authenticatePeer->>aclManager: fetchAllACLs(h3)
authenticatePeer->>notificationSystem: listen(userId, acls, peer.id, callback)
authenticatePeer-->>WebSocketHandler: true
else auth failed
authenticatePeer-->>WebSocketHandler: false
WebSocketHandler->>Client: send unauthenticated
WebSocketHandler->>WebSocketHandler: setTimeout(peer.close, AUTH_GRACE_PERIOD_MS)
end
WebSocketHandler->>WebSocketHandler: pendingAuth.delete(peer.id)
Client->>WebSocketHandler: message(peer, msg)
alt msg contains token
WebSocketHandler->>authenticatePeer: authenticatePeer(peer, headers[Authorization])
alt token auth ok
authenticatePeer-->>WebSocketHandler: true
WebSocketHandler->>WebSocketHandler: clearTimeout(authTimeouts.get(peer.id))
else token auth failed
WebSocketHandler->>Client: send unauthenticated
WebSocketHandler->>Client: close()
end
else non-token message
alt peer authenticated
WebSocketHandler->>WebSocketHandler: ignore message
else peer unauthenticated
WebSocketHandler->>Client: send unauthenticated
WebSocketHandler->>Client: close()
end
end
Client-->>WebSocketHandler: close(peer)
WebSocketHandler->>notificationSystem: unlisten(userId, peer.id)
WebSocketHandler->>notificationSystem: unlisten(system, peer.id)
Flow diagram for DOMPurify-based HTML sanitizationflowchart TD
A[Markdown input] --> B[micromark]
B --> C[HTML string]
C --> D[useSanitize]
D --> E[sanitize]
E --> F[v-html render in Vue components]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds AES-256-GCM database encryption, sanitizes Markdown HTML, supports WebSocket token re-authentication, updates CI and local review tooling, pins quick-xml, removes certificate verification, and replaces unsafe TypeScript casts and suppressions. ChangesSecurity and runtime hardening
Build and release tooling
Typing and service maintenance
Developer guidance and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WebSocketPeer
participant authenticatePeer
participant NotificationSessions
WebSocketPeer->>authenticatePeer: Authenticate on open or token message
authenticatePeer->>NotificationSessions: Register notification listener
authenticatePeer-->>WebSocketPeer: Return authentication result
WebSocketPeer->>WebSocketPeer: Send unauthenticated or clear grace timeout
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
database/src/interface.rs,decrypt_databaseassumes the input still includes the 4-byte magic prefix (payload = &encrypted[4..]), butload_from_pathpassespayload = &encrypted[4..]into it for v2, meaning you effectively skip 8 bytes (4 magic + first 4 bytes of the nonce); either pass the full buffer todecrypt_databaseor remove the internal[..4]slice there. - The
useSanitizecomposable callsDOMPurify.removeAllHooks()insideregisterHooks, which clears all global hooks for the shared isomorphic-dompurify instance; if any other module registers DOMPurify hooks, they will be silently removed—consider scoping sanitization to a dedicated DOMPurify instance or avoidingremoveAllHooksto prevent surprising cross-module side effects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `database/src/interface.rs`, `decrypt_database` assumes the input still includes the 4-byte magic prefix (`payload = &encrypted[4..]`), but `load_from_path` passes `payload = &encrypted[4..]` into it for v2, meaning you effectively skip 8 bytes (4 magic + first 4 bytes of the nonce); either pass the full buffer to `decrypt_database` or remove the internal `[..4]` slice there.
- The `useSanitize` composable calls `DOMPurify.removeAllHooks()` inside `registerHooks`, which clears all global hooks for the shared isomorphic-dompurify instance; if any other module registers DOMPurify hooks, they will be silently removed—consider scoping sanitization to a dedicated DOMPurify instance or avoiding `removeAllHooks` to prevent surprising cross-module side effects.
## Individual Comments
### Comment 1
<location path="desktop/src-tauri/database/src/interface.rs" line_range="51-57" />
<code_context>
+}
+
+/// Decrypt data produced by `encrypt_database`.
+fn decrypt_database(key: &[u8; 32], encrypted: &[u8]) -> Result<Vec<u8>, anyhow::Error> {
+ let key_slice = Key::<Aes256Gcm>::from_slice(key);
+ let cipher = Aes256Gcm::new(key_slice);
+ let payload = &encrypted[4..];
+ let (nonce_bytes, ciphertext) = payload.split_at(12);
+ let nonce = Nonce::from_slice(nonce_bytes);
+ cipher
+ .decrypt(nonce, ciphertext)
+ .map_err(|e| anyhow::anyhow!("decryption failed: {e}"))
</code_context>
<issue_to_address>
**issue (bug_risk):** decrypt_database slices past the first 4 bytes, but callers already strip the magic prefix, causing incorrect offsets and potential panics
`decrypt_database` currently assumes `encrypted` includes the 4-byte magic prefix and does `let payload = &encrypted[4..];`. In `set_up_database` (V2 path), the argument passed is already `&encrypted[4..]`, so another 4 bytes are skipped. This can cause an out-of-bounds slice on small inputs and incorrect nonce/ciphertext offsets on larger ones.
Please either (a) define `decrypt_database` to always take the full buffer (magic + nonce + ciphertext) and perform all slicing internally, or (b) define it to take a slice starting at the nonce and remove the extra `[..4]` offset, adding length checks to enforce the contract. Aligning the call site and function contract will avoid these panics and mis-decryptions.
</issue_to_address>
### Comment 2
<location path="server/composables/useSanitize.ts" line_range="64-71" />
<code_context>
+ }
+ hooksRegistered = true;
+
+ DOMPurify.removeAllHooks();
+
+ DOMPurify.addHook("afterSanitizeAttributes", (node) => {
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Removing all DOMPurify hooks globally may interfere with other sanitization consumers
`DOMPurify.removeAllHooks()` mutates the global instance, wiping out hooks registered elsewhere and potentially weakening sanitization in other parts of the app.
If this is currently the only place configuring hooks, it works for now but is fragile. Consider either:
- Avoiding `removeAllHooks` and making hook registration idempotent, or
- Centralizing DOMPurify setup (e.g., in a Nuxt plugin) so there’s a single, well-defined configuration point.
This keeps sanitization behavior consistent and safer as the app evolves.
```suggestion
if (hooksRegistered) {
return;
}
hooksRegistered = true;
DOMPurify.addHook("afterSanitizeAttributes", (node) => {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SonarCloud Analysis ✅No BLOCKER, CRITICAL, or MAJOR issues found. |
|
🔍 OpenCodeReview found 18 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| log "No unresolved issues — building coverage-only comment" | ||
| COMMENT_BODY="## SonarCloud Analysis ✅\n\nNo BLOCKER, CRITICAL, or MAJOR issues found.\n\n" | ||
| else | ||
| COMMENT_BODY="## SonarCloud Analysis\n\n" |
There was a problem hiding this comment.
When TOTAL=0 (no issues found), the COMMENT_BODY is assigned here on line 88, and then the else branch containing the original "Step 4: Build PR comment" section (lines 147-201) is skipped. However, the COMMENT_BODY variable is overwritten on line 147 when it was previously set. Wait — actually with the revised control flow, when TOTAL=0, the script sets COMMENT_BODY on line 88 and jumps to line 203 (fi). Lines 147-201 are inside the else branch (line 89-202). So COMMENT_BODY is set only once. This is actually correct. Let me reconsider...
Actually, looking at the diff again: The original code had COMMENT_BODY being set twice when TOTAL > 0 — once on line 90 (new) and again on line 147 (old code). Wait, lines 86-108 show the new if/else structure:
if [[ "$TOTAL" -eq 0 ]]; then
log "No unresolved issues — building coverage-only comment"
COMMENT_BODY="## SonarCloud Analysis ✅\n\nNo BLOCKER, CRITICAL, or MAJOR issues found.\n\n"
else
COMMENT_BODY="## SonarCloud Analysis\n\n"But the old code at line 147 (COMMENT_BODY="## SonarCloud Analysis\n\n") was part of the original Step 4 section. Now with the restructured if/else, when TOTAL>0, line 90 sets COMMENT_BODY="## SonarCloud Analysis\n\n" and then lines 147-201 (which are inside the else block due to the fi at line 202) set it again with the same value. This is redundant but not a bug — the second assignment just overwrites with the same value.
| uncovered: (((.measures[]? | select(.metric == "new_uncovered_lines") | .periods[0].value // .value) // "0") | tonumber), | ||
| coverage: (((.measures[]? | select(.metric == "new_coverage") | .periods[0].value // .value)) // "0.0") | ||
| } | ||
| | select(.uncovered > 0) |
There was a problem hiding this comment.
The script only hooks EXIT but not INT or TERM. When the script is cancelled (e.g., Ctrl+C or kill), EXIT still fires in bash, so in practice the cleanup runs. However, kill 0 in the trap won't work for EXIT alone (it would kill the script itself). Adding INT and TERM to the trap with kill 0 ensures background processes are properly terminated when the script is interrupted, preventing orphaned processes.
Suggestion:
| | select(.uncovered > 0) | |
| trap 'rm -rf "$TEMP_DIR"; kill 0' EXIT INT TERM |
| try { | ||
| const object = await init(); | ||
| if (!object) break; | ||
| this.authProviders[key as keyof typeof this.authProviders] = object as never; |
There was a problem hiding this comment.
Using as never to bypass TypeScript type safety is as problematic as using any. The return type of init() is Promise<unknown>, but authProviders expects boolean or OIDCManager | undefined. This cast suppresses all type checks and could lead to runtime type errors. Consider using proper type narrowing or a mapped type to ensure type safety.
Suggestion:
| this.authProviders[key as keyof typeof this.authProviders] = object as never; | |
| // Use a proper type assertion with validation, or restructure the types | |
| this.authProviders[key as keyof typeof this.authProviders] = object as any; |
| await drainPendingAuthBuffer(peer); | ||
| } | ||
| async message(peer, msg) { | ||
| await processMessage(peer, msg); | ||
| await drainPendingAuthBuffer(peer); | ||
| return; | ||
| } | ||
| await processMessage(peer, msg); | ||
| while (pendingAuthMessageBuffer.has(peer.id)) { | ||
| await drainPendingAuthBuffer(peer); |
There was a problem hiding this comment.
Syntax error: The message handler has duplicate processMessage and drainPendingAuthBuffer calls, an orphaned return; statement, and an unmatched closing brace }. This will cause runtime errors or unexpected behavior.
Suggestion:
| await drainPendingAuthBuffer(peer); | |
| } | |
| async message(peer, msg) { | |
| await processMessage(peer, msg); | |
| await drainPendingAuthBuffer(peer); | |
| return; | |
| } | |
| await processMessage(peer, msg); | |
| while (pendingAuthMessageBuffer.has(peer.id)) { | |
| await drainPendingAuthBuffer(peer); | |
| async message(peer, msg) { | |
| await processMessage(peer, msg); | |
| await drainPendingAuthBuffer(peer); | |
| }, |
| ); | ||
| rejectPeer(peer); | ||
| return; | ||
| } | ||
| // Skip re-authentication if peer is already authenticated |
There was a problem hiding this comment.
Syntax error: Orphaned code in processMessage function. Lines );, rejectPeer(peer);, return;, and } on lines 119-122 don't correspond to any valid block structure and will cause a parse error. These appear to be leftover code from a bad merge or copy-paste mistake.
Suggestion:
| ); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| // Skip re-authentication if peer is already authenticated | |
| // Skip re-authentication if peer is already authenticated |
| peer.send("unauthenticated"); | ||
| peer.close(); |
There was a problem hiding this comment.
Empty catch block silently swallows all errors. If an exception occurs during authentication or any part of the open handler, it will be completely hidden, making debugging extremely difficult.
Suggestion:
| peer.send("unauthenticated"); | |
| peer.close(); | |
| } catch (error) { | |
| logger.warn({ error: (error as Error)?.message }, "WebSocket open handler error"); | |
| } finally { |
| return; | ||
| } | ||
| // Token auth failed — close connection | ||
| logger.warn(`WebSocket token auth failed for peer ${peer.id}`); |
There was a problem hiding this comment.
Inconsistent logging format: The code mixes structured logger calls (e.g., logger.warn({ peerId: peer.id }, "...")) with string interpolation (e.g., logger.warn(`WebSocket auth failed for peer ${peer.id}`)). Using string interpolation bypasses structured logging and may break log aggregation, parsing, and filtering systems.
Suggestion:
| logger.warn(`WebSocket token auth failed for peer ${peer.id}`); | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth failed"); |
| elif $l == .current[1] + 1 then | ||
| {ranges: .ranges[:-1] + [[.current[0], $l]], current: [.current[0], $l]} |
There was a problem hiding this comment.
Inconsistent indentation: These lines start at column 0 while they should be indented to match the surrounding context inside the for loop (which uses 2-space indentation).
Suggestion:
| elif $l == .current[1] + 1 then | |
| {ranges: .ranges[:-1] + [[.current[0], $l]], current: [.current[0], $l]} | |
| SAFE_PATH=$(echo "$FILE_PATH" | sed 's/|/\|/g; s/`/`/g') | |
| COMMENT_BODY+="| \`${SAFE_PATH}\` | ${FILE_COV}% | ${FILE_UNC} | ${LINE_RANGES} |\n" |
| if (msgData.token.length === 0) { | ||
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is empty"); | ||
| rejectPeer(peer); | ||
| return; | ||
| } | ||
| ); | ||
| rejectPeer(peer); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Syntax Error — Orphaned code after token length check.
Lines 119–122 ( );, rejectPeer(peer);, return;, }) are stranded outside any control-flow block. The ); has no matching opening parenthesis and the rejectPeer/return statements will cause a syntax error (or silently break control flow if somehow parsed). This appears to be a copy-paste artifact from an incomplete refactor.
Fix: Remove the orphaned lines 119–122 and ensure the if (typeof msgData.token !== "string") and if (msgData.token.length === 0) blocks are properly closed before the token authentication logic.
Suggestion:
| if (msgData.token.length === 0) { | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is empty"); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| ); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| if (msgData.token.length === 0) { | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is empty"); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| // Skip re-authentication if peer is already authenticated | |
| if (socketSessions.has(peer.id)) return; |
| if (authenticated) { | ||
| // Clear the pending auth timeout — peer successfully re-authenticated | ||
| const timeoutId = authTimeouts.get(peer.id); | ||
| if (timeoutId) { | ||
| logger.warn({ peerId: peer.id }, "WebSocket token auth failed"); | ||
| authTimeouts.delete(peer.id); | ||
| } |
There was a problem hiding this comment.
Misleading log message in success path.
Line 136 logs "WebSocket token auth failed" inside the if (authenticated) branch — i.e., when authentication succeeded. This is backwards and would pollute logs with false warnings.
Suggestion:
| if (authenticated) { | |
| // Clear the pending auth timeout — peer successfully re-authenticated | |
| const timeoutId = authTimeouts.get(peer.id); | |
| if (timeoutId) { | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth failed"); | |
| authTimeouts.delete(peer.id); | |
| } | |
| if (authenticated) { | |
| // Clear the pending auth timeout — peer successfully re-authenticated | |
| const timeoutId = authTimeouts.get(peer.id); | |
| if (timeoutId) { | |
| authTimeouts.delete(peer.id); | |
| } |
| peer.send("unauthenticated"); | ||
| peer.close(); | ||
| } finally { | ||
| pendingAuth.delete(peer.id); | ||
| await drainPendingAuthBuffer(peer); |
There was a problem hiding this comment.
Empty catch block in open handler.
Lines 203–204 contain } catch (error) { } which swallows all exceptions thrown during authentication or cleanup. This can leave the WebSocket peer in an inconsistent state (e.g., pendingAuth not removed, buffer not drained, peer left open indefinitely).
Suggestion:
| peer.send("unauthenticated"); | |
| peer.close(); | |
| } finally { | |
| pendingAuth.delete(peer.id); | |
| await drainPendingAuthBuffer(peer); | |
| } catch (error) { | |
| logger.error({ error: (error as Error)?.message, peerId: peer.id }, "WebSocket open handler error"); | |
| } finally { | |
| pendingAuth.delete(peer.id); | |
| await drainPendingAuthBuffer(peer); | |
| } |
| peer.close(); | ||
| } finally { | ||
| pendingAuth.delete(peer.id); | ||
| await drainPendingAuthBuffer(peer); | ||
| } | ||
| } finally { | ||
| } finally { | ||
| // Re-add to pendingAuth before draining to prevent concurrent message processing | ||
| while (pendingAuthMessageBuffer.has(peer.id)) { | ||
| await drainPendingAuthBuffer(peer); | ||
| if (pendingAuthMessageBuffer.has(peer.id)) { | ||
| await drainPendingAuthBuffer(peer); |
There was a problem hiding this comment.
Malformed open handler — orphaned } finally { blocks.
After the first } finally { (line 204–207), the code continues with two more } finally { blocks (lines 208–209) and a while loop (lines 211–213) that are structurally orphaned. The brace matching is broken, and the while loop at line 211 re-deletes pendingAuth which was already removed at line 205. This is clearly incomplete refactoring and will cause a syntax/parse error.
| // Re-add to pendingAuth before draining to prevent concurrent message processing | ||
| while (pendingAuthMessageBuffer.has(peer.id)) { | ||
| await drainPendingAuthBuffer(peer); |
There was a problem hiding this comment.
Potential infinite loop from while on buffer without backoff.
The while (pendingAuthMessageBuffer.has(peer.id)) loop (lines 211–213 and 222–224) continuously drains the buffer. Since drainPendingAuthBuffer deletes the buffer entry with pendingAuthMessageBuffer.delete(peer.id) and processes each message, if a message handler re-adds entries to the buffer during processing, this loop could spin indefinitely, blocking the event loop.
Suggestion:
| // Re-add to pendingAuth before draining to prevent concurrent message processing | |
| while (pendingAuthMessageBuffer.has(peer.id)) { | |
| await drainPendingAuthBuffer(peer); | |
| let iterations = 0; | |
| while (pendingAuthMessageBuffer.has(peer.id) && iterations < MAX_BUFFERED_MSGS) { | |
| await drainPendingAuthBuffer(peer); | |
| iterations++; | |
| } |
| # The scan uploads findings to SonarCloud API before quality gate check. | ||
| # Run even when quality gate fails — the API still has data to comment. | ||
| # always() overrides needs dependency failure; !cancelled() alone does not. | ||
| if: github.event_name == 'pull_request' && always() && !cancelled() |
There was a problem hiding this comment.
always() causes this job to run even when the sonar job is skipped (not just failed). If the sonar scan was never executed, the script may still post a "No issues found" comment on the PR, which is misleading since no actual analysis occurred. Consider narrowing the condition to only run when sonar succeeded or failed, but not when skipped.
Suggestion:
| if: github.event_name == 'pull_request' && always() && !cancelled() | |
| if: github.event_name == 'pull_request' && !cancelled() && (needs.sonar.result == 'success' || needs.sonar.result == 'failure') |
| if command -v fallow >/dev/null 2>&1; then | ||
| FALLOW_JSON=$(FALLOW_AUDIT_BASE="${REMOTE_BASE}" fallow audit --format json --quiet --explain --gate-marker agent 2>/dev/null || echo '{"verdict":"error","error":true}') | ||
| if command -v jq >/dev/null 2>&1; then |
There was a problem hiding this comment.
Inconsistent indentation on line 17: the if command -v jq line lacks the expected 2-space indentation that matches the surrounding nested block structure. While this won't cause functional errors in bash, it harms readability and maintainability.
Suggestion:
| if command -v fallow >/dev/null 2>&1; then | |
| FALLOW_JSON=$(FALLOW_AUDIT_BASE="${REMOTE_BASE}" fallow audit --format json --quiet --explain --gate-marker agent 2>/dev/null || echo '{"verdict":"error","error":true}') | |
| if command -v jq >/dev/null 2>&1; then | |
| if command -v jq >/dev/null 2>&1; then |
| if payload.len() < 28 { | ||
| anyhow::bail!("V2 payload too short (min 28 bytes: 12 nonce + 16 GCM)"); | ||
| } |
There was a problem hiding this comment.
The payload.len() < 28 check (lines 153–155) duplicates the identical check already present inside decrypt_database (function body). Both checks validate the same invariant with the same minimum (28 bytes), but use slightly different error messages. This duplication creates a maintenance hazard: if the minimum payload length ever changes (e.g., due to a different AEAD algorithm or nonce size), both checks must be updated in sync. Consider removing the check here and letting decrypt_database handle the validation, or consolidating the constant into a shared named constant (e.g., MIN_GCM_PAYLOAD_LEN).
| if command -v jq >/dev/null 2>&1; then | ||
| FALLOW_VERDICT=$(echo "${FALLOW_JSON}" | jq -r '.verdict // "error"' 2>/dev/null || echo "error") |
There was a problem hiding this comment.
Missing indentation: This if statement is nested inside the if command -v fallow block (line 15) but starts at column 0, which obscures the control flow structure. All lines inside the outer if...then block should be consistently indented (e.g., 2 or 4 spaces).
Suggestion:
| if command -v jq >/dev/null 2>&1; then | |
| FALLOW_VERDICT=$(echo "${FALLOW_JSON}" | jq -r '.verdict // "error"' 2>/dev/null || echo "error") | |
| if command -v jq >/dev/null 2>&1; then | |
| FALLOW_VERDICT=$(echo "${FALLOW_JSON}" | jq -r '.verdict // "error"' 2>/dev/null || echo "error") |
| if command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then | ||
| ... | ||
| else | ||
| printf ':: gh or jq not found — skipping review thread check\n' >&2 | ||
| fi |
There was a problem hiding this comment.
Redundant condition check & incomplete implementation: The inner if command -v gh check (line 43) is redundant — the outer if on line 39 already verified that both gh and jq are available. The ... placeholder indicates incomplete logic. This entire nested block inside the if [ -z "${REPO}" ] branch also lacks a clear purpose (e.g., counting open review threads via gh api).
Suggestion:
| if command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then | |
| ... | |
| else | |
| printf ':: gh or jq not found — skipping review thread check\n' >&2 | |
| fi | |
| # TODO: Implement review thread check logic, e.g.: | |
| # gh api "repos/${REPO}/pulls/$(gh pr view --json number --jq '.number')/reviews" ... |
| fn encryption_key_impl() -> [u8; 32] { | ||
| let entry = keyring::Entry::new("drop", "database_key").expect("failed to open keyring"); |
There was a problem hiding this comment.
The keyring::Entry::new() call uses .expect(), which panics on any keyring initialization failure (e.g., no keyring daemon running, permission denied, or platform incompatibility). Since this function is called inside a LazyLock, a panic here is permanently cached — the process can never recover, even if the keyring service becomes available later. This is a denial-of-service vector and a reliability concern, especially on headless environments or containerized deployments where keyring may not be available at startup.
Consider making encryption_key_impl return Result<[u8; 32], Error> and either: (1) fall back to a file-based key store, or (2) propagate the error to a higher level that can retry or inform the user gracefully.
Suggestion:
| fn encryption_key_impl() -> [u8; 32] { | |
| let entry = keyring::Entry::new("drop", "database_key").expect("failed to open keyring"); | |
| fn encryption_key_impl() -> Result<[u8; 32], anyhow::Error> { | |
| let entry = keyring::Entry::new("drop", "database_key")?; | |
| ... // propagate errors instead of panicking | |
| } |
| # Stash metadata for ordered processing after parallel fetch | ||
| printf '%s|%s|%s|%s\n' "${FILE_KEY}" "${FILE_PATH}" "${FILE_COV}" "${FILE_UNC}" > "${TEMP_DIR}/meta_${file_index}" | ||
|
|
||
| # Fetch line-level data in background — all files run concurrently | ||
| { | ||
| # Consider documenting in the coverage table when file lines > SONAR_MAX_LINES | ||
| # e.g., by comparing the returned line count against SONAR_MAX_LINES |
There was a problem hiding this comment.
Background job failures are not tracked — The script uses wait without capturing job PIDs. While the curl commands have a fallback (2>/dev/null || echo '{"sources":[]}') that mitigates complete failure, set -e does not propagate to background processes. If a background job is killed abruptly or produces incomplete output, the script will silently process stale or malformed data. Consider tracking individual job PIDs and checking exit statuses after wait.
Suggestion:
| # Stash metadata for ordered processing after parallel fetch | |
| printf '%s|%s|%s|%s\n' "${FILE_KEY}" "${FILE_PATH}" "${FILE_COV}" "${FILE_UNC}" > "${TEMP_DIR}/meta_${file_index}" | |
| # Fetch line-level data in background — all files run concurrently | |
| { | |
| # Consider documenting in the coverage table when file lines > SONAR_MAX_LINES | |
| # e.g., by comparing the returned line count against SONAR_MAX_LINES | |
| } > "${TEMP_DIR}/lines_${file_index}" & | |
| pids["${file_index}"]=$! | |
| file_index=$((file_index + 1)) | |
| done < <(echo "$UNCOVERED_FILES" | jq -c '.[]') | |
| # Wait for all background fetches to complete | |
| for pid in "${pids[@]}"; do | |
| wait "$pid" || log "Warning: background fetch for file index failed" | |
| done |
| if .current == null then | ||
| {ranges: [[$l, $l]], current: [$l, $l]} | ||
| elif $l == .current[1] + 1 then | ||
| {ranges: .ranges[:-1] + [[.current[0], $l]], current: [.current[0], $l]} |
There was a problem hiding this comment.
Insufficient markdown escaping for table cell content — Even if the backtick escaping is fixed, file paths may contain other markdown special characters (underscore _, asterisk *, square brackets [], parentheses ()) that could break markdown table rendering or change formatting in the PR comment. These characters can appear in file paths and would not be properly escaped by the current approach.
Suggestion:
| {ranges: .ranges[:-1] + [[.current[0], $l]], current: [.current[0], $l]} | |
| SAFE_PATH=$(echo "$FILE_PATH" | sed -e 's/[|\\`*_\[\]()]/\\&/g' -e 's/\$/\\$/g') |
| while read -r file_entry; do | ||
| FILE_KEY=$(echo "$file_entry" | jq -r '.key') | ||
| FILE_PATH=$(echo "$file_entry" | jq -r '.path') | ||
| FILE_COV=$(echo "$file_entry" | jq -r '.coverage') | ||
| FILE_UNC=$(echo "$file_entry" | jq -r '.uncovered') | ||
|
|
There was a problem hiding this comment.
ENCODED_KEY assignment runs on every background invocation but has no error handling — If jq -sRr @uri fails (e.g., encoding edge case with special characters in FILE_KEY), ENCODED_KEY will be empty, causing a malformed API request URL. Since this runs inside the { ... } block in the background, the failure is silent and the file will contain invalid data. Consider validating the encoding or adding a guard.
Suggestion:
| while read -r file_entry; do | |
| FILE_KEY=$(echo "$file_entry" | jq -r '.key') | |
| FILE_PATH=$(echo "$file_entry" | jq -r '.path') | |
| FILE_COV=$(echo "$file_entry" | jq -r '.coverage') | |
| FILE_UNC=$(echo "$file_entry" | jq -r '.uncovered') | |
| ENCODED_KEY=$(printf '%s' "$FILE_KEY" | jq -sRr @uri) || ENCODED_KEY='' | |
| if [[ -z "$ENCODED_KEY" ]]; then | |
| echo '{"sources":[]}' > "${TEMP_DIR}/lines_${file_index}" | |
| else | |
| curl ... | |
| fi |
| logger.warn({ peerId: peer.id }, "WebSocket auth failed"); | ||
| const authenticated = await authenticatePeer( | ||
| peer, | ||
| peer.request?.headers ?? new Headers(), | ||
| ); | ||
| if (!authenticated) { | ||
| logger.warn(`WebSocket auth failed for peer ${peer.id}`); |
There was a problem hiding this comment.
Inconsistent logging style. Some log statements use structured logging (logger.warn({ peerId }, "message")) while others use string interpolation (logger.warn(WebSocket auth failed for peer ${peer.id})). This inconsistency reduces readability and may confuse log aggregation tools.
Suggestion:
| logger.warn({ peerId: peer.id }, "WebSocket auth failed"); | |
| const authenticated = await authenticatePeer( | |
| peer, | |
| peer.request?.headers ?? new Headers(), | |
| ); | |
| if (!authenticated) { | |
| logger.warn(`WebSocket auth failed for peer ${peer.id}`); | |
| logger.warn({ peerId: peer.id }, "WebSocket auth failed"); | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth failed"); |
| # always() overrides needs dependency failure; !cancelled() alone does not. | ||
| if: github.event_name == 'pull_request' && always() && !cancelled() |
There was a problem hiding this comment.
The condition change looks reasonable and the inline comments explain the motivation clearly. However, there is a subtle edge case to consider:
If the sonar job fails before the SonarCloud scan step (e.g., dependency installation fails at line 217-218 or 227), the scan never uploads any findings to the SonarCloud API. The always() expression will still trigger this job and the script will attempt to query the API. While the script has been improved to gracefully handle zero findings, it may still post a potentially misleading "No issues found" comment when in reality no scan ever ran.
Suggestion: Consider adding a guard in the script to detect whether the scan actually ran — e.g., by checking the sonar-scan step outcome if feasible, or by querying the API for a baseline check before posting a "clean" comment.
Suggestion:
| # always() overrides needs dependency failure; !cancelled() alone does not. | |
| if: github.event_name == 'pull_request' && always() && !cancelled() | |
| if: github.event_name == 'pull_request' && always() && !cancelled() |
| FILE_KEY=$(echo "$file_entry" | jq -r '.key') | ||
| FILE_PATH=$(echo "$file_entry" | jq -r '.path') |
There was a problem hiding this comment.
This comment indicates an intention to handle the case where a file's line count exceeds SONAR_MAX_LINES (e.g., by noting truncation in the coverage table), but no implementation is provided. This could be misleading if someone later sees the comment assumes the handling exists. Consider either implementing the check or removing/rewording the comment to avoid confusion.
| ); | ||
| rejectPeer(peer); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Syntax error: There is a stray ); on this line that does not correspond to any opening parenthesis or brace. This will cause a JavaScript parse error, preventing the entire module from loading. It appears to be leftover debris from a refactoring.
Suggestion:
| ); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| // Remove the stray line entirely. The token checks above already handle rejection. | |
| // After the token length check, the code should continue to the token auth flow: | |
| // Skip re-authentication if peer is already authenticated | |
| if (socketSessions.has(peer.id)) return; |
| const msgData = data as Record<string, unknown>; | ||
| if (typeof msgData.token !== "string") { | ||
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is not a string"); | ||
| rejectPeer(peer); | ||
| return; | ||
| } | ||
| if (msgData.token.length === 0) { | ||
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is empty"); | ||
| rejectPeer(peer); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing message type guard in processMessage. The code directly checks msgData.token without verifying the message's type field first (e.g., msgData.type === "token"). This means any message from an unauthenticated peer that happens to have a token property will be treated as an auth attempt, while any message without a token property is unconditionally rejected. The indentation also suggests a missing conditional wrapper here.
Suggestion:
| const msgData = data as Record<string, unknown>; | |
| if (typeof msgData.token !== "string") { | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is not a string"); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| if (msgData.token.length === 0) { | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is empty"); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| const msgData = data as Record<string, unknown>; | |
| // Only process token auth messages | |
| if (msgData.type !== "token") { | |
| logger.warn({ peerId: peer.id }, "Closing unauthenticated WebSocket: non-token message before auth"); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| if (typeof msgData.token !== "string") { | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is not a string"); | |
| rejectPeer(peer); | |
| return; | |
| } | |
| if (msgData.token.length === 0) { | |
| logger.warn({ peerId: peer.id }, "WebSocket token auth: token is empty"); | |
| rejectPeer(peer); | |
| return; | |
| } |
| # Re-stage formatted files so they're included in the commit | ||
| git add --update -- $changed_rs |
There was a problem hiding this comment.
There's a subtle risk here: git add --update will stage the working tree version of the files. If a developer had additional unstaged changes in their working tree for any of the .rs files that were originally staged (e.g., git add -p was used to stage only part of a file, or there are unrelated modifications in the working tree), those unintended changes would also be pulled into the commit after formatting.
Consider using git add without --update to only stage the modifications made by cargo fmt, or better yet, use git diff --cached to detect changes after formatting and only add those specific changes.
Suggestion (optional, based on caution level):
# After formatting, only stage the formatting-induced changes
cargo fmt_result=$(git diff --cached --name-only -- '*.rs')
git add --update -- $cargo_fmt_resultThis ensures only files that were actually modified by formatters get re-staged, reducing the risk of committing unintended working-tree changes.
| # fallow audit gate: block on 'fail' verdict, tolerate JSON parse errors. | ||
| if command -v fallow >/dev/null 2>&1; then | ||
| FALLOW_JSON=$(FALLOW_AUDIT_BASE="${REMOTE_BASE}" fallow audit --format json --quiet --explain --gate-marker agent 2>/dev/null || echo '{"verdict":"error","error":true}') | ||
| if command -v jq >/dev/null 2>&1; then |
There was a problem hiding this comment.
Inconsistent indentation: if command -v jq on line 17 is at the same indentation level as the outer if command -v fallow on line 15. Although this works syntactically in shell, it visually suggests the nested if is at the same nesting level as the outer one, harming readability. It should be indented with 2 spaces to match the surrounding FALLOW_JSON assignment.
Suggestion:
| if command -v jq >/dev/null 2>&1; then | |
| if command -v jq >/dev/null 2>&1; then |
| if command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then | ||
| ... | ||
| else |
There was a problem hiding this comment.
Placeholder code ... on line 44 indicates this section is incomplete. The PR review thread check has no real implementation yet — the ... placeholder will cause a command-not-found error at runtime. Either implement the actual logic or remove this entire section to avoid confusion.
Suggestion:
| if command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then | |
| ... | |
| else | |
| if command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then | |
| # TODO: implement PR review thread check logic here | |
| else |
| curl -sS -H "Authorization: Bearer ${SONAR_TOKEN}" \ | ||
| "https://sonarcloud.io/api/sources/lines?key=${ENCODED_KEY}&from=1&to=${SONAR_MAX_LINES}&pullRequest=${GITHUB_PR_NUMBER}" \ | ||
| 2>/dev/null || echo '{"sources":[]}' |
There was a problem hiding this comment.
Undefined variable ENCODED_KEY — ${ENCODED_KEY} is used in the curl URL to fetch line-level data (line 268) but is never assigned anywhere in the script. This will cause the curl request to use an empty key, resulting in a failed API call and an empty coverage table. The intended variable is likely FILE_KEY, which is extracted from the SonarCloud response on line 255, but it should be URL-encoded before being inserted into the URL.
Suggestion:
| curl -sS -H "Authorization: Bearer ${SONAR_TOKEN}" \ | |
| "https://sonarcloud.io/api/sources/lines?key=${ENCODED_KEY}&from=1&to=${SONAR_MAX_LINES}&pullRequest=${GITHUB_PR_NUMBER}" \ | |
| 2>/dev/null || echo '{"sources":[]}' | |
| # URL-encode FILE_KEY before use | |
| ENCODED_KEY=$(python3 -c "import urllib.parse; print(urllib.parse.quote('${FILE_KEY}'))" 2>/dev/null || echo "${FILE_KEY}") | |
| curl -sS -H "Authorization: Bearer ${SONAR_TOKEN}" \ | |
| "https://sonarcloud.io/api/sources/lines?key=${ENCODED_KEY}&from=1&to=${SONAR_MAX_LINES}&pullRequest=${GITHUB_PR_NUMBER}" |
| if [[ "$TOTAL_COMPONENTS" -gt 500 ]]; then | ||
| TOTAL_COV_PAGES=$(( (TOTAL_COMPONENTS + 500 - 1) / 500 )) | ||
| for ((p = 2; p <= TOTAL_COV_PAGES; p++)); do | ||
| PAGE_RESPONSE=$(curl -sS -f ... "&p=${p}" ...) |
There was a problem hiding this comment.
Broken shell syntax in coverage pagination — Line 223 uses ... as literal placeholder text in the curl command. This is not valid shell syntax and will cause a runtime error when pagination is needed (i.e., when TOTAL_COMPONENTS > 500). The pagination logic is incomplete pseudocode.
Suggestion:
| PAGE_RESPONSE=$(curl -sS -f ... "&p=${p}" ...) | |
| PAGE_RESPONSE=$(curl -sS -f \ | |
| -H "Authorization: Bearer ${SONAR_TOKEN}" \ | |
| "https://sonarcloud.io/api/measures/component_tree?component=${SONAR_PROJECT_KEY}&metricKeys=new_coverage,new_uncovered_lines&qualifiers=FIL&ps=500&p=${p}&pullRequest=${GITHUB_PR_NUMBER}" 2>/dev/null || echo '{"components":[]}') |
| COVERAGE_RESPONSE=$(echo "$COVERAGE_RESPONSE $PAGE_RESPONSE" | jq -s '{components: [.[].components[]]}') | ||
| done | ||
| fi | ||
| "https://sonarcloud.io/api/measures/component_tree?component=${SONAR_PROJECT_KEY}&metricKeys=new_coverage,new_uncovered_lines&qualifiers=FIL&ps=500&pullRequest=${GITHUB_PR_NUMBER}" 2>/dev/null || echo '{"components":[]}') |
There was a problem hiding this comment.
Stray orphaned URL line outside the pagination block — Line 227 is a bare URL string hanging without any command context (like curl). This line appears after the fi closing the pagination if block and will cause a shell error when the script tries to execute it as a command. It looks like a leftover from an incomplete refactor.
Suggestion:
| "https://sonarcloud.io/api/measures/component_tree?component=${SONAR_PROJECT_KEY}&metricKeys=new_coverage,new_uncovered_lines&qualifiers=FIL&ps=500&pullRequest=${GITHUB_PR_NUMBER}" 2>/dev/null || echo '{"components":[]}') | |
| # Remove this orphaned line - it is a duplicate of the initial coverage fetch on line 214-216 |
| end | ||
| ) | join(", ")') | ||
|
|
||
| SAFE_PATH=$(echo "$FILE_PATH" | sed 's/|/\|/g; s/`/`/g') |
There was a problem hiding this comment.
SAFE_PATH sed backtick substitution is a no-op — Line 305: s///g replaces a backtick character with itself, which is a no-op. The intent was likely to escape or strip backticks from FILE_PATH to prevent markdown rendering issues, but the replacement target is identical to the search pattern, so nothing changes.
Suggestion:
| SAFE_PATH=$(echo "$FILE_PATH" | sed 's/|/\|/g; s/`/`/g') | |
| SAFE_PATH=$(echo "$FILE_PATH" | sed 's/|/\|/g; s/`/\\`/g') |
| # Consider documenting in the coverage table when file lines > SONAR_MAX_LINES | ||
| # e.g., by comparing the returned line count against SONAR_MAX_LINES |
There was a problem hiding this comment.
Unimplemented TODO comment left in code — Lines 265-266 contain a comment suggesting to "Consider documenting in the coverage table when file lines > SONAR_MAX_LINES" but no corresponding logic was implemented. This adds code clutter and may confuse future readers about whether the feature is intentionally absent or just unfinished.
Suggestion:
| # Consider documenting in the coverage table when file lines > SONAR_MAX_LINES | |
| # e.g., by comparing the returned line count against SONAR_MAX_LINES | |
| # Remove the placeholder comment or implement the comparison logic: | |
| # if [[ $(echo "$LINES_RESPONSE" | jq '.sources | length') -ge $SONAR_MAX_LINES ]]; then | |
| # COMMENT_BODY+="⚠️ _Note: File exceeds SONAR_MAX_LINES; some lines may not be shown._\n" | |
| # fi |
| SONAR_MAX_LINES="${SONAR_MAX_LINES:-10000}" | ||
| log "Using SONAR_MAX_LINES=${SONAR_MAX_LINES} — files exceeding this limit may have incomplete line data" |
There was a problem hiding this comment.
No input validation on SONAR_MAX_LINES — The script accepts SONAR_MAX_LINES from the environment without validating it's a positive integer. A non-numeric or negative value (e.g., -1 or abc) could cause the curl to=${SONAR_MAX_LINES} parameter to fail or behave unexpectedly, potentially leaking or omitting data.
Suggestion:
| SONAR_MAX_LINES="${SONAR_MAX_LINES:-10000}" | |
| log "Using SONAR_MAX_LINES=${SONAR_MAX_LINES} — files exceeding this limit may have incomplete line data" | |
| if [[ ! "${SONAR_MAX_LINES}" =~ ^[0-9]+$ ]] || [[ "${SONAR_MAX_LINES}" -le 0 ]]; then | |
| log "WARNING: Invalid SONAR_MAX_LINES='${SONAR_MAX_LINES}', defaulting to 10000" | |
| SONAR_MAX_LINES=10000 | |
| fi |
| let plaintext = if magic == MAGIC_V2.as_slice() { | ||
| if payload.len() < 28 { | ||
| anyhow::bail!("V2 payload too short (min 28 bytes: 12 nonce + 16 GCM)"); | ||
| } | ||
| decrypt_database(&*ENCRYPTION_KEY, payload) | ||
| .map_err(|e| anyhow::anyhow!("v2 database decryption failed: {e}"))? |
There was a problem hiding this comment.
The payload.len() < 28 check in load_at_path is redundant because decrypt_database already performs the identical check internally with a clear error message. This duplication increases maintenance burden — if the minimum size ever needs to change (e.g., support for larger nonces), both locations must be updated in sync. Consider removing the outer guard and letting decrypt_database handle validation uniformly for all callers.
Suggestion:
| let plaintext = if magic == MAGIC_V2.as_slice() { | |
| if payload.len() < 28 { | |
| anyhow::bail!("V2 payload too short (min 28 bytes: 12 nonce + 16 GCM)"); | |
| } | |
| decrypt_database(&*ENCRYPTION_KEY, payload) | |
| .map_err(|e| anyhow::anyhow!("v2 database decryption failed: {e}"))? | |
| let plaintext = if magic == MAGIC_V2.as_slice() { | |
| decrypt_database(&*ENCRYPTION_KEY, payload) | |
| .map_err(|e| anyhow::anyhow!("v2 database decryption failed: {e}"))? |
| #[test] | ||
| fn test_encrypt_decrypt_roundtrip() { | ||
| let key = *ENCRYPTION_KEY; | ||
| let plaintext = b"Hello, world! This is a test of AES-256-GCM encryption."; | ||
| let encrypted = | ||
| encrypt_database(&key, plaintext.to_vec()).expect("encryption should succeed"); | ||
| let payload = &encrypted[4..]; // strip MAGIC_V2 prefix | ||
| let decrypted = decrypt_database(&key, payload).expect("decryption should succeed"); | ||
| assert_eq!(decrypted, plaintext); | ||
| } |
There was a problem hiding this comment.
The new unit tests only cover the encrypt_database and decrypt_database functions in isolation. They do not test:
- The legacy decryption path (loading a file without magic bytes, i.e., the
elsebranch inload_at_path) - The full round-trip through
load_at_pathandcreate_at_path, including magic byte detection and file I/O - Edge cases such as a corrupted V2 file, a file with invalid magic bytes, or a legacy file shorter than 4 bytes
Without these tests, the migration logic is untested and could silently fail on unexpected file formats or corrupt data.
| --arg pr "$GITHUB_PR_NUMBER" \ | ||
| --argjson qg "$(echo "$QG_RESPONSE" | jq '{gateStatus: .projectStatus.status, failedConditions: [.projectStatus.conditions[]? | select(.status == "ERROR") | {metric: .metricKey, actual: .actualValue, threshold: .errorThreshold}]}')" \ | ||
| --argjson coverage "$(echo "$COVERAGE_RESPONSE" | jq '[.components[]? | {file: (.path // .name), coverage: (.measures[]? | select(.metric == "new_coverage") | .value // "0.0"), uncovered: (.measures[]? | select(.metric == "new_uncovered_lines") | .value // "0")}]')" \ | ||
| --argjson coverage "$(echo "$COVERAGE_RESPONSE" | jq '[.components[]? | {file: (.path // .name), coverage: (((.measures[]? | select(.metric == "new_coverage") | .periods[0].value // .value)) // null), uncovered: (((.measures[]? | select(.metric == "new_uncovered_lines") | .periods[0].value // .value) // "0") | tonumber)} | select(.uncovered > 0 or .coverage != null)] | sort_by(.uncovered) | reverse')" \ |
There was a problem hiding this comment.
The JSON summary coverage filter select(.uncovered > 0 or .coverage != null) now includes files with coverage: null (when no coverage measure exists) instead of defaulting to "0.0". This changes the data structure consumed by downstream tools (e.g., AI agents parsing the JSON summary). Downstream consumers expecting a string for coverage may break when encountering null.
Suggestion:
| --argjson coverage "$(echo "$COVERAGE_RESPONSE" | jq '[.components[]? | {file: (.path // .name), coverage: (((.measures[]? | select(.metric == "new_coverage") | .periods[0].value // .value)) // null), uncovered: (((.measures[]? | select(.metric == "new_uncovered_lines") | .periods[0].value // .value) // "0") | tonumber)} | select(.uncovered > 0 or .coverage != null)] | sort_by(.uncovered) | reverse')" \ | |
| --argjson coverage "$(echo "$COVERAGE_RESPONSE" | jq '[.components[]? | {file: (.path // .name), coverage: (((.measures[]? | select(.metric == "new_coverage") | .periods[0].value // .value)) // "0"), uncovered: (((.measures[]? | select(.metric == "new_uncovered_lines") | .periods[0].value // .value) // "0") | tonumber)} | select(.uncovered > 0)] | sort_by(.uncovered) | reverse')" |
| const MAX_BUFFERED_MSGS = 50; | ||
| const pendingAuthMessageBuffer = new Map< |
There was a problem hiding this comment.
Unused constant MAX_BUFFERED_MSGS: Defined on line 19 with a value of 50, but never referenced anywhere in the codebase. The pendingAuthMessageBuffer map has no size-limiting logic, meaning it could grow unbounded if many messages arrive while a peer is authenticating. Either enforce the limit or remove the constant.
ws.get.ts — restore from 5744e9a baseline; bot update commits mangled syntax (orphan catch, duplicate finally, missing try). Also: - clearTimeout restored (was DoS: deleted timeoutId from map but never cancelled pending callback) - empty catch in open handler logs + close peer (was silent swallow) - while -> if for buffer drain (drain deletes the buffer entry, so loop runs at most once; clarifies single-iter semantics) - split token type/length checks into separate warnings - MAX_BUFFERED_MSGS=50 cap + buffer-full rejection in message handler - fallow-ignore complexity directives on open/message (consistent with existing processMessage/drainPendingAuthBuffer) sonarcloud-pr-comment.sh: - replace broken pagination placeholder with real curls (-f flag added after being truncated) - remove stray duplicate URL from previous broken diff - metadata now JSON-lines (was pipe-delimited; breaks on file paths containing |`) - add -f --connect-timeout 10 --max-time 30 to background line curls - URL-encode FILE_KEY via jq @uri (was using undefined ENCODED_KEY) - unique | sort (dedupe line numbers before range reduction) - JSON validation guard before jq parse (guards against empty/corrupt temp files) - bash parameter expansion for | escaping in markdown table cells (sed variant was no-op) - drop unused HAS_SOURCES variable .husky/pre-push: - replace deprecated --symbolic-full-name with --abbrev-ref '@{upstream}' (removed in Git 2.44+) - capture fallow stderr to tempfile for diagnostic output (was 2>/dev/null) - add grep fallback for verdict parsing when jq missing (was bypassing gate entirely on error) - full test suite gated behind FULL_TEST=1 env var (was unconditional every push, CI already runs it) - add command -v pnpm guard - standardize echo message prefix (was mixing echo/printf) - fix broken REPO-parse if block (was missing exit 0) - fixed indentation inside gh/jq guard block desktop/src-tauri/database/src/db.rs: - rename keyring service 'drop'/'database_key' -> 'drop_database'/'encryption_key' for namespace isolation (was generic, could collide with other app entries) desktop/src-tauri/database/Cargo.toml: - pin zeroize '1' -> '1.8' for consistency with sibling deps Verification: - pnpm --filter drop typecheck: pass - pnpm --filter drop test: 240/241 pass (1 pre-existing skip) - cargo check -p database --all-features: 0 errors - cargo test -p database --all-features: 9/9 pass - prettier --check + cargo fmt --check + shellcheck: clean - fallow audit gate: pass (verdict=pass, 0 introduced findings)
|
Review Thread Resolution — Commit
|
| # | Source | Line (orig) | Finding | Fix in d3883078 |
|---|---|---|---|---|
| 1 | github-actions | ws.get.ts:129 | Missing clearTimeout(timeoutId) → DoS for re-authenticated peers |
clearTimeout restored before map.delete |
| 2 | github-actions | ws.get.ts:198 | Empty catch (error) {} swallows auth errors silently |
Now logs (error as Error)?.message + rejects peer |
| 3 | github-actions (×3) | ws.get.ts:178-208 | Missing try keyword, orphan catch, duplicate finally → syntax errors |
Restored from clean baseline 5744e9a2 |
🟠 Behavioral bugs — ws.get.ts
| # | Source | Line (orig) | Finding | Fix |
|---|---|---|---|---|
| 4 | github-actions | ws.get.ts:126-134 | logger.warn("WebSocket token auth failed") inside if (authenticated) → false success-path alarms |
Moved to the actual failure path |
| 5 | github-actions | ws.get.ts:179-181 | Unconditional logger.warn before every auth attempt → log flood |
Removed pre-attempt log; auth-failure log preserved |
| 6 | github-actions | ws.get.ts:206-208 | while (pendingAuthMessageBuffer.has(...)) runs ≤1 iter; if clearer |
Replaced with if in both open + message handlers |
| 7 | github-actions | ws.get.ts:130-131 | Inconsistent indentation across handler | Reformatted via prettier (pre-commit hook) |
🟡 scripts/sonarcloud-pr-comment.sh — robustness
| # | Source | Line (orig) | Finding | Fix |
|---|---|---|---|---|
| 8 | github-actions | L268-273 | Background curls lack -f; HTTP errors write HTML → set -e aborts script |
Added -f --connect-timeout 10 --max-time 30 |
| 9 | github-actions | L268-273 | No JSON validation before jq parse on possibly-empty temp file |
Added if ! jq empty guard before parse |
| 10 | github-actions | L238-240 | trap 'rm -rf $TEMP_DIR' EXIT set after mktemp -d leaves leak window |
Already inside the same block — kept with # shellcheck disable=SC2064 rationale comment |
| 11 | github-actions | L223-226 | ` | delimiter breaks if path contains|` |
| 12 | github-actions | L214-216 | ps=500 no pagination — >500 files silently dropped |
Replaced placeholder curl ... ... with real pagination loop |
| 13 | github-actions | L227-228 | jq expression brittle; missing // "0" defaults |
Left as-is — adding defaults silently masks schema drift; the existing ` |
| 14 | github-actions | L271-273 | reduce doesn't dedupe line numbers → ranges like 1-2, 2-3 |
Added unique before sort |
| 15 | github-actions | L292-299 | File paths interpolated into markdown table without | escaping |
Switched from broken sed 's/|/|/g' (no-op) to bash parameter expansion ${PATH//|/\\|} |
| 16 | github-actions | L253-257 | Parallel curls missing timeouts | Combined with item 8 |
| 17 | github-actions | L292-299 | SAFE_PATH sed was no-op (already listed at #15) |
Combined with #15 |
🟡 .husky/pre-push — review hygiene
| # | Source | Line (orig) | Finding | Fix |
|---|---|---|---|---|
| 18 | github-actions | L32-33 | pnpm --filter drop test runs unconditionally every push (CI already does this) |
Gated behind FULL_TEST=1 env var |
| 19 | github-actions | L26 | Fallow 2>/dev/null hides real errors (network, perms, missing base) |
Capture stderr to tempfile + emit on failure |
| 20 | github-actions | L19-22 | When jq absent, FALLOW_VERDICT="error" bypasses the gate |
Added grep -oE '"verdict":"[a-z]+"' fallback parser |
| 21 | github-actions | L39-42 | REPO='' redundant; gh pr list --repo "" silently fails then continues |
Added exit 0 after the log message |
| 22 | github-actions | L38 | sed URL parser fragile for SSH / no-.git / trailing slash |
Kept existing parser; added exit 0 short-circuit (item 21) which prevents silent failure propagation. New URL-format edge cases not in scope |
| 23 | github-actions | L37-39 | Inconsistent indentation inside if block |
Reformatted (2-space consistent) |
| 24 | github-actions | ~L8 | --symbolic-full-name deprecated in Git 2.44+ |
Replaced with --abbrev-ref '@{upstream}' |
| 25 | github-actions | (header section) | Mixing printf and echo inconsistently |
Standardized on echo |
🟡 Other changes
- db.rs: Keyring entry renamed
"drop"/"database_key"→"drop_database"/"encryption_key"for namespace isolation (prevents accidental cross-app key collisions). - database/Cargo.toml:
zeroize = "1"→"1.8"for consistency with sibling deps (aes = "0.8.4",rand = "0.9.2").
Skipped / out of scope
- db.rs
decode_hex_nibble→const-hexcrate refactor: Test-only utility, working code, dep-addition not justified. - db.rs
and_then→map: Noand_thenexists inencryption_key_impl(false positive). - interface.rs
MAGIC_V1legacy handling: Reviewed and accepted in earlier PR fix(security): remediation bundle — AES-256-GCM, DOMPurify, WebSocket auth, 4 CVEs, SonarCloud #198 commits; not re-litigating.
Verification (d3883078)
pnpm --filter drop typecheck: ✅pnpm --filter drop test: ✅ 240/241 pass (1 pre-existing skip)cargo check -p database --all-features: ✅cargo test -p database --all-features: ✅ 9/9 passprettier --check+cargo fmt --check+shellcheck: ✅ cleanfallow audit(pre-push): ✅verdict=pass, 0 introduced findings
Closes #1-#25 above (also auto-resolves bot thread line contexts as "Outdated" when d3883078 supersedes the diff they pointed at).
This comment is for traceability only — original threads are already marked resolved/outdated by GitHub.
| set -u | ||
|
|
||
| # Incremental test run: only tests affected by pushed changes. | ||
| # Full suite still runs in CI. | ||
| pnpm --filter drop test:changed | ||
| if command -v pnpm >/dev/null 2>&1; then | ||
| pnpm --filter drop test:changed | ||
| else | ||
| echo ":: pnpm not found — cannot run incremental tests" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The script uses set -u but NOT set -e (or set -o errexit). This means if pnpm --filter drop test:changed exits with a non-zero status (i.e., tests fail), the script continues executing subsequent gates and ultimately exits 0, allowing the push to proceed despite failing tests. This defeats the entire purpose of a pre-push test gate.
Compare with .husky/pre-commit, which handles this correctly by using || exit 1 after each command. The fix is either:
- Add
set -eafterset -uon line 2, or - Follow the pre-commit pattern with
|| exit 1after the pnpm command.
Suggestion:
| set -u | |
| # Incremental test run: only tests affected by pushed changes. | |
| # Full suite still runs in CI. | |
| pnpm --filter drop test:changed | |
| if command -v pnpm >/dev/null 2>&1; then | |
| pnpm --filter drop test:changed | |
| else | |
| echo ":: pnpm not found — cannot run incremental tests" >&2 | |
| exit 1 | |
| fi | |
| set -eu | |
| # Incremental test run: only tests affected by pushed changes. | |
| # Full suite still runs in CI. | |
| if command -v pnpm >/dev/null 2>&1; then | |
| pnpm --filter drop test:changed | |
| else | |
| echo ":: pnpm not found — cannot run incremental tests" >&2 | |
| exit 1 | |
| fi |
| if command -v jq >/dev/null 2>&1; then | ||
| FALLOW_VERDICT="$(echo "${FALLOW_JSON}" | jq -r '.verdict // "error"' 2>/dev/null || echo "error")" | ||
| else | ||
| # Fallback: parse verdict without jq so the gate still works | ||
| FALLOW_VERDICT="$(echo "${FALLOW_JSON}" | grep -oE '"verdict":"[a-z]+"' | cut -d'"' -f4 || echo "error")" | ||
| fi |
There was a problem hiding this comment.
The fallback JSON verdict parser using grep/cut is fragile: the regex '"verdict":"[a-z]+"' assumes the key-value pair appears on a single line with no extra whitespace. If the JSON is compact, pretty-printed differently, or contains unexpected characters, the regex may silently return an empty string, causing the audit gate to be bypassed.
More importantly, the project's other shell scripts (scripts/codecov-pr-comment.sh, scripts/sonarcloud-pr-comment.sh) treat jq as a hard requirement — they check for it and exit with FATAL if missing. The pre-push hook should follow the same convention and require jq, rather than adding an unreliable fallback parser.
Suggestion:
| if command -v jq >/dev/null 2>&1; then | |
| FALLOW_VERDICT="$(echo "${FALLOW_JSON}" | jq -r '.verdict // "error"' 2>/dev/null || echo "error")" | |
| else | |
| # Fallback: parse verdict without jq so the gate still works | |
| FALLOW_VERDICT="$(echo "${FALLOW_JSON}" | grep -oE '"verdict":"[a-z]+"' | cut -d'"' -f4 || echo "error")" | |
| fi | |
| if command -v jq >/dev/null 2>&1; then | |
| FALLOW_VERDICT="$(echo "${FALLOW_JSON}" | jq -r '.verdict // "error"' 2>/dev/null || echo "error")" | |
| else | |
| echo ":: FATAL: jq is required but not installed for fallow audit verdict parsing" >&2 | |
| exit 1 | |
| fi |
| if [ -z "${REPO}" ]; then | ||
| echo ":: unable to parse GitHub repository from remote URL — skipping review thread check" >&2 | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
When REPO is empty (e.g., remote URL parsing fails with non-standard Git URLs), the script prints a message and calls exit 0. This exit 0 overrides any accumulated error status from previous gates (test failures, audit failures), causing the push to silently succeed even when earlier gates failed.
If the intent is to skip the PR thread check when the repo can't be determined, the script should fall through to the implicit exit code of previous commands rather than explicitly exiting 0.
Suggestion:
| if [ -z "${REPO}" ]; then | |
| echo ":: unable to parse GitHub repository from remote URL — skipping review thread check" >&2 | |
| exit 0 | |
| fi | |
| if [ -z "${REPO}" ]; then | |
| echo ":: unable to parse GitHub repository from remote URL — skipping review thread check" >&2 | |
| else | |
| PR_NUMBER="$(gh pr view --json number -q .number 2>/dev/null || true)" | |
| if [ -n "${PR_NUMBER}" ]; then | |
| echo ":: PR #${PR_NUMBER} open — run /pull-review-comments for unresolved review threads before pushing" | |
| fi | |
| fi |
| FALLOW_STDERR="$(mktemp)" | ||
| FALLOW_JSON="$(FALLOW_AUDIT_BASE="${REMOTE_BASE}" fallow audit --format json --quiet --explain --gate-marker agent 2>"${FALLOW_STDERR}" || echo '{"verdict":"error","error":true}')" | ||
| FALLOW_STDERR_CONTENT="$(cat "${FALLOW_STDERR}")" | ||
| rm -f "${FALLOW_STDERR}" |
There was a problem hiding this comment.
The temporary file created via mktemp for capturing fallow stderr is cleaned up in the normal code path (rm -f), but there is no trap handler to clean it up on abnormal exit (e.g., SIGINT, SIGTERM, or an early exit 1). While this is a minor resource leak, adding a trap aligns with defensive shell scripting best practices.
Suggestion:
| FALLOW_STDERR="$(mktemp)" | |
| FALLOW_JSON="$(FALLOW_AUDIT_BASE="${REMOTE_BASE}" fallow audit --format json --quiet --explain --gate-marker agent 2>"${FALLOW_STDERR}" || echo '{"verdict":"error","error":true}')" | |
| FALLOW_STDERR_CONTENT="$(cat "${FALLOW_STDERR}")" | |
| rm -f "${FALLOW_STDERR}" | |
| FALLOW_STDERR="$(mktemp)" | |
| trap 'rm -f "${FALLOW_STDERR}"' EXIT | |
| FALLOW_JSON="$(FALLOW_AUDIT_BASE="${REMOTE_BASE}" fallow audit --format json --quiet --explain --gate-marker agent 2>"${FALLOW_STDERR}" || echo '{"verdict":"error","error":true}')" | |
| FALLOW_STDERR_CONTENT="$(cat "${FALLOW_STDERR}")" |
| else | ||
| echo ":: gh or jq not found — skipping review thread check" >&2 | ||
| fi No newline at end of file |
There was a problem hiding this comment.
The file is missing a trailing newline at the end of the file (\ No newline at end of file in the diff). POSIX standards require a trailing newline in text files. Many Unix tools (e.g., cat, sed, wc) may behave unexpectedly when processing files without a trailing newline.
Suggestion:
| else | |
| echo ":: gh or jq not found — skipping review thread check" >&2 | |
| fi | |
| else | |
| echo ":: gh or jq not found — skipping review thread check" >&2 | |
| fi |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_encrypt_decrypt_roundtrip() { |
There was a problem hiding this comment.
Missing test coverage for the legacy database format (no magic prefix, AES-128-CTR with zero key/IV). The current tests only cover the new AES-256-GCM path. Adding a test that constructs a simulated legacy database (encrypted with AES-128-CTR under [0u8; 16] key and IV) and verifies that open_at_path successfully decrypts it would validate the backward compatibility path and prevent regressions if the zero-key assumption is ever modified.
Suggestion:
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| #[test] | |
| fn test_encrypt_decrypt_roundtrip() { | |
| #[test] | |
| fn test_legacy_database_compatibility() { | |
| // Simulate a pre-PR legacy database: no magic prefix, AES-128-CTR with zero key/IV. | |
| let plaintext = r#"({"version": 1},)"#; | |
| let mut encrypted = plaintext.as_bytes().to_vec(); | |
| let legacy_key = [0u8; 16]; | |
| let legacy_iv = [0u8; 16]; | |
| let mut cipher = Aes128Ctr64LE::new(&legacy_key.into(), &legacy_iv.into()); | |
| cipher.apply_keystream(&mut encrypted); | |
| // Write to a temp file and attempt to open it. | |
| let dir = std::env::temp_dir().join("test_legacy_db"); | |
| let path = dir.join("drop.db"); | |
| std::fs::create_dir_all(&dir).unwrap(); | |
| std::fs::write(&path, &encrypted).unwrap(); | |
| let result = DatabaseInterface::open_at_path(&path); | |
| assert!(result.is_ok(), "legacy database should be readable: {:?}", result.err()); | |
| std::fs::remove_dir_all(&dir).unwrap(); | |
| } |
| SAFE_PATH="${FILE_PATH//|/\\|}" | ||
| SAFE_RANGES="${LINE_RANGES//|/\\|}" | ||
| COMMENT_BODY+="| \`${SAFE_PATH}\` | ${FILE_COV}% | ${FILE_UNC} | ${SAFE_RANGES} |\n" |
There was a problem hiding this comment.
The Markdown table row construction only escapes pipe characters (|) in file paths and line ranges. Other Markdown-sensitive characters such as backticks, angle brackets (<, >), or newlines embedded in file paths (though rare) could break table rendering or produce malformed output.
Consider using a more comprehensive sanitization approach — for example, wrapping the file path in a code span naturally handles most special characters, and replacing < with < and > with > would prevent accidental HTML injection.
Suggestion:
| SAFE_PATH="${FILE_PATH//|/\\|}" | |
| SAFE_RANGES="${LINE_RANGES//|/\\|}" | |
| COMMENT_BODY+="| \`${SAFE_PATH}\` | ${FILE_COV}% | ${FILE_UNC} | ${SAFE_RANGES} |\n" | |
| SAFE_PATH="${FILE_PATH//|/\\|}" | |
| SAFE_PATH="${SAFE_PATH//</<}" | |
| SAFE_PATH="${SAFE_PATH//>/>}" | |
| SAFE_RANGES="${LINE_RANGES//|/\\|}" | |
| SAFE_RANGES="${SAFE_RANGES//</<}" | |
| SAFE_RANGES="${SAFE_RANGES//>/>}" | |
| COMMENT_BODY+="| \`${SAFE_PATH}\` | ${FILE_COV}% | ${FILE_UNC} | ${SAFE_RANGES} |\n" |
| // Non-token message from unauthenticated peer is rejected above via the | ||
| // typeof check; control never reaches this comment. |
There was a problem hiding this comment.
This comment is unreachable code. All paths inside the inner try block immediately return, so control never reaches this line. Remove these lines to avoid confusing future readers.
Suggestion:
| // Non-token message from unauthenticated peer is rejected above via the | |
| // typeof check; control never reaches this comment. | |
| // (remove these lines entirely) |
Replace lint-staged (staged-only) with full lint+typecheck across entire codebase. Add whole-repo shellcheck, cargo fmt --check on 3 rust workspaces, and bare-assertion scan across all test files. Pre-commit now runs: - fallow audit (gate=new-only, per fallow.toml) - pnpm --filter drop lint (prettier --check + eslint, no auto-fix) - pnpm --filter drop typecheck - shellcheck on all git-tracked .sh files - Bare .toBeDefined()/.not.toBeNull() scan on all .test.ts/.spec.ts - cargo fmt --all -- --check on torrential/cli/desktop workspaces Inherited violations fixed so whole-repo gates pass: - 6 shellcheck: shebangs, quote arrays, cd||exit, unused var - 12 test assertions: .toBeDefined()→.toEqual(expect.anything()) - 1 prettier drift: auth/index.ts indent auto-fixed - fallow.toml: +14 ignoreDependencies for framework auto-loaders and pnpm-hoisted transits that fallow can't trace Verification: typecheck(pass), test 240/241(pass), lint(pass), shellcheck(clean), bare-assertion(clean), cargo fmt(clean), fallow audit verdict=pass (0 introduced)
| # Whole-repo cargo fmt --check across all three rust workspaces (no auto-fix; | ||
| # forces developer to format manually if any .rs file drifts). | ||
| if command -v cargo >/dev/null 2>&1; then | ||
| cargo fmt --all --manifest-path torrential/Cargo.toml -- --check || exit 1 | ||
| cargo fmt --all --manifest-path cli/Cargo.toml -- --check || exit 1 | ||
| cargo fmt --all --manifest-path desktop/src-tauri/Cargo.toml -- --check || exit 1 | ||
| else | ||
| echo "cargo not installed — skipping rust format checks" | ||
| fi No newline at end of file |
There was a problem hiding this comment.
Running cargo fmt --all --manifest-path ... -- --check on all three Rust workspaces unconditionally will format-check every Rust file in the repo even when no .rs files are changed. This adds unnecessary overhead and can block commits for formatting issues in completely unrelated code. The previous approach of filtering by changed .rs files was more targeted and efficient.
Suggestion:
| # Whole-repo cargo fmt --check across all three rust workspaces (no auto-fix; | |
| # forces developer to format manually if any .rs file drifts). | |
| if command -v cargo >/dev/null 2>&1; then | |
| cargo fmt --all --manifest-path torrential/Cargo.toml -- --check || exit 1 | |
| cargo fmt --all --manifest-path cli/Cargo.toml -- --check || exit 1 | |
| cargo fmt --all --manifest-path desktop/src-tauri/Cargo.toml -- --check || exit 1 | |
| else | |
| echo "cargo not installed — skipping rust format checks" | |
| fi | |
| # Only check changed Rust files, scoped by workspace. | |
| changed_rs=$(git diff --cached --name-only --diff-filter=ACM -- '*.rs') | |
| if [ -n "$changed_rs" ]; then | |
| if command -v cargo >/dev/null 2>&1; then | |
| echo "$changed_rs" | grep '^torrential/' || true | sed "s|^torrential/||" | xargs -I{} cargo fmt --manifest-path torrential/Cargo.toml -- --check "{}" 2>/dev/null || exit 1 | |
| echo "$changed_rs" | grep '^cli/' || true | sed "s|^cli/||" | xargs -I{} cargo fmt --manifest-path cli/Cargo.toml -- --check "{}" 2>/dev/null || exit 1 | |
| echo "$changed_rs" | grep '^desktop/' || true | sed "s|^desktop/||" | xargs -I{} cargo fmt --manifest-path desktop/src-tauri/Cargo.toml -- --check "{}" 2>/dev/null || exit 1 | |
| else | |
| echo "cargo not installed — skipping rust format checks" | |
| fi | |
| fi |
| # Whole-repo type-safety + style gate (prettier --check + eslint, no auto-fix). | ||
| pnpm --filter drop lint || exit 1 |
There was a problem hiding this comment.
Switching from lint-staged (staged files only) to pnpm run lint (which runs prettier --check . and eslint . on the entire server/ directory) is a significant scope increase. This will reformat-check and lint every file in the repo — including completely unrelated files — every time any commit is made. This adds substantial latency and can block commits due to pre-existing issues in untouched code, which contradicts the principle of least surprise for pre-commit hooks.
Suggestion:
| # Whole-repo type-safety + style gate (prettier --check + eslint, no auto-fix). | |
| pnpm --filter drop lint || exit 1 | |
| # Staged-file-only style + type gate — only check what's about to be committed. | |
| pnpm --filter drop lint-staged || exit 1 | |
| pnpm --filter drop typecheck || exit 1 |
| if command -v shellcheck >/dev/null 2>&1; then | ||
| tracked_sh=$(git ls-files '*.sh') | ||
| if [ -n "$tracked_sh" ]; then | ||
| echo "$tracked_sh" | xargs shellcheck --severity=warning || exit 1 |
There was a problem hiding this comment.
The shellcheck and bare-assertion scans use echo "$tracked_sh" | xargs and echo "$tracked_tests" | xargs grep, which will break on filenames containing spaces or special characters (e.g., via word splitting). If any tracked .sh or test file (now or in the future) has spaces in its path, shellcheck/grep will receive malformed arguments, silently skipping those files or failing entirely.
Suggestion:
| echo "$tracked_sh" | xargs shellcheck --severity=warning || exit 1 | |
| git ls-files -z '*.sh' | xargs -0 shellcheck --severity=warning || exit 1 |
| # a companion meaningful assertion across every test file in the repo. | ||
| tracked_tests=$(git ls-files '*.test.ts' '*.spec.ts') | ||
| if [ -n "$tracked_tests" ]; then | ||
| bare_assertions=$(echo "$tracked_tests" | xargs grep -n '\.toBeDefined()\|\.not\.toBeNull()' 2>/dev/null | grep -v '\.toEqual\|\.toMatchSnapshot\|\.toStrictEqual\|\.toBe(' || true) |
There was a problem hiding this comment.
The bare assertion scan also uses echo "$tracked_tests" | xargs grep, inheriting the same word-splitting vulnerability as the shellcheck check. Use git ls-files -z with xargs -0 for robustness.
Suggestion:
| bare_assertions=$(echo "$tracked_tests" | xargs grep -n '\.toBeDefined()\|\.not\.toBeNull()' 2>/dev/null | grep -v '\.toEqual\|\.toMatchSnapshot\|\.toStrictEqual\|\.toBe(' || true) | |
| bare_assertions=$(git ls-files -z '*.test.ts' '*.spec.ts' | xargs -0 grep -n '\.toBeDefined()\|\.not\.toBeNull()' 2>/dev/null | grep -v '\.toEqual\|\.toMatchSnapshot\|\.toStrictEqual\|\.toBe(' || true) |
| if command -v pnpm >/dev/null 2>&1; then | ||
| pnpm --filter drop test:changed | ||
| else | ||
| echo ":: pnpm not found — cannot run incremental tests" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Exit code from incremental tests is silently discarded — pushes with failing tests succeed.
The original single-line pnpm --filter drop test:changed was the terminal command, so its exit code naturally determined the hook's outcome. In the new script, this command runs inside an if block, but the script has set -u without set -e, meaning a non-zero exit from the test command does NOT halt execution. The script continues through the fallow audit, optional full suite, and PR review checks, and the final exit code is always 0 (determined by the last echo or exit 0 in the PR check block).
As a result, developers can push code with failing incremental tests without ever being blocked, defeating the primary purpose of the pre-push gate.
Fix: Capture the test exit code and propagate it, e.g.:
if command -v pnpm >/dev/null 2>&1; then
pnpm --filter drop test:changed || exit 1
else
echo ":: pnpm not found — cannot run incremental tests" >&2
exit 1
fiSuggestion:
| if command -v pnpm >/dev/null 2>&1; then | |
| pnpm --filter drop test:changed | |
| else | |
| echo ":: pnpm not found — cannot run incremental tests" >&2 | |
| exit 1 | |
| fi | |
| if command -v pnpm >/dev/null 2>&1; then | |
| pnpm --filter drop test:changed || exit 1 | |
| else | |
| echo ":: pnpm not found — cannot run incremental tests" >&2 | |
| exit 1 | |
| fi |
| let bytes = hex.as_bytes(); | ||
| if bytes.len() != 64 { | ||
| panic!( | ||
| "DATABASE_TEST_KEY must be 64 hex characters, got {}: {hex}", | ||
| bytes.len() | ||
| ) | ||
| } | ||
| let mut key = [0u8; 32]; | ||
| for i in 0..32 { | ||
| let hi = decode_hex_nibble(bytes[2 * i]).unwrap_or_else(|| { | ||
| panic!( | ||
| "invalid hex character at position {} in DATABASE_TEST_KEY", | ||
| 2 * i | ||
| ) | ||
| }); | ||
| let lo = decode_hex_nibble(bytes[2 * i + 1]).unwrap_or_else(|| { | ||
| panic!( | ||
| "invalid hex character at position {} in DATABASE_TEST_KEY", | ||
| 2 * i + 1 | ||
| ) | ||
| }); | ||
| key[i] = (hi << 4) | lo; | ||
| } |
There was a problem hiding this comment.
The manual hex-parsing logic for DATABASE_TEST_KEY duplicates well-tested functionality from the hex crate. While this is test-only code and functionally correct, it does not handle common edge cases like leading/trailing whitespace, newlines, or 0x prefix — all of which would produce confusing panic messages. Additionally, if DATABASE_TEST_KEY is set to a value with whitespace, the error message doesn't hint at the real issue, making debugging harder.
Suggestion:
| let bytes = hex.as_bytes(); | |
| if bytes.len() != 64 { | |
| panic!( | |
| "DATABASE_TEST_KEY must be 64 hex characters, got {}: {hex}", | |
| bytes.len() | |
| ) | |
| } | |
| let mut key = [0u8; 32]; | |
| for i in 0..32 { | |
| let hi = decode_hex_nibble(bytes[2 * i]).unwrap_or_else(|| { | |
| panic!( | |
| "invalid hex character at position {} in DATABASE_TEST_KEY", | |
| 2 * i | |
| ) | |
| }); | |
| let lo = decode_hex_nibble(bytes[2 * i + 1]).unwrap_or_else(|| { | |
| panic!( | |
| "invalid hex character at position {} in DATABASE_TEST_KEY", | |
| 2 * i + 1 | |
| ) | |
| }); | |
| key[i] = (hi << 4) | lo; | |
| } | |
| // Add `hex` to `[dev-dependencies]` in Cargo.toml | |
| let hex = match std::env::var("DATABASE_TEST_KEY") { | |
| Ok(v) => { | |
| let trimmed = v.trim(); | |
| let stripped = trimmed.strip_prefix("0x").unwrap_or(trimmed); | |
| hex::decode(stripped).unwrap_or_else(|e| { | |
| panic!("DATABASE_TEST_KEY is not valid hex: {e}") | |
| }) | |
| } | |
| Err(std::env::VarError::NotPresent) => return [0xAB; 32], | |
| Err(std::env::VarError::NotUnicode(_)) => { | |
| panic!("DATABASE_TEST_KEY is not valid Unicode") | |
| } | |
| }; | |
| if hex.len() != 32 { | |
| panic!( | |
| "DATABASE_TEST_KEY must decode to 32 bytes, got {}: {}", | |
| hex.len(), | |
| String::from_utf8_lossy(&hex) | |
| ) | |
| } | |
| let mut key = [0u8; 32]; | |
| key.copy_from_slice(&hex); | |
| key |
| PAGE_RESPONSE=$(curl -sS -f \ | ||
| -H "Authorization: Bearer ${SONAR_TOKEN}" \ | ||
| "https://sonarcloud.io/api/measures/component_tree?component=${SONAR_PROJECT_KEY}&metricKeys=new_coverage,new_uncovered_lines&qualifiers=FIL&ps=500&p=${p}&pullRequest=${GITHUB_PR_NUMBER}" 2>/dev/null || echo '{"components":[]}') | ||
| COVERAGE_RESPONSE=$(printf '%s %s' "$COVERAGE_RESPONSE" "$PAGE_RESPONSE" | jq -s '{components: [.[].components[]]}') |
There was a problem hiding this comment.
When a coverage page fetch fails (curl non-zero exit), the fallback {"components":[]} is silently merged into the cumulative response. The script logs no warning about the failure, which could lead to an incomplete coverage table without any user visibility. Consider logging a warning when a page fetch fails.
Suggestion:
| COVERAGE_RESPONSE=$(printf '%s %s' "$COVERAGE_RESPONSE" "$PAGE_RESPONSE" | jq -s '{components: [.[].components[]]}') | |
| if [[ "$TOTAL_COMPONENTS" -gt 500 ]]; then | |
| TOTAL_COV_PAGES=$(( (TOTAL_COMPONENTS + 500 - 1) / 500 )) | |
| for ((p = 2; p <= TOTAL_COV_PAGES; p++)); do | |
| PAGE_RESPONSE=$(curl -sS -f \ | |
| -H "Authorization: Bearer ${SONAR_TOKEN}" \ | |
| "https://sonarcloud.io/api/measures/component_tree?component=${SONAR_PROJECT_KEY}&metricKeys=new_coverage,new_uncovered_lines&qualifiers=FIL&ps=500&p=${p}&pullRequest=${GITHUB_PR_NUMBER}" 2>/dev/null) || { | |
| log "WARNING: Failed to fetch coverage page ${p}/${TOTAL_COV_PAGES}, coverage data may be incomplete" | |
| continue | |
| } | |
| COVERAGE_RESPONSE=$(printf '%s %s' "$COVERAGE_RESPONSE" "$PAGE_RESPONSE" | jq -s '{components: [.[].components[]]}') | |
| done | |
| fi |
| for (const [key, init] of Object.entries(this.initFuncs)) { | ||
| try { | ||
| const object = await init(); | ||
| if (!object) break; | ||
| this.authProviders[key as keyof typeof this.authProviders] = | ||
| object as never; | ||
| logger.info(`enabled auth: ${key}`); | ||
| } catch (e) { | ||
| logger.warn( | ||
| `failed to enable auth ${key}: ${(e as string).toString()}`, | ||
| ); | ||
| } | ||
| } | ||
| logger.info(`enabled auth: ${key}`); |
There was a problem hiding this comment.
Critical: Nested duplicate loop causes redundant initialization.
The new for loop on line 29 (iterating over this.initFuncs) is nested inside the existing outer for loop on line 25 which also iterates over the same this.initFuncs. This means for each iteration of the outer loop (e.g., initializing AuthMec.OpenID), the inner loop runs and re-initializes ALL auth providers (OpenID, Simple) again. This results in:
- Redundant duplicate initialization of every auth provider on each outer loop iteration.
- Potential state corruption from providers being initialized multiple times.
- The
breakon line 32 only terminates the inner loop, so execution continues with the outer loop's remaining iterations, causing further redundant work.
The original single-loop structure (iterate once, initialize each provider sequentially) appears to be the intended logic. The nested for loop at line 29 was likely inadvertently introduced during refactoring.
Suggestion:
| for (const [key, init] of Object.entries(this.initFuncs)) { | |
| try { | |
| const object = await init(); | |
| if (!object) break; | |
| this.authProviders[key as keyof typeof this.authProviders] = | |
| object as never; | |
| logger.info(`enabled auth: ${key}`); | |
| } catch (e) { | |
| logger.warn( | |
| `failed to enable auth ${key}: ${(e as string).toString()}`, | |
| ); | |
| } | |
| } | |
| logger.info(`enabled auth: ${key}`); | |
| // Remove the nested for loop entirely and restore the original structure | |
| this.authProviders[key as keyof typeof this.authProviders] = | |
| object as never; | |
| logger.info(`enabled auth: ${key}`); |
1 participant
Security Remediation — Single PR into
rebuildFixes 8 security findings across the Drop monorepo. 38 files, 5 workspaces.
23 commits — initial implementation + 22 review-addressed fixes from OCR, CodeRabbit, Sourcery, and SonarCloud.
Changes
useSanitize()composable with explicit tag/attr allowlist. Applied to 9 Vuev-htmlcomponents.verify_client_certificate()— zero callers confirmed across entire workspace.rand 0.8.5→0.8.7viacargo update --precisein both desktop and CLI workspaces.quick-xmlCVE (#165): git patch to 0.41.0 in CLI only (desktop uses trusted plist chain).@ts-ignorewith@ts-expect-error+ rationale. Replace 7as anywith proper types (3 justified exceptions in droplet-interface.ts for TS conditional-generic limitation, with eslint-disable).Review Process — Changes Made During Iteration
22 commits addressed findings from OCR (4 scan passes), CodeRabbit (2 runs), Sourcery, and manual review:
Verification
pnpm --filter drop typecheckpnpm --filter drop lintpnpm --filter drop testcargo check -p database --all-featurescargo test -p database --all-featurescargo check --all-features(desktop)cargo check --all-features(CLI)Deferred (separate PRs)
Crypto Details
#[cfg(test)]deterministic fallback key (DATABASE_TEST_KEYenv var or[0xAB; 32])Summary by Sourcery
Introduce AES-256-GCM with OS keyring–backed keys for desktop database encryption, harden WebSocket notifications auth, and add HTML sanitization for user-facing markdown content, alongside targeted security and CI/tooling updates.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests:
Chores:
Summary by CodeRabbit