feat(server): persisted chain head anchor to detect audit-log tail truncation (#46)#50
Open
ydking0911 wants to merge 4 commits into
Conversation
…cation (#46) Add chain_meta table that snapshots head_hash/count after every insert, advancing the base anchor on DeleteBefore so orphaned tail rows trigger a tail_truncated detail in /verify. Dashboard shows amber "chain tail" badge.
📝 WalkthroughWalkthrough
Changeschain_meta 앵커 기반 해시 체인 무결성 검증
Sequence Diagram(s)sequenceDiagram
participant Client
participant VerifyHandler
participant Store
participant SQLite
Client->>VerifyHandler: GET /verify
VerifyHandler->>Store: ChainMeta()
Store->>SQLite: SELECT chain_meta
SQLite-->>Store: ChainMeta{count, headHash, baseRowID, ...}
Store-->>VerifyHandler: *ChainMeta
VerifyHandler->>Store: ForEachVerifyEvent(rowid ASC)
loop 각 이벤트
Store-->>VerifyHandler: VerifyEvent{RowID, ID, RawJSON, Hash}
VerifyHandler->>VerifyHandler: rowid <= baseRowID이면 skip
VerifyHandler->>VerifyHandler: hash 재계산 및 비교
alt hash 불일치
VerifyHandler-->>Client: {ok:false, broken_at:id, detail:"broken_link"}
end
end
alt checked < meta.Count
VerifyHandler-->>Client: {ok:false, broken_at:null, detail:"tail_truncated"}
else headHash 불일치
VerifyHandler-->>Client: {ok:false, detail:"broken_link"}
else 정상
VerifyHandler-->>Client: {ok:true, checked:N, detail:null}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/test_e2e.py (1)
240-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win성공 응답에서도
detail을 함께 단정하는 편이 좋겠습니다.지금은
ok만 확인해서,/verify가ok: true와 함께 잘못된detail값을 보내도 이 E2E가 통과합니다. 이번 PR에서 대시보드가detail에 의존하므로, 정상 체인 응답은detail is None까지 같이 검증해 두는 편이 안전합니다.제안된 수정
if verify_result.get("ok") is True: checked = verify_result.get("checked", 0) - ok(f"Chain intact: checked={checked}, broken_at=None, detail=None") - passed += 1 + if verify_result.get("detail") is None: + ok(f"Chain intact: checked={checked}, broken_at=None, detail=None") + passed += 1 + else: + fail(f"Unexpected detail on healthy chain: {verify_result.get('detail')}") + failed += 1 else:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_e2e.py` around lines 240 - 243, The success-path check in verify_result handling only asserts ok and can miss an invalid detail payload; update the E2E assertion in the chain verification block to also verify that detail is None when ok is true. Use the existing verify_result.get("ok"), verify_result.get("checked"), and the Chain intact logging path in scripts/test_e2e.py to keep the normal response contract explicit.server/internal/storage/verify_test.go (1)
173-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win비단조 타임스탬프 retention 케이스에 대한 커버리지가 빠져 있습니다.
이 테스트는
evt_old(ts=10) →evt_keep1(ts=20) →evt_keep2(ts=30)처럼 타임스탬프가 삽입(rowid) 순서와 단조 일치하는 경우만 검증합니다. 그래서DeleteBefore가 타임스탬프 기준으로 삭제하면서 rowid 기준 base 앵커를 전진시킬 때 발생하는 불일치(store.go Line 567-645 코멘트 참조)를 잡지 못합니다.타임스탬프가 rowid 순서와 어긋나는 케이스(예: ts=30, ts=10, ts=20 순으로 삽입 후 컷오프로 중간 행만 삭제)를 추가해,
ChainMeta.Count와/verify의checked가 일치하고tail_truncated로 오보되지 않는지 검증하면 위 회귀를 방어할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/storage/verify_test.go` around lines 173 - 212, Add coverage for a non-monotonic timestamp retention case in TestDeleteBeforeAdvancesChainBaseAnchor so DeleteBefore is exercised when row order and timestamp order differ. Use the existing verify helpers around s.Insert, s.DeleteBefore, and s.ChainMeta to set up inserts like ts=30, ts=10, ts=20, then delete with a cutoff that removes only the middle row and assert the chain anchor/count still advance correctly. Make the test verify the resulting ChainMeta.Count and base/head metadata so it catches the DeleteBefore rowid-vs-timestamp mismatch and prevents a false tail_truncated-style outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/internal/storage/store.go`:
- Around line 567-645: `DeleteBefore` is advancing chain metadata by timestamp
while `collector` can ingest non-monotonic client timestamps, which can leave
earlier `rowid` events behind and cause `/verify` to skip data. Fix the deletion
logic in `store.go` so the cutoff is consistent with the chain anchor: either
restrict deletions to a contiguous `rowid` prefix or compute/update `baseRowID`,
`BasePrevHash`, and `Count` using `rowid`-ordered records in the same
transaction. Make sure the `DeleteBefore` path and `upsertChainMetaTx` keep
`meta.BaseRowID` aligned with what was actually deleted.
In `@server/internal/verify/handler.go`:
- Around line 124-131: The `/verify` logic in `handler.go` only compares
`prevHash` with `meta.HeadHash` when `checked > 0`, which lets a zero-event
chain with a non-empty stored head anchor pass validation. Update the `meta !=
nil` validation path in `Result` generation so the `DetailBrokenLink` check also
enforces the empty-state invariant when `checked == 0` and `meta.Count == 0`,
using the existing `meta.HeadHash`, `checked`, and `prevHash` comparisons to
reject inconsistent metadata.
---
Nitpick comments:
In `@scripts/test_e2e.py`:
- Around line 240-243: The success-path check in verify_result handling only
asserts ok and can miss an invalid detail payload; update the E2E assertion in
the chain verification block to also verify that detail is None when ok is true.
Use the existing verify_result.get("ok"), verify_result.get("checked"), and the
Chain intact logging path in scripts/test_e2e.py to keep the normal response
contract explicit.
In `@server/internal/storage/verify_test.go`:
- Around line 173-212: Add coverage for a non-monotonic timestamp retention case
in TestDeleteBeforeAdvancesChainBaseAnchor so DeleteBefore is exercised when row
order and timestamp order differ. Use the existing verify helpers around
s.Insert, s.DeleteBefore, and s.ChainMeta to set up inserts like ts=30, ts=10,
ts=20, then delete with a cutoff that removes only the middle row and assert the
chain anchor/count still advance correctly. Make the test verify the resulting
ChainMeta.Count and base/head metadata so it catches the DeleteBefore
rowid-vs-timestamp mismatch and prevents a false tail_truncated-style outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 576194bf-ab7a-4a5a-8dd2-da2812951532
📒 Files selected for processing (9)
README.ko.mdREADME.mddashboard/src/App.tsxdocs/architecture.mdscripts/test_e2e.pyserver/internal/storage/store.goserver/internal/storage/verify_test.goserver/internal/verify/handler.goserver/internal/verify/handler_test.go
Comment on lines
+567
to
+645
| s.writeMu.Lock() | ||
| defer s.writeMu.Unlock() | ||
|
|
||
| tx, err := s.db.Begin() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| defer tx.Rollback() //nolint:errcheck | ||
|
|
||
| meta, err := loadChainMetaTx(tx) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| var baseRowID sql.NullInt64 | ||
| var baseHash sql.NullString | ||
| var deletedHashed int | ||
| if err := tx.QueryRow(` | ||
| SELECT rowid, hash | ||
| FROM events | ||
| WHERE datetime(timestamp) < datetime(?) | ||
| AND rowid > ? | ||
| AND raw_json IS NOT NULL AND raw_json != '' | ||
| AND hash IS NOT NULL AND hash != '' | ||
| ORDER BY rowid DESC | ||
| LIMIT 1 | ||
| `, t.UTC().Format(time.RFC3339Nano), meta.BaseRowID).Scan(&baseRowID, &baseHash); err != nil { | ||
| if err != sql.ErrNoRows { | ||
| return 0, err | ||
| } | ||
| } | ||
| if err := tx.QueryRow(` | ||
| SELECT COUNT(*) | ||
| FROM events | ||
| WHERE datetime(timestamp) < datetime(?) | ||
| AND rowid > ? | ||
| AND raw_json IS NOT NULL AND raw_json != '' | ||
| AND hash IS NOT NULL AND hash != '' | ||
| `, t.UTC().Format(time.RFC3339Nano), meta.BaseRowID).Scan(&deletedHashed); err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| // Use datetime() to compare so SQLite parses both sides as timestamps | ||
| // rather than relying on lexicographic TEXT ordering of RFC3339Nano | ||
| // strings (which can be unreliable when the fractional-second part | ||
| // has different widths, e.g. "...00Z" vs "...00.5Z"). | ||
| res, err := s.db.Exec( | ||
| res, err := tx.Exec( | ||
| `DELETE FROM events WHERE datetime(timestamp) < datetime(?)`, | ||
| t.UTC().Format(time.RFC3339Nano), | ||
| ) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return res.RowsAffected() | ||
| affected, err := res.RowsAffected() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| if deletedHashed > 0 && baseRowID.Valid && baseHash.Valid { | ||
| meta.BaseRowID = baseRowID.Int64 | ||
| meta.BasePrevHash = baseHash.String | ||
| meta.Count -= deletedHashed | ||
| if meta.Count < 0 { | ||
| meta.Count = 0 | ||
| } | ||
| if meta.Count == 0 { | ||
| meta.HeadHash = meta.BasePrevHash | ||
| meta.LastID = "" | ||
| } | ||
| meta.UpdatedAt = time.Now().UTC() | ||
| if err := upsertChainMetaTx(tx, meta); err != nil { | ||
| return 0, err | ||
| } | ||
| } | ||
|
|
||
| if err := tx.Commit(); err != nil { | ||
| return 0, err | ||
| } | ||
| return affected, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Timestamp 출처 및 단조성 보장 여부 확인
rg -nP --type=go -C3 '\.Timestamp\s*='
# 인입(핸들러)에서 Timestamp 파싱/기본값 처리
rg -nP --type=go -C4 'Timestamp' server/internal | rg -n 'time.Now|Parse|Decode|json'
# DeleteBefore 호출부 (retention) 컨텍스트
rg -nP --type=go -C5 'DeleteBefore\s*\('Repository: ydking0911/observr
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find relevant Go files under server/internal
printf '\n== Files ==\n'
git ls-files 'server/internal/**/*.go' | sed -n '1,200p'
printf '\n== DeleteBefore and verify references ==\n'
rg -n 'DeleteBefore\s*\(|BaseRowID|tail_truncated|broken_link|verify' server/internal -g '*.go' -C 3 || true
printf '\n== Timestamp assignments/parsing ==\n'
rg -n '\.Timestamp\s*=|Timestamp[:=]|time\.Now\(\)|Parse|Decode|json' server/internal -g '*.go' -C 3 || trueRepository: ydking0911/observr
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== storage.Insert / DeleteBefore / verify-related slices ==\n'
sed -n '120,240p' server/internal/storage/store.go
printf '\n---\n'
sed -n '560,645p' server/internal/storage/store.go
printf '\n---\n'
sed -n '1,140p' server/internal/verify/handler.go
printf '\n== collector handler timestamp path ==\n'
sed -n '1,220p' server/internal/collector/handler.go
printf '\n== non-monotonic timestamp test evidence ==\n'
sed -n '40,70p' server/internal/storage/verify_test.goRepository: ydking0911/observr
Length of output: 12588
타임스탬프/rowid 기준을 맞춰야 합니다. collector가 클라이언트 timestamp를 그대로 쓰고 없으면 now를 채우므로 비단조 timestamp는 충분히 들어올 수 있습니다. 이 상태에서 DeleteBefore()가 datetime(timestamp)로만 삭제하고 BaseRowID는 더 큰 rowid로 전진시키면, 남아 있는 더 이른 rowid 이벤트가 /verify에서 통째로 스킵되어 실제 변조가 없어도 tail_truncated가 납니다. 삭제 대상을 rowid 연속 prefix로 제한하거나, 앵커/카운트를 rowid 기준으로 함께 정렬해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/internal/storage/store.go` around lines 567 - 645, `DeleteBefore` is
advancing chain metadata by timestamp while `collector` can ingest non-monotonic
client timestamps, which can leave earlier `rowid` events behind and cause
`/verify` to skip data. Fix the deletion logic in `store.go` so the cutoff is
consistent with the chain anchor: either restrict deletions to a contiguous
`rowid` prefix or compute/update `baseRowID`, `BasePrevHash`, and `Count` using
`rowid`-ordered records in the same transaction. Make sure the `DeleteBefore`
path and `upsertChainMetaTx` keep `meta.BaseRowID` aligned with what was
actually deleted.
Comment on lines
+124
to
+131
| if meta != nil { | ||
| if checked < meta.Count { | ||
| detail := DetailTailTruncated | ||
| return Result{OK: false, Checked: checked, Skipped: skipped, BrokenAt: nil, Detail: &detail}, nil | ||
| } | ||
| if checked != meta.Count || (checked > 0 && prevHash != meta.HeadHash) { | ||
| detail := DetailBrokenLink | ||
| return Result{OK: false, Checked: checked, Skipped: skipped, BrokenAt: nil, Detail: &detail}, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
빈 체인에서도 저장된 head anchor를 검증해야 합니다.
Line 129은 checked > 0일 때만 meta.HeadHash를 비교합니다. 그래서 retained 이벤트가 0건인 상태에서 chain_meta.count == 0이더라도 head_hash가 비어 있지 않은 잘못된 메타를 /verify가 정상으로 통과시킬 수 있습니다. 이번 변경의 목적이 저장된 tip과 재계산 결과를 비교하는 것이라면, 0건 케이스도 empty-state invariant까지 함께 검증해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/internal/verify/handler.go` around lines 124 - 131, The `/verify`
logic in `handler.go` only compares `prevHash` with `meta.HeadHash` when
`checked > 0`, which lets a zero-event chain with a non-empty stored head anchor
pass validation. Update the `meta != nil` validation path in `Result` generation
so the `DetailBrokenLink` check also enforces the empty-state invariant when
`checked == 0` and `meta.Count == 0`, using the existing `meta.HeadHash`,
`checked`, and `prevHash` comparisons to reject inconsistent metadata.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
chain_metasingle-row SQLite table that persists the chain tip (head_hash,count,last_id) after every insert, enabling detection of tail truncation — the one tampering vector the feat: tamper-proof hash-chain audit log + GET /verify #43 hash chain could not catchStore.Insert()updates the head anchor inside the same transaction and write mutex as the event rows, so the anchor is always consistent with the DB it describesStore.DeleteBefore()(retention cleanup) advances a base anchor (base_rowid,base_prev_hash) so legitimate retention does not trigger a falsechain ✗GET /verifycross-checks the recomputed count and tip hash againstchain_meta; reports tail truncation distinctly from in-place tampering via a newdetailfieldchain tailfor tail truncation, distinct from greenchain ✓and redchain ✗Open()so upgrades from feat: tamper-proof hash-chain audit log + GET /verify #43 require no manual migrationHow it works
On every
Insert, the head anchor is written in the same transaction:GET /verifyrecomputes the chain starting frombase_rowidand compares:Response shape:
{ "ok": true, "checked": 1042, "skipped": 0, "broken_at": null, "detail": null } { "ok": false, "checked": 1042, "skipped": 0, "broken_at": null, "detail": "tail_truncated" } { "ok": false, "checked": 204, "skipped": 0, "broken_at": "evt_abc123", "detail": "broken_link" }Test plan
TestDeleteBeforeAdvancesChainBaseAnchor— partial retention advances base anchor correctlyTestDeleteBeforeAllHashedEventsResetsChainMeta— full retention resetscountto 0 without false alarmTestOpenBackfillsChainMetaForExistingHashedRows— upgrade from feat: tamper-proof hash-chain audit log + GET /verify #43 backfills anchor onOpen()TestHandlerReportsTailTruncatedFromPersistedMeta— fake loader with mismatched count triggerstail_truncatedTestHandlerReportsTailTruncatedAfterDirectLatestRowDelete— real DB, SQLite-level row delete triggerstail_truncatedTestHandlerVerifiesRetainedSuffixFromBaseAnchor— suffix-only chain verifies OK after retentionGET /verifyreturnsok=true,checked=6after real SDK event ingestionchain tailbadge on tail truncation, greenchain ✓on intact chain, redchain ✗on in-place tampergo test ./... -race -timeout 60s— all packages passgo vet ./...— cleannpm run build— dashboard build succeedsCloses #46
Summary by CodeRabbit
기능 개선
문서
테스트