Add comprehensive security scanning and hardening#39
Merged
Conversation
Add gosec to the enabled linters list in golangci-lint configuration. This catches common Go security issues including injection vulnerabilities, weak crypto, file permission issues, and more.
- Update pre-commit to golangci-lint v2.1.6 (match CI) - Add gitleaks hook to pre-commit config - Add GitHub Actions workflow for secret scanning on push/PR - Add .gitleaks.toml to allowlist public OAuth client secrets (native CLI apps cannot hide client secrets; security relies on PKCE)
- Configure GoReleaser to generate CycloneDX SBOMs for release archives - Add Cosign keyless signing for checksums via Sigstore - Install cosign in release workflow before GoReleaser runs
Add Go native fuzz tests for: - dateparse.ParseFrom: handles relative dates, weekdays, offsets - URL path parsing: extracts IDs from Basecamp URLs These help find edge cases and panics in user-facing input parsing.
New targets: - make security: runs lint + vuln + secrets - make vuln: runs govulncheck for dependency vulnerabilities - make secrets: runs gitleaks for secret detection - make fuzz: runs fuzz tests (30s each) - make fuzz-quick: quick fuzz run (10s each)
Address all gosec findings across the codebase: G101 (hardcoded credentials): - Mark public OAuth client secret as intentional with nolint comment (native CLI apps embed client secrets; security relies on PKCE) G104 (unhandled errors): - Add _ = for best-effort cleanup operations (file removal, cache writes) - Add _ = for operations where errors don't affect correctness (summary display, flag marking, etc.) - Handle crypto/rand.Read errors with panic (should never fail) G112 (Slowloris): - Add ReadHeaderTimeout to OAuth callback HTTP server G204 (subprocess): - Document that browser open command is hardcoded per-platform G301/G306 (file permissions): - Tighten directory permissions: 0755 → 0750 - Tighten file permissions: 0644 → 0600 G304 (file inclusion): - Add nolint comments for file reads from trusted config/cache paths G404 (weak random): - Document that math/rand is appropriate for jitter (not security-sensitive)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae06490e2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The gitleaks-action@v2 requires a paid license for organizations. Install and run gitleaks CLI directly, which is free for all use.
Single source of truth for gitleaks configuration and allowlist handling. Local dev and CI now use the same command.
Keyless signing uses Sigstore's Fulcio CA and Rekor transparency log, which requires an OIDC identity token from GitHub Actions. The id-token: write permission enables cosign to obtain this token.
Keyless signing is the default in cosign v2+. The COSIGN_EXPERIMENTAL=1 env var was only needed in cosign v1.x to opt into the feature. Added a comment noting the OIDC permission requirement.
3 tasks
jeremy
added a commit
that referenced
this pull request
Feb 19, 2026
Add comprehensive security scanning and hardening
jeremy
added a commit
to brianevanmiller/basecamp-cli
that referenced
this pull request
Mar 18, 2026
extractAttr returned raw HTML-encoded strings, so filenames like O&basecamp#39;Brien displayed with visible entities. Apply html.UnescapeString on the read path to match the encoding done by escapeAttr on writes.
jeremy
added a commit
to brianevanmiller/basecamp-cli
that referenced
this pull request
Mar 18, 2026
extractAttr returned raw HTML-encoded strings, so filenames like O&basecamp#39;Brien displayed with visible entities. Apply html.UnescapeString on the read path to match the encoding done by escapeAttr on writes.
jeremy
added a commit
that referenced
this pull request
Mar 18, 2026
* Add attachment listing for items Introduces `basecamp attachments list <id|url>` to enumerate file attachments embedded in Basecamp item rich text by parsing <bc-attachment> tags. Supports all recording types with auto-detection from URLs, styled TTY and JSON output, and context-aware breadcrumbs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Decode HTML entities in attachment attribute values extractAttr returned raw HTML-encoded strings, so filenames like O'Brien displayed with visible entities. Apply html.UnescapeString on the read path to match the encoding done by escapeAttr on writes. * Prefer comment ID when resolving attachment list URLs Comment URLs like .../todos/111#__recording_789 were using the parent recording ID (111) instead of the comment ID (789). Follow the established convention from comment.go and prefer CommentID when the URL fragment is present, auto-setting type to "comment". * Support question_answers type in attachment listing Check-in answer URLs auto-detect as type "question_answers" but typeToEndpoint had no case for it, causing "Unknown type" errors. Add "answer" and "question_answers" aliases and update all hint strings. * Address remaining PR review feedback - Guard CommentID preference on compatible recordType to avoid type/ID mismatch when --type is explicitly set - Add \b boundary to reBcAttachmentTag to prevent matching similarly-prefixed tag names like bc-attachment-foo - Use strings.EqualFold for mention content-type filtering and attribute name matching in extractAttr * Normalize breadcrumb types, fix parameter shadowing, case-insensitive IsImage - Normalize type aliases in show breadcrumb so copied commands work (e.g. question_answers → checkin, schedule_entries → schedule-entry) - Rename ParseAttachments parameter from html to content to avoid shadowing the html import - Make IsImage() case-insensitive with EqualFold prefix check * Omit --type for answer/question_answers in show breadcrumb show has no type alias for question_answers, so --type checkin would fetch the parent question instead of the answer. Return "" to let generic recording lookup find the correct endpoint. * Extract resolveAttachmentTarget helper, fix tag boundary, update comment - Extract URL/type resolution into resolveAttachmentTarget so tests exercise the actual function rather than duplicating logic inline - Replace \b with [\s/>] in reBcAttachmentTag since Go regex treats - as non-word, allowing <bc-attachment-custom> to match - Update typeToEndpoint comment to note it's a superset of show.go * Update surface snapshot with --in flag for attachments The --in persistent flag from main was missing from the attachments surface entries after the merge. * Fix reBcAttachmentTag to match bare and uppercase tags The regex required at least one char in [\s/>] after the tag name, which failed on bare <bc-attachment> with no attributes. Switch to (\s[^>]*|) to allow empty attrs while still requiring whitespace before any attribute names (preventing bc-attachment-foo matches). Add tests for uppercase tag names and bare tags. * Address review: NBSP normalization, variable shadowing, dedup validation - Normalize NBSP to regular space in extractAttr after html.UnescapeString - Rename shadowed recordType to resolvedType in runAttachmentsList - Remove duplicate type validation from fetchItemContent (caller validates) - Update SKILL.md to show <id|url> instead of <id> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Jeremy Daer <jeremy@37signals.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sets up modern security scanning, vulnerability detection, and supply chain security tooling for the bcq CLI.
Security Tooling Added
gosec Issues Fixed
All findings addressed across 24 files:
_ =for best-effort opsNew Makefile Targets
Test plan
make lintpasses (0 gosec issues)make vulnpasses (no vulnerabilities)make secretspasses (no leaks detected)make fuzz-quickpasses (dateparse + URL fuzzing)go test ./...passes (all tests green)