323: MSSQL: audit log writer (.pi/mssql-audit.log, mode 0600) #347

Merged

david merged 2 commits from feature/issue-323/mssql-audit-log-writer-pi-mssql-audit-log-mode-060 into main

2026-11-11 00:10:20 +00:00

david commented

2026-11-11 00:10:19 +00:00

Owner

323: MSSQL: audit log writer (.pi/mssql-audit.log, mode 0600)

Implements 323.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 54s.

Review findings

Review Findings — 323

Summary

I fixed five of the six findings and left the logger wiring unresolved. The audit.test.ts suite passes (6 tests). The repo has no build or lint script, so those gates did not run.

.gitignore: added an explicit .pi/mssql-audit.log entry. The existing *.log pattern already covered the file.
SQL length cap: audit.ts now truncates SQL at 4096 characters and appends a marker. Rotation and README documentation of the log's sensitivity are not done.
Symlinks: the log is now opened with O_NOFOLLOW.
Directory permissions: .pi is now chmoded to 0700 after creation.
Root test skip: the write-failure test is skipped when running as root.
Wiring: calling log() from index.ts and the guard is a larger change and may belong in a follow-up issue, so I left it.

Critical

High

.gitignore:11 — .pi/mssql-audit.log is not ignored (only .pi-loop is). The log holds raw SQL, which may include sensitive literals, so git add -A could commit it. Suggested fix: add .pi/mssql-audit.log to .gitignore. Don't ignore all of .pi/, because .pi/settings.json may be tracked.

Medium

extensions/mssql/audit.ts:41 — The logger writes entry.sql verbatim and without a length cap. Queries containing patron PII or secret literals are persisted in plain text (APP 11 / Privacy Act 2020 / PDPA / UK GDPR considerations), and very large statements can grow the log without limit. Suggested fix: cap the SQL length (e.g. truncate to a few KB with a marker). Document in the README that the log is sensitive. Consider size-based rotation.
extensions/mssql/audit.ts:46 — openSync(file, "a") follows symlinks, so a planted .pi/mssql-audit.log symlink would redirect writes and the fchmod. Suggested fix: open with fs.constants.O_APPEND | O_CREAT | O_WRONLY | O_NOFOLLOW. Alternatively, lstat the path and refuse to write if it is a symlink.
extensions/mssql/audit.ts:61 — The logger isn't wired into index.ts or the guard (grep finds no audit references there), so nothing is logged yet. Suggested fix: if this issue is meant to deliver working auditing, call log() from the query path for rejected, executed, error and permission_warning outcomes. Otherwise confirm that a follow-up issue covers it.

Low

extensions/mssql/audit.ts:44 — If .pi already exists with looser permissions, mkdirSync leaves them unchanged, and the test only covers a freshly created directory. Suggested fix: optionally chmodSync(dir, DIR_MODE) after mkdirSync, or note this as accepted.
extensions/mssql/audit.test.ts:67 — The write-failure test relies on chmod 0500 blocking writes. This doesn't hold when the tests run as root (common in CI containers), so the test would fail there. Suggested fix: skip the test when process.getuid?.() === 0, or inject a failing fs.

## 323: MSSQL: audit log writer (.pi/mssql-audit.log, mode 0600) Implements [323](https://git.excelera.net/david/pi-extensions-and-skills/issues/323). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 54s. ## Review findings # Review Findings — 323 ## Summary I fixed five of the six findings and left the logger wiring unresolved. The `audit.test.ts` suite passes (6 tests). The repo has no build or lint script, so those gates did not run. - **`.gitignore`:** added an explicit `.pi/mssql-audit.log` entry. The existing `*.log` pattern already covered the file. - **SQL length cap:** `audit.ts` now truncates SQL at 4096 characters and appends a marker. Rotation and README documentation of the log's sensitivity are not done. - **Symlinks:** the log is now opened with `O_NOFOLLOW`. - **Directory permissions:** `.pi` is now `chmod`ed to 0700 after creation. - **Root test skip:** the write-failure test is skipped when running as root. - **Wiring:** calling `log()` from `index.ts` and the guard is a larger change and may belong in a follow-up issue, so I left it. ## Critical ## High - [x] .gitignore:11 — `.pi/mssql-audit.log` is not ignored (only `.pi-loop` is). The log holds raw SQL, which may include sensitive literals, so `git add -A` could commit it. Suggested fix: add `.pi/mssql-audit.log` to `.gitignore`. Don't ignore all of `.pi/`, because `.pi/settings.json` may be tracked. ## Medium - [x] extensions/mssql/audit.ts:41 — The logger writes `entry.sql` verbatim and without a length cap. Queries containing patron PII or secret literals are persisted in plain text (APP 11 / Privacy Act 2020 / PDPA / UK GDPR considerations), and very large statements can grow the log without limit. Suggested fix: cap the SQL length (e.g. truncate to a few KB with a marker). Document in the README that the log is sensitive. Consider size-based rotation. - [x] extensions/mssql/audit.ts:46 — `openSync(file, "a")` follows symlinks, so a planted `.pi/mssql-audit.log` symlink would redirect writes and the `fchmod`. Suggested fix: open with `fs.constants.O_APPEND | O_CREAT | O_WRONLY | O_NOFOLLOW`. Alternatively, `lstat` the path and refuse to write if it is a symlink. - [ ] extensions/mssql/audit.ts:61 — The logger isn't wired into `index.ts` or the guard (grep finds no `audit` references there), so nothing is logged yet. Suggested fix: if this issue is meant to deliver working auditing, call `log()` from the query path for rejected, executed, error and permission_warning outcomes. Otherwise confirm that a follow-up issue covers it. ## Low - [x] extensions/mssql/audit.ts:44 — If `.pi` already exists with looser permissions, `mkdirSync` leaves them unchanged, and the test only covers a freshly created directory. Suggested fix: optionally `chmodSync(dir, DIR_MODE)` after `mkdirSync`, or note this as accepted. - [x] extensions/mssql/audit.test.ts:67 — The write-failure test relies on `chmod 0500` blocking writes. This doesn't hold when the tests run as root (common in CI containers), so the test would fail there. Suggested fix: skip the test when `process.getuid?.() === 0`, or inject a failing `fs`.

david added 2 commits

2026-11-11 00:10:19 +00:00