Conversation
Migration NoteThis PR replaces #155 which was opened from
All prior review feedback from #155 still applies — please see that PR for the full discussion history. |
There was a problem hiding this comment.
Pull request overview
Adds a new .NET skill and evaluation scenario covering correct file-upload handling patterns for ASP.NET Core 8 minimal APIs (size limits, antiforgery, safe filenames, and content validation), and assigns code ownership for the new content.
Changes:
- Added
minimal-api-file-uploadskill documentation underplugins/dotnet/skills/. - Added a new evaluation scenario under
tests/dotnet/for the skill. - Updated
.github/CODEOWNERSto include owners for the new skill and its tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| plugins/dotnet/skills/minimal-api-file-upload/SKILL.md | New skill guidance for file uploads in .NET 8 minimal APIs (binding, limits, antiforgery, validation, streaming). |
| tests/dotnet/minimal-api-file-upload/eval.yaml | New eval scenario and rubric for minimal API file upload handling. |
| .github/CODEOWNERS | Adds ownership entries for the new skill and its evals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| var fileName = contentDisposition.FileName.Value; | ||
| var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(fileName)}"; |
There was a problem hiding this comment.
In the streaming example, contentDisposition.FileName.Value is used directly to compute the extension. This value can include paths and other unexpected content; using it directly reintroduces path traversal / spoofing risks. Prefer extracting a safe base name first (e.g., Path.GetFileName(...) + quote removal) and, ideally, deriving the output extension from validated content rather than the client-provided filename.
| var fileName = contentDisposition.FileName.Value; | |
| var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(fileName)}"; | |
| var originalFileName = contentDisposition.FileName.Value ?? string.Empty; | |
| var sanitizedFileName = Path.GetFileName(originalFileName.Trim('"')); | |
| var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(sanitizedFileName)}"; |
| // CRITICAL: IFormFile buffers the entire file in memory by default | ||
| // For large files, use MultipartReader for streaming |
There was a problem hiding this comment.
The text claims IFormFile “buffers the entire file in memory by default”. ASP.NET Core typically buffers form bodies with a memory threshold and spills to disk, so the guidance here is misleading. Consider rephrasing to describe the real concern (multipart/form parsing buffers and can be expensive; for very large uploads prefer streaming with MultipartReader / disabling form binding) and, if relevant, mention the key knobs (FormOptions.MemoryBufferThreshold, temp file location, etc.).
| ### Step 1: CRITICAL — IFormFile Requires [FromForm] in Minimal APIs (Not Automatic) | ||
|
|
||
| ```csharp | ||
| // COMMON MISTAKE: Expecting IFormFile to bind automatically | ||
| app.MapPost("/upload", (IFormFile file) => ...); | ||
| // In early .NET versions, this worked differently. In .NET 8: | ||
|
|
||
| // CRITICAL: IFormFile IS bound automatically from form data in .NET 8 | ||
| // BUT when you mix IFormFile with other parameters, you need [FromForm] | ||
| app.MapPost("/upload-with-metadata", | ||
| ([FromForm] IFormFile file, [FromForm] string description) => | ||
| { | ||
| return Results.Ok(new { file.FileName, Description = description }); | ||
| }); | ||
|
|
||
| // CRITICAL: For multiple files, use IFormFileCollection |
There was a problem hiding this comment.
The Step 1 heading/example says it's a “COMMON MISTAKE” to expect IFormFile to bind automatically, but immediately below it says IFormFile IS bound automatically from form data in .NET 8. This is internally inconsistent and will confuse readers about when [FromForm] is required. Please rewrite this section to clearly state the real rule (e.g., IFormFile binds from multipart/form-data by default when it's the only complex parameter, but mixing with other parameters generally requires [FromForm] on all form-bound parameters), and align the examples accordingly.
| ### Step 1: CRITICAL — IFormFile Requires [FromForm] in Minimal APIs (Not Automatic) | |
| ```csharp | |
| // COMMON MISTAKE: Expecting IFormFile to bind automatically | |
| app.MapPost("/upload", (IFormFile file) => ...); | |
| // In early .NET versions, this worked differently. In .NET 8: | |
| // CRITICAL: IFormFile IS bound automatically from form data in .NET 8 | |
| // BUT when you mix IFormFile with other parameters, you need [FromForm] | |
| app.MapPost("/upload-with-metadata", | |
| ([FromForm] IFormFile file, [FromForm] string description) => | |
| { | |
| return Results.Ok(new { file.FileName, Description = description }); | |
| }); | |
| // CRITICAL: For multiple files, use IFormFileCollection | |
| ### Step 1: CRITICAL — Understand IFormFile Binding in Minimal APIs | |
| ```csharp | |
| // Simple case: a single IFormFile parameter binds automatically from multipart/form-data | |
| app.MapPost("/upload", (IFormFile file) => ...); | |
| // In early .NET versions, this worked differently. In .NET 8 minimal APIs: | |
| // IFormFile (and IFormFileCollection) are bound automatically when they are the only complex parameters. | |
| // BUT when you mix files with other form fields, be explicit and use [FromForm] on all form-bound parameters. | |
| app.MapPost("/upload-with-metadata", | |
| ([FromForm] IFormFile file, [FromForm] string description) => | |
| { | |
| return Results.Ok(new { file.FileName, Description = description }); | |
| }); | |
| // Multiple files: IFormFileCollection also binds automatically from multipart/form-data |
| // CRITICAL: Check content type AND file signature (magic bytes) | ||
| // NEVER trust file extension alone — it can be spoofed | ||
|
|
||
| var allowedTypes = new[] { "image/jpeg", "image/png", "image/gif" }; |
There was a problem hiding this comment.
The allowlist includes image/gif, but the eval scenario and the surrounding guidance emphasize JPEG/PNG-only uploads. Including GIF here risks training the model to accept additional types by default. Consider limiting the example to JPEG/PNG and mentioning how to extend the allowlist when needed.
| var allowedTypes = new[] { "image/jpeg", "image/png", "image/gif" }; | |
| // Allow only JPEG/PNG by default. To support more (e.g., GIF), | |
| // add the MIME type here AND validate its magic bytes below. | |
| var allowedTypes = new[] { "image/jpeg", "image/png" }; |
| // CRITICAL: Generate a safe filename — never use user-provided filename directly | ||
| var safeFileName = $"{Guid.NewGuid()}{Path.GetExtension(file.FileName)}"; | ||
| // NEVER: var path = Path.Combine("uploads", file.FileName); // Path traversal! |
There was a problem hiding this comment.
safeFileName uses Path.GetExtension(file.FileName), which is attacker-controlled. Even with ContentType/magic-byte validation, this can still produce unsafe or misleading extensions (e.g., saving a JPEG as .exe). Prefer deriving the extension from the validated file signature/content type (mapping jpeg→.jpg, png→.png) rather than trusting the user-provided name for any part of the output path.
| var boundary = context.Request.GetMultipartBoundary(); | ||
| if (string.IsNullOrEmpty(boundary)) | ||
| return Results.BadRequest("Not a multipart request"); |
There was a problem hiding this comment.
This snippet calls context.Request.GetMultipartBoundary(), but that isn't a standard ASP.NET Core API and there’s no helper shown/linked in the skill. As written, readers following this will hit a compile error. Please either include the helper/extension implementation in the skill (and ensure it handles quoted boundaries and length limits), or switch the snippet to use the supported pattern from Microsoft.AspNetCore.WebUtilities samples (e.g., extracting the boundary from the Content-Type header with HeaderUtilities).
Feedback carried over from #155Code Review CommentsCopilot on src/dotnet/skills/minimal-api-file-upload/SKILL.md (line 43): Step 1 is internally contradictory: itΓÇÖs titled as if IFormFile requires [FromForm] (not automatic), but later in this section it states IFormFile is bound automatically in .NET 8. Please rewrite this section to present one clear rule (e.g., IFormFile binds from multipart automatically, but mixed parameters should be annotated with [FromForm] or grouped). Discussion Commentsmrsharm: ## Eval Results: implementing-form-file-uploads-minimal-apis ### 3-Run Validation: +38.9% PASS | Metric | Value | |--------|-------| | Overall Improvement | +38.9% | | Confidence Interval | [+9.1%, +62.5%] significant | | Effect Size (g) | +100.0% | | Baseline Quality | 3.0/5 | | Skill Quality | 5.0/5 | | Quality Delta | +2.0 | | Task Completion | Baseline: Pass, Skill: Pass | | Token Usage | +51.7% (164K -> 250K) | | Tool Calls | -16.7% (18 -> 15) | ### Baseline Analysis (BL=3) The baseline gets the basics right but consistently misses: - Only configures one of the two required size limits (Kestrel MaxRequestBodySize OR FormOptions.MultipartBodyLengthLimit, but not both) - Relies on ContentType alone for validation (client-spoofable) - Uses user-provided filenames without sanitization ### Skill Impact With skill loaded, the model correctly: - Configures BOTH Kestrel and FormOptions size limits - Calls DisableAntiforgery() on upload endpoints - Generates safe filenames with GUIDs - Validates file content with magic bytes - Uses proper IFormFile binding patterns Model: claude-opus-4.6 (baseline + skill), claude-opus-4.6 (judge) --- ViktorHofer: As discussed offline in the "dotnet/skills content" chat, this PR will need to be re-submitted from a connected fork. Also please update this PR based on the new repo folder structure (plugins instead of src). --- |
|
/evaluate |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
Same feedback as in the other PRs regarding the skill not getting activated. If intentional, add |
|
/evaluate |
|
✅ Evaluation completed. View results | View workflow run |
|
@mrsharm - - the repo has undergone some restructuring to make everything more organized. Hence, we are asking all open PRs to update the branch. Sorry about this. |
mrsharm
force-pushed
the
musharm/implementing-form-file-uploads-minimal-apis
branch
from
d7e4ba5 to
4db7a40
Compare
mrsharm
force-pushed
the
musharm/implementing-form-file-uploads-minimal-apis
branch
from
4db7a40 to
fa88f24
Compare
…cenarios Per repo restructuring feedback, ASP.NET Core specific skills should be under the aspnetcore plugin rather than the dotnet plugin.
mrsharm
force-pushed
the
musharm/implementing-form-file-uploads-minimal-apis
branch
from
fa88f24 to
6a58890
Compare
|
/evaluate |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
|
If #207 creates a dotnet-aspnet plugin, this skill should presumably move there. |
Summary
Adds the minimal-api-file-upload skill for handling file uploads in ASP.NET Core 8 minimal APIs.
Eval Results (3-run)
Files