Second Look
GitHub

Real pull requests, after review

This is the reviewer's page for a pull request that has been through the whole flow: the AI's first pass, an engineer's decisions, a fix, and a second review. It is read-only because the reviews are published. What you see matches what the author got on GitHub. Green-edged findings were confirmed and sent. Greyed ones were rejected and the author never saw them.

To confirm and reject findings yourself, use the playground.

Add CSV export of work orders

Changliu53/fieldops-platform-sample#1 by Changliu53, commit dcf4888

Summary for the author

high risk

Adds a new web address that the office can call to download a customer's work orders as a spreadsheet (CSV) file, and also saves a copy of that file on the server's C: drive.

Ask them to double check

  • Confirm that someone who is not logged in cannot download the export — as written, anyone who knows the address can.
  • Log in as a user of one company and export for a common name (or a single letter like 'a'); check that you only see your own company's work orders. As written you will see other companies' data.
  • Export a work order whose description contains a comma or a line break and open it in Excel; check the columns don't shift.
  • Decide whether saving a copy on the server is really needed; if so, the storage location should come from configuration and the server must have that folder.
  • Check that dates in the file are in the expected format and time zone.
  1. criticalThe new Export action has no [Authorize] attribute, while every other action in this controller does. Every action needs its own permission check, so this one is open to anyone, including people who are not logged in, and they can download work order data.confirmed
  2. criticalIgnoreQueryFilters() switches off the automatic filter that keeps each company to its own data, and the team rules forbid it outside the Admin folder. The customer-name match is also a partial match ('Contains'). Together, a search for a short or common name returns work orders from every tenant, so one customer's prices and descriptions leak to another company.confirmed
  3. criticalThe file name is built from the 'customer' value sent in the request. A value like '..\..\Windows\something' writes outside the exports folder, so a caller can overwrite files on the server. The hard-coded C:\exports path also breaks the rule against file paths in code, and it will throw an error (failing the whole export) on any server where that folder doesn't exist, such as Linux or a container.confirmed
  4. warningValues are joined with commas but never quoted. Any customer name or description containing a comma, quote, or line break will split into extra columns or rows, so the spreadsheet comes out scrambled. Work order descriptions often contain commas. A value starting with '=' would also run as a formula in Excel (CSV injection). ScheduledFor and QuotedTotal are formatted using the server's regional settings, which may use commas as decimal separators.confirmed

src/FieldOps.Api/WorkOrders/WorkOrdersController.csmodifiedWhole file on GitHub

@@ -58,4 +58,23 @@ public async Task<IActionResult> Start(int id, CancellationToken ct)
58 await _db.SaveChangesAsync(ct);
59 return NoContent();
60 }
61+
62+ [HttpGet("export")]
63+ public async Task<IActionResult> Export([FromQuery] string customer, CancellationToken ct)
criticalaccess-controlWorkOrdersController.cs:62-6395% sureconfirmed by Changliu53

The new Export action has no [Authorize] attribute, while every other action in this controller does. Every action needs its own permission check, so this one is open to anyone, including people who are not logged in, and they can download work order data.

Suggested fix: Add [Authorize(Policy = "WorkOrders.Read")] above the Export method.

64+ {
65+ var rows = await _db.WorkOrders
66+ .IgnoreQueryFilters()
67+ .Where(w => w.CustomerName.Contains(customer))
criticalaccess-controlWorkOrdersController.cs:66-6795% sureconfirmed by Changliu53

IgnoreQueryFilters() switches off the automatic filter that keeps each company to its own data, and the team rules forbid it outside the Admin folder. The customer-name match is also a partial match ('Contains'). Together, a search for a short or common name returns work orders from every tenant, so one customer's prices and descriptions leak to another company.

Suggested fix: Remove .IgnoreQueryFilters(). Filter by a customer id (or an exact name match) rather than Contains, and return 400 if the customer parameter is missing.

68+ .OrderBy(w => w.ScheduledFor)
69+ .ToListAsync(ct);
70+
71+ var csv = "Id,Customer,Description,Scheduled,Total\n";
72+ foreach (var row in rows)
73+ {
74+ csv += row.Id + "," + row.CustomerName + "," + row.Description + "," + row.ScheduledFor + "," + row.QuotedTotal + "\n";
warningbugWorkOrdersController.cs:7485% sureconfirmed by Changliu53

Values are joined with commas but never quoted. Any customer name or description containing a comma, quote, or line break will split into extra columns or rows, so the spreadsheet comes out scrambled. Work order descriptions often contain commas. A value starting with '=' would also run as a formula in Excel (CSV injection). ScheduledFor and QuotedTotal are formatted using the server's regional settings, which may use commas as decimal separators.

Suggested fix: Use a CSV library (e.g. CsvHelper), or wrap every field in double quotes with inner quotes doubled. Prefix values starting with =, +, - or @ with a single quote. Format dates and numbers with CultureInfo.InvariantCulture (e.g. ISO 8601 dates).

75+ }
76+
77+ System.IO.File.WriteAllText("C:\\exports\\workorders-" + customer + ".csv", csv);
criticalsecurityWorkOrdersController.cs:7790% sureconfirmed by Changliu53

The file name is built from the 'customer' value sent in the request. A value like '..\..\Windows\something' writes outside the exports folder, so a caller can overwrite files on the server. The hard-coded C:\exports path also breaks the rule against file paths in code, and it will throw an error (failing the whole export) on any server where that folder doesn't exist, such as Linux or a container.

Suggested fix: Drop the server-side copy, or write to a location from configuration using a generated, safe file name (for example tenant id + timestamp + GUID), never the raw query value. Handle a write failure without failing the download.

78+ return File(System.Text.Encoding.UTF8.GetBytes(csv), "text/csv", "work-orders.csv");
79+ }
80 }