From cdb3320c4a0f794885a03e852b225c361be51aca Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 22:13:09 +0000 Subject: [PATCH] Kvalitetsrevision ur en OEM-kunds perspektiv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit En genomgång av koden — inte av dokumentationens påståenden — med den granskning en fordonstillverkares leverantörskvalitet skulle göra. Dokumentet säger uttryckligen att det är en intern övning med Volkswagen AG som referenskund, inte en revision utförd av eller på uppdrag av dem. Fyra kritiska fynd, varav två underminerar produktens kärnlöfte: Servern tar emot anvandare och tidpunkt från klienten och kontrollerar bara att de är strängar respektive sanningsvärda. JWT:n bär redan den verifierade identiteten men används inte på den vägen. En append-only logg vars författare och tidpunkt sätts av anroparen är inte en revisionskedja. Kvalitetsgrinden finns bara i webbläsaren. Ett ärende kan avslutas utan evidens genom ett anrop till API:t, och rapporten presenterar det som komplett. Dokumentationen säger att systemet aldrig kan skriva en slutsats ECM inte godkänt; det stämmer inte vid API-gränsen. De två övriga är rättsliga: append-only utan raderingsväg går inte att förena med dataskyddsförordningens artikel 17, och överföringen av kunduppgifter och VIN till modell-API:t saknar dokumenterad grund. Sex allvarliga fynd, bland dem att mätvärden rankas som hög evidens utan att bära vilket instrument som mätte eller om det var kalibrerat, och att högvoltsteget går att svara nej på och ändå fortsätta. Dokumentet listar också det som håller — och varför det bör skyddas i åtgärdsarbetet. En revision som bara räknar fel ger en falsk bild. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012EQg3rJsrQ1ZNTvkzmQAtt --- felsokning/docs/QUALITY-AUDIT.md | 353 +++++++++++++++++++++++++++++++ 1 file changed, 353 insertions(+) create mode 100644 felsokning/docs/QUALITY-AUDIT.md diff --git a/felsokning/docs/QUALITY-AUDIT.md b/felsokning/docs/QUALITY-AUDIT.md new file mode 100644 index 0000000..7aa9ff7 --- /dev/null +++ b/felsokning/docs/QUALITY-AUDIT.md @@ -0,0 +1,353 @@ +# 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.