Close threat model review gaps#166
Merged
Merged
Conversation
Harden output file writes, tighten option parsing, and sync the threat model/spec wording for issue #37 close-out. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR finishes several hardening and documentation-sync items from the threat-model review work by tightening CLI parsing and making --output safer (no clobbering, and no output files for non-success results).
Changes:
- Make
--outputcreate a new file only forCletRunStatus.Okresults (refuse existing paths instead of truncating). - Resolve aliases earlier, validate clet-specific
--<opt> <value>options, and avoid the “unknown option swallows next token” behavior. - Resync threat-model/spec/decision docs and add targeted unit tests for the new behaviors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Clet.UnitTests/OutputFormatterTests.cs | Adds regression tests for non-overwrite semantics and “no file on error” behavior. |
| tests/Clet.UnitTests/CommandLineRootTests.cs | Adds tests covering alias-first --initial cap behavior and unknown-option validation. |
| src/Clet/Hosting/OutputFormatter.cs | Implements CreateNew semantics and skips writing output files for non-success results. |
| src/Clet/Hosting/CommandLineRoot.cs | Reorders alias resolution, validates options, and adds helper logic to prevent option-value swallowing. |
| specs/decisions.md | Updates decision record text/pointers to match the safer behavior and correct issue references. |
| specs/clet-spec.md | Updates the CLI spec to document the new parsing/output semantics and the alias-first --initial cap. |
| docs/threat-model.md | Updates scope/mitigations and adds a release-pipeline risk section. |
Comments suppressed due to low confidence (2)
src/Clet/Hosting/CommandLineRoot.cs:353
IsKnownOptionTokentreats-<short>tokens fromclet.Optionsas “known option tokens”, but the parser never actually supports single-dash clet options (only host-j/-f/-i/-t/-o/-r). This means values like--initial -o(for clets that define shortNameo) will be rejected as “requires a value”, even though-oshould be a valid literal value. Consider removing thetoken.Length==2 && token[0]=='-'clet-option check, or adding real parsing support for clet short options with an escape hatch for literal values.
if (token.StartsWith ("--", StringComparison.Ordinal))
{
return IsCletOption (clet, token[2..]);
}
if (token.Length == 2 && token[0] == '-')
{
string shortName = token[1..];
return clet.Options.Any (opt => string.Equals (opt.ShortName, shortName, StringComparison.OrdinalIgnoreCase));
}
src/Clet/Hosting/CommandLineRoot.cs:323
- TryReadOptionValue currently rejects any candidate value that matches a known option token (e.g.
--json,--output, or any--<cletOption>). This prevents passing legitimate literal values that happen to start with--(e.g.--title --jsonor--initial --output). If this stricter behavior is intentional, it would be safer to add an explicit escape hatch (common patterns:--end-of-options marker, or supporting--initial=<value>/--title=<value>), so callers can still pass such literals when needed.
private static bool TryReadOptionValue (string[] args, int optionIndex, IClet clet, out string value)
{
value = string.Empty;
if (optionIndex + 1 >= args.Length)
{
return false;
}
string candidate = args[optionIndex + 1];
if (IsKnownOptionToken (candidate, clet))
{
return false;
}
value = candidate;
return true;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use non-nullable out locals when reading option values so call sites match TryReadOptionValue and reflect that successful reads produce non-null strings. Co-authored-by: Copilot <[email protected]>
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
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.
Issue #37's concrete attack-path work was split out and closed separately, leaving parser/output housekeeping plus doc resync work before the threat model can be reviewed cold for v0.9 RC.
This PR hardens the remaining surfaces by making
--outputcreate result files only for successful clet results and refusing existing paths instead of truncating them. It also resolves aliases before applying the oversized--initialvalidation path and rejects unknown clet-specific options before they can swallow following host flags as values.Docs are synced with the implemented behavior: the threat model, spec Appendix A, and existing decision records now describe the safer output behavior, the alias-first input cap, markdown file-access confinement details, and the correct linked issue references.
Validation:
dotnet build --no-restoredotnet run --project tests\Clet.UnitTests --no-builddotnet run --project tests\Clet.SmokeTests --no-buildFixes: #37