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 0ef7e03

Summary for the author

low risk

Adds an export link that downloads a customer's work orders as a spreadsheet (CSV) file. Since the last review, the export checks that the user is allowed to read work orders. It stays within the user's own company and quotes values so they don't break the spreadsheet or run as formulas. The earlier code that saved a copy on the server has been removed.

Ask them to double check

Export a customer whose name is part of another customer's name (for example 'Smith' and 'Smithson'). Make sure the file only holds the customer you meant to send it to.
Open an export whose descriptions contain commas, quotes, line breaks, or text starting with '=' in Excel, and check the columns line up.
The description still says a copy is saved on the server, but the code no longer does this. Update the description, or confirm you don't need that record.
Check whether you want a date range (for example one month), because the export currently includes all of the customer's work orders, up to 5,000.

Since the last review

4 of 4 sent findings fixed

✓ FixedcriticalThe 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.Export now has [Authorize(Policy = "WorkOrders.Read")], like the other read actions.
✓ FixedcriticalIgnoreQueryFilters() 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.IgnoreQueryFilters() was removed, so the tenant query filter applies again. The partial name match remains, but it now stays within one company and is reported separately as a customer-to-customer issue.
✓ FixedcriticalThe 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.The code that wrote a file to a hard-coded server path built from the request value has been removed entirely.
✓ FixedwarningValues 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.Text values are now quoted with embedded quotes doubled, values starting with a formula character get a leading apostrophe, dates use the 'O' format, and totals use the invariant culture.

This is the model's reading of the new commit. Findings you rejected last time were not raised again.

warningThe customer is matched with 'Contains', which is a partial text match. Exporting 'Smith' also pulls in 'Smithson Ltd' and 'Blacksmith Co'. An empty or very short value matches almost every customer. This file is meant to be sent to one customer at month end, so that customer could receive another customer's work orders, prices, and descriptions. The remembered rule: when picking records for one specific owner, match on an exact ID, not on a fragment of a name.WorkOrdersController.cs:70confirmed

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

@@ -1,3 +1,5 @@

1+using System.Globalization;

2+using System.Text;

3 using FieldOps.Api.Auditing;

4 using FieldOps.Api.Data;

5 using FieldOps.Api.Data.Entities;

@@ -58,4 +60,38 @@ public async Task<IActionResult> Start(int id, CancellationToken ct)

60 await _db.SaveChangesAsync(ct);

61 return NoContent();

62 }

63+

64+ [HttpGet("export")]

65+ [Authorize(Policy = "WorkOrders.Read")]

66+ public async Task<IActionResult> Export([FromQuery] string customer, CancellationToken ct)

67+ {

68+ // The DbContext's query filter limits this to the current tenant.

69+ var rows = await _db.WorkOrders

70+ .Where(w => w.CustomerName.Contains(customer))

warninglogicWorkOrdersController.cs:7065% sureconfirmed by Changliu53

The customer is matched with 'Contains', which is a partial text match. Exporting 'Smith' also pulls in 'Smithson Ltd' and 'Blacksmith Co'. An empty or very short value matches almost every customer. This file is meant to be sent to one customer at month end, so that customer could receive another customer's work orders, prices, and descriptions. The remembered rule: when picking records for one specific owner, match on an exact ID, not on a fragment of a name.

Suggested fix: Take a customer ID (for example [FromQuery] int customerId) and filter with w.CustomerId == customerId. If only the name is available, use an exact match (w.CustomerName == customer), and return 400 when it is empty.

71+ .OrderBy(w => w.ScheduledFor)

72+ .Take(5000)

73+ .ToListAsync(ct);

74+

75+ var csv = new StringBuilder("Id,Customer,Description,Scheduled,Total\n");

76+ foreach (var row in rows)

77+ {

78+ csv.Append(row.Id).Append(',')

79+ .Append(Csv(row.CustomerName)).Append(',')

80+ .Append(Csv(row.Description)).Append(',')

81+ .Append(row.ScheduledFor.ToString("O")).Append(',')

82+ .Append(row.QuotedTotal.ToString(CultureInfo.InvariantCulture)).Append('\n');

83+ }

84+

85+ return File(Encoding.UTF8.GetBytes(csv.ToString()), "text/csv", "work-orders.csv");

86+ }

87+

88+ /// <summary>

89+ /// Quotes a value for CSV. A leading =, +, - or @ is prefixed with a quote

90+ /// mark so a spreadsheet does not run it as a formula.

91+ /// </summary>

92+ private static string Csv(string value)

93+ {

94+ var safe = value.Length > 0 && "=+-@".Contains(value[0]) ? "'" + value : value;

95+ return "\"" + safe.Replace("\"", "\"\"") + "\"";

96+ }

97 }