fix: sanitize Google-sourced text in every printer by rianjs · Pull Request #24 · open-cli-collective/google-cli

rianjs · 2026-09-04T01:27:40Z

Summary

New internal/sanitize package (Output, Filename) replaces the copy that lived in the mail command package; the two write packages that imported mail just to reach it now import sanitize.
Every printer under internal/cmd/* and internal/rwcmd/* wraps Google-sourced text at the print site: file names, contact names and fields, event summaries and locations, label and folder names, filter criteria, attachment names, profile display names and emails. JSON output and identifiers are untouched.
New architecture test TestPrintedDTOTextIsSanitized: collects exported string/[]string fields from the DTOs in internal/api/* (minus an explicit identifier exclusion list), then walks every print, view, and table-row call in the command packages and fails on any DTO text field not wrapped in sanitize.Output/sanitize.Filename. A floor of twenty wrapped sites keeps the detector honest.
Golden principle 6 documents the rule; the architecture doc lists the shared package.

Closes #22

Test plan

make check, make test-cover-check (75.6%), go test ./internal/architecture/
Negative check: unwrapping one event.Summary print fails the test at the right file and line
gro drive --help and grw mail send --help unchanged

Moves the terminal sanitizer to internal/sanitize and applies it wherever names, summaries, addresses, subjects, or filenames from Google reach the terminal. TestPrintedDTOTextIsSanitized keeps it that way. Closes #22

monit-reviewer

Automated PR Review

Reviewed commit: d2bfa64

Summary

Reviewer	Findings
documentation:docs-reviewer	1
harness-engineering:harness-enforcement-reviewer	1
harness-engineering:harness-self-documenting-code-reviewer	1
security:security-code-auditor	1

documentation:docs-reviewer (1 findings)

💡 Suggestion - docs/architecture.md:43

The updated support-package list adds both sanitize and view, but the PR description only introduces internal/sanitize; there is no mention of a new or previously-undocumented view package. Confirm internal/view exists and that its addition here is intentional rather than an unrelated/accidental edit, since an incorrect package name in this list would mislead readers about the codebase's structure.

harness-engineering:harness-enforcement-reviewer (1 findings)

⚠️ Should Fix - internal/architecture/sanitize_test.go:108

The new detector (isOutputCall/inspectPrintedArgument) only matches direct struct-field selectors (e.g. event.Summary) against the DTO text-field set. It does not recognize text reached through a getter method (e.g. contact.GetDisplayName(), contact.GetPrimaryEmail(), contact.GetPrimaryPhone(), contact.GetOrganization() in internal/cmd/contacts/output.go), since the call's selector name never matches a raw field name in the collected set. Those exact call sites are correctly wrapped in sanitize.Output today, but if a future edit drops the wrapper around a getter-derived value, this architecture test — the enforcement mechanism golden-principles.md item 11 cites — would not catch the regression. Worth extending the detector to also flag calls whose selector matches 'Get' for any field in the DTO text-field set.

harness-engineering:harness-self-documenting-code-reviewer (1 findings)

💡 Suggestion - internal/architecture/sanitize_test.go:22

files, paths := parseNonTestFiles(t, dir), nonTestGoFiles(t, dir) produces two independently-computed slices that the loop below assumes stay index-aligned (paths[i] is used to report errors for files[i]). That invariant — both functions must list the same directory's .go files in the same order — isn't stated anywhere. Consider a single helper that returns matched (ast.File, path) pairs together, or a comment noting the required ordering.

security:security-code-auditor (1 findings)

⚠️ Should Fix - internal/rwcmd/drive/output.go:37

Terminal injection regression: printFile previously sanitized file.ID and file.MimeType, but this PR now prints them raw (fmt.Printf("ID: %s\n", file.ID) and fmt.Printf("Type: %s\n", file.MimeType)), while only Name and Parents keep sanitize.Filename/sanitize.Output. The function's own preserved comment states a malicious collaborator may have set these fields and that they are sanitized before reaching the terminal — but two of the four fields no longer are. Drive mimeType/ID are attacker-controllable by any external collaborator who shares a file with the victim, so ANSI escape sequences embedded there would print unsanitized when the victim runs a grw drive command that calls printFile. Wrap both fields with sanitize.Output, consistent with the rest of this PR and the file's own comment.

1 info-level observations excluded. Run with --verbose to include.

Completed in 2m 36s | $3.41 | sonnet | daemon 0.2.142 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	2m 36s wall · 2m 32s compute (Reviewers: 1m 44s · Synthesis: 48s)
Cost	$3.41 (estimated)
Tokens	650.6k in / 37.7k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	63.0k	4.2k	26.4k	36.5k (1h)	$0.29
documentation:docs-reviewer	sonnet	55.1k	2.9k	26.4k	28.7k (1h)	$0.22
harness-engineering:harness-architecture-reviewer	sonnet	103.1k	2.5k	26.4k	76.7k (1h)	$0.50
harness-engineering:harness-enforcement-reviewer	sonnet	106.9k	5.7k	26.4k	80.4k (1h)	$0.58
harness-engineering:harness-knowledge-reviewer	sonnet	105.0k	3.9k	26.4k	78.6k (1h)	$0.54
harness-engineering:harness-self-documenting-code-reviewer	sonnet	111.3k	11.2k	26.4k	84.9k (1h)	$0.69
security:security-code-auditor	sonnet	106.2k	7.3k	26.4k	79.8k (1h)	$0.60

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

monit-reviewer · 2026-09-04T01:30:24Z

-	fmt.Printf("Type: %s\n", mailcmd.SanitizeOutput(file.MimeType))
+	fmt.Printf("ID: %s\n", file.ID)
+	fmt.Printf("Name: %s\n", sanitize.Filename(file.Name))
+	fmt.Printf("Type: %s\n", file.MimeType)


🟡 Medium (security:security-code-auditor): Terminal injection regression: printFile previously sanitized file.ID and file.MimeType, but this PR now prints them raw (fmt.Printf("ID: %s\n", file.ID) and fmt.Printf("Type: %s\n", file.MimeType)), while only Name and Parents keep sanitize.Filename/sanitize.Output. The function's own preserved comment states a malicious collaborator may have set these fields and that they are sanitized before reaching the terminal — but two of the four fields no longer are. Drive mimeType/ID are attacker-controllable by any external collaborator who shares a file with the victim, so ANSI escape sequences embedded there would print unsanitized when the victim runs a grw drive command that calls printFile. Wrap both fields with sanitize.Output, consistent with the rest of this PR and the file's own comment.

Reply to this thread when addressed.

Addressed in cce4479: ID and MIME type are wrapped again and the comment now says every field is sanitized.

monit-reviewer · 2026-09-04T01:30:24Z

+	}
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return false


🟡 Medium (harness-engineering:harness-enforcement-reviewer): The new detector (isOutputCall/inspectPrintedArgument) only matches direct struct-field selectors (e.g. event.Summary) against the DTO text-field set. It does not recognize text reached through a getter method (e.g. contact.GetDisplayName(), contact.GetPrimaryEmail(), contact.GetPrimaryPhone(), contact.GetOrganization() in internal/cmd/contacts/output.go), since the call's selector name never matches a raw field name in the collected set. Those exact call sites are correctly wrapped in sanitize.Output today, but if a future edit drops the wrapper around a getter-derived value, this architecture test — the enforcement mechanism golden-principles.md item 11 cites — would not catch the regression. Worth extending the detector to also flag calls whose selector matches 'Get' for any field in the DTO text-field set.

Reply to this thread when addressed.

Addressed in cce4479: the text-source set now includes every exported argument-less string getter declared on the DTOs, so contact.GetDisplayName() and friends are held to the same rule. Verified by unwrapping one getter site and watching the test fail.

Follow-up in 774ac60: the getter rule is limited to Get* methods; the first cut also caught Event.FormatTimeRange, which renders only dates.

monit-reviewer · 2026-09-04T01:30:24Z

+	for _, kind := range []string{"cmd", "rwcmd"} {
+		for _, pkg := range packageDirs(t, kind) {
+			dir := filepath.Join(root, "internal", kind, pkg)
+			files, paths := parseNonTestFiles(t, dir), nonTestGoFiles(t, dir)


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): files, paths := parseNonTestFiles(t, dir), nonTestGoFiles(t, dir) produces two independently-computed slices that the loop below assumes stay index-aligned (paths[i] is used to report errors for files[i]). That invariant — both functions must list the same directory's .go files in the same order — isn't stated anywhere. Consider a single helper that returns matched (ast.File, path) pairs together, or a comment noting the required ordering.

Reply to this thread when addressed.

Addressed in cce4479: a nonTestSources helper returns (path, fileset, file) triples, and line numbers come from the file set instead of a hand-rolled count.

monit-reviewer · 2026-09-04T01:30:24Z

 - `cmd/gro` cannot reach `internal/rw` or `internal/rwcmd` through its link graph.

-Support packages such as `auth`, `bulk`, `config`, `keychain`, `output`, and `testutil` stay under `internal` and are available to both applications where appropriate.
+Support packages such as `auth`, `bulk`, `config`, `keychain`, `output`, `sanitize`, `testutil`, and `view` stay under `internal` and are available to both applications where appropriate.


🔵 Low (documentation:docs-reviewer): The updated support-package list adds both sanitize and view, but the PR description only introduces internal/sanitize; there is no mention of a new or previously-undocumented view package. Confirm internal/view exists and that its addition here is intentional rather than an unrelated/accidental edit, since an incorrect package name in this list would mislead readers about the codebase's structure.

Reply to this thread when addressed.

internal/view exists (the Success/Info/Error output helper used by init and profiles) and was missing from the list; adding it alongside sanitize is intentional.

monit-reviewer

Automated PR Review

Reviewed commit: 774ac60 | Previous: d2bfa64 (incremental)

Summary

No issues found.

4 PR discussion threads considered.

Completed in 1m 16s | $8.73 | sonnet | daemon 0.2.142 | Glorfindel

Field	Value
Model	sonnet
Mode	Re-review · Cycle 2 · Session resumed
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	1m 16s wall · 57s compute (Reviewers: 41s · Synthesis: 16s)
Cost	$8.73 (estimated)
Tokens	1698.5k in / 46.4k out
Turns	28

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	141.7k	5.2k	89.4k	52.2k (1h)	$0.42
documentation:docs-reviewer	sonnet	121.5k	3.2k	48.9k	72.6k (1h)	$0.50
harness-engineering:harness-architecture-reviewer	sonnet	267.5k	2.9k	54.3k	213.2k (1h)	$1.34
harness-engineering:harness-enforcement-reviewer	sonnet	279.8k	7.2k	54.3k	225.5k (1h)	$1.48
harness-engineering:harness-knowledge-reviewer	sonnet	273.2k	4.4k	54.3k	218.9k (1h)	$1.40
harness-engineering:harness-self-documenting-code-reviewer	sonnet	295.2k	15.0k	54.3k	240.9k (1h)	$1.69
security:security-code-auditor	sonnet	276.7k	7.8k	54.3k	222.4k (1h)	$1.47
discussion-summarizer	—	42.9k	789	0	42.8k (1h)	$0.45

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

fix: sanitize Google-sourced text in every printer

d2bfa64

Moves the terminal sanitizer to internal/sanitize and applies it wherever names, summaries, addresses, subjects, or filenames from Google reach the terminal. TestPrintedDTOTextIsSanitized keeps it that way. Closes #22

monit-reviewer suggested changes Sep 4, 2026

View reviewed changes

rianjs added 2 commits September 3, 2026 21:31

fix(sanitize): cover DTO getters and re-wrap drive ids

cce4479

test(sanitize): treat only Get* methods as DTO text getters

774ac60

monit-reviewer approved these changes Sep 4, 2026

View reviewed changes

rianjs merged commit 0f74ec7 into main Sep 4, 2026
11 checks passed

rianjs deleted the fix/22-shared-sanitizer branch September 4, 2026 01:35

Navigation Menu

fix: sanitize Google-sourced text in every printer - #24

fix: sanitize Google-sourced text in every printer#24
rianjs merged 3 commits into
mainfrom
fix/22-shared-sanitizer

rianjs commented Sep 4, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

monit-reviewer Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

monit-reviewer Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

monit-reviewer Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rianjs commented Sep 4, 2026

Summary

Test plan

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants