[object Object]

← back to Dw Photo Capture

review: fix 3 findings from session code-review (cycle 9)

f6b8dec407536237297bc430dbe451c02e59ba46 · 2026-06-26 01:41:21 -0700 · Steve Abrams

Independent code-reviewer pass over the session diff — no CRITICAL. Fixed 3 real
robustness/security findings:
- /api/learn: cap vendor length at 120 chars. brandRe(vendor) is recompiled into the
  lexicon on every learn and tested against OCR text every ~600ms; an oversized vendor
  name (body allows 64KB) could hang the scanner. Now returns 400.
- build_logo_fingerprints validation: require len(nr) >= 4 before the containment check,
  so a 3-char VLM read ('Sch') can't false-validate a logo via 'sch' in 'schumacher'.
- checkAuth: split the Basic-auth credential on the FIRST colon only (RFC allows colons
  in passwords) instead of String.split(':') which truncates. Default pw unaffected.

Verified: lint + 21 unit tests green; auth good→200 / bad→401; oversized vendor→400;
normal learn→200. Two latent MINORs documented + deferred in the ledger. $0 (local).

Files touched

Diff

commit f6b8dec407536237297bc430dbe451c02e59ba46
Author: Steve Abrams <steve@designerwallcoverings.com>
Date:   Fri Jun 26 01:41:21 2026 -0700

    review: fix 3 findings from session code-review (cycle 9)
    
    Independent code-reviewer pass over the session diff — no CRITICAL. Fixed 3 real
    robustness/security findings:
    - /api/learn: cap vendor length at 120 chars. brandRe(vendor) is recompiled into the
      lexicon on every learn and tested against OCR text every ~600ms; an oversized vendor
      name (body allows 64KB) could hang the scanner. Now returns 400.
    - build_logo_fingerprints validation: require len(nr) >= 4 before the containment check,
      so a 3-char VLM read ('Sch') can't false-validate a logo via 'sch' in 'schumacher'.
    - checkAuth: split the Basic-auth credential on the FIRST colon only (RFC allows colons
      in passwords) instead of String.split(':') which truncates. Default pw unaffected.
    
    Verified: lint + 21 unit tests green; auth good→200 / bad→401; oversized vendor→400;
    normal learn→200. Two latent MINORs documented + deferred in the ledger. $0 (local).
---
 OVERNIGHT_LEDGER.md        | 7 ++++++-
 build_logo_fingerprints.py | 2 +-
 server.js                  | 4 +++-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/OVERNIGHT_LEDGER.md b/OVERNIGHT_LEDGER.md
index e04cdb5..8dd0373 100644
--- a/OVERNIGHT_LEDGER.md
+++ b/OVERNIGHT_LEDGER.md
@@ -20,7 +20,7 @@ the single highest-value safe one, the officer executes it, and the loop resched
 - [x] /learn: shows logo fingerprint + flags rejected logos with their read-as
 - [x] moondream fast-toggle for /api/identify ({fast}/{model}, allow-listed, documented in API.md)
 - [x] Auto-fire /api/identify in background on a lock-with-code-but-no-vendor (bgIdentify)
-- [ ] /code-review (or claude-codex) pass on the full session diff; fix findings
+- [x] /code-review pass — NO critical; 3 robustness/security findings fixed (learn cap, fp len-guard, auth colon)
 - [x] GATED (drafted to pending-approval): weekly launchd refresh — plist staged
 
 ## Cycle log
@@ -35,3 +35,8 @@ the single highest-value safe one, the officer executes it, and the loop resched
 | 6 | — (clear win) | analyzeOcr/vendor unit tests — harness slices the REAL fn source from server.js + eval-tests vs crafted inputs & real profile data; wired into npm test + selfcheck | 21/21 pass; lint clean; covers parseOcrRows/analyzeOcr(brand,barcode,prefix,weak,empty)/fuzzyVendor/fingerprintVendor | $0 |
 | 7 | — (clear win) | moondream fast-toggle for /api/identify: ollamaVision gains a model arg, allow-listed VISION_MODELS, {fast:true}/{model} per-request override (echoed back), API.md docs | qwen 21.5s (Kravet ✓) vs moondream 12.2s; lint+21 tests green; off-list ignored (no arbitrary pulls) | $0 |
 | 8 | — (clear win) | auto-fire /api/identify in background on a lock-with-code-but-no-vendor: thread captured frame into applyScan, bgIdentify() fires VLM logo-ID non-blocking, binds discovered vendor to the SKU via /api/learn | lint+21 tests green; page 200; fire-and-forget, never blocks resolve | $0 |
+| 9 | code-review (code-reviewer agent) | independent review of session diff → NO critical; fixed 3 real findings: /api/learn vendor length cap (regex-rebuild DoS), fingerprint validation len(nr)>=4 (short-read false-accept), checkAuth split on FIRST colon only (RFC passwords) | lint+21 tests green; auth good→200/bad→401; oversized vendor→400; normal learn→200 | $0 (local agent) |
+
+### Code-review MINORs deferred (latent, low-risk, not fixed this cycle)
+- OLLAMA_URL declared after ollamaVision def (works — only called in handlers; moving it risks colliding with the concurrent voice-feature session that owns that const).
+- bgIdentify may teach a prefix from an unresolved OCR code (intentional fire-and-forget; vendor must already be real via lexicon/fuzzy; low impact).
diff --git a/build_logo_fingerprints.py b/build_logo_fingerprints.py
index 66df587..01d7f4d 100644
--- a/build_logo_fingerprints.py
+++ b/build_logo_fingerprints.py
@@ -50,7 +50,7 @@ for v, p in todo:
             vwords = {w for w in re.findall(r"[a-z0-9]+", v.lower()) if len(w) >= 4}
             rwords = {w for w in re.findall(r"[a-z0-9]+", reads.lower()) if len(w) >= 3}
             shared = vwords & rwords
-            valid = bool(nr) and (
+            valid = bool(nr) and len(nr) >= 4 and (              # ≥4 chars: a 3-char read like "Sch" must not pass via containment
                 nr == nv or nr in nv or nv in nr or            # one name contains the other
                 len(shared) >= 2 or                            # ≥2 shared significant words
                 (len(vwords) <= 1 and bool(shared))            # single-word vendor, exact word hit
diff --git a/server.js b/server.js
index 4c3a143..90d9007 100644
--- a/server.js
+++ b/server.js
@@ -190,7 +190,8 @@ async function shopifyActivateIfReady(productId) {
 function checkAuth(req) {
   const h = req.headers.authorization || '';
   if (!h.startsWith('Basic ')) return false;
-  const [u, p] = Buffer.from(h.slice(6), 'base64').toString().split(':');
+  const raw = Buffer.from(h.slice(6), 'base64').toString();
+  const ci = raw.indexOf(':'); const u = ci < 0 ? raw : raw.slice(0, ci), p = ci < 0 ? '' : raw.slice(ci + 1);  // RFC: only first ':' splits
   return u === AUTH_USER && p === AUTH_PASS;
 }
 
@@ -574,6 +575,7 @@ const appHandler = (req, res) => {
     req.on('end', () => {
       let p; try { p = JSON.parse(body); } catch (e) { return send(res, 400, { err: 'bad json' }); }
       const vendor = String(p.vendor || '').trim();
+      if (vendor.length > 120) return send(res, 400, { err: 'vendor too long' });   // bound the regex rebuildLexicon compiles
       const sku = String(p.sku || '').toUpperCase().replace(/\s+/g, '');
       if (!vendor || !sku) return send(res, 400, { err: 'vendor + sku required' });
       const prof = VENDOR_PROFILES[vendor] || (VENDOR_PROFILES[vendor] = {

← a412dce auto-save: 2026-06-26T01:38:39 (1 files) — data/build.json  ·  back to Dw Photo Capture  ·  test: harden analyzeOcr against adversarial OCR inputs (cycl 2c6e052 →