Neutralize CSV formula injection in recording output

[BrettKinny]

Summary

Stacked on #170 (fix/csv-invariant-output) — same file, adjacent logic. Review/merge that first; this will retarget to main once it merges.

Neutralizes CSV formula injection (CWE-1236) in the recording output, from the v1.0 follow-up review (docs/V1-REVIEW-FOLLOWUP.md, low list).

• EscapeCsvField now runs the field through a NeutralizeFormulaInjection pass before the RFC 4180 quoting decision: fields starting with =, +, -, @, tab, or CR get a leading ' prefix, so a server-supplied DisplayName like =cmd|'/C calc'!A0 no longer executes as a formula when the CSV is opened in Excel/LibreOffice. DisplayName, NodeId, Value and Status all flow through it; the timestamp is locally generated in a fixed format and the header is constant.
• Critical exception: fields that parse as a number under InvariantCulture are not neutralized — recorded values are routinely negative (-12.5), and prefixing them would corrupt the data column for downstream tools. Documented in a comment.

Test plan

• dotnet build Opcilloscope.sln -c Release — 0 warnings, 0 errors
• CsvRecordingManager tests: 34/34 passing (4 new: formula DisplayName neutralized, negative value untouched, +SomeTag neutralized, combined neutralize+quote case)
• Full suite in CI (sandbox couldn't bind the integration test server's sockets; non-integration tests all pass)

---

Part of the v1 follow-up punch list (docs/V1-REVIEW-FOLLOWUP.md, CSV formula injection low).

https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie

---

Generated by Claude Code

[claude]

Code Review: CSV Formula Injection Neutralization

Overview

This PR fixes a real security vulnerability (CWE-1236) in the CSV recording output. OPC UA server-supplied fields (DisplayName, NodeId, Value, Status) are passed through a new NeutralizeFormulaInjection pass before RFC 4180 quoting, prepending a ' to fields that start with formula trigger characters. The numeric exception (don't neutralize values that parse as valid double) is sensible since negative sensor readings like -12.5 are both inert in spreadsheets and must not be corrupted.

The approach is correct and the logic is sound. A few things worth discussing:

---

Correctness

field[0] is safe — EscapeCsvField has an early IsNullOrEmpty return before the new call, so NeutralizeFormulaInjection is never given an empty string. ✓

Combined neutralize + quote test is correct — '=HYPERLINK(...) contains internal double-quotes so RFC 4180 wraps the whole thing; the assertion matches the expected layout. ✓

Numeric exception overfits slightly — double.TryParse("+Infinity", NumberStyles.Float, InvariantCulture, out _) returns true, meaning the literal string +Infinity (which is a formula trigger) would not be neutralized. This is an extreme edge case for an OPC UA sensor value, but worth noting if you ever add structured validation later.

---

Missing trigger character

The OWASP CSV Injection guidance and common spreadsheet behaviour also list \n (LF) as a potential formula injection vector in some tools (a cell can start a new row that begins with =). The PR handles \r (CR) but not bare \n. That said, a field containing \n will already be wrapped in RFC 4180 double-quotes by the existing logic, which most parsers handle correctly — the risk is real but low.

---

Test coverage gaps

The four new tests cover the most important cases. A few triggers go untested:

• @ trigger — no test (low risk but easy to add)
• Tab (\t) trigger — no test
• NodeId field — no test for formula injection in NodeId (server-controlled, same risk surface as DisplayName)
• Status field — similarly untested

These are not blocking issues for merging, but if you're building a regression suite the missing trigger chars are the easier wins.

---

Minor nit

In RecordValue_DoesNotNeutralizeNegativeNumericValue the test sets both Value = "-12.5" and RawValue = "-12.5" while the other three tests only set Value. The CSV writer uses Value (not RawValue) for the recorded field, so the RawValue assignment is dead weight in the test — harmless, just inconsistent.

---

Summary

Area	Assessment
Security fix correctness	Correct, well-scoped
Numeric exception logic	Sensible, well-documented
Empty-string safety	Guarded by existing null check
Test coverage (core cases)	All four new cases pass
Missing trigger: bare LF	Low risk; RFC 4180 quoting partially mitigates
Missing trigger tests (@, \t, NodeId)	Not blocking
Stacked on #170	Fine as long as #170 merges first

This is a clean, well-reasoned security fix. The numeric carve-out is the right call and the code comment explaining it is valuable. Happy to approve once PR #170 lands and this retargets to main.

[claude]

Code Review — PR #175: Neutralize CSV formula injection in recording output

Note: This PR is already merged. Leaving this review for the record and any follow-up work.

Overview

Good targeted fix for CWE-1236. The NeutralizeFormulaInjection method cleanly separates the security concern from RFC 4180 quoting, and the ordering (neutralize → quote) is correct: the quoting decision sees the final field content including the ' prefix, so a combined case like =HYPERLINK(...,...) gets both a leading ' and double-quote wrapping.

---

Positives

• Well-scoped: only the four server-supplied fields (DisplayName, NodeId, Value, Status) are neutralized; the locally-generated timestamp and the constant header line are explicitly excluded in the comment.
• Number exemption is the right call: -12.5 is inert in a spreadsheet, and prefixing it would break downstream numeric parsing. Using double.TryParse with NumberStyles.Float / InvariantCulture is the correct tool for this check.
• Trigger set is comprehensive: =, +, -, @, \t, \r covers all vectors in the OWASP CSV injection guide.
• Tests are well-structured: the four new cases (formula DisplayName, negative number exemption, + prefix, combined neutralize+quote) form a good behavioural matrix.

---

Issues / Suggestions

1. NeutralizeFormulaInjection has no self-contained null/empty guard (low risk, but fragile)

field[0] throws IndexOutOfRangeException on an empty string. Right now it's safe because EscapeCsvField already guards with IsNullOrEmpty before calling it. But NeutralizeFormulaInjection is a private static with no precondition documented on it — a future refactor that calls it directly, or a future test, would get a surprising exception.

// Suggested hardening:
private static string NeutralizeFormulaInjection(string field)
{
    if (field.Length == 0)
        return field;
    ...
}

2. Test setup inconsistency: RawValue sometimes set, sometimes not

RecordValue_DoesNotNeutralizeNegativeNumericValue sets both Value = "-12.5" and RawValue = "-12.5", while the other three new tests set only Value. If WriteRecord uses RawValue (not Value) as the CSV payload for the value column, the negative-number test is exercising the right code path but the other three tests may be testing a field that never reaches the CSV. Worth verifying which property WriteRecord actually uses and aligning all test setups accordingly.

3. Assertion in RecordValue_NeutralizesFormulaInjection_InDisplayName implicitly assumes no RFC 4180 quoting

Assert.Contains(",'=cmd|'/C calc'!A0,", content);

This expects the neutralized field to appear unwrapped (surrounded by bare commas). That's correct for '=cmd|'/C calc'!A0 because it contains no comma/double-quote/newline. However, if the test data ever changes to a field that also triggers RFC 4180 quoting, this assertion silently starts failing for the wrong reason. A more robust form:

// check the neutralized prefix appears and the raw trigger does not
Assert.Matches(@",'=cmd\|'/C calc'!A0,|""'=cmd\|.*"",", content);
Assert.DoesNotContain(",=cmd", content);

Or simply add a comment noting why bare-comma wrapping is expected here.

4. Minor: double.TryParse with NumberStyles.Float passes +Infinity / -Infinity / NaN

These are valid .NET doubles and won't be neutralized. That's the correct behaviour (they're not formula injection risks), but it's worth a brief note in the XML doc comment for reviewers who might otherwise wonder why +Infinity doesn't get a ' prefix.

---

Summary

The fix is correct, well-tested, and addresses a genuine security concern. The main actionable items are:

1. Low-risk but worth fixing: add a field.Length == 0 guard inside NeutralizeFormulaInjection.
2. Verify: confirm whether the three new tests that omit RawValue are actually exercising the value column or a different field.
3. Nice-to-have: tighten the assertion in the formula-DisplayName test or add a comment explaining the bare-comma expectation.