Skip to content

[codex] Address post-merge review follow-ups#18

Merged
tyvsmith merged 2 commits intomainfrom
codex/follow-up-pr16-review-comments
Mar 28, 2026
Merged

[codex] Address post-merge review follow-ups#18
tyvsmith merged 2 commits intomainfrom
codex/follow-up-pr16-review-comments

Conversation

@tyvsmith
Copy link
Copy Markdown
Owner

Summary

  • fix SQLite sidecar permission hardening so custom database filenames secure the real -wal and -shm files
  • make privileged config-path detection fail safe when procfs UID parsing is unavailable
  • align SHA256 validation messaging with the accepted format and cover uppercase hex in tests
  • make run_test_contains in the integration container harness handle pipefail consistently with run_test

Why

PR #16 merged with four unresolved review threads. Two were real hardening/correctness gaps:

  • SQLite sidecar permissions were derived by replacing the extension instead of appending to the full filename
  • privileged config detection failed open if /proc/self/status could not be read

The other two were smaller follow-ups:

  • SHA256 validation text claimed lowercase-only input while the parser accepts uppercase hex
  • the integration test helper was slightly more fragile than the oneshot helper under pipefail

Impact

  • custom db_path values now secure the actual SQLite sidecar files
  • privileged config resolution remains locked down even if procfs status parsing is unavailable
  • config validation errors now match real accepted input
  • the integration harness is more robust for future pipeline-based assertions

Verification

  • cargo test -p facelock-core -p facelock-store
  • bash -n test/run-integration-tests.sh
  • cargo test --workspace
  • cargo clippy --workspace -- -D warnings
  • just test-pam
  • just test-integration

Notes

  • just test-oneshot was skipped because this patch set does not change the oneshot-specific direct camera/auth flow; it only touches shared config/storage logic already covered by workspace tests, plus the daemon integration test harness.

@tyvsmith tyvsmith marked this pull request as ready for review March 28, 2026 16:52
Copilot AI review requested due to automatic review settings March 28, 2026 16:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Follow-up hardening/correctness fixes from post-merge review on config-path privilege detection, SQLite sidecar permission handling, SHA256 validation messaging, and integration test harness robustness.

Changes:

  • Secure SQLite -wal / -shm sidecar permissions by appending suffixes to the full DB filename (not replacing extensions), with added tests.
  • Make privileged config-path detection fail safe when /proc/self/status can’t be read/parsed, with added parser/unit tests and test serialization.
  • Align SHA256 validation error messages with accepted input (uppercase hex allowed) and add coverage; make integration harness run_test_contains handle pipefail like run_test.

Reviewed changes

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

File Description
test/run-integration-tests.sh Align run_test_contains pipefail handling with run_test for consistent pipeline behavior.
crates/facelock-store/src/db.rs Fix SQLite sidecar path derivation and add tests ensuring actual sidecars are secured.
crates/facelock-core/src/paths.rs Fail-safe privileged detection when procfs UID parsing fails; add tests and serialize env-mutating tests.
crates/facelock-core/src/config.rs Update SHA256 validation messaging and add tests for uppercase hex + message text.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@tyvsmith tyvsmith merged commit e0f0aa3 into main Mar 28, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants