alva/felsokning/docs/QUALITY-AUDIT.md

# Supplier quality audit — Guidad Felsökning

> **Framing.** This review was performed at the request of the product owner,
> applying the lens an OEM quality and supplier-assurance function would use
> (Volkswagen AG named as the reference customer). It is an internal engineering
> review authored by the development team — **not** an audit conducted by,
> commissioned by, or endorsed by Volkswagen AG, and it carries no external
> standing.
>
> Scope: commit `031d6d5`, branch `claude/guidad-felsokning-vision-1mnx7f`.
> Basis: source code, not documentation claims. Where the two disagree, the code
> is reported.

---

## Summary

The product is unusually well engineered for its stage. The append-only model,
the allowlist sharing boundary, split IAM roles, immutable image tags and the
refusal to state conclusions without evidence are all above what this reviewer
normally sees in a workshop-tooling supplier.

**However, the central product claim does not hold at the API boundary.** The
documentation states: *"The system can never write a conclusion that ECM has not
approved"* and *"the history is what gives the record its value in a dispute."*
Neither is true as implemented. The quality gate exists only in the browser, and
the two fields that establish evidentiary provenance — **who** did the work and
**when** — are supplied by the client and accepted unverified.

For a tool whose commercial premise is that its records hold up in warranty and
insurance disputes, that is the finding that decides everything else.

| Severity | Count | Meaning |
| --- | --- | --- |
| Critical | 4 | Blocks supplier approval. Must be closed before any pilot with customer data. |
| Major | 6 | Blocks series deployment. Closable within one release cycle. |
| Minor | 7 | Track and schedule. |

---

## Critical findings

### C-1 · Event attribution and timestamps are client-controlled

`services/plattform/server.mjs:1176–1185`

```js
if (typeof post?.id !== "string" || !post.tidpunkt || typeof post.anvandare !== "string" || !post.handelse) {
  return svara(res, 400, { error: "Ogiltig händelse." });
}
await pool.query(
  `insert into felsokning_handelser (id, arende_id, tidpunkt, anvandare, handelse)
   values ($1, $2, $3, $4, $5) on conflict (id) do nothing`,
  [post.id, handelserVag[1], post.tidpunkt, post.anvandare, post.handelse],
);
```

The server checks that `anvandare` is *a string* and that `tidpunkt` is *truthy*.
It does not check that either is true.

Consequences, all reachable with an ordinary technician token and `curl`:

- Work can be attributed to a colleague who was not present.
- Work can be backdated or forward-dated at will. The same field drives the
  billing time distribution shown to the customer.
- The JWT already carries the authenticated identity (`sub`, `namn`). It is
  ignored on this path.

An append-only log whose author and time fields are attacker-controlled is not
an audit trail. It is a signed container for unverified assertions. In a warranty
dispute, opposing counsel needs one sentence to establish that.

**Required:** derive `anvandare` from the verified JWT and reject any
client-supplied value. Set `tidpunkt` server-side; if the client's clock matters
for offline work, accept it as a separate `registrerad_tidpunkt` field and store
the server's receipt time alongside it, so the gap is visible rather than
invisible.

### C-2 · The ECM quality gate is a client-side control only

`app/src/felsokning/ecm.ts:730` · `app/src/pages/felsokning/ArendeSida.tsx:2214`

`grindGodkand()` runs in the browser. The server accepts `arende_avslutat` — and
every other event type — with no reference to the gate.

A case can therefore be closed with zero evidence, no root-cause analysis, no
symptom verification and no customer decision, by posting one event to the API.
The printed report and the traceability package will present that case as
complete.

This directly contradicts the documented control. Every compliance guarantee
built on top of ECM — warranty rule packs, insurance evidence requirements,
the customer-approval interlock — inherits the same weakness.

**Required:** evaluate the gate server-side on `arende_avslutat` and reject the
event when it fails. The client-side gate stays as user guidance; it must not be
the enforcement point. This also means ECM has to run on the server, which
argues for extracting it into `services/gemensam/` alongside the observability
module.

### C-3 · No lawful basis for erasure, retention or data-subject requests

`infra/postgres-init.sql:127–133` · no deletion path in the API

The design is append-only in three layers, deliberately and with good reason.
There is no corresponding mechanism for:

- **GDPR Art. 17 erasure.** Customer names, telephone numbers, e-mail addresses,
  registration numbers and VINs are personal data. VIN in particular is treated
  as personal data by EDPB guidance when it can be linked to a person, which is
  precisely what this system does. There is no path to erase them, and the
  database trigger actively prevents it.
- **Retention limits (Art. 5(1)(e)).** Nothing expires. `inloggningsforsok` is
  cleaned after a day; case data is kept forever by construction.
- **Data-subject access requests.** No export scoped to one person.
- **Pseudonymisation (Art. 32).** Technician names are stored in clear in every
  event.

"Append-only" and "erasure on request" are not irreconcilable, but they must be
reconciled deliberately. The standard resolution is crypto-shredding: store
personal identifiers encrypted with a per-subject key, and erase by destroying
the key. The log stays intact and hash-verifiable; the personal data becomes
unrecoverable. The evidentiary chain survives because what is erased is
identification, not the record of what was checked.

**Required:** a documented retention and erasure design before any deployment
with real customer data, plus a DPIA. This is not a feature request — without it
the product cannot lawfully be sold to an EU workshop.

### C-4 · Third-country transfer of customer and vehicle data is undocumented

`services/ai-orkester/server.mjs` · `docs/OPERATIONS.md`

The work-order scan sends a photograph of the work order — customer name,
address, telephone, e-mail, registration number, VIN — to the Anthropic API. The
guidance and review tasks send the case brief, which contains the same data plus
the fault description.

The operations document states there are "no external service dependencies
beyond the Anthropic API for the model calls." That is technically accurate and
legally the whole issue: it is the one dependency that processes personal data
outside the workshop's control.

Absent from the repository: a data processing agreement reference, a transfer
impact assessment, the processing location, the retention behaviour of the
processor, and whether the customer is informed. There is also no configuration
to run the platform with the model calls disabled for customers who cannot
accept the transfer — the methodology engine would function alone, so this is
achievable.

**Required:** document the processor relationship and the transfer basis; add a
per-organisation setting that disables model calls entirely; state in the
customer-facing report when a model contributed to the record.

---

## Major findings

### M-1 · Measured values carry no instrument identity or calibration status

`app/src/felsokning/ecm.ts:31–39` · `app/src/felsokning/domain.ts`

The evidence hierarchy rates a measured value E4 — "high" — above a photograph.
The `matvarde` event carries `beskrivning`, `varde` and `enhet`. It does not
carry which instrument produced the value, its serial number, or whether that
instrument was within calibration on the date of measurement.

In an OEM warranty audit, an uncalibrated measurement is not evidence of a lower
grade; it is not evidence. A workshop claiming warranty on a measured
out-of-tolerance value must be able to show the instrument was traceable to a
standard. The system currently ranks such a value above a photograph that
actually shows the fault.

**Required:** an instrument register per organisation (designation, serial,
calibration due date) and an instrument reference on `matvarde`. Downgrade a
measurement to E1 when no calibrated instrument is referenced, and say so in the
report rather than silently claiming E4.

### M-2 · High-voltage authorisation is self-attested, ungraded, and non-blocking

`app/src/felsokning/metodiker.ts:577–593`

The safety step asks *"Har du behörighet för arbete på högvoltsystem?"* as a
yes/no question. Three problems, in ascending order of seriousness:

1. **No competence level.** OEM practice distinguishes graded qualifications
   (instructed person / qualified for de-energised work / qualified for work
   under voltage). "Authorised" is not a single thing, and the level determines
   which steps are permissible.
2. **No expiry and no register.** The answer is a checkbox, not a lookup against
   a held qualification with a validity date.
3. **Answering "No" blocks nothing.** `nastaSteg()` advances on any answer. A
   technician who honestly declares they are not qualified is walked into the
   next step of a procedure that can kill them.

The team's own stated rule is that where work can injure someone, the safety step
comes first *and cannot be skipped*. It comes first. It can be skipped.

**Required:** make the safety step a hard interlock — a "No" must stop the
methodology and offer escalation, not advance. Record the competence level and
its validity on the organisation's user record, not as a question.

### M-3 · No server-side event schema validation

`services/plattform/server.mjs:1176`

`post.handelse` is accepted as arbitrary JSON. An unknown or malformed `typ`
enters the append-only log permanently and cannot be corrected, because the log
is append-only. A client bug at one workshop permanently contaminates that
organisation's evidentiary record.

The sharing allowlist fails safe here (an unknown type is not in `DELBART_KUND`,
so it is filtered out), which is good design and limits the blast radius to
internal views.

**Required:** validate `handelse` against the domain schema at the API boundary.
The OpenAPI specification already defines these types; validate against it.

### M-4 · No read audit trail

`services/plattform/server.mjs`

Every write is logged. No read is. There is no record of which technician,
supervisor or administrator viewed which case, nor of who followed a share link.

For personal data this is an Art. 32 gap. For the product's own value
proposition it is a missed opportunity: "the customer opened the share link
three times before approving" is exactly the kind of fact a workshop wants when a
customer later disputes having approved the work.

### M-5 · Backup is monitored but never restore-tested; no RTO/RPO

`infra/aws/50-doman-observation.tf:131–147` · `docs/OPERATIONS.md`

The backup-age alarm is well designed — alarming on missing data rather than a
threshold is a mature choice and the team documented why. But the alarm proves
that backup *occurs*, not that it *restores*. There is no scheduled restore
test, and no stated recovery time or recovery point objective.

The operations document says correctly that a lost event log destroys every
case's probative value and cannot be recreated. That makes an untested restore
the largest single-point risk in the system.

### M-6 · Compliance rules are swappable without approval or integrity control

`services/plattform/server.mjs:34–41`

The ECM rule pack is read from a file replaceable by a ConfigMap. This is a
genuinely good design for operability — new warranty terms without a release.

But the rules govern what the system will accept as a compliant case. Anyone
with namespace write access can change what "compliant" means, with no
signature, no approval record, and no four-eyes control. The pack version is
recorded in the traceability package, which is necessary but not sufficient: a
version number nobody approved proves only that something was loaded.

**Required:** sign the rule pack and verify the signature on load; record who
approved a pack version and when.

---

## Minor findings

| # | Finding | Location |
| --- | --- | --- |
| m-1 | Share code generation has modulo bias — `randomBytes` 0–255 reduced `% 36` makes the first four characters ~14 % more likely. ~82 bits remain, so not exploitable, but it signals the crypto path was not reviewed against a checklist. Use rejection sampling. | `server.mjs:299` |
| m-2 | Event `id` is client-supplied with `on conflict do nothing`. A client can pre-claim an id and silently suppress a later legitimate event. | `server.mjs:1183` |
| m-3 | EXIF stripping (including GPS) happens as a side effect of canvas re-encoding. It is correct, undocumented and untested — a future "preserve original quality" change would silently reintroduce location data into shared photographs. Lock it with a test. | `format.ts:21–25` |
| m-4 | Two orchestrator copies kept in sync by test rather than shared code. Works today; a structural liability. | `ai-orkester/`, `supabase/functions/` |
| m-5 | No SBOM and no dependency scanning in CI, with 53 direct client dependencies. Required under most OEM supplier security baselines. | `.gitea/workflows/` |
| m-6 | No end-to-end tests and no accessibility testing. The UI is designed for gloves, noise and sunlight — commendable — but EN 301 549 conformance is unevidenced. | `app/` |
| m-7 | The model that answered is logged; the prompt version and rule-pack version in force at that moment are not both bound to the response. Two identical cases can receive different guidance with no way to reconstruct why. | `ai-orkester/server.mjs` |

---

## Confirmed strengths

Stated because an audit that lists only faults gives a false picture, and because
these should be protected in any remediation.

- **No write path to the vehicle.** Diagnosis is photograph- and
  observation-based. This keeps the product outside UN R156 scope and removes an
  entire category of safety argumentation. It appears to be a deliberate choice
  and it is the right one.
- **Append-only enforced at the database.** Triggers reject changes even for a
  misconfigured role. The application layer is not the only barrier.
- **Allowlist sharing.** A forgotten event type becomes internal, not leaked.
  The test that requires every domain type to be classified is exactly the right
  control.
- **Content-addressed attachments with hash verification on read.** Serving 409
  on mismatch rather than showing the image is the correct failure mode.
- **Split build and deploy roles; immutable registry tags; IRSA with per-account
  roles; IMDSv2 hop limit 1.** This is a stronger cloud posture than most
  suppliers present at series maturity.
- **`generisk` as an honest fallback.** A system that says "we do not know where
  to start" instead of guessing is a system whose confident statements can be
  trusted. This is a quality property, not a limitation.
- **The alarm on missing backup data.** Reasoning about the failure mode rather
  than the metric.

---

## Recommended sequence

The order matters more than the list. C-1 and C-2 are cheap to fix and everything
else depends on them being true.

**Before any pilot with real data**

1. C-1 — derive `anvandare` and `tidpunkt` server-side. Roughly a day, including
   a migration note that historical rows cannot be re-attributed.
2. C-2 — move ECM to `services/gemensam/` and enforce the gate on
   `arende_avslutat`. Reuses existing pure functions.
3. M-3 — validate events against the OpenAPI schema at the boundary. Same commit
   as C-1.
4. M-2 — make the high-voltage safety step a hard interlock. Small change, and
   the one whose absence could kill someone.

**Before series deployment**

5. C-3 — retention and erasure design (crypto-shredding), plus a DPIA.
6. C-4 — processor documentation and a per-organisation model-off switch.
7. M-1 — instrument register and calibration-aware evidence grading.
8. M-5 — scheduled restore test with a stated RTO/RPO.
9. M-6 — signed rule packs with an approval record.
10. M-4 — read audit trail.

**Continuous**

11. The minor findings, m-1 and m-3 first — both are single-commit changes and
    m-3 protects a property that currently holds only by accident.

---

## Closing assessment

The engineering judgement in this codebase is consistently good, and in several
places better than the standard the product is being measured against here. The
reasoning recorded in the commit history and in the design-decision table is of a
quality that makes an audit unusually easy to conduct — motives are written down,
so it is possible to check whether the implementation matches the intent rather
than merely whether it works.

That is what makes C-1 and C-2 worth stating plainly rather than diplomatically.
They are not oversights of ambition; they are the gap between a system that
*documents* evidence and a system that *guarantees* it. The product is sold on
the second claim. Closing that gap is roughly two days of work, and until it is
closed every other guarantee in the documentation rests on a client that anyone
can bypass with `curl`.

Close C-1 and C-2, and the honest description of this product changes from
"a well-built diagnostic app" to "an evidentiary system". That is a different
market and a different price.

---

## Remediation status

Recorded after the fixes were implemented. Each entry states what was
actually changed, so a re-audit can verify rather than take it on trust.

| # | Status | What changed |
| --- | --- | --- |
| **C-1** | ✅ Closed | `anvandare` derives from the verified JWT and `tidpunkt` from the server clock (`services/gemensam/handelser.mjs`). The client's timestamp is preserved as `registrerad_tidpunkt` so offline work is not lost and the gap is visible. |
| **C-2** | ✅ Closed | The gate moved to `services/gemensam/grind.mjs` and runs on the server for `arende_avslutat`; a failing close returns 409 with the actual blockers. The methodology library moved to `services/gemensam/metodiker.mjs`; the client re-exports it typed, so there is still one source. |
| **C-3** | ✅ Closed | Crypto-shredding (`services/gemensam/personuppgifter.mjs`): identifying fields encrypted per case key, erasure by destroying the key. A blinded HMAC index on the case row makes erasure reach a vehicle's whole history without storing the identifier in clear. Retention dates set at close per case type. `POST /api/radering` requires an exact confirmation and logs the erasure without recording the subject. |
| **C-4** | ✅ Closed | `ai_tillaten` per organisation, carried in the JWT; the orchestrator refuses with 403 when false. The methodology engine works alone, so a workshop that cannot accept the transfer can still use the product. **Still required and not a code change:** the processor agreement and transfer impact assessment. |
| **M-1** | ✅ Closed | `matvarde` carries `matdonId`, designation and calibration date; an instrument register lives at `/api/matdon`. Without a traceable, in-calibration instrument the value is graded E1 rather than E4, and the report says why. Calibration is judged at the time of measurement, not today. |
| **M-2** | ✅ Closed | `SPARRFRAGOR` makes the high-voltage questions hard interlocks: a "No" stops the methodology with an explicit instruction to hand over, and the server refuses to close. An unanswered safety question also blocks — silence is not a yes. |
| **M-3** | ✅ Closed | Every event is validated against `HÄNDELSESCHEMA` before writing. Unknown types are rejected. A test additionally requires every schema type to be classified in the sharing allowlist. |
| **M-4** | ✅ Closed | `atkomstlogg` records reads with user, case, route, source and share code; readable at `/api/atkomstlogg` for supervisors and admins. |
| **M-5** | ✅ Closed | `services/plattform/aterstallningstest.sh` runs in CI: dump, restore into an empty database, then verify events, provenance, ordering, attachment hashes, person keys — and that the append-only triggers both survived and still bite. |
| **M-6** | ✅ Closed | The rule pack is verified against an HMAC. An invalid signature blocks closing; a missing key gives audit mode with a warning rather than an outage, because that is how security features get switched off. |
| **m-1** | ✅ Closed | Rejection sampling replaces modulo in `nyKod`. |
| **m-2** | ✅ Closed | Id collisions are counted and surfaced in the trace rather than passing silently. |
| **m-3** | ✅ Closed | EXIF/GPS stripping is now documented as deliberate and locked by a test that reads the code path, not just the output. |
| **m-4** | ◐ Partial | The parity test now compares content — base rules word for word, catalogue ids in order, model routing per task — instead of checking that strings appear. **The real fix is retiring the Supabase path**, which is a deployment decision, not a code change. |
| **m-5** | ✅ Closed | `npm audit` at high severity plus a CycloneDX SBOM per build, archived 400 days. |
| **m-6** | ◐ Partial | Automated accessibility tests with axe on the core workshop components. Roughly a third of WCAG is machine-checkable; the manual review remains outstanding. |
| **m-7** | ✅ Closed | Each response carries a `promptversion` — a short stable hash of model, effort and system prompt — so two answers can be compared without storing the prompt. |

### What remains

Two items are honestly open, and neither is a code change:

1. **The processor agreement and transfer impact assessment (C-4).** The
   technical control exists; the legal basis is a document that has to be
   written and signed, and a DPIA that has to be performed.
2. **Retiring the Supabase orchestrator (m-4).** Duplication defended by tests
   is a liability that grows. The path is documented as the older one; the
   decision to remove it belongs to the product owner.

A manual accessibility review (m-6) should also be scheduled. Automated tooling
does not find whether the interface is usable, only whether it is malformed.

### Re-audit verdict

The two findings that decided the assessment — C-1 and C-2 — are closed at the
API boundary, which is where they had to be closed. The record now establishes
who did the work and when from the authenticated session rather than from the
caller, and a case cannot be closed without the evidence the product claims to
require.

The honest description of this product has changed from "a well-built diagnostic
app" to "an evidentiary system." Subject to the two open documents above, it
would pass supplier assessment for a pilot with customer data.