fix: resolve security, performance and output backlog#265
Merged
Conversation
The target argument was concatenated into the sync-state file path without sanitization while only id was sanitized, letting untrusted input (e.g. ../) escape the ~/.skret/sync-state directory. Sanitize both target and id, and harden sanitizeID to collapse .. sequences, neutralize NUL bytes and path separators, and reject empty/dot-only results.
Preallocate the attribute slice in RedactingHandler.Handle using NumAttrs() and add attributes with a single variadic AddAttrs call instead of a per-attribute loop. Preallocate slices with known capacity in local provider GetBatch and the import dedup path.
When no secrets are found, list --format=json and env --format=json/yaml
now still emit the valid empty structure ([] or {}) on stdout while
routing the human-readable hint to stderr, so scripts parsing the
output keep working. Also preallocate the filter/env-pair slices.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The skret run command replaces the process image via syscall.Exec on Unix, so executing it inline terminated the test binary and silently skipped every cli test ordered after it. Drive it from a re-exec of the test binary instead, and back the env empty-state test with a dedicated empty local store rather than a path filter the env command does not apply.
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
Consolidates the distinct substantive changes from the open bot/Renovate PR backlog into clean, tested commits.
StatePathForconcatenatedtargetinto the path unsanitized (onlyidwas sanitized), letting../escape~/.skret/sync-state. Bothtargetandidare now sanitized, andsanitizeIDcollapses.., neutralizes NUL bytes / separators, and rejects empty/dot-only results.RedactingHandler.Handle(preallocated attr slice + variadicAddAttrs) and in the local-provider / import batch paths.list --format=jsonandenv --format=json/yamlnow emit the valid empty structure ([]/{}) on stdout while routing the human hint to stderr.cloudflare/wrangler-actionto v4.New tests cover 8 path-traversal vectors (target + id), end-to-end traversal containment, the redaction handler with many/zero attrs, and empty machine-readable output.
Supersedes bot PRs #255, #258, #260, #261, #262, #263, #264 and Renovate PR #257.
Test plan
go build ./...go test -race ./...— all packages passgolangci-lint run ./...(v2.12.2 / go1.26) — 0 issuesgo vet ./.../gofmt -lcleanhttps://claude.ai/code/session_01USTAfaQs3ZtdhxvBrz44DJ
Generated by Claude Code