[TTAHUB-5243] Add security findings register ADR and operating specification

[kryswisnaskas]

Description of change

Adds ADR 0027 proposing to maintain a repository owned security findings register under security/, and security/README.md defining the full operating specification. The spec covers the control inventory (Semgrep, Yarn Audit, ZAP, ClamAV), the common finding schema, disposition model, approval authority requirements, CI enforcement policy, and SCA specific pending observations escalation rules.
.

How to test

Review docs/adr/0027-security-findings-register.md and security/README.md and verify they meet the TTAHUB-5243 acceptance criteria.

Jira Issue(s)

• https://jira.acf.gov/browse/TTAHUB-5243

Checklists

Every PR

• Linked Jira issue
• JIRA issue status updated
• [n/a] Code is meaningfully tested — documentation only, no executable code
• [n/a] Meets accessibility standards (WCAG 2.1 Levels A, AA) — no UI changes
• [n/a] API Documentation updated — no API changes
• [n/a] Boundary diagram updated — no new external integrations
• [n/a] Logical Data Model updated — no schema changes
• Architectural Decision Records written for major infrastructure decisions — ADR 0027 added
• [n/a] UI review complete — no UI changes
• QA review complete

Before merge to main

• OHS demo complete
• Ready to create production PR

Production Deploy

• PR created as Draft
• Staging smoke test completed
• PR transitioned to Open (this ready_for_review transition triggers the Slack/Jira automation)
• Reviewer added after the PR is Open (elainaparrish is the authorized approver under normal circumstances)
  • Sequence: Draft PR → Smoke test → Open PR (automation runs) → Add reviewer
  • Confirm that the Slack notification was sent after the PR was opened
  • Confirm that linked Jira ticket(s) transitioned as expected; if not, review the GitHub Actions workflow logs

After merge/deploy

• Update JIRA ticket status

[Copilot]

Pull request overview

This PR adds design documentation for a repository-owned Security Findings Register under security/, establishing governance and an operating specification ahead of implementing tooling and CI enforcement.

Changes:

• Added ADR 0027 documenting the decision to maintain a repository-owned security findings register and its governance model.
• Added security/README.md specifying the proposed register schema, identity rules, approval requirements, and CI enforcement policies (including SCA-specific escalation behavior).

Impact assessment: Benefits medium (clear, centralized audit evidence model and enforcement rules); risks low (documentation-only), with one clarification needed to avoid ambiguous CI behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
security/README.md	Operating specification for the proposed security findings register (schema, identity, approval, and CI enforcement policy).
docs/adr/0027-security-findings-register.md	ADR establishing the decision and summarizing the register model and enforcement approach.

[thewatermethod]

Consider logging tickets or addressing the following (cherry-picked from UI review):

---

git show arg is interpolated from user input without an allow-list

File: tools/security-findings.js:1112-1117

contents = execFileSync('git', ['show', `${pendingRef}:${gitPath}`], {
  cwd,
  encoding: 'utf8',
  stdio: ['ignore', 'pipe', 'pipe'],
});

Problem: pendingRef originates from the --previous-pending-ref CLI flag (tools/security-findings.js:1499, 1605, 1628). Because execFileSync does not invoke a shell, there is no shell-injection risk. However, the value is concatenated directly into a git refspec without validation. A malicious or careless caller can pass things like HEAD -- some/other/file or rely on Git's flexible revision parser to read arbitrary tracked files (e.g., HEAD:.env.production if that file ever exists). In the current CI use this is harmless because the value is a hard-coded literal HEAD~1 in package.json, but the tool is intended to be re-used and the input is unvalidated.

Impact: Low — local-only tool, no shell expansion, no current untrusted input path. Surface to harden because the project does in fact run secrets in CI runners.

Suggested Fix: Validate the ref format and reject anything containing whitespace, :, or .. patterns that would re-target the path component:

const GIT_REF_PATTERN = /^[A-Za-z0-9_./~^@-]+$/;

function loadPendingObservationsStoreFromGitRef({ pendingPath, pendingRef, cwd = process.cwd() } = {}) {
  if (!GIT_REF_PATTERN.test(pendingRef)) {
    throw new Error(`Invalid --previous-pending-ref ${JSON.stringify(pendingRef)}`);
  }
  const gitPath = toGitPath(pendingPath);
  // …
}

Additional Context: Project convention from AGENTS.md "Traps to Avoid → SQL injection in filters" calls for "independently validate expected types … before using them in queries"; the same hygiene applies to ref values passed to git show.

---

arraysMatch is order-sensitive and serializes whole arrays

File: tools/security-findings.js:685-687

function arraysMatch(left = [], right = []) {
  return JSON.stringify(left) === JSON.stringify(right);
}

Problem: Used by validateSastBaselineEvidence to detect drift between security/sast/baseline.json and security/sast/scan-config.json for fields like configs, include, exclude, blockingSeverities. If the two arrays contain the same logical set of values in a different order — for example because Semgrep reordered config sources, or someone hand-edits one file — the validator will warn "differs from scan config" even though nothing meaningful changed. Conversely, deeply nested objects-in-arrays that match shallowly but have undefined-vs-missing keys can mismatch under JSON.stringify.

Impact: Low — drives warnings, not errors. But noisy warnings train people to ignore them, undermining the value of the drift check.

Suggested Fix: For these specific config fields, compare as sorted sets where order is not semantically meaningful, and keep deep equality only for fields where order matters:

function arraysMatchUnordered(left = [], right = []) {
  if (left.length !== right.length) return false;
  const sortedLeft = [...left].map(String).sort();
  const sortedRight = [...right].map(String).sort();
  return sortedLeft.every((value, idx) => value === sortedRight[idx]);
}

const driftChecks = [
  ['version', baseline.scanner.version, scanConfig.semgrepVersion, 'scalar'],
  ['configs', baseline.scanner.configs, scanConfig.configs, 'unordered'],
  ['include', baseline.scanner.include, scanConfig.include, 'unordered'],
  ['exclude', baseline.scanner.exclude, scanConfig.exclude, 'unordered'],
  // …
];

Additional Context: If order is in fact semantically significant for configs (Semgrep applies them in order), keep arraysMatch for that one field and use unordered comparison for the others.

---

tools/security-findings.js is becoming a god-file

File: tools/security-findings.js (1674 lines)

Problem: A single module now contains: baseline construction, register seeding, register validation, deadline enforcement, pending-observation store management, git-ref I/O, severity mapping, business-day math, ISO date validation, CLI parsing, and pretty-printing. Most functions are small and well-named, but the file is hard to scan and the module.exports selectively re-exports 16 helpers, making it unclear what is "public API" vs internal.

Impact: Future contributors will struggle to find logic; PR diffs touching unrelated concerns will sit on top of each other; testing requires importing from a large surface.

Suggested Fix: Split into a directory of focused modules under tools/security-findings/:

tools/security-findings/
  index.js                  // CLI entrypoint, parseCliArguments, main, printHelp
  paths.js                  // DEFAULT_* constants, resolveProjectPath, toGitPath
  dates.js                  // operationalDate, parseOperationalDate, businessDaysSince, calendarDaysBetween, validateIsoDate
  scan-types.js             // loadScanTypes, validateScanTypes, mapSeverity
  ids.js                    // buildSastFindingId, buildScaFindingId, sanitizeIdComponent
  sca-baselines.js          // collectAuditAdvisoriesFromFile, createScaBaseline(s)
  register.js               // build*RegisterEntries, createRegister, collectMigrationGaps, validateRegister
  pending-observations.js   // load*, validatePendingObservations, updatePendingObservations, buildPendingObservationEntry, slaThresholdForSeverity

This also lets you write per-module tests, which are easier to target than the current single 1.4k-line test file. Not blocking; a follow-up ticket is fine.

---

Default ticket = 'TTAHUB-5243' in createRegister will mis-attribute future re-seeds

File: tools/security-findings.js:551 (and 639)

function buildScaRegisterEntries({
  // …
  owner = 'TTA Hub AppDev',
  ticket = 'TTAHUB-5243',
  // …

Problem: TTAHUB-5243 is the implementation ticket for the register itself. Using it as the default owner ticket for SCA entries is appropriate for the one-time initial migration seed. However, if someone runs yarn security:register:seed again later (e.g., after a fresh build-sca-baselines), all newly-seeded entries will be tagged with the implementation ticket — masking the real outstanding work and burning audit trust.

Impact: Low — the JIRA ticket can be corrected by hand, and validation does not enforce ticket uniqueness. But the placeholder is sticky and easy to forget.

Suggested Fix: Either (a) require --ticket to be passed explicitly for seed-register and fail when it is not, or (b) leave ticket null by default so the resulting migration.gaps include missing-ticket, which forces explicit assignment. Option (b) is minimally invasive:

function buildScaRegisterEntries({
  // …
  owner = 'TTA Hub AppDev',
  ticket = null,
  closureTarget = null,
  justification = 'Migrated from the legacy Yarn Audit active exception set. ' +
    'Technical assessment and approval evidence still need to be added.',
  // …

Then package.json (or CI) can pass --ticket TTAHUB-5243 only for the genuine initial migration.

---

Nested ternary in enforceDispositionDeadlines is harder to read than necessary

File: tools/security-findings.js:850-855

const dueField =
  entry.disposition === 'deferred'
    ? 'closureTarget'
    : entry.disposition === 'accepted'
      ? 'reviewBy'
      : null;

Problem: Minor readability. The conditional could be expressed as a lookup, which also makes it trivial to extend if new dispositions are added later.

Suggested Fix:

const DUE_FIELD_BY_DISPOSITION = {
  deferred: 'closureTarget',
  accepted: 'reviewBy',
};

// …
const dueField = DUE_FIELD_BY_DISPOSITION[entry.disposition] || null;

---

parseArgs will throw on unknown options with an opaque error

File: tools/security-findings.js:1483-1513

Problem: node:util parseArgs throws on unknown flags. The CLI catches errors at line 1647 and prints error.message, but the message is unhelpful (e.g., Unknown option '--prevous-pending-ref') and there is no --help flag wired in — help is only available as a positional. A user with a typo gets a wall of stack with no remediation.

Impact: UX of the new CLI.

Suggested Fix: Add help: { type: 'boolean', short: 'h' } to the options, and in main short-circuit to printHelp() when values.help is set. Also wrap the parseArgs call so unknown-option errors print printHelp() after the error message:

function parseCliArguments(args = process.argv.slice(2)) {
  try {
    return parseArgsImpl(args);
  } catch (error) {
    console.error(error.message);
    printHelp();
    process.exit(2);
  }
}

---

Validation messages embed the same magic strings the tests pattern-match on

File: tools/security-findings.test.js:386-391, 505-509, 722, 865-866, etc.

Problem: Tests assert against substrings of human-readable error strings ("missing approval evidence", "due in 8 calendar days", "missing closure target"). Reasonable for now, but it couples message wording to test correctness. If a future tweak rewords a warning, tests will break for cosmetic reasons.

Impact: Low — tests are easy to update; nobody else parses these strings.

Suggested Fix (optional, for a future refactor): Have collectMigrationGaps return structured codes (e.g., MISSING_APPROVAL_EVIDENCE, MISSING_CLOSURE_TARGET) and translate to messages at the print boundary. Tests then assert on codes. Same idea for enforceDispositionDeadlines warning/error categories.

---

paths in SCA baseline can leak internal dependency graph details

File: tools/security-findings.js:213-222 and SCA baseline JSON output

Problem: SCA baselines record paths like email-templates>@ladjs/i18n>qs (see security/dependencies/backend-baseline.json and tests at line 268). These reveal the project's transitive dependency graph in version control. The information is already implicit in yarn.lock, so this is not a leak per se, but the data is also low-value for the register — it doesn't appear in the schema table in security/README.md:100-118 and is not used by the validator.

Impact: Cosmetic; modest churn in baseline diffs whenever transitive paths change without the advisory itself changing.

Suggested Fix (optional): Either document paths as a deliberate source-field for triage in the schema table, or drop it from the baseline and rely on yarn.lock for path provenance. If kept, consider keeping only the leaf module rather than the full ancestry chain.

[AdamAdHocTeam]

Three things to double check via AI:

1. Tooling and tests are not wired into CI (severity: high)
   Backend yarn test runs jest build/server/src --runInBand (package.json:74). The new test file at security-findings.test.js lives outside src and is not in the TypeScript build output, so its 1,384 lines never execute in yarn test or yarn test:ci. None of security:validate, security:validate:live, security:validate:strict, or security:sca:pending are referenced in config.yml — only the pre-existing sast_scan job. Net effect: the central guarantee in the README ("security:validate fails when a current finding is absent from the register…") is not enforced for any pull request. The register can drift, the tool can regress, and CI will not catch either. Wire the validator and its test file into CircleCI before merging, or this PR delivers documentation without enforcement.

2. firstSeen cannot be trusted without a working --previous-pending-ref (severity: high)
   In security-findings.js:1281-1298, validatePendingObservations chooses the "trusted prior" entry as:

When no prior store is supplied, the comparison uses actual itself, so expected.firstSeen === actual.firstSeen is tautological — a contributor can edit pending-observations.json to reset firstSeen and the validator will pass. Combined with issue #3 below, this defeats the SLA timer the ADR introduces. security:validate:live is the only path that supplies --previous-pending-ref HEAD~1, and even that is not invoked anywhere in CI today (see issue #1). The README documents this as "bootstrap" behavior, but the practical consequence is that firstSeen is mutable by anyone with commit access. At minimum, scheduled CI must run security:validate:live, and the validator should refuse to "bootstrap" when running in CI (e.g., require a non-null trusted store unless an explicit --bootstrap flag is set).

3. loadPendingObservationsStoreFromGitRef swallows real errors as "no prior snapshot" (severity: medium-high)
   In security-findings.js:1115-1133, any git show failure whose stderr contains one of several substrings ('exists on disk, but not in', 'invalid object name', 'unknown revision or path not in the working tree', 'bad revision', or the very loose pair 'path' + 'does not exist') is treated as "the file did not exist in that ref" and returns null. This silently falls back to the untrusted bootstrap path described in issue #2.

The error-string detection is brittle in two ways:

It depends on English git messages and the exact phrasing of the installed git version. Localized environments (e.g., LC_ALL=de_DE) will not match and will throw — or worse, will match by accident.
More importantly, CircleCI's default checkout produces a shallow clone. HEAD1 is frequently unavailable; git show HEAD1:... then fails with messages like fatal: bad revision 'HEAD~1', which matches 'bad revision' and gets silently downgraded. The scheduled SCA workflow would then accept whatever firstSeen is committed today, with no warning — eliminating the SLA integrity guarantee the README promises.
Recommend: differentiate "ref/object missing" from "path missing inside an existing ref" (e.g., git cat-file -e $ref:$path first), fail loudly when the ref itself cannot be resolved, and ensure the scheduled workflow fetches enough depth before running.

[kryswisnaskas]

@thewatermethod , thanks. I addressed the findings that affect actual behavior and failure handling: ref validation/bootstrap handling, default ticket guarding, and clearer CLI errors. I did not address the others in this PR because they are either intentional (arraysMatch ordering, dependency paths) or stylistic (nested ternary, test message assertions). I also created follow-up ticket TTAHUB-5488 for the larger file refactor.

[github-actions]

⚠️ Diff size advisory: This PR is 7566 lines (7512+, 54−), exceeding the 500-line guideline. Consider splitting into smaller changes.

[github-actions]

⚠️ Review count advisory: 1 of 2 required human approvals. 1 more needed. Current approvers: thewatermethod.