Add minimal-api-file-upload skill - #155
Conversation
Eval Results: implementing-form-file-uploads-minimal-apis3-Run Validation: +38.9% PASS
Baseline Analysis (BL=3)The baseline gets the basics right but consistently misses:
Skill ImpactWith skill loaded, the model correctly:
Model: claude-opus-4.6 (baseline + skill), claude-opus-4.6 (judge) |
There was a problem hiding this comment.
Pull request overview
Adds a new .NET skill focused on correct/safe multipart file upload handling in ASP.NET Core 8 minimal APIs, plus an evaluation scenario to measure model improvement for this topic.
Changes:
- Introduces the
implementing-form-file-uploads-minimal-apisskill documentation (SKILL.md) covering size limits, antiforgery behavior, and validation patterns. - Adds a new eval scenario and rubric for validating expected guidance in model outputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md | New skill content explaining recommended patterns and common pitfalls for minimal-API file uploads. |
| src/dotnet/tests/implementing-form-file-uploads-minimal-apis/eval.yaml | New evaluation scenario + rubric for the skill. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### 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.
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).
| ### 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 and [FromForm] in Minimal APIs | |
| ```csharp | |
| // In .NET 8 minimal APIs, IFormFile is bound automatically from multipart/form-data | |
| // when it is the only body parameter. | |
| app.MapPost("/upload", (IFormFile file) => ...); | |
| // When you mix files with other form fields, annotate the body-bound parameters | |
| // with [FromForm] (or group them into a single [FromForm] DTO). | |
| app.MapPost("/upload-with-metadata", | |
| ([FromForm] IFormFile file, [FromForm] string description) => | |
| { | |
| return Results.Ok(new { file.FileName, Description = description }); | |
| }); | |
| // For multiple files, IFormFileCollection also binds automatically from multipart/form-data. | |
| // You only need [FromForm] if you mix it with other form fields, as shown above. |
| // 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.
The “safe filename” still uses Path.GetExtension(file.FileName), which derives the extension from user-controlled input. Since the skill emphasizes not trusting client metadata, prefer choosing the extension based on the validated signature (map magic bytes to ".jpg"/".png") or omit the original extension entirely.
| var boundary = context.Request.GetMultipartBoundary(); | ||
| if (string.IsNullOrEmpty(boundary)) | ||
| return Results.BadRequest("Not a multipart request"); | ||
|
|
||
| var reader = new MultipartReader(boundary, context.Request.Body); |
There was a problem hiding this comment.
This example uses context.Request.GetMultipartBoundary(), but that helper isn’t defined anywhere in this repo and isn’t a built-in ASP.NET Core API. Please include the helper implementation (or switch to the standard boundary parsing approach) so the snippet is complete/compilable.
| - "Configures BOTH Kestrel MaxRequestBodySize AND FormOptions.MultipartBodyLengthLimit — either using builder.WebHost.ConfigureKestrel or WebApplication options for the request body, AND services.Configure<FormOptions> for multipart limits. ONLY configuring one of these is INCORRECT." | ||
| - "Uses .DisableAntiforgery() on the file upload endpoint to prevent automatic 400 errors from the anti-forgery middleware, OR explicitly handles anti-forgery token submission. Simply ignoring the UseAntiforgery() pipeline issue is INCORRECT." | ||
| - "Generates a safe filename (e.g., using Guid.NewGuid() or similar) instead of using the user-provided IFormFile.FileName directly. Using file.FileName directly in file paths is a security vulnerability (path traversal). Sanitizing the user filename is acceptable but using it raw is INCORRECT." | ||
| - "Validates file content beyond just checking file extension — either checking ContentType against allowed MIME types, or checking magic bytes/file signatures, or both. Checking only the file extension is INSUFFICIENT because extensions can be spoofed." |
There was a problem hiding this comment.
Rubric criterion allows validating only ContentType (“either checking ContentType… or checking magic bytes…”), but the PR description explicitly calls out ContentType as client-spoofable and emphasizes magic-byte validation as a key gotcha. Tighten this rubric to require signature/magic-byte checking (at least for JPEG/PNG) so the eval enforces the intended lesson.
| options.Limits.MaxRequestBodySize = 100 * 1024 * 1024; // 100 MB | ||
| }); | ||
|
|
||
| // 2. Form options — multipart body length limit — default is 128MB | ||
| builder.Services.Configure<FormOptions>(options => | ||
| { | ||
| options.MultipartBodyLengthLimit = 100 * 1024 * 1024; // 100 MB |
There was a problem hiding this comment.
These examples set the global Kestrel request body limit to 100MB, which can be confusing given the scenario is about enforcing a 10MB maximum. Consider using 10MB in the sample (or explicitly explain why the global limit is higher than the per-endpoint limit).
| options.Limits.MaxRequestBodySize = 100 * 1024 * 1024; // 100 MB | |
| }); | |
| // 2. Form options — multipart body length limit — default is 128MB | |
| builder.Services.Configure<FormOptions>(options => | |
| { | |
| options.MultipartBodyLengthLimit = 100 * 1024 * 1024; // 100 MB | |
| options.Limits.MaxRequestBodySize = 10 * 1024 * 1024; // 10 MB | |
| }); | |
| // 2. Form options — multipart body length limit — default is 128MB | |
| builder.Services.Configure<FormOptions>(options => | |
| { | |
| options.MultipartBodyLengthLimit = 10 * 1024 * 1024; // 10 MB |
| // CRITICAL: Check magic bytes for file type verification | ||
| using var stream = file.OpenReadStream(); | ||
| var header = new byte[8]; | ||
| await stream.ReadAsync(header, 0, 8); |
There was a problem hiding this comment.
When reading magic bytes, the code ignores the return value from ReadAsync. For empty/short files this leaves parts of header as zeroes and can misclassify content. Capture the bytes-read and fail fast if fewer than the required bytes are available (e.g., < 4 for PNG, < 3 for JPEG).
| await stream.ReadAsync(header, 0, 8); | |
| var bytesRead = await stream.ReadAsync(header, 0, header.Length); | |
| if (bytesRead < 4) | |
| return Results.BadRequest("File content is too short or invalid"); |
| // 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 allowed MIME types include image/gif, but the scenario/rubric for this skill is JPEG + PNG only. Including GIF here makes the example inconsistent with the stated requirements and may cause models to permit additional types.
| var allowedTypes = new[] { "image/jpeg", "image/png", "image/gif" }; | |
| var allowedTypes = new[] { "image/jpeg", "image/png" }; |
| // 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 statement that “IFormFile buffers the entire file in memory by default” is inaccurate for ASP.NET Core: multipart parsing uses buffering with a memory threshold and typically spills to a temp file. Please reword to the actual risk (request/form parsing can buffer large uploads and consume memory/disk) so the guidance is technically correct.
| var contentDisposition = section.GetContentDispositionHeader(); | ||
| if (contentDisposition == null) continue; | ||
|
|
||
| if (contentDisposition.IsFileDisposition()) | ||
| { |
There was a problem hiding this comment.
section.GetContentDispositionHeader() / contentDisposition.IsFileDisposition() also aren’t built-in APIs and aren’t defined in this repo. Either provide these helpers or show the canonical parsing with ContentDispositionHeaderValue so readers can use the sample as-is.
| @@ -0,0 +1,196 @@ | |||
| --- | |||
| name: implementing-form-file-uploads-minimal-apis | |||
| description: Implement file upload handling in ASP.NET Core 8 minimal APIs. Use when handling multipart form file uploads with proper size limits and anti-forgery. | |||
There was a problem hiding this comment.
Any validation that "8" is going to influence too much?
| description: Implement file upload handling in ASP.NET Core 8 minimal APIs. Use when handling multipart form file uploads with proper size limits and anti-forgery. | ||
| --- | ||
|
|
||
| # Implementing File Uploads in ASP.NET Core 8 Minimal APIs |
There was a problem hiding this comment.
Strike "8" and put more information in the 'when to use'? (note below)
| # Implementing File Uploads in ASP.NET Core 8 Minimal APIs | ||
|
|
||
| ## When to Use | ||
| - File upload endpoints in minimal APIs |
There was a problem hiding this comment.
- File upload endpoings in ASP.NET minimal APIs (.NET 8+)
Teaches correct file upload handling in ASP.NET Core minimal APIs: - IFormFile binding with [FromForm] annotation rules - Dual size limits (Kestrel + FormOptions) - DisableAntiforgery() for upload endpoints - Magic-byte content validation over ContentType - Safe filename generation with GUIDs
f8f71ba to
4c93090
Compare
| ### Step 3: CRITICAL — Anti-Forgery Auto-Validates Form Uploads in .NET 8 | ||
|
|
||
| ```csharp | ||
| // CRITICAL: In .NET 8 with UseAntiforgery(), ALL form-bound endpoints | ||
| // automatically validate anti-forgery tokens, INCLUDING file uploads | ||
|
|
||
| builder.Services.AddAntiforgery(); | ||
| var app = builder.Build(); | ||
| app.UseAntiforgery(); | ||
|
|
||
| // This endpoint now REQUIRES an anti-forgery token: | ||
| app.MapPost("/upload", (IFormFile file) => Results.Ok(file.FileName)); | ||
| // Without the token → 400 Bad Request | ||
|
|
||
| // CRITICAL: For API-only file uploads (no anti-forgery needed), opt out: | ||
| app.MapPost("/api/upload", (IFormFile file) => Results.Ok(file.FileName)) | ||
| .DisableAntiforgery(); // CRITICAL: Must explicitly opt out | ||
|
|
||
| // COMMON MISTAKE: Getting 400 errors on file uploads and not realizing | ||
| // it's because UseAntiforgery() is in the pipeline | ||
| ``` |
There was a problem hiding this comment.
@GrabYourPitchforks @blowdart This should be okay for unauthenticated endpoints and endpoints using JWT bearer authentication, but I worry that this might cause people to disable antiforgery for endpoints authenticated with cookies. I wonder what the best way to communicate this potential security threat in the skill document.
|
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). |
|
Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. |
New Skill: minimal-api-file-upload
Teaches proper file upload handling in ASP.NET Core 8 minimal APIs, covering multiple counterintuitive configuration requirements that the model consistently gets wrong.
Key Gotchas Covered
Eval Results (3-run validation)