fix(16): revise plans based on checker feedback
This commit is contained in:
parent
00b8afc546
commit
d63b9adadf
3 changed files with 11 additions and 4 deletions
|
|
@ -123,7 +123,11 @@ New exports from lib/services/eml-parser.ts (define these — they are the contr
|
|||
From the repo root `/opt/stacks/pulse`, run `npm install mailparser linkify-it` as a single deliberate command (NOT inside a throwaway verification script). Target versions confirmed clean by the research slopcheck audit: mailparser 3.9.14, linkify-it 6.0.0 (both MIT, no postinstall scripts, [OK] verdict). Also install the matching `@types/mailparser` and `@types/linkify-it` devDependencies if the packages do not ship their own bundled types (check `npm view <pkg> types` first). After installing, run `git diff --stat package.json` and confirm ONLY `mailparser` and `linkify-it` (plus their `@types/*` if needed) were added to `dependencies`/`devDependencies` — no unrelated churn from a cwd that drifted. If `git diff package.json` shows any package other than these being added, revert with `git checkout -- package.json package-lock.json` and re-run from the confirmed repo root.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /opt/stacks/pulse && node -e "require('mailparser'); require('linkify-it'); console.log('deps-ok')" && git diff package.json | grep -E '^\+' | grep -vE 'mailparser|linkify-it|@types/(mailparser|linkify-it)|^\+\+\+' | grep -E '"[a-z]' && echo "UNEXPECTED_DEP" || echo "clean"</automated>
|
||||
<!-- Two INDEPENDENT checks. The require-check exits non-zero on its own failure
|
||||
(not swallowed by the diff-check's `|| echo clean` fallback), and the
|
||||
diff-check's fallback only reports on the diff-check itself. -->
|
||||
<automated>cd /opt/stacks/pulse && node -e "require('mailparser'); require('linkify-it'); console.log('deps-ok')" || { echo "DEP_REQUIRE_FAILED"; exit 1; }
|
||||
git -C /opt/stacks/pulse diff package.json | grep -E '^\+' | grep -vE 'mailparser|linkify-it|@types/(mailparser|linkify-it)|^\+\+\+' | grep -qE '"[a-z]' && { echo "UNEXPECTED_DEP"; exit 1; } || echo "clean"</automated>
|
||||
</verify>
|
||||
<done>Both packages import at runtime; `git diff package.json` added lines contain only mailparser, linkify-it, and optionally their @types — the guard command prints "clean" (no UNEXPECTED_DEP).</done>
|
||||
<acceptance_criteria>
|
||||
|
|
|
|||
|
|
@ -169,10 +169,10 @@ Existing (migrations/097_phishing_triage_schema.sql lines 100-108) — indicator
|
|||
- CLAUDE.md (migration caveat: Postgres init applies migrations on first volume boot only; existing dev DB needs manual apply via docker exec)
|
||||
</read_first>
|
||||
<action>
|
||||
Create `migrations/099_indicators_metadata.sql` following the migrations/083 doc-comment shape: a banner referencing Phase 16 / D-07, a one-sentence purpose ("lets an attachment-hash indicator carry filename/content-type/size, a URL indicator carry which message part it came from, without duplicating into the parent messages row"), and a one-sentence "does not change existing rows" note. The DDL is `ALTER TABLE indicators ADD COLUMN IF NOT EXISTS metadata JSONB;` — nullable, no NOT NULL, no default (per D-07's literal wording; no backfill needed since no rows exist yet). Add a `COMMENT ON COLUMN indicators.metadata IS '...'` line. Then apply it to the running dev database manually (the migration file is the source of truth for fresh installs, but the long-lived dev volume will not auto-run it) via `docker exec -i pulse-postgres psql -U <user> -d <db>` piping the file — confirm the exact container name and credentials from docker-compose.yml / .env before running.
|
||||
Create `migrations/099_indicators_metadata.sql` following the migrations/083 doc-comment shape: a banner referencing Phase 16 / D-07, a one-sentence purpose ("lets an attachment-hash indicator carry filename/content-type/size, a URL indicator carry which message part it came from, without duplicating into the parent messages row"), and a one-sentence "does not change existing rows" note. The DDL is `ALTER TABLE indicators ADD COLUMN IF NOT EXISTS metadata JSONB;` — nullable, no NOT NULL, no default (per D-07's literal wording; no backfill needed since no rows exist yet). Add a `COMMENT ON COLUMN indicators.metadata IS '...'` line. Then apply it to the running dev database using the project's existing helper `scripts/apply-migrations.sh 099_indicators_metadata.sql` (per CLAUDE.md's guidance to prefer that script for applying migrations to an existing DB). The script already carries the project's correct credential defaults (`POSTGRES_USER:-pulse_user`, `POSTGRES_DB:-pulse_autotask`) and applies a single named migration via `docker exec -i pulse-postgres psql`, exiting non-zero on failure — do NOT hand-roll a raw `docker exec ... -U <default> -d <default>` command, whose fallback defaults would be wrong for this project (real values are `pulse_user` / `pulse_autotask`, not `pulse` / `pulse`). The migration file remains the source of truth for fresh installs; the long-lived dev volume needs this manual apply because Postgres init only runs migrations on first volume boot.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /opt/stacks/pulse && grep -q "ADD COLUMN IF NOT EXISTS metadata JSONB" migrations/099_indicators_metadata.sql && docker exec pulse-postgres psql -U "${POSTGRES_USER:-pulse}" -d "${POSTGRES_DB:-pulse}" -tAc "SELECT data_type FROM information_schema.columns WHERE table_name='indicators' AND column_name='metadata'" | grep -q jsonb && echo "migration-applied"</automated>
|
||||
<automated>cd /opt/stacks/pulse && grep -q "ADD COLUMN IF NOT EXISTS metadata JSONB" migrations/099_indicators_metadata.sql && docker exec pulse-postgres psql -U "${POSTGRES_USER:-pulse_user}" -d "${POSTGRES_DB:-pulse_autotask}" -tAc "SELECT data_type FROM information_schema.columns WHERE table_name='indicators' AND column_name='metadata'" | grep -q jsonb && echo "migration-applied"</automated>
|
||||
</verify>
|
||||
<done>migrations/099_indicators_metadata.sql exists with the ALTER + COMMENT following the 083 shape; the metadata JSONB column exists on the live dev indicators table.</done>
|
||||
<acceptance_criteria>
|
||||
|
|
|
|||
|
|
@ -404,22 +404,25 @@ mail.attachments[0].checksum; // e.g. "5b6543922ff74615afd92401489840af7b95a8ec0
|
|||
|
||||
All other claims in this research are `[VERIFIED]` (live Autotask API calls, live npm registry queries, live `mailparser`/`linkify-it` install-and-run tests, direct reads of this codebase's source) or `[CITED]` (Context7 official mailparser/mailauth documentation, the project's own `SKILL.md`). No claim about the Authentication-Results grammar itself is deeper than RFC 8601's well-established structure, used only as design input for hand-rolled code the planner will fully specify and test.
|
||||
|
||||
## Open Questions
|
||||
## Open Questions (RESOLVED)
|
||||
|
||||
1. **Should `Authentication-Results-Original` be parsed into its own structured field, or is the outermost `Authentication-Results` sufficient for D-06?**
|
||||
- What we know: real Mimecast/M365-protected tickets can carry both; they can disagree (e.g. Mimecast remediation can downgrade a verdict after receipt).
|
||||
- What's unclear: whether Phase 19's classifier needs the pre-remediation verdict specifically, or just "the current, authoritative one."
|
||||
- Recommendation: parse both if present under distinct keys (e.g. `authResults` and `authResultsOriginal`) — cheap to add now, expensive to retrofit once Phase 19 depends on a single-verdict shape.
|
||||
- RESOLVED: Adopted in 16-01-PLAN.md Task 3 — `NormalizedMessage.authResultsOriginal` is a distinct structured field, populated from an `Authentication-Results-Original` header when present (parsed by iterating `mail.headerLines`), null otherwise.
|
||||
|
||||
2. **Should inline/CID-referenced attachments be excluded from `messages.attachments`/`indicators`, or included with a flag?**
|
||||
- What we know: mailparser's `attachment.related` boolean distinguishes them.
|
||||
- What's unclear: whether an inline tracking pixel (a classic phishing technique) should be treated as attachment metadata or promoted to its own indicator type.
|
||||
- Recommendation: keep both, but persist the `related` flag so Phase 19 can weight/filter as needed rather than losing the distinction at parse time.
|
||||
- RESOLVED: Adopted in 16-01-PLAN.md Task 3 — inline/CID parts are kept, not dropped, and each `AttachmentMeta` carries a `related` boolean so downstream phases can weight/filter without losing the distinction.
|
||||
|
||||
3. **What is the practical max size of a real `.eml` this pipeline will see?**
|
||||
- What we know: sampled real `OriginatingEmail.eml` files ranged 26KB-365KB; well under B2's existing 25MB cap.
|
||||
- What's unclear: whether any historical ticket has a pathological outlier (e.g. an email with many large attachments) that would stress mailparser's default buffering behavior (mailparser's own docs recommend streaming for large messages).
|
||||
- Recommendation: buffer (not stream) for the MVP given observed sizes, but add an explicit size guard (e.g. reject/flag anything over a few MB) before calling `simpleParser` — cheap insurance, not premature optimization.
|
||||
- RESOLVED: Adopted in 16-01-PLAN.md Task 3 — `parseEml` buffers and enforces an exported `MAX_EML_BYTES` guard (a few MB, below B2's 25 MB cap), throwing before `simpleParser` is called on an oversized buffer (DoS mitigation T-16-01).
|
||||
|
||||
## Environment Availability
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue