Skip to content

feat: add RuleSync .NET SDK#1303

Closed
rudironsoni wants to merge 1 commit intodyoshikawa:mainfrom
rudironsoni:feat/ruesync-dotnet-sdk
Closed

feat: add RuleSync .NET SDK#1303
rudironsoni wants to merge 1 commit intodyoshikawa:mainfrom
rudironsoni:feat/ruesync-dotnet-sdk

Conversation

@rudironsoni
Copy link
Copy Markdown
Contributor

  • Add RuleSync .NET SDK with full feature support
  • Add GitHub Actions workflow for .NET SDK CI/CD
  • Add JSON Schema support documentation
  • Fix: drop renamed/deleted skills from lockfile on re-fetch
  • Release v7.18.1
  • Include local configuration updates

@github-actions

This comment has been minimized.

@rudironsoni rudironsoni force-pushed the feat/ruesync-dotnet-sdk branch from 9507e3d to 596f23c Compare March 10, 2026 15:10
@dyoshikawa-claw

This comment has been minimized.

@github-actions

This comment has been minimized.

@rudironsoni rudironsoni force-pushed the feat/ruesync-dotnet-sdk branch 11 times, most recently from d957e33 to c6e71e4 Compare March 10, 2026 17:02
@dyoshikawa-claw

This comment has been minimized.

- Add RuleSync .NET SDK with full feature support
- Add GitHub Actions workflow for .NET SDK CI/CD
- Add JSON Schema support documentation
- Fix: drop renamed/deleted skills from lockfile on re-fetch
- Release v7.18.1
- Include local configuration updates
@rudironsoni rudironsoni force-pushed the feat/ruesync-dotnet-sdk branch from c6e71e4 to 6e8b582 Compare March 10, 2026 17:03
@rudironsoni rudironsoni deleted the feat/ruesync-dotnet-sdk branch March 10, 2026 17:05
@github-actions
Copy link
Copy Markdown
Contributor


PR Review Summary for Rulesync .NET SDK (PR #20)

Overall Mergeability Verdict: NOT MERGEABLE

This PR has 2 critical and 5 high severity issues that must be addressed before merging.


Code Review Findings

🔴 Critical Issues

  1. Command Injection Vulnerability in Argument Building - sdk/dotnet/src/RuleSync.Sdk.DotNet/RulesyncClient.cs:147-213

    • BuildGenerateArgs and BuildImportArgs construct command-line arguments by string concatenation without proper escaping
    • Paths with spaces or shell metacharacters could break the split or lead to injection
    • Fix: Use ArgumentList.Add() directly instead of string concatenation and splitting
  2. Missing Input Validation for ImportOptions.Target - sdk/dotnet/src/RuleSync.Sdk.DotNet/RulesyncClient.cs:95-97

    • No validation that Target is properly set before execution
    • Fix: Add enum validation before executing import

🟠 High Severity Issues

  1. Incorrect Repository URL - sdk/dotnet/src/RuleSync.Sdk.DotNet/RuleSync.Sdk.DotNet.csproj:20

    • Points to rudironsoni/rulesync instead of dyoshikawa/rulesync
  2. Race Condition in ProcessExtensions Polyfill - sdk/dotnet/src/RuleSync.Sdk.DotNet/Polyfills/ProcessExtensions.cs:15-51

    • Potential race between HasExited check and event subscription
  3. Dependabot Configuration Uses Wrong Directory - .github/dependabot-dotnet.yml:4

    • Points to / instead of /sdk/dotnet
  4. Tag Validation After Use in publish.yml - .github/workflows/publish.yml:46-68

    • Tag value is used before validation step runs

Security Review Findings

🔴 High Severity

  1. Command Injection Risk (Same as fix: Update pnpm version to 10.12.2 in CI and release workflows #1 above)
    • User-provided values like ConfigPath are passed directly to command line
    • Arrays are joined with commas without validation

🟠 Medium Severity

  1. Script Injection Risk in publish-assets.yml - .github/workflows/publish-assets.yml:69-79

    • inputs.tag and branch refs should be validated before use
  2. Path Traversal Risk - sdk/dotnet/src/RuleSync.Sdk.DotNet/RulesyncClient.cs:36-43

    • nodeExecutablePath and rulesyncPath used without validation
  3. Missing Comprehensive Input Validation - RulesyncClient.cs (multiple)

    • No validation for path traversal, null bytes, or shell metacharacters

Positive Findings ✅

  • Well-designed Result<T> pattern with immutability and functional methods
  • Proper multi-targeting (netstandard2.1, net6.0, net8.0)
  • AOT compatibility enabled
  • Central package management with Directory.Packages.props
  • NuGet config clears all sources (prevents dependency confusion)
  • Dependabot configured for both NuGet and GitHub Actions

Must Fix Before Merge

# Issue Location
1 Command injection vulnerability RulesyncClient.cs:147-213
2 Missing Target validation RulesyncClient.cs:95-97
3 Incorrect repository URL RuleSync.Sdk.DotNet.csproj:20
4 Wrong dependabot directory dependabot-dotnet.yml:4
5 Tag validation timing publish.yml:46-68

github run

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.

2 participants