Fix RCE via unescaped OpenAPI path/header in Refit attributes (GHSA-3fhm-p725-h3g3)#1175
Fix RCE via unescaped OpenAPI path/header in Refit attributes (GHSA-3fhm-p725-h3g3)#1175christianhelle wants to merge 16 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEscapes OpenAPI-derived strings in generated Refit attributes, validates unsafe path/header/security-scheme inputs, and adds regression coverage for OpenAPI 3.0.0 and Swagger 2.0 injection cases. ChangesAttribute String Injection Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/Refitter.Core/Validation/AttributeStringValidator.cs`:
- Around line 18-50: Validate header-style API key security scheme names in
AttributeStringValidator as well as operation header parameters, since
HeaderParameterExtractor now uses securityScheme.Name for [Header("...")].
Update Validate to inspect document.SecurityDefinitions and reject API-key
schemes with HeaderLocation names containing unsafe characters, adding an
OpenApiError with the scheme name and a clear message consistent with the
existing path/parameter checks. Also add a regression test around
AttributeStringValidator covering a malicious header-based security scheme name
so the bypass no longer occurs without --skip-validation.
In `@src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs`:
- Around line 18-49: Add OpenAPI 2.0 coverage to the
SecurityAttributeInjectionTests regression cases by parameterizing the malicious
path and header inputs across both dialects instead of only using the 3.0 YAML
snippets. Update the existing PathInjectionSpec and HeaderInjectionSpec-style
test setup so the same injection payloads are exercised through both the 2.0 and
3.0 parsing/generation paths, keeping the escaping and validation assertions
centralized in the relevant test methods.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be84eaa1-7c35-4364-a06c-165893214e05
📒 Files selected for processing (6)
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/Scenarios/SecurityAttributeInjectionTests.cs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1175 +/- ##
==========================================
- Coverage 94.82% 94.78% -0.05%
==========================================
Files 80 81 +1
Lines 3557 3604 +47
==========================================
+ Hits 3373 3416 +43
- 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:
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs (1)
161-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the validator-specific diagnostic, not just
IsValid.These specs are intentionally adversarial, so
IsValid == falsecan stay green if parsing fails for an unrelated reason. Please also assert thatresult.OpenApiDiagnostic.Errorscontains the illegal-character rejection for the path/header/security-scheme value so the tests keep provingAttributeStringValidatoris the failure source.Also applies to: 208-298
🤖 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 161 - 174, The test in SecurityAttributeInjectionTests only checks OpenApiValidator.Validate(...).IsValid, which can pass for the wrong failure reason. Update the affected tests, including Validation_Rejects_Path_With_Breakout_Characters and the other adversarial cases mentioned, to also assert that result.OpenApiDiagnostic.Errors contains the expected illegal-character rejection coming from AttributeStringValidator for the path/header/security-scheme value.
🤖 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.
Inline comments:
In `@src/Refitter.Core/Validation/AttributeStringValidator.cs`:
- Line 3: The import in AttributeStringValidator is using an invalid OpenAPI
namespace that does not exist in the current Microsoft.OpenApi surface. Update
the using at the top of AttributeStringValidator to reference the correct root
Microsoft.OpenApi namespace, or remove the unused import entirely if the file
does not need it, so the build resolves cleanly.
In `@src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs`:
- Around line 223-236: Add the missing OpenAPI 3.0 header-name validation
regression to `Validation_Rejects_Header_With_Breakout_Characters_V2` by also
exercising `OpenApiValidator.Validate(HeaderInjectionSpec)` alongside the
existing `HeaderInjectionSpecV2` case. Keep the same assert pattern used in this
test method so both OpenAPI 2.0 and 3.0 header-validation paths are covered, and
ensure the new non-_V2 case also verifies `IsValid` is false.
---
Nitpick comments:
In `@src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs`:
- Around line 161-174: The test in SecurityAttributeInjectionTests only checks
OpenApiValidator.Validate(...).IsValid, which can pass for the wrong failure
reason. Update the affected tests, including
Validation_Rejects_Path_With_Breakout_Characters and the other adversarial cases
mentioned, to also assert that result.OpenApiDiagnostic.Errors contains the
expected illegal-character rejection coming from AttributeStringValidator for
the path/header/security-scheme value.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0472135-1775-4852-a1c3-4b2c2b10158f
📒 Files selected for processing (3)
src/Refitter.Core/Validation/AttributeStringValidator.cssrc/Refitter.Core/Validation/OpenApiValidator.cssrc/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Refitter.Core/Validation/OpenApiValidator.cs
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Pull request overview
This PR addresses GHSA-3fhm-p725-h3g3 by preventing OpenAPI-sourced strings (paths, header names, media types) from breaking out of generated Refit attribute string literals, and adds validation + regression tests to ensure injection payloads are rejected or rendered inert.
Changes:
- Escape spec-derived strings emitted into Refit attributes (
[Get("…")],[Header("…")],[Headers("Accept: …")],[Headers("Content-Type: …")]). - Add validation (
AttributeStringValidator) to reject paths/header names containing breakout characters unless validation is skipped. - Add regression tests covering path/header/security-scheme injection attempts (OAS3 + Swagger2).
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Refitter.Tests/Scenarios/SecurityAttributeInjectionTests.cs | Adds regression tests for path/header/security-scheme injection scenarios and validation rejection. |
| src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreSingleInterfaceWithPolly.g.cs | Updates generated snapshot imports placement. |
| src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreSingleInterfaceWithHttpResilience.g.cs | Updates generated snapshot imports placement and method-chain formatting. |
| src/Refitter.SourceGenerator.Tests/AdditionalFiles/Generated/SwaggerPetstoreMultipleInterfaces.g.cs | Updates generated snapshot imports placement and method-chain formatting. |
| src/Refitter.Core/Validation/OpenApiValidator.cs | Wires in AttributeStringValidator during validation. |
| src/Refitter.Core/Validation/AttributeStringValidator.cs | New validator rejecting unsafe path/header strings that could break generated attribute literals. |
| src/Refitter.Core/ParameterExtraction/HeaderParameterExtractor.cs | Escapes header names when generating [Header("…")] attributes. |
| src/Refitter.Core/Generation/MethodAttributeGenerator.cs | Escapes media types used in generated Accept / Content-Type header literals. |
| src/Refitter.Core/Generation/InterfaceGenerator.cs | Escapes OpenAPI paths emitted into Refit HTTP method attributes. |
|
Superseded by a new PR that covers all three related advisories (GHSA-3fhm-p725-h3g3, GHSA-58x9-vjvp-6mx8, GHSA-p32v-8v8j-j534) from the same branch. Closing in favor of the broader fix. |
|
Superseded by #1178. |
|
Summary
Fixes GHSA-3fhm-p725-h3g3. Refitter emitted each OpenAPI path key and header name verbatim into Refit
[Get("…")]/[Header("…")]attributes. A crafted single-line path closes the attribute string, the interface, and the namespace, injects a top-levelfile classwith a[ModuleInitializer], then reopens a namespace/interface — yielding RCE at assembly load in any app that compiles the generated client. Verified reproducible on the current build with no--skip-validation.Changes
--skip-validation):[Get/Post/...]path —InterfaceGenerator[Header(name)]parameter & security-scheme names —HeaderParameterExtractor[Headers("Accept:"/"Content-Type:")]media types —MethodAttributeGenerator",\, control/newlines) in paths and header names during validation; errors out unless--skip-validation(AttributeStringValidator).Verification
--skip-validationstill produces safe, escaped code.Summary by CodeRabbit
Bug Fixes
Accept/Content-Typeheader literals.Security Validation
Tests