diff --git a/CLAUDE.md b/CLAUDE.md index b8cb4287..783224b8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -182,7 +182,9 @@ restores the image but never the schema, so keep migrations additive. units refresh removes the board once nothing else is on it (`boards.RemoveAbandoned`, #452); `openipc boards remove` does it by hand. A member may edit their own report from `/club` (`reports.Store.Edit`, migration 031): the - note, the camera proposed, files out and in; ipctool's output and a backup + note, the camera proposed, files out and in, and a flash dump into a report + without one (#456: a programmer's image, or the ipctool backup taken with + the report's own output, with its consent); ipctool's output and a backup stay as sent. Like takedown it stands the guard down for its transaction, and every change is logged in append-only `report_edits`. An edit to a report already decided adds a review row `edit`: pending again, off the diff --git a/frontend/apps/site/src/components/club/EditReport.test.tsx b/frontend/apps/site/src/components/club/EditReport.test.tsx index 462cef05..51b98187 100644 --- a/frontend/apps/site/src/components/club/EditReport.test.tsx +++ b/frontend/apps/site/src/components/club/EditReport.test.tsx @@ -70,3 +70,24 @@ test('a text kind says what it is, as the send forms do: the console stopped at expect(queryByText(/printenv/)).toBeNull(); expect(queryByText(/from power-on to the login prompt/)).not.toBeNull(); }); + +test('a report without a dump can be given one, private unless its owner ticks the box', async () => { + const sent = capture(); + const saved = vi.fn(); + const { container, getByText, getByLabelText } = render( {}} />); + fireEvent.change(container.querySelector('select')!, { target: { value: 'backup' } }); + expect(getByText(/as a programmer read it/)).toBeTruthy(); + fireEvent.change(getByLabelText('Add', { selector: 'input' }), { target: { files: [new File([new Uint8Array(1 << 20)], 'flash.bin')] } }); + fireEvent.click(getByText('Save')); + await waitFor(() => expect(saved).toHaveBeenCalled()); + expect((sent[0].form.get('backup') as File).name).toBe('flash.bin'); + expect(sent[0].form.get('consent')).toBe('private'); +}); + +test('a report that has a dump is not offered another', () => { + capture(); + const withDump = report({ files: [{ position: 1, kind: 'backup', name: 'backup.bin', bytes: 1 << 20, url: '', points: 10 }] }); + const { container } = render( {}} onCancel={() => {}} />); + const values = [...container.querySelectorAll('option')].map((o) => o.value); + expect(values).toEqual(['photo', 'boot_log', 'uboot_env']); +}); diff --git a/frontend/apps/site/src/components/club/EditReport.tsx b/frontend/apps/site/src/components/club/EditReport.tsx index 4444719b..1c13fe31 100644 --- a/frontend/apps/site/src/components/club/EditReport.tsx +++ b/frontend/apps/site/src/components/club/EditReport.tsx @@ -1,16 +1,16 @@ /** * A member changing a report of theirs from /club * (POST /api/v1/club/reports/{id}/edit): the note, the camera they proposed, - * files taken out, photos and text added. ipctool's output and a backup stay - * as sent. A report a maintainer has decided goes back to them: the form says + * files taken out, photos and text added, and a flash dump added to a report + * that has none (#456). ipctool's output and a backup stay as sent. A report a maintainer has decided goes back to them: the form says * so before it is saved. */ import { useState } from 'preact/hooks'; import type { BoardsT } from '../../lib/boards-i18n'; import { editReport, type MemberReport } from '../../lib/club'; -type AddKind = 'photo' | 'boot_log' | 'uboot_env'; -const ADD_KINDS: AddKind[] = ['photo', 'boot_log', 'uboot_env']; +type AddKind = 'photo' | 'boot_log' | 'uboot_env' | 'backup'; +const ADD_KINDS: AddKind[] = ['photo', 'boot_log', 'uboot_env', 'backup']; const field = 'rounded-md border border-hairline px-2.5 py-1.5 text-sm text-body'; export default function EditReport({ report, t, onSaved, onCancel }: { @@ -23,9 +23,12 @@ export default function EditReport({ report, t, onSaved, onCancel }: { const [remove, setRemove] = useState>(new Set()); const [kind, setKind] = useState('photo'); const [add, setAdd] = useState([]); + const [publicDump, setPublicDump] = useState(false); const [busy, setBusy] = useState(false); const [error, setError] = useState(null); const decided = report.status === 'published' || report.status === 'rejected'; + // One dump a report: the one it has stays as sent, so none is offered. + const kinds = report.files.some((f) => f.kind === 'backup') ? ADD_KINDS.filter((k) => k !== 'backup') : ADD_KINDS; const toggle = (pos: number) => { const next = new Set(remove); @@ -43,6 +46,7 @@ export default function EditReport({ report, t, onSaved, onCancel }: { } for (const pos of remove) form.append('remove', String(pos)); for (const f of add) form.append(kind, f, f.name); + if (kind === 'backup' && add.length > 0) form.set('consent', publicDump ? 'public' : 'private'); setBusy(true); setError(null); editReport(report.id, form) @@ -86,13 +90,23 @@ export default function EditReport({ report, t, onSaved, onCancel }: { {t('club.edit_add')}
{/* keyed on the kind: switching it clears what was picked, input and all */} - setAdd([...((e.target as HTMLInputElement).files ?? [])])} class="text-sm text-body" />
- {kind !== 'photo' &&

{t(`club.send_hint_${kind}`)}

} + {(kind === 'boot_log' || kind === 'uboot_env') &&

{t(`club.send_hint_${kind}`)}

} + {kind === 'backup' && ( + <> +

{t('club.send_dump')}

+ + + )}
diff --git a/service/internal/club/edit_test.go b/service/internal/club/edit_test.go index 4a97b19e..cae9d262 100644 --- a/service/internal/club/edit_test.go +++ b/service/internal/club/edit_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "mime/multipart" + "os" "strconv" "strings" "testing" @@ -191,3 +192,78 @@ func TestAnEditNeverSlipsPastAReviewer(t *testing.T) { t.Errorf("taking out a backup: %d %v", code, d) } } + +// A flash dump can be added to a report sent without one (#456): a whole +// chip read with a programmer, or an ipctool backup taken with the report's +// own output, asked for its consent as on sending. A decided report goes +// back for review and the dump is paid then. One a report already has +// stays as sent, and nothing else's dump can come in this way. +func TestAnEditAddsAFlashDumpToAReportWithoutOne(t *testing.T) { + e := newEnv(t) + ctx := context.Background() + ivan := e.signedIn(t, 777, "ivan_k", "198.51.100.1") + maint := e.maintainer(t) + _, out := ivan.send(t, map[string]string{"channel": "web", "model": "anjoy-ms-j10"}, map[string][]byte{"photo": photo}) + id := out["id"].(string) + if code, d := maint.json(t, "POST", "/api/v1/club/review/"+id, map[string]any{"decision": "publish"}); code != 200 || d["points"].(float64) != 1 { + t.Fatalf("publish: %d %v", code, d) + } + chip := bytes.Repeat([]byte{0x3c}, 1<<20) + + // What is refused, leaving the report as it was. + if code, d := ivan.edit(t, id, map[string]string{"consent": "everyone"}, map[string][]byte{"backup": chip}); code != 400 || !strings.Contains(d["error"].(string), "consent") { + t.Errorf("an unknown consent: %d %v", code, d) + } + if code, d := ivan.edit(t, id, nil, map[string][]byte{"backup": bytes.Repeat([]byte{1}, 1000)}); code != 400 || !strings.Contains(d["error"].(string), "backup:") { + t.Errorf("neither a backup nor a chip: %d %v", code, d) + } + backup := append(append([]byte("chip:\n vendor: HiSilicon\n"), 0), 4, 0, 0, 0, 'b', 'o', 'o', 't') + if code, d := ivan.edit(t, id, nil, map[string][]byte{"backup": backup}); code != 400 || !strings.Contains(d["error"].(string), "joins this one") { + t.Errorf("an ipctool backup on a report without ipctool's output: %d %v", code, d) + } + petr := e.signedIn(t, 888, "petr", "198.51.100.2") + if code, _ := petr.edit(t, id, nil, map[string][]byte{"backup": backup}); code != 404 { + t.Errorf("a dump on another member's report: %d", code) + } + if r := e.status(t, ivan, id); r["status"] != "published" || len(r["files"].([]any)) != 1 { + t.Fatalf("a refused edit changed the report: %v", r) + } + + // The chip, public: the report goes back for review, and is paid for + // the dump when it is published again. + code, d := ivan.edit(t, id, map[string]string{"consent": "public"}, map[string][]byte{"backup": chip}) + if code != 200 || d["status"] != "pending" || d["rereview"] != true { + t.Fatalf("adding the chip: %d %v", code, d) + } + var consent, name string + _ = e.pool.QueryRow(ctx, `SELECT r.backup_consent, f.name FROM reports r JOIN report_files f ON f.report_id = r.id AND f.kind = 'backup' WHERE r.id = $1`, id).Scan(&consent, &name) + if consent != "public" || name != "flash.bin" { + t.Errorf("the dump: consent %q, name %q", consent, name) + } + if code, d := maint.json(t, "POST", "/api/v1/club/review/"+id, map[string]any{"decision": "publish"}); code != 200 || d["points"].(float64) != 10 { + t.Errorf("publishing it again: %d %v", code, d) + } + // A second one is refused: the first stays as it was sent. + if code, d := ivan.edit(t, id, nil, map[string][]byte{"backup": bytes.Repeat([]byte{0x3d}, 1<<20)}); code != 400 || !strings.Contains(d["error"].(string), "has one already") { + t.Errorf("a second dump: %d %v", code, d) + } + + // ipctool's report gets the backup ipctool took with it, and no other. + yaml, err := os.ReadFile("../reports/testdata/hi3516cv300-imx291.txt") + if err != nil { + t.Fatal(err) + } + _, out = ivan.send(t, map[string]string{"channel": "web", "yaml": string(yaml)}, nil) + withYAML := out["id"].(string) + if code, d := ivan.edit(t, withYAML, nil, map[string][]byte{"backup": backup}); code != 400 || !strings.Contains(d["error"].(string), "other ipctool output") { + t.Errorf("a backup taken with other output: %d %v", code, d) + } + own := append(append(append([]byte{}, yaml...), 0), 4, 0, 0, 0, 'b', 'o', 'o', 't') + if code, d := ivan.edit(t, withYAML, nil, map[string][]byte{"backup": own}); code != 200 || d["status"] != "pending" { + t.Errorf("the backup taken with its output: %d %v", code, d) + } + _ = e.pool.QueryRow(ctx, `SELECT r.backup_consent, f.name FROM reports r JOIN report_files f ON f.report_id = r.id AND f.kind = 'backup' WHERE r.id = $1`, withYAML).Scan(&consent, &name) + if consent != "private" || name != "backup.bin" { + t.Errorf("the ipctool backup: consent %q, name %q", consent, name) + } +} diff --git a/service/internal/reports/club.go b/service/internal/reports/club.go index 47c7f1d2..5e871ad6 100644 --- a/service/internal/reports/club.go +++ b/service/internal/reports/club.go @@ -85,10 +85,17 @@ type MemberFile struct { func (s *Store) Owner(ctx context.Context, id string) (string, error) { var m *string err := s.DB.QueryRow(ctx, `SELECT member_id FROM report_submissions WHERE report_id = $1`, id).Scan(&m) - if errors.Is(err, pgx.ErrNoRows) || m == nil { + // No submission, or one sent signed out, has no owner. A query that + // failed is an error, never that: m is nil then too. + switch { + case errors.Is(err, pgx.ErrNoRows): + return "", nil + case err != nil: + return "", err + case m == nil: return "", nil } - return *m, err + return *m, nil } // Mine is every report a member sent, newest first. diff --git a/service/internal/reports/edit.go b/service/internal/reports/edit.go index 98d9257a..dbe6d1ed 100644 --- a/service/internal/reports/edit.go +++ b/service/internal/reports/edit.go @@ -15,8 +15,9 @@ import ( ) // A member changes what they sent (migration 031): the note, the camera -// they proposed, a file taken out, photos and text added. ipctool's output -// and a backup are what the camera said and stay as sent. An edit to a +// they proposed, a file taken out, photos and text added, and a flash dump +// added to a report that has none (#456). ipctool's output and a backup are +// what the camera said and stay as sent. An edit to a // report a maintainer has decided puts it back in the queue -- a review row // 'edit', after which it is pending: off the board and the public pages // until it is accepted again, its stars settled by that decision. @@ -37,6 +38,8 @@ type Change struct { Proposal *Proposal Remove []int Add []File + // Consent is the added backup's, private or public; "" when Add has none. + Consent string } // Edited is what an edit did. @@ -143,9 +146,35 @@ func (s *Store) Edit(ctx context.Context, id, member, by string, c Change, noteP } removed[pos] = true } - if len(files)-len(removed)+len(c.Add) > maxFiles { + // A backup is not one of the files the limit counts, as on sending. + kept := 0 + for pos, h := range files { + if h.kind == "backup" && c.Consent != "" { + return refusal{errors.New("backup: this report has one already, and it stays as it was sent")} + } + if h.kind != "backup" && !removed[pos] { + kept++ + } + } + for _, f := range c.Add { + if f.Kind != "backup" { + kept++ + } + } + if kept > maxFiles { return refusal{fmt.Errorf("at most %d files per report", maxFiles)} } + if c.Consent != "" { + // The consent was "none": a report without a backup had nothing + // to consent to. A dump added now is asked for it, as on sending. + tag, err := tx.Exec(ctx, `UPDATE reports SET backup_consent = $2 WHERE id = $1 AND backup_consent = 'none'`, id, c.Consent) + if err != nil { + return err + } + if tag.RowsAffected() != 1 { + return refusal{errors.New("backup: this report has one already, and it stays as it was sent")} + } + } if hasProposal { photos := 0 for pos, h := range files { @@ -319,6 +348,17 @@ func (a *API) Edit(w http.ResponseWriter, r *http.Request, member, by, id string } c.Remove = in.removes + // Someone else's report answers as no report at all, before anything of + // it -- whether it holds ipctool's output, below -- can show in a + // refusal. Edit checks again under the report's lock. + if owner, err := st.Owner(ctx, id); err != nil || owner != member { + if err != nil { + a.fail(w, "the report", err) + } else { + a.refuse(w, http.StatusNotFound, "no report of yours has this id") + } + return false + } // The report's own facts redact what the edit adds, as they did what it // was sent with; any MAC is caught regardless (Redact). rep, err := st.Private(ctx, id) @@ -335,6 +375,43 @@ func (a *API) Edit(w http.ResponseWriter, r *http.Request, member, by, id string _, facts, _ = Parse(rep.YAML) } var places []func() error + if in.backup != nil { + // A dump added later is what one sent with the report would be: an + // ipctool backup taken with this report's own output, or a whole + // chip read with a programmer. The report already says which camera + // it is, so the image needs no board named beside it. + consent := orDefault(in.fields["consent"], "private") + if consent != "private" && consent != "public" { + a.refuse(w, http.StatusBadRequest, "consent is private (OpenIPC's maintainers only) or public (published with the report)") + return false + } + name := "backup.bin" + f, err := in.backup.Open() + var b Backup + if err == nil { + b, err = ReadBackup(f) + } + switch { + case err == nil && rep.YAML == "": + a.refuse(w, http.StatusBadRequest, "backup: it carries ipctool's output, which an edit cannot add; send it as a new report, or from the camera with a club code that joins this one") + return false + case err == nil && Clean(rep.YAML) != Clean(b.YAML): + a.refuse(w, http.StatusBadRequest, "backup: it was taken with other ipctool output than this report's") + return false + case err == nil: + case flashImage(in.backup.Bytes): + name = "flash.bin" + default: + a.refuse(w, http.StatusBadRequest, "backup: "+err.Error()) + return false + } + bf, err := a.prepare(part{kind: "backup", name: name, mime: "application/octet-stream", in: in.backup}, consent, facts, key, &places) + if err != nil { + a.fail(w, "the backup", err) + return false + } + c.Add, c.Consent = append(c.Add, bf), consent + } for _, p := range in.parts { f, err := a.prepare(p, "", facts, key, &places) if err != nil { diff --git a/service/internal/reports/handler.go b/service/internal/reports/handler.go index b2d93c62..2efb9957 100644 --- a/service/internal/reports/handler.go +++ b/service/internal/reports/handler.go @@ -479,9 +479,9 @@ func (a *API) read(r *http.Request, edit bool) (*received, int, error) { } in.removes = append(in.removes, pos) continue - case p.FileName() == "" && name != "note" && name != "maker" && name != "board" && name != "soc", - name == "yaml", name == "backup": - return in, http.StatusBadRequest, fmt.Errorf("%q cannot be edited: an edit changes the note, the camera proposed, and adds or removes photos and text files", name) + case p.FileName() == "" && name != "note" && name != "maker" && name != "board" && name != "soc" && name != "consent", + name == "yaml": + return in, http.StatusBadRequest, fmt.Errorf("%q cannot be edited: an edit changes the note, the camera proposed, adds or removes photos and text files, and adds a flash dump to a report without one", name) } } // "note" is both a field (a line of text) and a file kind (a note