Security & robustness hardening — v1.1.0

[Copilot]

Addresses a set of security, robustness, and architectural findings: unvalidated weight parameters silently accepting NaN/Infinity, DOM-XSS sink in the dashboard's buildTable, missing security response headers, and a fragile bare-tuple weight-resolution API.

Security

• Weight validation (metrics.py): _validate_real_number() now called on all weight parameters in compute_G, compute_CG, compute_UGS — non-finite weights raise ValueError immediately
• G/CG validation (compute_UGS): added _validate_real_number(G, "G") and _validate_real_number(CG, "CG") for direct callers bypassing the API pipeline
• API boundary (GoodwillInput): @model_validator rejects non-finite weight overrides before they reach compute functions
• Security headers: @app.middleware("http") now sets X-Content-Type-Options: nosniff, X-Frame-Options: DENY, and Content-Security-Policy on every response
• XSS hardening (dashboard.html): buildTable refactored from innerHTML string interpolation to createElement/textContent
• Client-side NaN guard: extracted _collectPayload() helper that checks all num() calls with isNaN() before submitting to the API, surfacing a user-friendly error

Architecture

• ResolvedWeights NamedTuple: replaces the anonymous 10-tuple returned by _resolve_weights; _build_result now takes a single weights: ResolvedWeights argument instead of 11 positional parameters

class ResolvedWeights(NamedTuple):
    g_w1: float; g_w2: float; g_w3: float; g_w_t: float
    cg_w1: float; cg_w2: float; cg_w3: float; cg_w4: float
    ugs_w1: float; ugs_w2: float

Maintainability

• __version__ = "1.1.0" added to goodwill/__init__.py as single source of truth; app/main.py imports it for FastAPI(version=...)
• Python 3.13 added to CI matrix
• Major-version upper-bound caps added to all four entries in requirements.txt
• CITATION.cff bumped to 1.1.0

Original prompt

Security & Robustness Audit — Actionable Improvements

This PR addresses findings from a comprehensive security, robustness, and architectural review of the Goodwill-KPI codebase. All changes preserve the immutability of the core equations in goodwill/metrics.py and respect the project's separation-of-concerns philosophy.

---

1. SECURITY HARDENING

1a. Weight parameters accept unvalidated values (HIGH)

Files: app/main.py (lines 76–89), goodwill/metrics.py (lines 97–109, 164–174, 227–233)

Problem: Weight parameters (g_w1, g_w2, etc.) in the GoodwillInput Pydantic model have no range constraints. A caller can submit NaN, Infinity, or extremely large/negative weights. While _validate_real_number is called on metric inputs, weights are never validated — they pass directly into arithmetic. This means:

• w1=float('inf') → produces inf results without error
• w1=float('nan') → produces nan results without error
• w1=-1e308 → produces astronomically wrong results silently

Fix:

• Add _validate_weight in goodwill/metrics.py that calls _validate_real_number on each weight parameter inside compute_G, compute_CG, and compute_UGS.
• Add Pydantic-level Field constraints on weight fields in GoodwillInput to reject non-finite values at the API boundary (e.g., add a model validator or use Field(..., allow_inf_nan=False) where supported, or add a @model_validator that checks math.isfinite() on each weight).

1b. Dashboard HTML uses innerHTML with server-rendered values (MEDIUM)

File: app/templates/dashboard.html (line 149)

Problem: The buildTable function uses template literal interpolation into innerHTML:

t.innerHTML = '<tr>...' + rows.map(r => `<tr><td>${r[0]}</td><td>${r[1]}</td>...`).join('');

While the values come from the server JSON response (which is numeric), this is a DOM-XSS sink. If the API response ever included user-controlled strings, this would be exploitable. Defense-in-depth says: don't use innerHTML when textContent + DOM construction would work.

Fix: Refactor buildTable to use document.createElement / textContent for cell values instead of string interpolation into innerHTML.

1c. Content-Disposition header filename not sanitized (LOW)

File: app/main.py (lines 226, 329, 344)

Problem: The filename variable in export responses is constructed from format which is regex-validated to xlsx|csv, so this is safe today. However, the timestamp component uses datetime.now().strftime() which is safe. No immediate vulnerability, but add a comment documenting why this is safe (the format query parameter pattern constraint).

1d. No X-Content-Type-Options or other security headers (MEDIUM)

File: app/main.py

Problem: The FastAPI application does not set standard security headers. While the SECURITY.md notes TLS is the platform's responsibility, the application should set:

• X-Content-Type-Options: nosniff (prevents MIME-type sniffing on exports)
• X-Frame-Options: DENY (prevents clickjacking of the dashboard)
• Content-Security-Policy: default-src 'self'; script-src 'unsafe-inline' (CSP for the dashboard — unsafe-inline is needed for the current inline script, but documents the boundary)

Fix: Add a simple ASGI middleware or @app.middleware("http") that sets these headers on every response.

---

2. ROBUSTNESS & RESILIENCE

2a. compute_UGS does not validate G and CG inputs (HIGH)

File: goodwill/metrics.py (lines 227–271)

Problem: compute_UGS validates T but does not validate G or CG with _validate_real_number. If a caller passes G=float('nan') or CG=float('inf') directly (not through the API pipeline), the function silently returns a non-finite result.

Fix: Add _validate_real_number(G, "G") and _validate_real_number(CG, "CG") at the top of compute_UGS.

2b. Dashboard num() helper returns NaN silently (MEDIUM)

File: app/templates/dashboard.html (line 144)

Problem: parseFloat("") returns NaN. If a user clears an input field and clicks Calculate, NaN is sent to the API. Pydantic will reject it, but the error message is opaque to the user.

Fix: Add client-side pre-validation in runCalculation() and exportFile() that checks all num() calls for isNaN() and displays a user-friendly error before making the API call.

2c. No explicit __version__ in the package (LOW)

File: goodwill/__init__.py

Problem: The package has no __version__ attribute. The version is only in CITATION.cff and app/main.py. For library consumers, goodwill.__version__ is the standard introspection mechanism.

Fix: Add __version__ = "1.1.0" to goodwill/__init__.py and reference it from app/main.py's FastAPI(version=...).

---

3. ARCHITECTURAL CLARITY

3a. _resolve_weights returns a bare 10-tuple (MEDIUM)

File: `a...

This pull request was created from Copilot chat.

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Copilot]

Pull request overview

This PR hardens the Goodwill-KPI service against non-finite numeric inputs, reduces DOM-XSS risk in the dashboard UI, and improves robustness/maintainability via clearer weight handling and centralized versioning.

Changes:

• Add finiteness validation for weight parameters (and direct G/CG inputs to compute_UGS) across compute functions and at the API boundary.
• Harden the dashboard by removing innerHTML table rendering and adding client-side payload validation before POSTing to the API.
• Introduce security response headers middleware, replace tuple-based weight resolution with a ResolvedWeights NamedTuple, and centralize package versioning.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
requirements.txt	Add major-version upper bounds to dependencies.
goodwill/metrics.py	Validate weights as finite real numbers; validate G/CG in compute_UGS; update docstrings accordingly.
goodwill/init.py	Add __version__ as package-level version source of truth.
app/templates/dashboard.html	Replace innerHTML with DOM construction; add _collectPayload() NaN guard reused by calculate/export flows.
app/main.py	Add security-headers middleware; validate non-finite weight overrides; refactor weight resolution to ResolvedWeights; use package __version__.
CITATION.cff	Bump citation version/date to 1.1.0.
.github/workflows/ci.yml	Add Python 3.13 to CI matrix.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The new security-headers middleware is user-visible behavior (and part of the PR’s security goals), but the existing dashboard/service tests don’t assert these headers are present. Add/extend tests to verify at least X-Content-Type-Options, X-Frame-Options, and Content-Security-Policy are set on representative HTML and API responses.

[Copilot]

Weight parameters are now validated for finiteness, but there are no unit tests covering the new failure modes (e.g., w1=float('nan') / float('inf')). Add tests to assert compute_G rejects non-finite weights (and ideally reports the correct parameter name).

[Copilot]

compute_UGS now validates G and CG for finiteness, but the test suite currently only covers non-finite T. Add unit tests to ensure non-finite G / CG inputs raise ValueError (and that the error message points to the correct field).

[Copilot]

Client-side validation only checks isNaN(v). parseFloat('Infinity') yields Infinity (not NaN) and JSON.stringify will serialize non-finite numbers as null, which for the optional weight fields would be silently treated as “no override” (defaults) instead of surfacing an error. Consider validating with Number.isFinite(v) (or equivalent) so Infinity/-Infinity are rejected client-side with the same user-friendly message.

[Copilot]

The CSP added here allows inline scripts but does not allow inline styles. This dashboard relies on a <style> block and multiple inline style attributes, which will be blocked by this policy in modern browsers (unstyled/broken layout). Consider either adding an appropriate style-src directive (or hashes/nonces) or moving styles to an external stylesheet so the CSP can remain strict.

Suggested change

	# 'unsafe-inline' is required for the dashboard's inline <script> block.
	# Moving scripts to an external file would allow a stricter policy.
	"default-src 'self'; script-src 'unsafe-inline'"
	# 'unsafe-inline' is required for the dashboard's inline <script> block
	# and inline styles (e.g., <style> blocks and style attributes).
	# Moving scripts/styles to external files (with hashes/nonces) would
	# allow a stricter policy.
	"default-src 'self'; "
	"script-src 'self' 'unsafe-inline'; "
	"style-src 'self' 'unsafe-inline'"