-
Notifications
You must be signed in to change notification settings - Fork 0
sec: fix token leak, shell injection, and add operation logging #28
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ import 'dart:io'; | |
| import '../../triage/utils/config.dart'; | ||
| import '../../triage/utils/run_context.dart'; | ||
| import 'logger.dart'; | ||
| import 'process_runner.dart'; | ||
|
|
||
| /// Utilities for release management. | ||
| abstract final class ReleaseUtils { | ||
|
|
@@ -75,17 +74,18 @@ abstract final class ReleaseUtils { | |
| } | ||
| buf.writeln(); | ||
| } | ||
| } catch (_) { | ||
| // Skip if parse fails | ||
| } catch (e) { | ||
| Logger.warn('Could not parse contributors.json: $e'); | ||
| } | ||
| } | ||
|
|
||
| // Commit range | ||
| final commitCount = CiProcessRunner.runSync( | ||
| 'git rev-list --count "$prevTag"..HEAD 2>/dev/null', | ||
| repoRoot, | ||
| verbose: verbose, | ||
| ); | ||
| // Commit range — use array args to avoid shell injection via prevTag. | ||
| final commitCountResult = Process.runSync('git', [ | ||
| 'rev-list', | ||
| '--count', | ||
| '$prevTag..HEAD', | ||
| ], workingDirectory: repoRoot); | ||
| final commitCount = commitCountResult.exitCode == 0 ? (commitCountResult.stdout as String).trim() : '?'; | ||
| buf.writeln('---'); | ||
| buf.writeln('Automated release by CI/CD pipeline (Gemini CLI + GitHub Actions)'); | ||
| buf.writeln('Commits since $prevTag: $commitCount'); | ||
|
|
@@ -98,18 +98,28 @@ abstract final class ReleaseUtils { | |
| static List<Map<String, String>> gatherVerifiedContributors(String repoRoot, String prevTag) { | ||
| final repo = Platform.environment['GITHUB_REPOSITORY'] ?? '${config.repoOwner}/${config.repoName}'; | ||
|
|
||
| // Step 1: Get one commit SHA per unique author email | ||
| final gitResult = Process.runSync('sh', [ | ||
| '-c', | ||
| 'git log "$prevTag"..HEAD --format="%H %ae" --no-merges | sort -u -k2,2', | ||
| // Step 1: Get one commit SHA per unique author email. | ||
| // Use array args to avoid shell injection via prevTag, then deduplicate | ||
| // in Dart instead of piping through `sort -u`. | ||
| final gitResult = Process.runSync('git', [ | ||
| 'log', | ||
| '$prevTag..HEAD', | ||
| '--format=%H %ae', | ||
| '--no-merges', | ||
| ], workingDirectory: repoRoot); | ||
|
|
||
| if (gitResult.exitCode != 0) { | ||
| Logger.warn('Could not get commit authors from git log'); | ||
| return []; | ||
| } | ||
|
|
||
| final lines = (gitResult.stdout as String).trim().split('\n').where((l) => l.isNotEmpty); | ||
| // Deduplicate by email (replaces the shell `sort -u -k2,2` pipe). | ||
| final seenEmails = <String>{}; | ||
| final lines = (gitResult.stdout as String).trim().split('\n').where((l) => l.isNotEmpty).where((l) { | ||
| final parts = l.split(' '); | ||
| if (parts.length < 2) return false; | ||
| return seenEmails.add(parts[1]); // returns false if already seen | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fallback contributor list now contains duplicate entriesMedium Severity The conversion from |
||
| final contributors = <Map<String, String>>[]; | ||
| final seenLogins = <String>{}; | ||
|
|
||
|
|
@@ -140,8 +150,8 @@ abstract final class ReleaseUtils { | |
| contributors.add({'username': login}); | ||
| } | ||
| } | ||
| } catch (_) { | ||
| // API call failed for this SHA, skip | ||
| } catch (e) { | ||
| Logger.warn('GitHub API call failed for commit $sha: $e'); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,14 +56,18 @@ ValidationResult validateInvestigationResult(String path) { | |
|
|
||
| /// Writes a JSON object to a file with pretty formatting. | ||
| void writeJson(String path, Map<String, dynamic> data) { | ||
| File(path).writeAsStringSync('${const JsonEncoder.withIndent(' ').convert(data)}\n'); | ||
| final file = File(path); | ||
| final label = file.uri.pathSegments.last; | ||
| print('[triage] Writing $label (${data.length} keys)'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unconditional print in writeJson pollutes stdout outputLow Severity The |
||
| file.writeAsStringSync('${const JsonEncoder.withIndent(' ').convert(data)}\n'); | ||
| } | ||
|
|
||
| /// Reads and parses a JSON file, returning null on error. | ||
| Map<String, dynamic>? readJson(String path) { | ||
| try { | ||
| return jsonDecode(File(path).readAsStringSync()) as Map<String, dynamic>; | ||
| } catch (_) { | ||
| } catch (e) { | ||
| print('[triage] Could not read JSON from $path: $e'); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||


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.
This release flow uses
_exec()(which printsargs.join(" ")in verbose mode) elsewhere in the same function to rungit remote set-url origin https://x-access-token:...@github.com/.... Without redaction in_exec(), enabling_verbosewill still leak the token even thoughCiProcessRunnernow redacts. Add a redaction step to_exec()'s verbose command log and its stderr logging (similar toCiProcessRunner._redact()).