Resolve v1.10 technical debt: gosec gate, integration suite, test hygiene#38
Open
jjuanrivvera99 wants to merge 10 commits into
Open
Resolve v1.10 technical debt: gosec gate, integration suite, test hygiene#38jjuanrivvera99 wants to merge 10 commits into
jjuanrivvera99 wants to merge 10 commits into
Conversation
The commands/testing package was never wired to the real getAPIClient() function, so its mock server was silently bypassed on every invocation. No Go file outside the package imported it. Delete the dead code and move the debt item to Resolved in TECHNICAL_DEBT.md.
Add newTestClient(t, serverURL) in internal/api/testhelpers_test.go and migrate all 201 identical four-line NewClient construction blocks (BaseURL: server.URL, Token: "test-token", RequestsPerSec: 10) to use it. Migration was done with a Python regex script; follow-up fixes converted bare `err =` reuses to `err :=` in functions where the NewClient block was the only prior declaration of err. Leaves variant call sites untouched (different tokens, RetryInitialBackoff, TokenSource, AsUserID, cache, DryRun fields).
Adds test/integration/ with 12 cases exercising the compiled canvas binary as a product: version, help, unknown command, courses list (table/JSON/CSV), missing required flag, 401 handling, alias expansion, context propagation, dry-run token redaction, and repl --help. Each test gets its own isolated HOME/USERPROFILE temp directory and a per-test httptest mock Canvas server; no shared state between tests. The suite is gated behind //go:build integration so go test ./... is unaffected. Also adds a CI integration job (ubuntu-only), a make test-integration target, and a docs paragraph explaining the suite and its invocation.
…lper Introduces mustMarkRequired in commands/helpers.go to replace all 205 cmd.MarkFlagRequired(...) calls that were silently dropping errors (G104). The new helper panics on a missing flag name — a programmer error caught by any test that builds the command tree — so the error can never be silently lost at runtime. Beyond G104, each remaining gosec rule is addressed: - G112: add ReadHeaderTimeout to the OAuth callback and webhook HTTP servers - G117: #nosec on token marshal calls (output goes to keyring/AES-GCM, not logs) - G122/G304/G306: #nosec in gendocs (developer tool, controlled output dir) - G204: #nosec on browser-open exec.Command (URL is constructed internally) - G301: fix update state dir to 0750; #nosec for docs/skills dirs (world-readable by design) - G302: #nosec on self-update binary chmod (executable bits required) - G304: #nosec on all file paths that are user-controlled by design (--file, --destination flags) or derived from internal app directories (config, cache, token store) - G401/G501: #nosec on MD5 imports/uses (cache filename hashing, not cryptographic) - Remaining G104s: use _ = assignment for best-effort cleanup (os.Remove, Close) and #nosec for interactive Scanln prompts and viper.BindPFlag calls All #nosec annotations include rule IDs and one-line justifications. TECHNICAL_DEBT.md updated to mark the gosec backlog as resolved.
Remove -no-fail from the gosec action args. The previous comment explaining ~300 open findings is replaced with a note that all findings have been resolved and gosec now enforces a zero-finding gate on every PR.
# Conflicts: # TECHNICAL_DEBT.md
# Conflicts: # TECHNICAL_DEBT.md
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The stacked '#nosec ... //nolint' comment satisfied gosec but not golangci-lint's errcheck, which requires nolint to start its own comment. Explicit discards satisfy both without annotations.
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
2 participants
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
Resolves all four active items from TECHNICAL_DEBT.md.
gosec: 283 findings → 0, CI now blocking
mustMarkRequiredhelper checks the 205 previously-ignoredMarkFlagRequirederrors (G104 bulk)ReadHeaderTimeoutadded to the OAuth callback and webhook servers (G112, slowloris hardening); update-state dir perms tightened to 0750_ =for best-effort cleanups or#nosecannotations carrying written justifications-no-failremoved from the CI gosec step — the scanner now gates merges alongside govulncheckBinary-level integration tests (new)
test/integrationbehind theintegrationbuild tag: compiles the binary once, drives it viaos/execagainst a mock Canvas server--dry-runtoken redaction (asserts zero requests reach the server), and the repl/shell aliasmake test-integrationlocally; ~2s runtime, flake-checked over three runsTest hygiene
commands/testingframework — it never routedgetAPIClient()to its mock server, so its tests looked like integration tests but exercised no HTTP dispatchnewTestClient(t, serverURL)helper ininternal/apitests; 201 exact-pattern duplicated constructions migrated by script (variant constructions intentionally left explicit)Docs
Test plan
gosec ./...exits 0go test ./... -count=1green (integration suite correctly excluded without the tag)go test -tags integration ./test/integration/green, 3 consecutive runsgo test -race ./internal/api/ ./commands/greengofmt/go vetclean