Structural secret redaction - #191
Open
Liam-Doodson wants to merge 38 commits into
Open
Conversation
…and SafeFlags Build the common/redact package with Secret (a string wrapper with masked String/MarshalJSON and Reveal for actual values) and SafeFlags allow-list to classify CLI flags as safe-to-echo or unsafe. Include Args() function to mask values following unsafe flags, with comprehensive table-driven test coverage for Secret masking and Args flag value redaction across multiple flag types.
Add permanent regression guard test at common/redact/argscope_test.go that walks the repository's Go source tree and fails if raw os.Args[1:] reads appear outside sanctioned locations. Currently fails as expected, naming specific files/lines where the unredacted os.Args[1:] leaks are found (Finding 01). Will turn green as a side-effect of task-003 when the sanctioned capture point is introduced in neo4j-cli/aura/cmd/main.go. Test output demonstrates the guard catches violations at the code level with precise file and line numbers.
…Secret This change wraps the ClientSecret field with the redact.Secret type to ensure secrets are automatically masked in JSON output and display paths, while remaining unredacted in on-disk storage. Changes: - Changed AuraCredential.ClientSecret type from string to redact.Secret - Implemented custom UnmarshalJSON on AuraCredential to wrap loaded secrets - Modified Credentials.save() to reveal secrets when writing to disk - Updated token.go to call .Reveal() when building HTTP auth requests - Created new list_test.go promoted from audit finding F05 - Updated add_test.go and instance/get_test.go for field ordering The on-disk credentials file still stores the real, unredacted secret, while json.Encode calls (via Print) automatically mask it through Secret.MarshalJSON(). No changes needed to the Print() method itself. Test results: - new TestCredentialListMasksClientSecret: shows "****" in JSON output - new TestCredentialListFileStillContainsRealSecret: file has real value - all credential, clicfg, and redact package tests pass
…/error output This commit implements task-003 of the secret-redaction feature: - Promote audit test f01_audit_test.go to response_test.go, updated with working redacted-args implementation - Add package-level RedactedArgs variable in api package to store redacted command-line arguments - Introduce sanctioned capture points in both main() functions (neo4j-cli/aura/cmd/main.go and neo4j-cli/main.go) - Replace all 13 raw os.Args[1:] reads with RedactedArgs: - 11 panic/error branches in response.go - 1 in corspolicy/allowedorigin/utils.go - 2 in main() recover blocks (already captured as redactedArgs variables) - Update argscope guard test to include both main.go files as sanctioned locations - All tests pass including the guard regression test The test asserts that command-line secrets do not leak into panic messages even when unrecognized HTTP status codes are encountered.
…vert untyped-map save() hack
Replace the unsafe hand-built map[string]interface{} approach in credentials.save() with typed struct marshaling. This fixes a regression where new fields added to AuraCredential would silently vanish from the on-disk file.
Changes:
- Add private auraCredentialsOnDisk and auraCredentialOnDisk structs for on-disk representation
- Modify save() to use typed marshaling via auraCredentialsOnDisk
- Modify load() to unmarshal into typed struct then convert to in-memory format
- Delete AuraCredential.UnmarshalJSON - loading now works through struct tags
- Add conversion methods (toOnDisk, toAuraCredentials) to handle the round-trip
- Restore add_test.go to whole-file JSON assertions
- Add comprehensive test suite verifying typed structure and round-trip behavior
The on-disk file still contains the real, unredacted secrets, while display paths mask them via Secret.MarshalJSON(). The typed approach ensures field-to-key mapping lives in one place (Go struct tags) instead of three.
…edacted-args capture Addresses three related issues found in the security audit: 1. Fix response_test.go: The test was vacuous because it set os.Args directly while response.go reads from api.RedactedArgs (which was nil in tests). Updated test to set redacted args the same way main() does, and assert the mask (**** ) appears alongside safe tokens, not just that the raw secret is absent. 2. Collapse response.go's 11 near-identical panic/error strings into a single unexpectedStatusError() helper that is the sole reader of the redacted-args value. This eliminates duplication while preserving the exact message text. 3. Relocate the redacted-args capture point from api.RedactedArgs/ aura.SetRedactedArgs to common/redact package (new capture.go file). Both main() functions and corspolicy/allowedorigin/utils.go now read from redact.CapturedArgs() directly, eliminating the "two routes to the same variable" issue. - Create common/redact/capture.go with SetCapturedArgs/CapturedArgs functions - Update both neo4j-cli main.go files to call redact.SetCapturedArgs - Update utils.go to read from redact package - Delete aura.SetRedactedArgs and api.RedactedArgs - Simplify response.go with unexpectedStatusError helper - Improve response_test.go to assert redaction is actually working Sanctioned os.Args capture points remain unchanged (still two files).
…trim argscope_test.go
Fix three accuracy/quality issues in the common/redact package:
1. Args() masking now correctly masks unsafe flag values that start with '-'
(e.g., generated secrets like '-abc123'). Previously, values beginning
with '-' were incorrectly treated as flags and left unmasked.
2. SafeFlags is rebuilt to match actual CLI registered flags: changed 'dbid'
to 'db-id', removed non-existent flags ('id', 'format', 'credential-id',
'origin'), and added missing safe flags (organization-id, deployment-id,
key-id, server-id, source-instance-id, source-snapshot-id, import-type,
description, date, ttl, import-model-id). Made SafeFlags unexported
(safeFlags) since it's only used internally by Args().
3. Broadened argscope_test.go's regex pattern from '\bos\.Args\s*\[' to
'\bos\.Args\b' to catch any raw os.Args reference (not just indexed
access), including direct assignments and function arguments. Trimmed
the file from 153 to 131 lines by removing unnecessary comments and
helper functions while preserving the exclusion-list mechanism.
…branch Remove comments that merely restate what adjacent code already says via its name and signature. Keep comments that explain non-obvious constraints, invariants, or WHY decisions were made. Also remove task/process references from comments. No behavioral changes; only comments and blank lines modified.
…k representation Replace hand-maintained auraCredentialOnDisk mirror with embedding approach. auraCredentialOnDisk now embeds *AuraCredential and shadows only the ClientSecret field (as plain string for on-disk storage). This ensures new fields added to AuraCredential are automatically preserved in the on-disk JSON representation without requiring manual edits to the mirror struct. Conversion methods toOnDisk/toAuraCredentials now handle only the shadowed field. Added MarshalJSON for auraCredentialOnDisk to ensure correct JSON field ordering. Updated tests to verify embedding preserves all struct fields via reflection.
…e-blob JSON assertion Restore the strict whole-file JSON blob comparison in TestUnauthorizedAccessTokenRefresh that was weakened during task-004 to tolerate the untyped-map save() hack's non-deterministic key ordering. Task-007 reverted that hack to a typed representation, restoring deterministic key order, making this assertion valid again. This is a pure test-quality fix with no production code change.
…an flags Fixed regression from task-009 where Args() was unconditionally masking the token after any unsafe flag, including boolean flags that don't take values. Now distinguishes between flags that take values and boolean flags using an explicit set of known boolean flags. For commands like "data-api graphql create --enabled --name my-api", the flag name "--name" is no longer incorrectly masked. - Added booleanFlags map identifying flags that don't consume next argument - Updated Args() condition to skip value consumption for boolean flags - Added test case for unsafe boolean flag followed by safe flag with value - Verified existing dash-prefixed secret case from task-009 still passes
fmt.Sprintf("%s", resBody) on a []byte is a no-op conversion; use
string(resBody) directly. Flagged by CI's staticcheck run on the draft PR.
Leftover from an earlier UnmarshalJSON approach, no longer referenced after task-012's embedding rewrite. Flagged by staticcheck (U1000).
Whitespace/formatting only, no logic change.
Third audit pass found the embedding fix (task-012) traded the mirror-struct field-drop risk for two new problems: a nil-pointer panic when loading a credential object missing every promoted field, and a custom MarshalJSON that reintroduced a hand-copied field list on the write path (making the comment claiming automatic field preservation false again). - Delete auraCredentialOnDisk.MarshalJSON entirely; it existed only for cosmetic key ordering. Removing it removes the second hand-copied list and makes the existing comment accurate again (trimmed for the no-comments policy regardless, since a comment asserting a guarantee is a liability). - Fix toAuraCredentials to handle a nil embedded *AuraCredential instead of dereferencing it, with a regression test loading a partial credential object. - Update the two credential-list JSON test assertions and one get_test.go assertion for the new (default, unforced) key order. Also addresses the second pass's non-blocking findings while in this code: - booleanFlags (task-010): removed a phantom "show-progress" entry, added the two flags cobra registers automatically (help, version). - Deleted a vacuous assertion in TestArgsPreservesStructure and fixed a stale test-case name left over from before task-010. - unexpectedStatusError's variadic-as-optional body param split into a plain no-body function and a WithBody variant. - Removed leftover WHAT-comments in list_test.go, a redundant import alias in both main.go entrypoints, an assert.True/strings.Contains where assert.Contains already reads better one line up, and a test name/comment referencing a since-removed triage artifact.
Two more comments asserting guarantees the code didn't hold (the exact class of issue that motivated the branch's comment policy): - TestOnDiskJSONStructure's comment claimed new fields are "preserved" when the test's hardcoded allowlist actually fails on any unrecognised field. Consolidated it with TestEmbeddingPreservesNewFields (which had it right, via reflection) into one test; deleted TestConversionMethods as redundant with TestRoundTripSaveAndLoad. - redact.go's safeFlags comment claimed "any flag not in this map is masked" — false since booleanFlags entries are never masked either (a token after a bool flag is positional, correctly left alone). Removed the comment rather than trying to describe both maps' interplay. Also, per the verified-safe simplification the audit found: embed AuraCredential by value instead of by pointer in auraCredentialOnDisk. Nothing needed the pointer (toOnDisk builds fresh, toAuraCredentials copies out), so this removes the nil-guard branch entirely rather than just testing it — the "partial object" case is now a zero-valued struct, not a nil pointer that must be checked. Smaller fixes: removed "version" from booleanFlags (it takes a value on `instance create`; only safe by accident since it's also in safeFlags). Removed a redundant guard in argscope_test.go's comment-stripping (fully subsumed by the truncation two lines below). Collapsed the local redactedArgs variable in both main() entrypoints into one CaptureArgs call read back via CapturedArgs(), same as every other call site.
capture.go's comment asserted "set by main() at the top of each CLI entrypoint" - already loose (CaptureArgs is also called from tests) and unenforced, the same shape as the comments removed in the prior pass. Also dropped a one-line WHAT-restatement in redact_test.go.
…s to MaskArgs Part 1: Treat AuraCredential.AccessToken as a redact.Secret type to mask it in display output (credential list) while preserving the real token value on disk and in authorization contexts. Updated AccessToken type in both the runtime AuraCredential struct and the on-disk auraCredentialOnDisk mirror, with proper Reveal() calls where the real token is needed (getToken, HasValidAccessToken). Extended existing credential list tests with access-token masking cases. Part 2: Renamed redact.Args() to redact.MaskArgs() for clarity (the function name alone now conveys it masks arguments). Updated the single internal call site in CaptureArgs and renamed all test functions consistently (TestArgs → TestMaskArgs, TestArgsMasksSecrets → TestMaskArgsMasksSecrets, TestArgsPreservesStructure → TestMaskArgsPreservesStructure). Acceptance criteria met: - credential list's JSON output masks access-token with "****" - On-disk credentials file stores the real access token in plaintext - HasValidAccessToken correctly compares revealed token against empty string - MaskArgs function renamed throughout common/redact with no behavior change - go build/vet/gofmt/staticcheck/test all pass
Liam-Doodson
commented
Aug 20, 2026
…or messages Update all "please report an issue" messages that incorrectly pointed to https://github.com/neo4j/cli to use the correct repository URL https://github.com/neo4j/aura-cli instead. This fix applies to all 7 occurrences across error/panic messages in main entry points and error handlers.
Refactor MaskArgs in common/redact/redact.go to improve readability by: - Extracting flag parsing logic into a separate parseFlagFromArg function that handles both --flag value and --flag=value forms - Improving clarity of the main loop structure Also fix three t.Errorf messages in common/redact/redact_test.go that still referred to Args() instead of MaskArgs() after the earlier rename.
…assert Convert argscope_test.go and credentials_test.go from manual t.Errorf/t.Fatalf checks to github.com/stretchr/testify/assert equivalents, matching the assertion style used throughout the rest of this branch's test files.
…y conversion task-019 converted manual t.Fatalf/t.Fatal checks to testify/assert in two test files, but several of these checks are preconditions that later code in the same test depends on. assert.* logs a failure and continues execution, unlike t.Fatalf which halts immediately via runtime.Goexit(). This turned clean test failures into actual hangs and panics: - findRepoRoot's loop termination guard (assert.NotEqual checking parent == current) would spin forever if the assertion fired - TestOnDiskJSONStructure's chain of type-assertion guards would continue with nil values, causing index-out-of-range panics Fix by changing these specific precondition-guarding checks from assert.* to require.* (same github.com/stretchr/testify/require package, which calls t.FailNow() to stop execution). Leave genuinely final assertions as assert.* since nothing downstream depends on their checked values. - common/redact/argscope_test.go: findRepoRoot's two precondition checks - common/clicfg/credentials/credentials_test.go: TestOnDiskJSONStructure's four type-assertion checks that guard subsequent indexing/use
…kJSONStructure task-020's fix missed one: the "expected 1 credential" length check still guards the very next line's credentialsData[0] index. Same bug class - assert.Equal doesn't halt on failure, so an empty credentialsData would panic on the index rather than failing cleanly. Changed to require.Equal.
…unction Refactored MaskArgs() to extract flag-processing logic into a single maskArg() helper that returns both output tokens and how many additional args were consumed. This eliminates three readability issues: the index-based loop with in-place mutation (result[len(result)-1]), the compound boolean condition, and the nesting in the main flag-handling logic. The main loop is now a simple for-i with append and advance-by-extraConsumed, while maskArg uses early returns for each case (non-flag, inline value, boolean flag, no next arg, safe+flag-like, safe+value, unsafe+value). No behavioral change; all existing tests pass unchanged.
The action's version input defaults to "latest" when unset, so CI was resolving go install honnef.co/go/tools/cmd/staticcheck@latest against whatever Go "stable" happened to be at that moment. The two floating versions drifted out of sync, producing a staticcheck internal error decoding newer Go export data it doesn't yet support. Pin to 2026.1, verified locally against the actual toolchain in use.
The prior refactor still iterated via for i := 0; i < len(args); i++, which was the specific pattern flagged as hard to follow. Rewritten so maskArg operates on the remaining slice's front element and reports how many leading elements it consumed; MaskArgs shrinks remaining by that amount each iteration instead of tracking an index.
go-version: "stable" resolved to Go 1.27.0 on the runner, a release newer than staticcheck 2026.1's bundled export-data parser supports (format version 4 vs. max supported 2) -- the actual source of the CI staticcheck failure, not the staticcheck version itself. Pin to 1.26.2, the combination verified locally against staticcheck 2026.1.
Matches the style already used elsewhere on this branch (argscope_test.go, credentials_test.go). Precondition-guarding checks that later code in the same test depends on (element-count checks before indexing, marshal/unmarshal errors before using the result, type assertions before using the asserted value) use require so a failure halts instead of continuing into a panic.
Liam-Doodson
commented
Aug 20, 2026
Liam-Doodson
marked this pull request as ready for review
August 20, 2026 10:51
Liam-Doodson
changed the title
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
common/redact: aSecretstring-wrapper type (maskedString()/MarshalJSON(), explicitReveal()), aSafeFlagsallow-list built from this CLI's actual registered flags, andArgs()to redact an argv-shaped slice.common/redact/argscope_test.go) initially caught 13 rawos.Argsreads acrossresponse.go's panic branches, a secondneo4j-cli/main.goentrypoint, and onecorspolicyleak — all now fixed via two sanctioned capture points (one per CLI binary entrypoint), with the guard green.AuraCredential.ClientSecretis now aredact.Secret;credential listoutput is masked while the on-disk credentials file still stores the real value.changiefragment describing the change without naming trigger conditions.