Fix RCE attribute injection in generated Refit clients (GHSA-3fhm-p725-h3g3, GHSA-58x9-vjvp-6mx8, GHSA-p32v-8v8j-j534)#1178
Open
christianhelle wants to merge 18 commits into
Conversation
Fixed 3 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGenerators now escape OpenAPI path, header-name, and content-type values before emitting Refit attributes. A new validator flags unsafe characters in those inputs and is called from ChangesAttribute String Injection Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs (1)
381-408: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer explicit types over
varper repo convention.
swaggerFile,result,settings,generator, anddirectoryusevar; the rest of the new test suite (AttributeStringValidatorTests) uses explicit types. Aligning here keeps style consistent.As per coding guidelines: "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent per .editorconfig)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs` around lines 381 - 408, The test helper in SecurityAttributeInjectionTests should follow the repo’s explicit-type convention instead of using inferred locals. Update GenerateCode and CleanUp to declare the local values with their concrete types, matching the style used in AttributeStringValidatorTests and the csharp_style_var_* settings, and keep the existing helper method names (GenerateCode, CleanUp) unchanged so the rest of the test suite remains consistent.Source: Coding guidelines
src/Refitter.Core/Validation/AttributeStringValidator.cs (1)
18-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
Validateinto per-sink passes.Sonar is already flagging this method at 35 cognitive complexity. Extracting separate validators for security schemes, paths, header parameters, and content types would make future sink additions much easier to audit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Refitter.Core/Validation/AttributeStringValidator.cs` around lines 18 - 82, The Validate method in AttributeStringValidator is doing too many sink checks in one pass, which is driving cognitive complexity. Split it into focused helpers such as one for security schemes, one for path validation, one for header parameter validation, and one for content type validation, then have Validate orchestrate those passes while preserving the existing early returns and the validateContentTypes gating. Use the existing Validate and ValidateContentTypeKeys symbols as the entry points to refactor around.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Refitter.Core/Validation/AttributeStringValidator.cs`:
- Around line 18-82: The Validate method in AttributeStringValidator is doing
too many sink checks in one pass, which is driving cognitive complexity. Split
it into focused helpers such as one for security schemes, one for path
validation, one for header parameter validation, and one for content type
validation, then have Validate orchestrate those passes while preserving the
existing early returns and the validateContentTypes gating. Use the existing
Validate and ValidateContentTypeKeys symbols as the entry points to refactor
around.
In `@src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs`:
- Around line 381-408: The test helper in SecurityAttributeInjectionTests should
follow the repo’s explicit-type convention instead of using inferred locals.
Update GenerateCode and CleanUp to declare the local values with their concrete
types, matching the style used in AttributeStringValidatorTests and the
csharp_style_var_* settings, and keep the existing helper method names
(GenerateCode, CleanUp) unchanged so the rest of the test suite remains
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f660d4c9-686d-4a72-bc10-ac589ac22586
⛔ Files ignored due to path filters (3)
src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreMultipleInterfaces.g.csis excluded by!**/generated/**src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreSingleInterfaceWithHttpResilience.g.csis excluded by!**/generated/**src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreSingleInterfaceWithPolly.g.csis excluded by!**/generated/**
📒 Files selected for processing (7)
src/Refitter.Core/Generation/InterfaceGenerator.cssrc/Refitter.Core/Generation/MethodAttributeGenerator.cssrc/Refitter.Core/ParameterExtraction/HeaderParameterExtractor.cssrc/Refitter.Core/Validation/AttributeStringValidator.cssrc/Refitter.Core/Validation/OpenApiValidator.cssrc/Refitter.Tests/OpenApi/AttributeStringValidatorTests.cssrc/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1178 +/- ##
==========================================
- Coverage 94.82% 94.79% -0.04%
==========================================
Files 80 81 +1
Lines 3557 3612 +55
==========================================
+ Hits 3373 3424 +51
- Misses 75 76 +1
- Partials 109 112 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
christianhelle
added
enhancement
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Refitter’s generated Refit clients against attribute-string literal injection leading to potential RCE by escaping spec-derived strings at generation time and adding validation that rejects unsafe values by default.
Changes:
- Escape OpenAPI-derived values before emitting them into Refit attribute string literals (paths, header names, Accept/Content-Type).
- Add
AttributeStringValidatorand wire it intoOpenApiValidatorto reject paths/header names/content-type keys containing breakout characters (unless validation is skipped). - Add regression/unit tests covering injection attempts and validator behavior across OpenAPI v2/v3.
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs | New regression suite for injection payloads across path/header/security-scheme/content-type sinks. |
| src/Refitter.Tests/OpenApi/AttributeStringValidatorTests.cs | Unit tests for the new validator’s unsafe-character detection and document traversal behavior. |
| src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreSingleInterfaceWithPolly.g.cs | Updated generated fixture output (namespace-level using placement). |
| src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreSingleInterfaceWithHttpResilience.g.cs | Updated generated fixture output (namespace-level using placement / formatting). |
| src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreMultipleInterfaces.g.cs | Updated generated fixture output (namespace-level using placement / formatting). |
| src/Refitter.Core/Validation/OpenApiValidator.cs | Invokes AttributeStringValidator as part of OpenAPI validation. |
| src/Refitter.Core/Validation/AttributeStringValidator.cs | New validator rejecting unsafe characters in paths/header names/content-type keys to prevent attribute literal breakouts. |
| src/Refitter.Core/ParameterExtraction/HeaderParameterExtractor.cs | Escapes header names (including security-scheme-derived headers) before emitting [Header("...")]. |
| src/Refitter.Core/Generation/MethodAttributeGenerator.cs | Escapes media types used in generated Accept / Content-Type header attributes. |
| src/Refitter.Core/Generation/InterfaceGenerator.cs | Escapes operation paths before emitting [Get/Post/...("...")]. |
Comment on lines
+16
to
+17
| private const string MaliciousPath = | ||
| @"/p"")] Task<string> Dummy(); } } file class Evil { [System.Runtime.CompilerServices.ModuleInitializer] internal static void Init(){ } } namespace N2 { public partial interface I2 { [Get(""/q"; |
Comment on lines
+19
to
+20
| private const string MaliciousHeaderName = | ||
| @"X"")] string a); } file class H {} public partial interface J { [Header(""Y"; |
Comment on lines
+22
to
+23
| private const string MaliciousContentType = | ||
| @"application/json"")] Task<string> Dummy(); } } file class CtEvil { [System.Runtime.CompilerServices.ModuleInitializer] internal static void Init(){ } } namespace N3 { public partial interface I3 { [Get(""/ctq"; |
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.
Summary
Fixes three related critical security advisories (CVSS 9.3, CWE-94) in which Refitter emitted attacker-influenceable OpenAPI strings verbatim into Refit attribute string literals. A crafted single-line value closes the attribute string, the interface, and the namespace, injects a top-level
file classwith a[ModuleInitializer]method, then reopens namespace/interface so the file still compiles — yielding remote code execution at assembly load in any application that compiles the generated client. No--skip-validationis required.[Get("…")](and other verbs)parameters[].name,in: header)[Header("…")]requestBody/responsescontent map key)[Headers("Accept: …", "Content-Type: …")]Changes
ParameterShared.EscapeString(escapes",\, and control characters). This is always-on, even with--skip-validation, and is the primary defense:InterfaceGeneratorHeaderParameterExtractorAccept/Content-Typemedia types →MethodAttributeGeneratorAttributeStringValidator, unless--skip-validation) for:Verification
--skip-validationstill produces safe, escaped code.Supersedes #1175 (which documented only GHSA-3fhm-p725-h3g3).
Summary by CodeRabbit