GUI Attestation & Comment Engine — supersedes #8278#8283
Conversation
aaronlippold
force-pushed
the
feature/attestation-comment-engine
branch
from
727c296 to
b8038a0
Compare
|
|
||
| Build a GUI attestation and comment engine that: | ||
|
|
||
| 1. Makes Heimdall Server the **single source of truth** for compliance review |
There was a problem hiding this comment.
It's not going to be the single source of truth. The downstream GRC tool (eMASS, etc) is the ultimate source of truth. Heimdall is for aggregating and normalizing data and producing reports for downstream and giving people a day-to-day view of their posture. Telling ISSOs that we're letting them do full reviews in Heimdall will require a lot more features than just attestation and commenting -- POAMs, risk adjustments etc. like we were building into HDF Libs as amendments.
We should not try to add all of those things here because at that point we'd be actually doing the bedrock-level schema update. But I also think this PR in general is trying to add tons of rigor to the comment process because it's trying to make it a compliance tool.
There was a problem hiding this comment.
Updated. §8.1 now reads: "Heimdall becomes the day-to-day posture view and review collaboration tool for compliance teams (downstream GRC tools like eMASS remain the source of truth for formal authorization)." §2 and §3.2 already incorporated your feedback about keeping Phase 1 simple — timestamps + identity, no RBAC. Commit 2b7770adb.
| - `updated_by`: current user | ||
| - `control_id`: from the control being annotated | ||
|
|
||
| **Effect on display (immediate, in-memory):** |
There was a problem hiding this comment.
One thing I've been trying to figure out from reading this is just where the user can read prior comments in the Heimdall UI. From context later I guess it's a pullout drawer. Can a user reply to another comment (if no, I am NOT suggesting we do that in this PR, because that was it's own large amount of effort when we did that in Vulcan)? Do other users see displayed who said what and when (I know it's in the data layer but is it in the presentation layer?)
There was a problem hiding this comment.
Addressed in §4.2 — Annotations Tab inside control expansion shows comment history (newest-first) with who/when. No reply threading in Phase 1. Component: AnnotationsTab.vue.
|
|
||
| **1. Failed → Not Applicable / Risk Acceptance** | ||
|
|
||
| ISSOs routinely mark failed controls as Not Applicable (system doesn't have the component) or risk-accepted (finding is real but accepted with mitigation). This is NOT an edge case. The current `attestationCanBeAdded()` restricts attestation to `skipped` (NR) controls only. Phase 1 should at minimum support Failed→NA by extending the status check in the side panel UI (the store can accept any status; the restriction is in the validator). Full risk-acceptance with POA&M fields is Phase 3. |
There was a problem hiding this comment.
Later it says this PR is just supposed to be phase 1.
I'd think that if we do phase 3 -- full on POAM support - then we're pretty much rebuilding a bunch of the new HDF libs right back in the heimdall2 monorepo where we were specifically trying not to put it.
There was a problem hiding this comment.
Agreed. §7.3 now explicitly states Phase 3 POA&M should consume the updated HDF Libs rather than reimplementing in the Heimdall monorepo. Card S1 is a wrapper around HDF Libs, not new domain logic.
| - Available for ALL controls, not just NR | ||
| - Maps to CKL `COMMENTS ::` section in structured comments | ||
|
|
||
| ### 3.1.3 Domain Gaps to Address |
There was a problem hiding this comment.
The updated libraries in HDF Libs allow for doing a lot of these gaps (via CLI). We need to be careful we are not duplicating work just to backport it to the old schema.
There was a problem hiding this comment.
Noted in §3.1.3 — we reference HDF Libs for these gaps. Phase 1 builds only the GUI attestation/comment workflow that HDF Libs doesn't cover (interactive review in the browser).
|
|
||
| ### 3.4 Export Artifacts | ||
|
|
||
| Two distinct export types: |
There was a problem hiding this comment.
we probably want a third -- export the original HDF file from before it was attested
There was a problem hiding this comment.
Added as §3.4.2 'Export Original (un-attested HDF)'. Card 9go.53 created.
| │ ┌────────┬──────────────────────┬───────┬──────┬───────────────────┐ │ | ||
| │ │Status │ Title │ ID │ NIST │ Actions │ │ | ||
| │ ├────────┼──────────────────────┼───────┼──────┼───────────────────┤ │ | ||
| │ │NR ▾ │ htpasswd files... │V-2255 │ AC-3 │ [Attest 📝] │ │ |
There was a problem hiding this comment.
The way this is written makes me think that you are saying you can ONLY do an attestation on an NR control and you can't just comment on it. You should have a comment button on all controls, and also an attestation button on the NR status ones.
There was a problem hiding this comment.
Clarified in §4.2 — comment section renders for ALL controls. Attest section renders only for NR controls. Both are in the Annotations Tab.
| └──────────────────────────────────────────────────────────────────────────┘ | ||
| ``` | ||
|
|
||
| ### 4.2 Review Side Panel |
There was a problem hiding this comment.
I'd prefer to move this into the tabs in the individual controls. So you'd expand out V-123456 and se:
| Test | Details | Code | Annotations (N) |
Just my two cents. That merges this feature into the existing UI in a reasonably intuitive way instead of adding a whole new drawer that is even noted in this doc to not really be used before elsewhere.
From the Annotations tab you can leave a comment or create an attestation in pretty much the same way you have it laid out in the drawer. We might even keep buttons for "Comment" and "attest" in the collapsed row for easy access; when clicked they'd just expand the row and focus the Annotations tab.
There was a problem hiding this comment.
Adopted. §4.2 now uses Annotations Tab inside control expansion: | Test | Details | Code | Annotations (N) |. Side panel/drawer design replaced. §8.3 tradeoff table updated. Card 9go.54 rewritten for this UX.
| All options: save file (no dirty state change — exports the overlay, not the results) | ||
| ``` | ||
|
|
||
| ### 5.5 How Import Works |
There was a problem hiding this comment.
What happens if you have multiple HDF files loaded in the dashboard when you load an attestation? Do we try to apply the attestation to each of them? Just one?
E.g. what if we scan two windows servers and upload an attestation file that's only supposed to be applied to one of them?
There was a problem hiding this comment.
Addressed in §3.5 — file selection modal when multiple HDF files are loaded. User picks which file(s) to apply attestations to, with control match counts displayed.
|
|
||
| **State machine:** | ||
| ``` | ||
| DRAFT ──save──▸ IN REVIEW ──mark final──▸ FINAL |
There was a problem hiding this comment.
This is what I'm talking about when I say that the idea that Heimdall is now the "single source of truth" for compliance data leads to very heavy complexity.
We could implement attestations and comments that have timestamps and identity in them that still do not require us to add a workflow to review and finalize the whole file from edits. So the user still has the basic auditability of "who said what and when" about a control but we don't have to implement a ton of RBAC and workflows to have an ISSO formally bless it.
There was a problem hiding this comment.
Agreed and updated. §8.1 changed from 'single source of truth' to 'day-to-day posture view and review collaboration tool.' §3.2 simplified: Phase 1 has no formal state machine — just timestamps + identity. No RBAC, no approval gates, no finalization workflow.
Addresses 7 review comments on PR #8283: 1. Heimdall is day-to-day posture view, not GRC source of truth (eMASS is) 2. Phase 3 POA&M should wrap HDF Libs, not reimplement — follow-on PR 3. Add "export original un-attested HDF" as third export option 4. Comment button on ALL controls, attest button on NR only (clarified) 5. Annotations tab inside control expansion replaces side panel drawer 6. Attestation import: file-selection modal when multiple HDF files loaded 7. Drop Draft→In Review→Final state machine from Phase 1 — timestamps + identity provide auditability without RBAC/workflow complexity Authored by: Aaron Lippold<lippold@gmail.com>
…f-converters Add setControlDescription (write-side complement to getDescription), sanitizeCklSectionMarkers (prevents CKL section injection on round-trip), and syncChecklistVulnComments (export-time passthrough sync using existing ChecklistVuln types). 21 tests covering array-form, object-form, structured comment preservation, marker escaping, and multi-field edits. Part of epic heimdall2-9go: GUI Attestation & Description Editing Engine. Supersedes the ad-hoc CKL parsing in PR #8278. Authored by: Aaron Lippold<lippold@gmail.com>
…ion warnings - Split type-only re-exports from value re-exports in inspecjs index files (TS1205: export type required with isolatedModules) - Add import type for ContextualizedControl and HDFControlSegment in Vue components (TS1272: decorated signature requires import type) - Configure silenceDeprecations in vue.config.js sassOptions for known Vuetify/sass deprecations (import, global-builtin, slash-div, etc.) Reduces build warnings from 2105 to 25. Authored by: Aaron Lippold<lippold@gmail.com>
…odules)
Change import { TypeName } to import type { TypeName } for all type-only
imports (interfaces, type aliases) across ~50 frontend files. Split mixed
imports that combine types and values into separate statements.
Fixes all webpack "export not found" warnings caused by isolatedModules
stripping type-only re-exports from barrel files. Reduces frontend build
warnings from 25 to 0 (code warnings; asset size advisories remain).
Authored by: Aaron Lippold<lippold@gmail.com>
Replace 31 individual import {version} from '../package.json' statements
with a single HeimdallToolsVersion export from utils/global.ts. Fixes
webpack "named export from default-exporting module" warnings and
eliminates the DRY violation of every mapper reading package.json
independently.
Authored by: Aaron Lippold<lippold@gmail.com>
…s spam Configure silenceDeprecations in vue.config.js sassOptions for Vuetify 2 / Bootstrap 4 deprecations (import, global-builtin, slash-div, legacy-js-api, color-functions, if-function). Extracted to shared const for DRY. Set devServer.client.progress: false to suppress webpack progress-plugin percentage and "Build finished" output during development. Authored by: Aaron Lippold<lippold@gmail.com>
Convert AppConfig to proper singleton pattern (static getInstance() with private constructor). Replace all new AppConfig() calls with AppConfig.getInstance() in config.service.ts, database.ts, groups.service.ts. Replace console.log with NestJS Logger in app_config.ts and token.providers.ts. Remove noisy .env-read logs from seeder (keep only console.warn for missing .env, which is correct for Sequelize CLI context). Remove express:* from DEBUG env var to eliminate express router debug noise. Authored by: Aaron Lippold<lippold@gmail.com>
…ata_store Add updateControlDescription mutation that calls setControlDescription from hdf-converters and patches control.hdf.descriptions cache. Zero CKL/format knowledge in the store. Add dirty file tracking via FileID[] array (Vue 2 reactive pattern matching data_filters.ts selectedEvaluationIds). Getters: hasUnsavedFiles, isFileDirty. Actions: markFileDirty, markFileSaved, clearDirtyFiles. removeFile clears dirty state. reset() clears dirty state. Refactor: allFiles getter simplified. loadedDatabaseIdsForFileId and loadedFileIsForDatabaseIds converted from async Actions to synchronous getters (databaseIdForFile, fileIdForDatabaseId) — they did no async work. Callers updated. loadedDatabaseIds getter uses filter+map instead of forEach+push. Loose equality (==) replaced with strict (===). Authored by: Aaron Lippold<lippold@gmail.com>
Document why the as-cast in contextualExecutionProfiles is required: narrowing SourcedContextualizedEvaluation.contains causes a circular type reference that breaks vitest module loading. The cast is safe because report_intake creates all evaluation profiles as SourcedContextualizedProfile. Authored by: Aaron Lippold<lippold@gmail.com>
Comprehensive Architecture Decision Record covering the attestation and comment engine that replaces PR #8278. Three rounds of multi-agent review (8 independent reviewers) with all findings resolved. Covers: business workflows (attest NR controls, comment any control), review lifecycle (Draft→InReview→Final), UX design (right-side review panel, control row actions, notification bar), data architecture (annotation store with O(1) indexed lookups, immutable original + overlay), export/import (SAF CLI compatible attestation files + Heimdall annotation bundles), CKL round-trip support, Phase 3 approval flow design. Documents existing code inventory, prior art (checklistView branch), and complete Phase 1-3 card breakdown with dependencies. Authored by: Aaron Lippold<lippold@gmail.com>
…nalysis
Round 3 review findings (architecture + implementer + compliance):
- Split card B into B1 (forms+save) and B2 (history+nav+a11y)
- Added A2 card (fileIdForControl public getter)
- ReviewPanel mounts in Results.vue, not ControlTable
- Drawer coordination via ui store flag
- Failed→NA is must-have (ISSO routine workflow)
- Severity Override + Finding Details fields added to §3.1.3
- XSS security note: all user text via {{ }}, never v-html
- Export attestations (H) marked must-have (only Phase 1 persistence)
- CKL round-trip detection (J) deferred to Phase 2
- Changelog (§4.6) deferred to Phase 2
- attestationCanBeAdded is private — use status check instead
- originalStatuses baseline added to FileAnnotationState
checklistView branch analysis:
- Cataloged 8 components: 3 high-reuse (edit form, rules table, severity override)
- Store analysis: SearchEntry<T> with negation is reusable for NR filtering
- Card mapping: ~2 of 12 cards benefit (B1 widgets only — data model is antipattern)
- Lessons: branch failed due to scope creep, broken filtering, CKL-only model
Authored by: Aaron Lippold<lippold@gmail.com>
…ferences Split Phase 3 POA&M card into: - S1: FromHDFToPOAMMapper in hdf-converters (library, reusable by SAF CLI) - S2: Heimdall GUI "Export as POA&M" button (depends on S1) References mitre/ckl2POAM for severity/status mapping logic and mitre/emasser for eMASS API field mappings. Authored by: Aaron Lippold<lippold@gmail.com>
…position rules Which attestation dispositions are POA&M items? Open = yes, Risk Accepted = yes (with AO justification), Not Applicable = probably not (dispositioned). ckl2POAM maps CKL statuses but doesn't account for attestation types. Must research NIST SP 800-37 Rev 2 §3.4 before implementing mapper. Authored by: Aaron Lippold<lippold@gmail.com>
… UI → integration Each layer fully testable before the next starts. Opposite of PR #8278 (UI first) and checklistView (everything at once). Authored by: Aaron Lippold<lippold@gmail.com>
Add AnnotationStore module for attestation records + comment log per file. Object-replacement reactive pattern for Vue 2 (no Vue.set/Map/Set). addAttestation auto-populates updated/control_id per ADR §3.1.1. addCommentWithControl patches live display via cross-module commit. Inline dirty-marking in updateControlDescription (fixes nested mutation). 22 tests including cross-module behavior verification. Authored by: Aaron Lippold<lippold@gmail.com>
Add toAttestationJson, toAttestationYaml (yaml pkg), toHeimdallBundle, toAttestationXlsx (@e965/xlsx with parseXLSXAttestations round-trip format). Wire exportAttestations store action with saveAs per format. XLSX uses sheet 'attestations' matching import column format. 16 serialization tests + 6 store action tests with file-saver mock. Authored by: Aaron Lippold<lippold@gmail.com>
Add prepareEvaluationForCklExport to hdf-converters — pure function that deep-clones evaluation, applies attestations via addAttestationToHDF, and syncs description edits to CKL passthrough vulns via syncChecklistVulnComments. Add buildEditsMapFromProfiles helper for format-agnostic edit extraction. ExportCKLModal simplified to thin caller with .then/.catch/.finally chain. 27 hdf-converters tests + 2 frontend integration tests. Authored by: Aaron Lippold<lippold@gmail.com>
Enhance cleanUpFilename with optional targetExtension param that strips matching extension before appending (case-insensitive). Also strips colons (invalid on Windows). Remove 3 duplicate cleanUpFilename functions from ExportCSVModal, ExportJson, ExportHTMLModal — centralized in export_util.ts. Updated: CKL, CSV, JSON, XCCDF, ASFF, HTML modals. Verified no-change: CAAT (date-based), NIST (sheet name), Splunk (API). 16 unit tests covering extension normalization + edge cases. Authored by: Aaron Lippold<lippold@gmail.com>
… dialog + ADR updates Environment: - Rename PORT → HEIMDALL_BACKEND_PORT across all config (12-factor compliance) - Backend start:dev loads own .env via dotenv (no global injection) - Root start:dev removed dotenv wrapper — each service owns its env - Frontend reads HEIMDALL_FRONTEND_PORT + HEIMDALL_BACKEND_PORT from .env.development.local - Deprecation warning logged when legacy PORT is used - Updated: Dockerfile, docker-compose.yml, docker-compose.okta.yml, .env-example, .env-ci, README.md, RPM packaging docs + CLI UI wiring (9go.26): - Vuetify ConfirmDialog component + Vue.observable confirm service - Router beforeEach dirty-check guard with confirm dialog - DB save icon changed to mdi-database-arrow-up - Removed native beforeunload handler ADR-001 updates: - Added severity/finding details to FileAnnotationState (§5.2.3) - Added cards G2 (HDF export) + G3 (severity pipeline) to §7.1 - Updated dependency graph + sp totals Authored by: Aaron Lippold<lippold@gmail.com>
…ng changes Regenerated style.css and embedded-assets.ts from yarn build in libs/hdf-converters. Required after adding prepareEvaluationForCklExport and buildEditsMapFromProfiles to description-editing.ts. Authored by: Aaron Lippold<lippold@gmail.com>
…9go.49)
Add optional prettyPrint param to toCkl(). Default returns raw JSONIX XML
(31ms vs 69ms on 261-control RHEL8 file). Pretty-print available via
toCkl({prettyPrint: true}) for debugging. Existing tests updated to use
prettyPrint to match expected fixtures. 4 new performance tests verify
speedup, valid XML, and VULN count parity. All exports validated against
DISA CKL Schema V2.5 via xmllint.
Authored by: Aaron Lippold<lippold@gmail.com>
…script to .mts (9go.46) Add "type": "commonjs" to 5 package.json files (hdf-converters, inspecjs, common, password-complexity, backend) to eliminate MODULE_TYPELESS_PACKAGE_JSON warning. Rename convert-to-embedded-strings.ts to .mts for explicit ESM marking since Node 24 treats .mts as always-ESM regardless of package type field. Authored by: Aaron Lippold<lippold@gmail.com>
…9go.43) Apply attestations to all raw-data export formats via clone+addAttestationToHDF: - ExportJson: async populate_files() with clone before JSON.stringify - ExportXCCDF: clone before FromHDFToXCCDFMapper + markFileSaved/catch chain - ExportASFF: clone before FromHdfToAsffMapper + markFileSaved/catch chain - ExportCSV: Attestation Status + Attestation Explanation columns added ADR updated with §5.4.2a (XCCDF/ASFF), §5.4.2b (CSV), §5.4.2c (coverage matrix). 9 integration tests covering clone+apply behavior and CSV column lookup. Authored by: Aaron Lippold<lippold@gmail.com>
Add sourceFormat field to InspecFile type, threaded through loadFile→ loadText→loadExecJson. Native HDF='inspec-json', profiles='inspec-profile', converted files get fingerprint() result (INPUT_TYPES value). DB-loaded files default appropriately; legacy loadExecJson without sourceFormat remains undefined. 6 behavioral tests using real store methods + ubuntu_profile.json fixture. Authored by: Aaron Lippold<lippold@gmail.com>
…ecisions Disable rules with known dangerous auto-fix behavior: - perfectionist/sort-modules: breaks circular type declarations - unicorn/prefer-spread: converts _.concat() to [..._] (spreading lodash) - e18e/prefer-spread-syntax: same as above - consistent-type-definitions: converts interface→type, breaks circular refs - import-x/namespace: false positives on CJS lodash - n/no-missing-import, n/no-unpublished-import: can't resolve TS/monorepo - n/no-unsupported-features/es-syntax: wrong Node version default Reconfigure: - consistent-type-imports: inline-type-imports (prevents unsafe import splitting) - restrict-template-expressions: allow number + boolean + nullish - unicorn/filename-case: allow kebab + snake + camel + pascal Add docs/development/eslint-config-decisions.md with rationale, upstream issue links, and auto-fix safety guide for each rule. Authored by: Aaron Lippold<lippold@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| }); | ||
| const fullUrl = `${host_url.replace(/\/$/v, '')}/rest/currentUser`; | ||
| const result = await axios.get(fullUrl, { headers: { 'x-apikey': `accesskey=${accesskey}; secretkey=${secretkey}` } }); |
| } | ||
|
|
||
| /** Sets the local storage value to the given value, stringified */ | ||
| set(val: T): void { | ||
| const nv = JSON.stringify(val); | ||
| window.localStorage.setItem(this.storageKey, nv); | ||
| globalThis.localStorage.setItem(this.storageKey, nv); |
| @@ -134,58 +136,58 @@ | |||
| if (guessOptions.filename.toLowerCase().endsWith('.nessus')) { | |||
| return INPUT_TYPES.NESSUS; | |||
| } else if ( | |||
| guessOptions.data.match(/xmlns.*http.*\/xccdf/) || // Keys matching (hopefully) all xccdf formats | |||
| guessOptions.filename.toLowerCase().indexOf('xccdf') !== -1 | |||
| XCCDF_XMLNS_RE.test(guessOptions.data) | |||
| ) { | ||
| return INPUT_TYPES.XCCDF; | ||
| } else if ( | ||
| guessOptions.data.match(/<netsparker-.*generated.*>/) || | ||
| guessOptions.data.match(/<invicti-.*generated.*>/) | ||
| NETSPARKER_RE.test(guessOptions.data) |
Tests for 3 reverse mappers (CAAT, HTML, XCCDF) import embedded-assets.ts which is generated by tailwindcss + convert-to- embedded-strings.mts. The file is gitignored (841KB, deterministic from committed template files). Previously, tests silently failed on fresh clones or after rebases when the file was missing. Fix: vitest globalSetup checks for the file and generates it if absent (vitest's documented pre-test lifecycle hook). Also adds pretest/pretest:ci npm hooks as belt-and-suspenders for yarn test and yarn run test:ci. Extracts the generation command into a DRY generate:assets script used by prebuild, pretest, and globalSetup. Authored by: Aaron Lippold<lippold@gmail.com>
… of inline literal
AC verify caught: 3 mappers imported the helper but never called it,
using hand-written {sourceFormat, toolVersion} inline instead. Dead
imports caused lint errors. Fixed by converting to actual helper calls:
- snyk: nested-key → single transformer wrapping createHeimdallPassthrough
- ionchannel: nested-key → single transformer with _.get for metadata path
- veracode: keep nested-key (complex MappedTransform), call helper for .heimdall
Authored by: Aaron Lippold<lippold@gmail.com>
…pressions, style Fix lint errors on Session 5 batch mappers that had DEFAULT_PROFILE_FIELDS wired but were not individually lint-cleaned: - withRaw → shouldIncludeRaw (unicorn/consistent-boolean-name) in dbprotect, nikto, sarif, dependency-track, scoutsuite, jfrog-xray, gosec, twistlock, veracode, snyk, trufflehog - String() wraps for _.get() in template literals (restrict-template-expressions) in gosec, jfrog-xray, dependency-track, veracode, snyk, trufflehog - Remove useless template literals (unicorn/no-useless-template-literals) in gosec, twistlock, veracode, trufflehog - Extract nested call in scoutsuite constructor (unicorn/max-nested-calls) - Remove useless else in snyk (unicorn/no-useless-else) - Prefer ternary in twistlock title (unicorn/prefer-ternary) All 11 files now pass eslint --max-warnings 0. 212 tests pass. Authored by: Aaron Lippold<lippold@gmail.com>
…ct-injection) Convert MappingData from bracket-notation Record access to Map.get() per eslint-config-decisions.md documented pattern. Eliminates detect-object-injection warning without suppression. Authored by: Aaron Lippold<lippold@gmail.com>
|
|
||
| export function setup(): void { | ||
| if (!existsSync(EMBEDDED_ASSETS)) { | ||
| execSync('yarn generate:assets', { |
…dFixture DRY
- ESLint auto-fix: comma-dangle, object-curly-spacing, sort-objects,
object-curly-newline, arrow-parens, spaced-comment, indent
- Normalize encoding identifier 'utf-8' to 'utf8' across all test files
(unicorn/text-encoding-identifier-case)
- Add loadFixture() helper to test/utils.ts — DRYs the repeated
JSON.parse(fs.readFileSync(path, {encoding: 'utf8'})) pattern
- Replace nested fixture loading with loadFixture() in 22 test files,
fixing unicorn/max-nested-calls (120 → 13 remaining)
- Add normalizeASFF() compose helper in asff_reverse_mapper.spec.ts
to flatten omitASFFVersions(omitASFFTimes(omitASFFTitle(...))) nesting
- Fix ZAP test URLs: restore http:// fixture data URLs that auto-fix
incorrectly changed to https:// (with targeted eslint-disable)
Authored by: Aaron Lippold<lippold@gmail.com>
…rals, style - Remove useless else after return/throw/continue (unicorn/no-useless-else) — 30 auto-fixed, 11 remaining need manual review - Remove useless template literal expressions (unicorn/no-useless-template-literals) — replace backtick wrapping with direct value or String() - ESLint auto-fix: comma-dangle, object-curly-spacing, sort-objects, perfectionist/sort-imports, consistent-type-imports Total lint reduction this session: 2815 → 311 (89% reduction) Remaining 311 are structural issues requiring per-case review or blocked on 4qm.44.5 (parseHtml async init refactor). Authored by: Aaron Lippold<lippold@gmail.com>
- Rename withRaw → shouldIncludeRaw in 10 remaining mappers (unicorn/consistent-boolean-name) - Rename other boolean params: verifySSLCertificates → shouldVerify, containsChecklist → hasChecklist, returnWorkBook → shouldReturnWorkBook, collapseResults → shouldCollapseResults, addPrefix → shouldAddPrefix, collapse → shouldCollapse, expired → isExpired, condition → shouldInclude, showAsPercentage → shouldShowAsPercentage, dateOverride → hasDateOverride, isoTime → isIsoTime, sizeCheck → shouldCheckSizeOnly - Wrap _.get() return values with String() in template literals (restrict-template-expressions) across 8 files - Remove redundant `as string` cast after _.isString() narrowing Authored by: Aaron Lippold<lippold@gmail.com>
- Remove all remaining useless else after return/throw (11 files) - Convert .then()/.catch() chains to try/await/catch in sonarqube, ionchannel, and splunk reverse mappers (14 instances) - Add String() wraps for restrict-template-expressions in asff, conveyor, fortify, zap, and global utils Authored by: Aaron Lippold<lippold@gmail.com>
- Replace Number.parseFloat() with Number() (unicorn/prefer-number-coercion) in fortify, cci-nist, splunk, compliance utils - Replace `key in obj` with Object.hasOwn(obj, key) for dynamic property existence checks (unicorn/no-computed-property-existence-check) in asff, aws-config, nikto-nist, global utils Authored by: Aaron Lippold<lippold@gmail.com>
| } | ||
| if ( | ||
| NETSPARKER_RE.test(guessOptions.data) | ||
| || INVICTI_RE.test(guessOptions.data) |
…use, early return
- Remove unused fs imports after loadFixture migration (3 test files)
- Replace JSON.parse(JSON.stringify()) with structuredClone (description-editing tests)
- Add { cause: error } to re-thrown errors (preserve-caught-error, sonarqube)
- Remove unnecessary await on non-Promise toHTML() (html reverse mapper tests)
- Use logical assignment ||= (prisma-mapper)
- Use this in static method (caat reverse mapper)
- Rename MetaData → Metadata (consistent-compound-words, splunk-mapper)
- Early return guard (checklist-metadata-utils)
- Replace duplicate loadJsonFile with shared loadFixture (checklist_performance)
Authored by: Aaron Lippold<lippold@gmail.com>
…ause
- Extract variables to reduce max-nested-calls in transformers,
xccdf-results, and zap mappers
- Move regex literals to module scope (e18e/prefer-static-regex)
in test utils, attestation tests, checklist perf, regen fixtures
- Rename MetaData → Metadata in splunk-mapper (consistent-compound-words)
- Add { cause: error } to sonarqube error re-throws
- Use logical ||= assignment in prisma-mapper
- Use this in static method for CAAT mapper
- Early return guard in checklist-metadata-utils
- Remove unused fs imports, use loadFixture, structuredClone
Authored by: Aaron Lippold<lippold@gmail.com>
…lint - Deduplicate describe block names in anchore-grype, trufflehog, twistlock, zap, and attestation test files (vitest/no-identical-title) - Extract regex to module scope in test utils, attestation tests, checklist perf, regen fixtures (e18e/prefer-static-regex) - Extract nested calls in transformers, xccdf-results, zap mappers - Use structuredClone, remove unused fs imports, logical ||= assignment, early return, this in static, error cause preservation Authored by: Aaron Lippold<lippold@gmail.com>
| | **CKL** | Checklist — DISA's legacy XML format used by STIG Viewer 2 for recording STIG review results. Uses statuses: Not A Finding, Open (Finding), Not Applicable, Not Reviewed. | | ||
| | **CKLB** | Checklist JSON — DISA's newer JSON format introduced by STIG Viewer 3 (SV3). File extension `.cklb`. Uses statuses: `not_a_finding`, `open`, `not_applicable`, `not_reviewed`. | | ||
| | **SV3** | STIG Viewer 3 — DISA's tool for reviewing STIGs and producing checklists | | ||
| | **HDF / OHDF** | Heimdall Data Format / Outcome-based HDF — JSON output format from InSpec and other scanners | |
There was a problem hiding this comment.
the O in OHDF does not stand for 'outcome-based'
There was a problem hiding this comment.
Fixed. Glossary now reads: HDF / OHDF — Heimdall Data Format / OASIS HDF. Updated in both ADR-001 and ADR-003.
| ### 1.3 The CKLB Schema (SV3 v1.0) | ||
|
|
||
| The schema defines a JSON structure with: | ||
| - **Top level:** `title` (filename, **required**), `id` (UUID, **required**), `cklb_version` ("1.0"), `target_data`, `stigs[]` |
There was a problem hiding this comment.
missing three fields that are used by SV internally but we've always said that we will bring through all data regardless
There was a problem hiding this comment.
§1.3 expanded with additional fields. Added note that generated types from the CKLB JSON schema (§6.5, card 1af.5) are authoritative — all schema fields must be brought through, no silent drops. §3.4 updated with expanded passthrough field lists.
|
|
||
| The schema defines a JSON structure with: | ||
| - **Top level:** `title` (filename, **required**), `id` (UUID, **required**), `cklb_version` ("1.0"), `target_data`, `stigs[]` | ||
| - **Per STIG:** `stig_name`, `display_name`, `stig_id`, `release_info`, `uuid`, `size`, `rules[]` |
There was a problem hiding this comment.
§1.3 and §3.4 expanded. The generated CKLB types (card 1af.5) will be the definitive field list — any field in the schema that we missed will be caught at type generation time.
|
|
||
| Build a CKLB converter (forward + reverse) by extracting the shared checklist domain logic from the existing CKL mapper into a common module, then adding a thin CKLB serialization layer. The extraction and the new converter are one body of work — not "DRY first, then build." | ||
|
|
||
| ### 2.1 Architecture: Shared Intermediate (Option B) |
There was a problem hiding this comment.
Added. §2.2 now presents all four options: A (standalone — rejected, DRY violation), B (shared intermediate — rejected, unnecessary abstraction), C (CKLB→CKL→HDF — rejected, lossy), D (CKLB as canonical — chosen).
| **Option A (fully standalone CKLB mapper):** Duplicates ~400 lines of validated severity/status/CCI/comment logic. Guarantees the two mappers drift over time. Violates DRY. | ||
|
|
||
| **Option C (CKLB→CKL→HDF, convert internally):** Forces CKLB through XML's lossy `STIG_DATA` key-value representation. Discards CKLB-native fields. Strictly more lossy and more fragile. | ||
|
|
There was a problem hiding this comment.
no option d presented of ckl -> cklb -> hdf which is what i'm proposing.
There was a problem hiding this comment.
If someone loads an old ohdf derived from an old ckl, will the process be backward compatible?
There was a problem hiding this comment.
Accepted as Option D — now the chosen architecture. §2.1 revised: CKL XML → CKLB object → HDF. CKLB is the canonical intermediate. CKL becomes a thin XML adapter. ADR-003 fully rewritten for this approach. Commit 2b7770adb.
There was a problem hiding this comment.
Yes — fully backward compatible. Added §2.5 to ADR-003 addressing this directly. Option D changes the internal pipeline (XML → CKLB object → HDF instead of XML → JSONIX intermediate → HDF) but the HDF output is byte-for-byte identical. Existing CKL fixture regression tests enforce this. Existing HDF files with CKL passthrough data continue working — passthrough.checklist key and shape are preserved unchanged.
| ### 5.4 The JSONIX Type Coupling (Critical Extraction Risk) | ||
|
|
||
| The shared intermediate types are currently **defined in terms of JSONIX types** from `checklistJsonix.ts`: | ||
| - `ChecklistVuln = Omit<Vuln, 'status' | 'stigdata'> & {...}` — extends the JSONIX `Vuln` type |
There was a problem hiding this comment.
seems to have a slight misunderstanding of where these types are coming from - jsonix just derives these types from the og ckl type
There was a problem hiding this comment.
Corrected. §5.3 clarified — JSONIX derives its types from the original CKL XSD. The types are XSD-derived, not invented. Under Option D, CKLB generated types replace the JSONIX-derived types as the canonical intermediate.
|
|
||
| **Fix:** Relocate the domain enums (`Assettype`, `Role`, `Techarea`, `Severityoverride`), the base `Vuln` shape, the `Asset` type, and their transitive dependencies (`Status`, `StigdatumElement`, `Vulnattribute`) into `checklist-common/types.ts`. Have `checklistJsonix.ts` re-import/alias them. Constraint: `checklist-common/types.ts` must import NOTHING from `ckl-mapper/` — enforce this with an ESLint `no-restricted-imports` rule or a build test. | ||
|
|
||
| ### 5.4a The Instance-Method Problem (Critical Extraction Challenge) |
There was a problem hiding this comment.
this seems like a massive headache during the refactor process
There was a problem hiding this comment.
Eliminated by Option D. No shared base class extraction, no JSONIX type relocation, no ChecklistConverterBase. Domain functions move directly to cklb-mapper as plain functions. Cards 1af.1, 1af.2, 1af.3 closed as obsolete.
|
|
||
| - **`DEFAULT_PROFILE_FIELDS`** (ADR-002): Wire into `ChecklistMapper` (shared intermediate→HDF) and `CklbMapper`. The current `ChecklistMapper` hand-declares `attributes/groups/sha256/status/supports` — it is NOT in ADR-002's list of wired mappers and should be. | ||
| - **`createHeimdallPassthrough('cklb', ...)`** (ADR-002 / 9go.52): Use the standard passthrough helper. | ||
| - **Regen tool** (ADR-002): Add CKLB fixtures to the `regenerate-fixtures.mts` registry. Use the same `omitVersions + toEqual` fixture-comparison pattern as all other mappers. |
There was a problem hiding this comment.
Explained in §6.5 with link to ADR-002. The fixture regeneration tool lives at libs/hdf-converters/scripts/regenerate-fixtures.mts — supports --validate, --dry-run, --revert, and size guards for safe mapper refactoring. CKLB fixtures will be added to its registry.
|
|
||
| --- | ||
|
|
||
| ## 8. Edge Cases |
There was a problem hiding this comment.
Thanks — kept and carried forward into the revised ADR.
|
|
||
| ### 1.4 Key Insight | ||
|
|
||
| **CKLB is structurally the existing CKL intermediate object with renamed fields and no XML layer.** Every piece of format-agnostic logic the CKL mapper already has — status mapping, severity↔impact, CCI↔NIST, finding-details parsing, structured-comment parsing, multi-STIG parent-profile synthesis — applies unchanged to CKLB. The difference is serialization: CKL needs JSONIX for XML; CKLB is `JSON.parse`/`JSON.stringify`. |
There was a problem hiding this comment.
CKLB is structurally the existing CKL intermediate object with renamed fields and no XML layer.
this is what i've been saying this entire time. so instead of having to continue dealing with the intermediate object let's replace it outright with just cklb instead
There was a problem hiding this comment.
Agreed and adopted as the core architecture decision. Your insight is cited directly in §1.4. Option D (CKLB as canonical intermediate) is now the chosen approach. CKL becomes a thin XML adapter. The entire shared extraction (Option B) is eliminated. Commit 2b7770adb.
|
|
|
||
| **Effect on display (immediate, in-memory):** | ||
| - Comment entry added to annotation store comment log (audit trail — append-only, accumulates across review cycles) | ||
| - `control.data.descriptions.comments` and `control.hdf.descriptions.comments` patched for live display (display optimization, see §5.1) |
There was a problem hiding this comment.
Just checking - if the source hdf already has a passthrough with checklist data, are we maintaining the comments fields here as well? iirc, it is used to accurately reconstitute all of the checklist upon export.
There was a problem hiding this comment.
Ah! I think I see it in 3.3. Correct?
| ``` | ||
|
|
||
| **On CKL import, Heimdall must:** | ||
|
|
There was a problem hiding this comment.
- Is the import treated as an entirely new evaluation in Heimdall, or somehow associated with the earlier one in Heimdall?
- If the later, what if someone commented on the earlier one in Heimdall after export. How are the comments from the externally modified one reconciled?
| ``` | ||
| User imports a CKL file that was previously exported and edited in STIG Viewer: | ||
| 1. Normal CKL import runs (checklist-mapper.ts) → produces HDF | ||
| 2. After import, if the file was previously loaded in Heimdall: |
There was a problem hiding this comment.
Similar comment from above - how does it know it was previously loaded?
| - Does NOT change status or compliance score | ||
|
|
||
| **Effect on export:** | ||
| - `descriptions.comments` already contains the latest comment text from live mutation |
There was a problem hiding this comment.
Please clarify, which are you proposing? Are the comments field in the OHDF be an append or replace with latest
| ] | ||
| ``` | ||
|
|
||
| **Comment file format** (Heimdall extension — separate file, NOT mixed with attestations): |
There was a problem hiding this comment.
Not that I'm requesting it, but are you proposing some sort of export for comments (comment log?)
|
This pull request has a conflict. Could you fix it @aaronlippold? |
Summary
GUI attestation and comment engine for Heimdall — enabling compliance reviewers to attest Not Reviewed controls, add comments to any control, and export/import attestation files compatible with SAF CLI.
Supersedes PR #8278 (codex/editable-comments) which was architecturally wrong (buried editing in Details tab, triple-write mutation, CKL format logic in Vuex store, zero tests).
Builds on prior art from
checklistViewbranch — adapts the proven form widgets (ChecklistRuleEdit.vue severity override + status dropdown, ChecklistSeverityOverride.vue justification-gate pattern) while discarding the CKL-only data model and direct-mutation antipattern. The checklistView branch was never merged due to scope creep (54 files, 3,881 lines), broken filtering, and CKL-only typing. This PR takes the good UX patterns and rebuilds with a format-agnostic architecture that works for InSpec, Nessus, ZAP, Burp, and every other HDF format — not just STIGs.Design document:
docs/adr-001-attestation-comment-engine.md— comprehensive ADR covering business workflows, UX design, data architecture, code inventory, and 3-phase implementation plan. Reviewed by 11 independent review agents across 3 rounds.What this replaces
The CKL-email-STIG-Viewer loop where reviewers email CKL files back and forth, lose track of versions, and have no audit trail. Heimdall becomes the single source of truth with:
saf attest applyCompleted so far
Foundation (committed)
setControlDescription+syncChecklistVulnComments+sanitizeCklSectionMarkersin hdf-converters (21 tests)updateControlDescriptionmutation +dirtyFileIdstracking in data_store (6 tests)beforeunloadguard + SidebarFileList dirty indicator + ExportJson markFileSaved wiringCodebase cleanup (committed)
isolatedModulestype export errors across inspecjs + 59 frontend filesHeimdallToolsVersionin shared helper (DRY: 31 files → 1 import)console.log→ NestJS Logger auditDesign (committed)
Remaining work
Phase 1 — Core Attest/Comment Engine (~42 sp, ~2 hrs Claude-pace)
Implementation order: store → export/import → UI → integration. Each layer fully testable before the next starts. Store works without UI. Export works without side panel. Side panel consumes what's already tested.
12 cards covering: annotation Vuex store, review side panel (forms + history + a11y), control row actions, notification bar, NR status card action, dirty tracking wiring, CKL export sync, attestation file export/import, integration tests.
Phase 2 — Server Persistence + Collaboration (~38 sp, ~2 hrs)
7 cards: DB schema, API endpoints, save-as-revision, Draft→InReview→Final state machine, changelog sidebar, revision diff, CKL round-trip detection.
Phase 3 — Enterprise Workflows (~39 sp, ~2.25 hrs)
6 cards: approval workflows, POA&M/eMASS export, bulk attestation, expiration dashboard, shared edit notification, false positive/risk acceptance attestation.
Key architectural decisions
saf attest applytoolchainTest plan
npx vue-cli-service buildwith 0 code warnings