Skip to content

Close threat model review gaps#166

Merged
tig merged 2 commits into
developfrom
tig/threat-model-review
May 22, 2026
Merged

Close threat model review gaps#166
tig merged 2 commits into
developfrom
tig/threat-model-review

Conversation

@tig

@tig tig commented May 22, 2026

Copy link
Copy Markdown
Member

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 --output create result files only for successful clet results and refusing existing paths instead of truncating them. It also resolves aliases before applying the oversized --initial validation 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-restore
  • dotnet run --project tests\Clet.UnitTests --no-build
  • dotnet run --project tests\Clet.SmokeTests --no-build

Fixes: #37

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]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --output create a new file only for CletRunStatus.Ok results (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

  • IsKnownOptionToken treats -<short> tokens from clet.Options as “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 shortName o) will be rejected as “requires a value”, even though -o should be a valid literal value. Consider removing the token.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 --json or --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.

Comment thread src/Clet/Hosting/CommandLineRoot.cs
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]>
@tig
tig merged commit c449933 into develop May 22, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Threat model: review and close open attack surfaces before v0.9 RC

2 participants