Remove rendered manifest values from verbose logging (#649) - #668
Draft
leynos wants to merge 3 commits into
Draft
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueWarning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
Contributor
Reviewer's GuideThe PR closes the verbose logging secret leak by eliminating rendered-manifest JSON serialization, emitting only bounded structural counts through telemetry, and adding an end-to-end test that verifies env()-resolved secrets remain in generated output but never appear in CLI logs; associated dead localization entries are removed. Sequence diagram for secret-safe verbose manifest telemetrysequenceDiagram
participant CLI
participant Runner
participant Manifest as NetsukeManifest
participant Telemetry
participant Output as NinjaOutput
CLI->>Runner: generate_ninja_with_shell()
Runner->>Manifest: load_manifest_with_stage_reporting()
Manifest-->>Runner: rendered manifest
Runner->>Telemetry: record_manifest_structure(manifest)
Telemetry->>Telemetry: describe_counter()
Telemetry->>Telemetry: trace_span(variable_count, macro_count, rule_count, action_count, target_count, default_count)
Telemetry->>Telemetry: counter().increment(1)
Runner->>Output: generate Ninja graph
Output-->>CLI: generated file contains resolved values
Runner-->>CLI: verbose logs contain structural counts only
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace the raw AST JSON dump with bounded structural telemetry that emits only fixed-vocabulary aggregate counts of manifest collections. Retire the runner.context.serialise_manifest Fluent key across all locales now that no call site remains.
Replace the retired raw AST dump with a fixed-vocabulary structural telemetry module emitting only collection-size counts, add a secret-absence regression test covering command, script, and variable rendering sites, and retire the serialise-manifest localization key across all locales.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
force-pushed
the
feat/remove-secret-leaks-from-verbose-ast-tracing
branch
from
September 3, 2026 01:11
42a905e to
e06f18c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: Seek a systemic fix rather than tactical. Ask yourself, can this happen again or happen elsewhere? If so, think about a long term fix of the underlying issue. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate files =='
fd -t f . src tests | rg '(^|/)(graph_generation|manifest_structure_telemetry|verbose_secret_absence|logging_stderr|command_env).*\.rs$' || true
printf '%s\n' '== Affected-file outlines =='
for f in \
src/runner/graph_generation.rs \
src/runner/manifest_structure_telemetry.rs \
tests/logging_stderr/verbose_secret_absence.rs
do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all || true
fi
done
printf '%s\n' '== Environment setup and generated-output assertions =='
rg -n -C 5 --type rust \
'(Command::new|cargo_bin|assert_cmd|env\(|env_remove\(|CI_SECRET|generated output|output\(\)|read_to_string|Ninja)' \
tests src
printf '%s\n' '== Test module wiring and platform conditionals =='
rg -n -C 4 --type rust \
'(verbose_secret_absence|cfg\!\(windows\)|target_os\s*=\s*"windows"|windows)' \
tests srcLength of output: 170678 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Secret-absence regression =='
cat -n tests/logging_stderr/verbose_secret_absence.rs
printf '%s\n' '== Shared logging test support =='
cat -n tests/logging_stderr/support.rs
printf '%s\n' '== Test crate entry point and verbose sibling =='
sed -n '1,180p' tests/logging_stderr_tests.rs
sed -n '1,150p' tests/logging_stderr/verbose.rs
printf '%s\n' '== CLI integration helper definitions and uses =='
rg -n -C 6 --type rust \
'(fn\s+(netsuke|command|run_|assert_).*|CommandCargoExt|cargo_bin\(|assert\(\)|get_output\()' \
tests test_support src
printf '%s\n' '== env() implementation and comparable fixture coverage =='
rg -n -C 5 --type rust \
'(env\(\x27|env\(\x22|fn\s+.*env|ENV.*FILTER|Environment.*filter)' \
src/stdlib src/manifest tests \
-g '*.rs'Length of output: 170388 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Exact regression source =='
sed -n '1,120p' tests/logging_stderr/verbose_secret_absence.rs
printf '%s\n' '== Locate and inspect CLI runner helper =='
helper=$(fd -t f -a 'netsuke.rs' test_support tests | head -n 1 || true)
if [ -z "$helper" ]; then
rg -n -l --type rust 'fn\s+run_netsuke_in_with_env\b' test_support tests
else
echo "--- $helper"
rg -n -C 35 --type rust 'fn\s+run_netsuke_in_with_env\b' "$helper"
fi
printf '%s\n' '== Generate dispatch semantics =='
sed -n '59,100p' src/runner/dispatch.rs
printf '%s\n' '== Manifest loading environment boundary =='
rg -n -C 14 --type rust \
'(process_env_reader\(|from_path_with_policy_and_env\(|from_path.*env_reader|load_manifest)' \
src/runner src/manifest
printf '%s\n' '== Existing end-to-end env interpolation tests =='
rg -n -C 20 --type rust \
'(run_netsuke_in_with_env\(|NETSUKE_TEST_ENV|CI_SECRET|env\(\x27.*\x27\).*(generate|output))' \
tests \
-g '*.rs'Length of output: 96989 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Isolated Netsuke CLI helper =='
cat -n test_support/src/netsuke/mod.rs
printf '%s\n' '== Secret-site fixture =='
cat -n tests/data/env_secret_sites.yml
printf '%s\n' '== Windows/PowerShell generated command rendering =='
ast-grep outline src/ninja_gen_recipe_shell.rs --items all
sed -n '1,210p' src/ninja_gen_recipe_shell.rs
printf '%s\n' '== Recipe and variable Ninja emission =='
rg -n -C 10 --type rust \
'(command_value\(|RecipeShell|vars|variables|description|escape_ninja_value)' \
src/ninja_gen* src/ir \
-g '*.rs'
printf '%s\n' '== CLI helper contract documentation and tests =='
rg -n -C 15 --type rust \
'(run_netsuke_in_with_env|env_clear\(\)|NetsukeRun)' \
test_support tests \
-g '*.rs'Length of output: 182348
Fix the regression test. Do not change the secret-leak remediation. Cause
The first assertion is correct. The second assertion is not portable. On Windows, The test failure proves this representation difference. It does not show that The subprocess environment is correct:
Systemic fixSeparate semantic assertions from serialization assertions. Keep the binary-level test responsible for the security boundary:
Move the positive interpolation assertion to a representation-aware boundary:
This fix applies to all future generated-output tests. A generated format can legitimately encode, escape, hash, compress, or externalize rendered content. Tests must validate semantic content after decoding the format-specific representation. Tests must not require a plaintext representation unless plaintext is part of the public contract. Do not force AI coding agent promptYou are interacting with an AI system. |
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
This branch removes the raw rendered-manifest JSON dump from
--verbosetracing output so secrets interpolated through
env()can no longer leakinto terminal history, CI logs, or diagnostic bundles, closing the medium
severity Codex finding for issue #649.
Closes #649.
Review walkthrough
src/runner/graph_generation.rs
to see the leak point removed: the
tracing::enabled!(DEBUG)guard,serde_json::to_string_pretty(&manifest)serialisation, and thedebug!("AST:...")event are replaced by a call to the new boundedstructural telemetry entry point immediately after the manifest loads.
src/runner/manifest_structure_telemetry.rs
for the replacement boundary. It records only fixed-vocabulary integer
counts of the loaded manifest shape (variables, macros, rules, actions,
targets, defaults) through a
TRACEspan and ametrics::counter!with aone-time
describe_counter!registration. No manifest text, paths, recipecontents, variable values, or descriptions cross the telemetry boundary,
because rendered values can carry secret material from
env()interpolation.
tests/logging_stderr/verbose_secret_absence.rs
for the regression proof. The test drives the real
--verbose generateCLI path with a distinctive sentinel injected through
env('CI_SECRET')at the command, script, and variable rendering sites, and asserts the
sentinel is absent from both stdout and stderr while the generated Ninja
file still resolves the interpolation.
Validation
make check-fmt: passes (Rust, Python, and Markdown formatting clean).make lint: passes (Clippy-D warnings, Whitaker Dylint suite, Pylint10.00/10, ambrleaks).
make test: passes (cargo-nextest full workspace suite plus doctests,including the new secret-absence regression test).
make doc-coverage: passes at 99.13%, above the 80% threshold.Notes
runner.context.serialise_manifestFluent key was removedfrom
src/localization/keys.rsand all 35locales/*/messages.ftlcatalogues in one coordinated change so the localisation audit stays
balanced without suppressing dead-code warnings.
may occur in an arbitrary string with no secret-bearing field name, so
the fix removes the leak at the serialisation boundary instead.
Summary by Sourcery
Prevent rendered manifest contents from being emitted by verbose logging while retaining safe structural observability.
Bug Fixes:
Enhancements:
Tests: