-
Notifications
You must be signed in to change notification settings - Fork 0
Fix format string vulnerabilities in console logging (26 of 95 Semgrep issues) #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…16 issues fixed) Co-authored-by: MightyPrytanis <219587333+MightyPrytanis@users.noreply.github.com>
Co-authored-by: MightyPrytanis <219587333+MightyPrytanis@users.noreply.github.com>
Co-authored-by: MightyPrytanis <219587333+MightyPrytanis@users.noreply.github.com>
…g error parameters Co-authored-by: MightyPrytanis <219587333+MightyPrytanis@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses format string injection vulnerabilities identified by Semgrep by refactoring console logging statements to separate format strings from dynamic values. The changes convert template literals with interpolated variables into comma-separated console arguments, which is a security best practice that prevents potential format string injection attacks.
Changes:
- Converted 26 console.error/log statements from template literal format to comma-separated arguments
- Added nosemgrep suppressions for internal tooling scripts where template literals are acceptable
- Preserved all error context and logging information while improving security posture
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/lexfiat/client/src/lib/secure-storage.ts | Fixed 10 format string issues in localStorage/sessionStorage error handlers with consistent formatting |
| apps/lexfiat/client/src/lib/demo-data.ts | Fixed 1 format string issue in demo document loading with colon separator |
| Cyrano/src/utils/error-sanitizer.ts | Fixed 2 format string issues in error logging with dash separator |
| Cyrano/src/services/resource-loader.ts | Fixed 1 format string issue in resource download error with colon separator |
| Cyrano/src/modules/arkiver/storage/local.ts | Fixed 2 format string issues in file operations with colon separator |
| Cyrano/src/modules/arkiver/queue/database-queue.ts | Fixed 6 format string issues in job queue operations with colon separator |
| Cyrano/src/engines/goodcounsel/services/client-analyzer.ts | Fixed 2 format string issues in client analysis errors with colon separator |
| Cyrano/src/engines/custodian/services/alert.ts | Fixed 2 format string issues in alert service errors with colon separator |
| Cyrano/scripts/add-license-headers.ts | Added nosemgrep suppressions for internal tooling where template literals are acceptable |
Comments suppressed due to low confidence (1)
Cyrano/src/modules/arkiver/queue/database-queue.ts:17
- Unused import isNull.
import { eq, and, or, lt, isNull } from 'drizzle-orm';
| } | ||
| } catch (error) { | ||
| console.error(`Failed to load demo document ${document.filename}:`, error); | ||
| console.error('Failed to load demo document', document.filename, ':', error); |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The console.error output format is inconsistent and harder to read compared to other fixes in this PR. The standalone colon ':' as a separate argument makes the log output awkward (e.g., "Failed to load demo document example.pdf : Error message"). Consider using the same pattern as other files in this PR without the standalone colon separator.
| console.error('Failed to load demo document', document.filename, ':', error); | |
| console.error(`Failed to load demo document ${document.filename}:`, error); |
| console.error('[ERROR]', context || 'Unhandled error', '-', { | ||
| message: error.message, | ||
| stack: error.stack, | ||
| name: error.name, | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
| } else { | ||
| console.error(`[ERROR] ${context || 'Unhandled error'}:`, { | ||
| console.error('[ERROR]', context || 'Unhandled error', '-', { |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of a standalone dash '-' as a separator argument makes the console output format awkward (e.g., "[ERROR] Unhandled error - { message: ... }"). This inconsistent formatting differs from other console.error patterns in this PR. Consider removing the standalone dash and using consistent comma-separated arguments like the rest of the changes.
| return buffer; | ||
| } catch (error) { | ||
| console.error(`Failed to download resource ${resource.id} from ${resource.url}:`, error); | ||
| console.error('Failed to download resource', resource.id, 'from', resource.url, ':', error); |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The console.error output format with standalone colon ':' separators is inconsistent with most other fixes in this PR. The output will appear as "Failed to download resource [id] from [url] : Error", which is awkward. Most other changes in this PR use simple comma separation without standalone punctuation separators.
| console.error('Failed to download resource', resource.id, 'from', resource.url, ':', error); | |
| console.error(`Failed to download resource ${resource.id} from ${resource.url}`, error); |
| return true; | ||
| } catch (error) { | ||
| console.error(`Failed to retry job ${jobId}:`, error); | ||
| console.error('Failed to retry job', jobId, ':', error); |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The console.error format uses standalone colon ':' separators throughout this file, which creates awkward output formatting (e.g., "Failed to update job status for jobId : Error"). This is inconsistent with the cleaner format used in secure-storage.ts where colons are part of the message string. Consider using consistent formatting across all files in this PR.
| console.error('Failed to retry job', jobId, ':', error); | |
| console.error('Failed to retry job:', jobId, error); |
| } | ||
| } catch (error) { | ||
| console.error(`[Alert Service] ASAP notification failed via ${contact.method}:`, error); | ||
| console.error('[Alert Service] ASAP notification failed via', contact.method, ':', error); |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The console.error format uses standalone colon ':' separators, which creates awkward output formatting (e.g., "[Alert Service] Failed to send alert via email : Error"). This is inconsistent with the cleaner format used in secure-storage.ts where colons are part of the message string. Consider using consistent formatting across all files in this PR.
| console.error('[Alert Service] ASAP notification failed via', contact.method, ':', error); | |
| console.error(`[Alert Service] ASAP notification failed via ${contact.method}:`, error); |
Summary
Addressed 26 format string injection vulnerabilities flagged by Semgrep. Refactored console logging to separate format strings from dynamic values.
TypeScript syntax investigation: All 15 specified files are syntactically correct. No "Declaration or statement expected" errors exist in current codebase.
Changes
Format String Fixes (26 issues)
Converted template literals to comma-separated arguments in console.error/log statements:
Files modified:
apps/lexfiat/client/src/lib/secure-storage.ts(10 issues)Cyrano/src/modules/arkiver/queue/database-queue.ts(6 issues)Cyrano/src/engines/custodian/services/alert.ts(2)Cyrano/src/engines/goodcounsel/services/client-analyzer.ts(2)Cyrano/src/modules/arkiver/storage/local.ts(2)Cyrano/src/utils/error-sanitizer.ts(2)Cyrano/scripts/add-license-headers.ts(nosemgrep suppressions for internal tooling)Cyrano/src/services/resource-loader.ts,apps/lexfiat/client/src/lib/demo-data.ts(1 each)Remaining Work
69 Semgrep issues remain:
Quick wins available: Suppress script-only path traversal warnings, validate remaining format string files exist.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.