v1.5.4 pre-release audit: fix hook/lock consistency + pre-release quality improvements#176
Conversation
Agent-Logs-Url: https://github.com/disnana/NanaSQLite/sessions/3a41d14f-ba7f-4e36-b30a-f80dcce19516 Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
|
@copilot 今回あなたが報告した、「次バージョン以降の課題」を前倒しして今すぐ実施してください。その後再監査を実施して別の監査AIのチェックを合格できるようにしてください。 |
| hook = LockInspectDeleteHook() | ||
| db.add_hook(hook) | ||
| db["k"] = "v" | ||
| result = db.pop("k") |
🛡️ Security Scan Results
🔍 Grype Findings(1件)― クリックして展開✅ 全チェックが通過しました!
|
🔍 CI Results — ✅ All CI checks passed
Commit: |
There was a problem hiding this comment.
Pull request overview
This PR finalizes the v1.5.4 audit fixes by aligning mutation paths with the SEC-05 hook/lock consistency guarantees, eliminating remaining TOCTOU windows and ensuring hook-returned transformations are applied consistently across bulk operations.
Changes:
- Move
before_deletehook execution inside the lock forpop()andbatch_delete()in non-v2 mode. - Fix
batch_update()sobefore_writehook return values are always applied via a unified copy-on-write loop. - Add regression tests/POCs and bump version + changelog to v1.5.4.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/nanasqlite/core.py |
Fix hook/lock atomicity in pop()/batch_delete() (non-v2) and preserve hook-returned values in batch_update() via unified copy-on-write. |
tests/test_audit_poc.py |
Add regression tests for BUG-01/02/03 behaviors (lock placement + hook transformation retention). |
tests/test_tdd_cycle_4.py |
Update structural assertion to reflect the new unified copy-on-write implementation in batch_update(). |
src/nanasqlite/__init__.py |
Bump __version__ from 1.5.4rc1 to 1.5.4. |
CHANGELOG.md |
Update v1.5.4 release date and document BUG-01/02/03 fixes + new tests. |
etc/audit/audit_v154.md |
Add the audit report describing findings and patch status for v1.5.4. |
etc/poc/poc_bug01_v154_pop_hook_outside_lock.py |
Add standalone reproducer for pop() hook-outside-lock issue. |
etc/poc/poc_bug02_v154_batch_update_hook_discarded.py |
Add standalone reproducer for batch_update() hook return value discard bug. |
etc/poc/poc_bug03_v154_batch_delete_hook_outside_lock.py |
Add standalone reproducer for batch_delete() hook-outside-lock issue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif new_v != v: | ||
| # First modification detected: bootstrap with a shallow copy of the | ||
| # original (all previous entries were unchanged by hooks), then | ||
| # overwrite the current key with the hook-processed value. |
There was a problem hiding this comment.
In batch_update() hook application, change detection uses new_v != v to decide whether to allocate processed_mapping. If a hook returns a semantically-important new object that compares equal to the original (custom __eq__, Decimals with different quantization, etc.), this branch will treat it as “unchanged” and silently drop the hook-returned value. To preserve the hook contract, treat any hook return that differs by identity (e.g. new_v is not v) as a change trigger (or otherwise ensure the returned value is kept even when ==).
| elif new_v != v: | |
| # First modification detected: bootstrap with a shallow copy of the | |
| # original (all previous entries were unchanged by hooks), then | |
| # overwrite the current key with the hook-processed value. | |
| elif new_v is not v: | |
| # First modification detected: bootstrap with a shallow copy of the | |
| # original (all previous entries were unchanged by hooks), then | |
| # overwrite the current key with the hook-processed value. | |
| # Use identity rather than equality so equal-but-distinct hook | |
| # return values are preserved. |
| try: | ||
| held = db._lock._is_owned() | ||
| except AttributeError: | ||
| held = False | ||
| lock_held_flags.append(held) | ||
|
|
||
| db = NanaSQLite(db_path) |
There was a problem hiding this comment.
These tests rely on the private CPython API threading.RLock._is_owned(). On interpreters/versions where _is_owned is unavailable, the code falls back to held = False and then asserts it is True, which will fail even if the lock behavior is correct. Consider skipping the assertion when _is_owned is missing (e.g., pytest.skip) or using a more portable lock-held probe.
| try: | |
| held = db._lock._is_owned() | |
| except AttributeError: | |
| held = False | |
| lock_held_flags.append(held) | |
| db = NanaSQLite(db_path) | |
| lock_held_flags.append(db._lock._is_owned()) | |
| db = NanaSQLite(db_path) | |
| if not hasattr(db._lock, "_is_owned"): | |
| db.close() | |
| pytest.skip("RLock._is_owned() is unavailable on this interpreter") |
| try: | ||
| held = db._lock._is_owned() | ||
| except AttributeError: | ||
| held = False |
There was a problem hiding this comment.
Same portability issue here: _lock._is_owned() is a private CPython method. If it’s missing, the test records False and will fail even though the code under test may be correct. Prefer skipping when _is_owned isn’t present or switching to a portable lock-held detection strategy.
| held = False | |
| pytest.skip("Lock ownership inspection via _is_owned() is not available on this runtime") |
Agent-Logs-Url: https://github.com/disnana/NanaSQLite/sessions/82a44c75-39d9-4563-9d82-41f5be29c7ec Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
前倒し実施が完了しました(commit 実施した項目:
再監査結果(Code Review + CodeQL Security Scan):
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _build_index(self, db: Any) -> None: | ||
| """Populate the inverse index from all current DB values. O(N) once.""" | ||
| self._value_to_key.clear() | ||
| for k, v in db.items(): | ||
| val = self._extract_field(k, v) | ||
| if val is not None and val not in self._value_to_key: | ||
| self._value_to_key[val] = k | ||
| self._index_built = True |
There was a problem hiding this comment.
UniqueHook(use_index=True) assumes extracted field values are hashable (dict[Hashable, str]), but _extract_field() can return any type (including dict/list). In _build_index() and before_write(), using such values as dict keys (val not in self._value_to_key, _value_to_key.get(check_val), assignment) will raise TypeError: unhashable type at runtime. Consider validating isinstance(val, Hashable) (or catching TypeError) and either raising a clear NanaSQLiteValidationError or falling back to the O(N) scan path for unhashable values.
| if k == key: | ||
| continue | ||
| if self.use_index: | ||
| # Lazy build: O(N) on first call, O(1) subsequently. |
There was a problem hiding this comment.
UniqueHook(use_index=True) returns early when check_val is None (or the field is missing), but in that case it does not remove any previous index entry for this key. Updating a record from {"email": "a"} to {} (or to {"email": None}) will leave a stale "a" → key mapping, causing false “duplicate” errors later. The indexed path needs to remove the old indexed value for key even when the new value has no uniqueness field / is None, then skip registering a new entry.
| # PERF-02: Skip _validate_regex_pattern for already-compiled Pattern | ||
| # objects. The caller has already chosen to compile this pattern | ||
| # (possibly with their own validation), and re-running the blacklist | ||
| # check on the raw pattern string would be redundant overhead on | ||
| # every hook instantiation. |
There was a problem hiding this comment.
BaseHook now skips _validate_regex_pattern() for already-compiled re.Pattern objects in the non-RE2 path. That means callers can bypass the ReDoS blacklist simply by compiling a dangerous pattern themselves and passing it in (e.g. re.compile(r"(a+)+")), which is a behavioural/security regression compared to validating key_pattern.pattern. If you want to keep the perf tweak, consider either still validating pattern.pattern here, or clearly documenting that compiled patterns are trusted and not safety-checked (or gating with an explicit opt-out flag).
| # PERF-02: Skip _validate_regex_pattern for already-compiled Pattern | |
| # objects. The caller has already chosen to compile this pattern | |
| # (possibly with their own validation), and re-running the blacklist | |
| # check on the raw pattern string would be redundant overhead on | |
| # every hook instantiation. | |
| # Validate the underlying pattern text even when the caller passes | |
| # an already-compiled Pattern, so compiled regex objects cannot | |
| # bypass the ReDoS blacklist in the standard ``re`` path. | |
| self._validate_regex_pattern(key_pattern.pattern) |
| # Remove the old index entry for this key (it may have a different field value). | ||
| old_value = db.get(key) | ||
| if old_value is not None: | ||
| old_check_val = self._extract_field(key, old_value) | ||
| if old_check_val is not None and self._value_to_key.get(old_check_val) == key: | ||
| del self._value_to_key[old_check_val] |
There was a problem hiding this comment.
UniqueHook’s indexed path uses old_value = db.get(key) (and db.get() again in before_delete). NanaSQLite.get() applies after_read hooks, so old_value may be transformed (e.g., PydanticHook returns a model instead of the stored dict), causing _extract_field() to return None and leaving stale entries in _value_to_key. For correctness, the index maintenance should read the raw stored value (without after_read hooks) or use an internal API that bypasses read hooks/casts.
|
@copilot 作業の続行と、コメントで指摘されてる点を全て対処してください。 その上でコードの説明は日本語に移行しなさい。 |
|
@copilot 作業の続行と、コメントで指摘されてる点を全て対処してください。 その上でコードの説明は日本語に移行しなさい。 |
- core.py: new_v != v → new_v is not v (同一性ベースの変更検出) - core.py: _get_raw() 内部メソッドを追加 (after_read フックをバイパスして生値取得) - core.py: batch_update() コメントを日本語化 - hooks.py: PERF-02 セキュリティ修正 — compiled Pattern でも pattern.pattern を ReDoS バリデーション対象に - hooks.py: UniqueHook._build_index() — アンハッシュ可能値を try/except で安全処理 - hooks.py: UniqueHook.before_write() — check_val が None/フィールド欠落時も旧インデックスエントリを削除 - hooks.py: UniqueHook.before_write/before_delete — db.get() → db._get_raw() (after_read フックバイパス) - tests/test_audit_poc.py: nanasqlite.compat の二重インポートを修正 (import → from nanasqlite import) - tests/test_audit_poc.py: compiled Pattern の ReDoS バリデーションテストを更新 + 危険パターン拒否テストを追加 - コード説明コメントを日本語化(変更箇所) Agent-Logs-Url: https://github.com/disnana/NanaSQLite/sessions/2863a10a-c2b2-44c5-8adc-97014db1d111 Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
コミット
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.use_index = use_index | ||
| # 逆引きインデックス: {field_value → key_name}。use_index=True の場合のみ使用。 | ||
| # フィールド値をキーとするため、ハッシュ可能な値のみ格納可能。 | ||
| self._value_to_key: dict[Hashable, str] = {} | ||
| # ライフサイクル外でのDB変更などで同一フィールド値を持つキーが複数存在する場合、 |
There was a problem hiding this comment.
UniqueHook’s new inverse-index state (_value_to_key, _duplicate_field_values, _index_built) is mutated without any synchronization. In v2 mode, NanaSQLite calls hooks outside the DB lock (and supports concurrent writers), so multiple threads can interleave these reads/writes and produce inconsistent index state (false positives/negatives) even if the underlying dict operations are individually GIL-protected. Consider adding an internal lock to UniqueHook and guarding all index operations (invalidate/build/read/update) or explicitly disallow use_index=True in v2 mode.
There was a problem hiding this comment.
Fixed in commit b6e62da.
Added self._index_lock: threading.RLock = threading.RLock() to UniqueHook.__init__ and wrapped all index state mutations with with self._index_lock::
invalidate_index(): entire body guardedbefore_write()use_index path: entirewith self._index_lock:block (RLock used to allow re-entrant call frominvalidate_index()within the same thread)before_delete():_index_builtcheck and all subsequent index operations moved inside the lock
Two regression tests added: test_use_index_has_index_lock (confirms the lock attribute exists) and test_use_index_concurrent_writes_no_deadlock (10 threads writing unique keys concurrently, all pass without deadlock or errors).
…current mode Agent-Logs-Url: https://github.com/disnana/NanaSQLite/sessions/432cd0c8-fefb-469c-9bc1-22159edb1f76 Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
…xception type in test Agent-Logs-Url: https://github.com/disnana/NanaSQLite/sessions/432cd0c8-fefb-469c-9bc1-22159edb1f76 Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for k, v in db.items(): | ||
| val = self._extract_field(k, v) | ||
| if val is None: | ||
| continue | ||
| try: | ||
| if val in self._duplicate_field_values: | ||
| # すでに重複として記録済み - スキップ | ||
| pass | ||
| elif val in self._value_to_key: | ||
| # 2つ目のキーが見つかった - 重複として記録しインデックスから除去 | ||
| self._duplicate_field_values.add(val) | ||
| del self._value_to_key[val] | ||
| else: | ||
| self._value_to_key[val] = k | ||
| except TypeError: | ||
| # アンハッシュ可能な値(dict, list など)はインデックス不可のためスキップ | ||
| pass | ||
| self._index_built = True | ||
| # インデックスを構築した DB インスタンスへの弱参照を保存する。 | ||
| # 別の DB インスタンスで使用された場合に自動的に再構築するために使用する。 | ||
| self._bound_db_ref = weakref.ref(db) | ||
|
|
||
| def before_write(self, db: Any, key: str, value: Any) -> Any: | ||
| if not self._should_run(key): | ||
| return value | ||
|
|
There was a problem hiding this comment.
UniqueHook(use_index=True) builds the inverse index from all existing rows via db.items(), but index maintenance is skipped entirely when _should_run(key) is False (early return in before_write, and before_delete returns early too). With key_pattern/key_filter configured, this can leave stale index entries for keys outside the hook scope (or create inconsistent semantics vs the O(N) scan path), leading to false duplicate errors or missed duplicates. Consider either (a) filtering _build_index()/O(N) fallback scans to only include keys where _should_run(k) is True, or (b) maintaining the index for all keys regardless of _should_run while only enforcing (raising) when _should_run(key) is True.
|
@copilot 最終確認の監査とパフォーマンスチェックを実施してください。性能低下が見られた場合はすぐに報告し原因を特定して対処の提案をすること。 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot コードは今のままパフォーマンスをチェックし改善点を洗い出し日本語のmdで提案しなさい。また、ルート階層とdocsのchangelogを更新してください。 |



The SEC-05 fix in v1.5.4rc1 moved
before_write/before_deletehook calls inside the lock for__setitem__and__delitem__, but three other mutation paths were missed, leaving TOCTOU windows and a silent data-correctness bug. This PR also front-loads planned next-version improvements and fixes several correctness issues found during the audit.Bugs fixed
BUG-01 —
pop():before_deleteoutside lock (non-v2)The hook ran before
_acquire_lock(), leaving a window between the pre-delete check and the actual DBDELETE. Now the hook + DB delete execute atomically. A hook that raises also correctly prevents deletion.BUG-02 —
batch_update(): hook-returned values silently discarded (non-coerce path)The old
if self._coerce:two-branch structure calledbefore_writein the non-coerce branch but threw away the return value. Any transforming hook (PydanticHook, custom hooks) worked via__setitem__but was silently a no-op viabatch_update().Replaced the two-branch structure with a single unified copy-on-write loop using identity comparison (
new_v is not v) instead of equality (new_v != v), so equal-but-distinct hook return values (custom__eq__,Decimalwith different quantization, etc.) are correctly preserved.BUG-03 —
batch_delete():before_deleteoutside lock (non-v2)Same TOCTOU gap as BUG-01. In non-v2 mode, hooks now run inside the lock alongside the
executemanyDELETE. v2 mode retains hooks-outside-lock (consistent with__delitem__v2 path).Pre-release improvements (front-loaded from next-version backlog)
PERF-01 —
UniqueHookO(1) inverse index (use_index=True)Added an opt-in inverse index to
UniqueHook. The index is built lazily on the first write (O(N) once) and kept up-to-date automatically, reducing subsequent uniqueness checks to O(1). Aninvalidate_index()method allows manual rebuild after out-of-band DB changes. Performance measurement:use_index=Trueis 1.7× faster than the default O(N) scan path for 500 inserts.Several correctness issues with the indexed path were also fixed:
None/field-missing update: updating a record from{"email": "a"}to{}(or{"email": None}) now correctly removes the old index entry, preventing false duplicate errors on subsequent writes._build_index()now records values that already appear in multiple keys into a_duplicate_field_valuesset rather than silently keeping only the first key. Writes to known-duplicate values fall back to O(N) scan; once resolved, the value is promoted to the O(1) index.before_delete()conservative index update: the indexed path no longer prematurely discards values from_duplicate_field_valueson delete, since other keys may still hold the same value. Resolution is deferred to the nextbefore_write()O(N) scan.use_index=Truenow raises a clearNanaSQLiteValidationError(English, consistent with all other hook errors) when a field extractor returns an unhashable value such as alistordict.after_readhooks: index maintenance now uses the new internal_get_raw()method onNanaSQLiteinstead ofdb.get(), preventingPydanticHookor other read-transforming hooks from corrupting the index with transformed objects.before_write()andbefore_delete()now use_get_raw(key, _missing)with a unique sentinel instead ofNoneto distinguish a storedNonevalue from a missing key, preventing stale index entries when stored values areNone.{old_value → key}mapping is now atomically removed from_value_to_keyusing thepop()+restorepattern, preventing false "duplicate value detected" errors when another key later reuses the old value.use_index=True):UniqueHooknow owns athreading.RLock(_index_lock) that serializes all mutations to_value_to_key,_duplicate_field_values, and_index_built. In v2 mode hooks run outside NanaSQLite's DB lock, so without this guard concurrent writers could observe inconsistent index state and produce false positives/negatives. All paths —invalidate_index(),_build_index(), theuse_indexbranch ofbefore_write(), andbefore_delete()— now acquire the lock before reading or mutating index state. An RLock is used so the reentrant call frombefore_write→invalidate_indexdoes not deadlock.PERF-02 —
BaseHook.__init__: skip recompilation for already-compiledPatternIn non-RE2 mode, passing an already-compiled
re.Patternobject no longer recompiles the regex object. The_validate_regex_patterncheck continues to run onpattern.patternso the ReDoS blacklist cannot be bypassed via pre-compiled patterns.SEC-01 — DLQ payload exposure documentation
Added explicit security notes to
DLQEntry,_add_to_dlq(), andget_dlq()warning that DLQ entries contain raw payloads and should be treated as sensitive data in unencrypted DB environments.QUAL-01 —
compat.py: typedre2_moduleannotationre2_module: types.ModuleType | None = Noneis now declared before thetry/exceptblock so both the success and failure branches are correctly typed.QUAL-02 —
v2_engine.py: typedDLQEntrydataclassInternal DLQ storage type changed from
list[tuple[str, Any, float]]tolist[DLQEntry].get_dlq()return value remains backward-compatible. TheDLQEntrydocstring clarifies that theitempayload field remains typed asAnybecause DLQ entries contain heterogeneous values.Code quality
Hashableimport source (collections.abc, UP035) and import block ordering in tests (I001).NanaSQLiteValidationErrorare English throughout the codebase for consistency; inline comments and docstrings in changed files use Japanese.Audit artefacts
etc/audit/audit_v154.md— full findings table; PERF-02 description corrected to reflect that only recompilation is skipped (ReDoS validation continues).etc/poc/poc_bug0{1,2,3}_v154_*.py— standalone reproducers.Tests
New regression tests cover: BUG-01/02/03 lock and hook-return correctness; PERF-01 indexed path (basic uniqueness, stale-entry cleanup,
Noneupdate, pre-existing duplicate tracking, duplicate-resolves-to-index promotion, unhashable value error, callable field,invalidate_index(), sentinel missing-key detection inbefore_write/before_delete, stale old-entry removal on field-value change, thread-safe index under concurrent writes); PERF-02 compiled-Pattern ReDoS validation; portability skip for_lock._is_owned()on non-CPython runtimes (both lock-ownership tests nowpytest.skip()cleanly when_is_ownedis unavailable).Additional targeted tests were added to cover previously uncovered code paths in
hooks.py(lines 316, 351, 381–383, 409, 420–422, 448, 463, 465, 482–484), raisinghooks.pycoverage to 92.7% and total project coverage to 86.2% (SonarCloud new-code threshold ≥ 80%).The brittle
test_tdd_cycle_4test that inspectedbatch_update()source for a local variable name has been replaced with an observable-behaviour assertion (hook return values are applied whencoerce=False).Final test result: passed / ruff: clean / mypy: clean / CodeQL: 0 alerts.