Release v1.19.2 #10
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "release/1.19.2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Release v1.19.2 — PR Review Hardening
Changes
Features
Refactors
Bug Fixes
CI / Tests
Checklist
References
Version: 1.19.2 (Gravity PM v1.19.2 — PR Review Hardening, version ID 124)
Epic: OP#1746
Adds deleted_at TIMESTAMPTZ to 8 tables (projects, versions, work packages, comments, wiki pages, time entries, news, themes) via migration 008. All SELECT queries now filter deleted_at IS NULL; DELETE operations become UPDATE SET deleted_at = now(). Application-level cascade (SoftDeleteProjectTx) replaces implicit DB CASCADE. Admin purge endpoint (DELETE /projects/{id}/purge) provides hard-delete recovery. Closes OP#1694 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>PR Review
Verdict: Not Acceptable
This review was conducted from a fresh context against the current branch state (26 commits, 37 files, +1753/-406 lines). First review cycle — no prior reviews or followups.
Findings
Test Results
Human Review Items
3 items flagged for human review — see items marked with HUMAN REVIEW.
Key Findings
AND deleted_at IS NULL— tx.go</user_content>delimiter injection bypasses prompt sanitization — sanitize.go:50(Grouped by file below)
apps/gravity-pm/internal/store/tx.go
[BLOCKING] Lines 56, 172, 205: Three
UpdateXxxTxmethods are missingAND deleted_at IS NULLin their WHERE clauses.UpdateVersionTx,UpdateWikiPageTx, andUpdateThemeTxcan update soft-deleted rows, violating the soft-delete invariant that deleted records are invisible to normal operations. All other read/update queries correctly filter bydeleted_at IS NULL.Suggested fix:
[BLOCKING] Lines 52, 108, 167, 200, 229: When the updates map is empty, all
UpdateXxxTxmethods fall back to pool-basedGetmethods (e.g.,s.GetVersion,s.GetWikiPage). This reads from the connection pool OUTSIDE the caller's transaction, breaking snapshot isolation. The caller passed apgx.Txspecifically for transactional consistency, but the empty-update path silently bypasses it.Suggested fix: Execute the SELECT on the provided
txdirectly instead of falling back to pool-based methods. Either add Tx-aware Get variants or inline the query:apps/gravity-pm/internal/auth/manager.go
[BLOCKING] Lines 42-66:
ValidateIssuerURLis enforced at the HTTP handler boundary (settings.go, setup.go) when a user submits a new issuer URL, but it is NOT called insideOIDCManager.Init(). When the NATSconfig.oidc.updatedevent fires andSubscribeReloadtriggersInit(), the issuer URL is read from credentials and passed directly tooidc.NewProviderwithout SSRF validation. If an attacker can write to the credentials file, set environment variables, or inject a NATS message onconfig.oidc.updated, they bypass all SSRF protections — the OIDC discovery fetch will reach private/loopback/metadata addresses.Suggested fix:
apps/event-engine/internal/dispatch/sanitize.go
[IMPORTANT] Line 50: The user-controlled
Bodycan contain the literal string</user_content>which would break out of the XML-style delimiter and allow injected text to appear outside the designated user content block. For example:This undermines the stated prompt injection defence.
Suggested fix: Escape or strip occurrences of the closing delimiter in
sanitizeBody:Alternatively, use a nonce-based delimiter that cannot be predicted by the attacker.
[IMPORTANT] (sanitize_test.go — missing test): No test exists for the closing-tag injection bypass.
TestBuildPrompt_InjectionAttemptContainedonly verifies malicious text appears between the tags but does not verify the body cannot break out of the delimiters.Suggested fix: Add a test case where
event.Bodycontains</user_content>and verify the output does not contain an unescaped closing tag within the body region.apps/gravity-pm/internal/handler/settings.go
[IMPORTANT] Lines 174-176:
UpdateAuthSettingspublishes the rawupdatesmap into the outbox event log viaevt.NewValues = updates. Sinceoidc_client_secretis in the allowed-fields list (line 139), a legitimate admin PATCH that sets the secret will persist the cleartext secret intopm_event_log.new_values. Anyone with read access to the event history endpoint could retrieve it.Suggested fix:
[IMPORTANT] Lines 46-58, 136-155: Both
UpdateSettingsandUpdateAuthSettingsuse inlinemap[string]boolallowlists for field names, but there is no validation on field values. A client can setoperation_modeto an arbitrary string like"banana",default_theme_idto a negative number, orsite_titleto a multi-megabyte string. The handler accepts these and passes them directly to the store.Suggested fix: Add value validation — at minimum, max string lengths and enum checks for known fields. This could live in a thin validation helper or the model package.
[IMPORTANT] Line 197:
UpdateAuthSettingscallsh.GetAuthSettings(w, r)to construct the response, which performs a second DB read. This is a TOCTOU race — the response may reflect different state than what was just saved. It also doubles the DB round-trips.Suggested fix: Construct the masked response inline from the
settingsvalue already returned byUpdateSettingsTx, matching the pattern used byGetSettings/UpdateSettings.apps/gravity-pm/internal/auth/issuer_url.go
[IMPORTANT] Lines 56-63:
isBlockedIPdoes not handle IPv4-mapped IPv6 addresses.isMetadataIPcompares againstnet.ParseIP("169.254.169.254")which produces a 4-byte IPv4 representation. If a DNS server returns::ffff:169.254.169.254,net.ParseIPproduces a 16-byte slice andnet.IP.Equalagainst the 4-byte form returns false, bypassing the metadata IP check.Suggested fix:
Add a test case for
https://[::ffff:169.254.169.254].HUMAN REVIEW: Lines 36-49: DNS-resolution-based SSRF checks are vulnerable to Time-of-Check-Time-of-Use (TOCTOU) via DNS rebinding.
ValidateIssuerURLresolves the hostname and checks IPs, butoidc.NewProviderresolves again independently. An attacker controlling DNS can return a safe IP during validation and a private IP during the subsequent request. Full mitigation requires a customhttp.Transportwith aDialContextthat re-validates resolved IPs at connect time. At minimum, document this as a known limitation.[IMPORTANT] (issuer_url_test.go — missing tests): Several important edge cases are missing: IPv6 loopback (
https://[::1]), IPv4-mapped IPv6 (https://[::ffff:169.254.169.254]), URLs with userinfo (https://evil@internal:8080), and exotic schemes (javascript:,data:).apps/event-engine/internal/dispatch/claude.go
[IMPORTANT] Line 42: On dispatch failure, full stderr is included in the returned error. If stderr contains internal paths, environment variables, or stack traces from the claude CLI, this propagates to the job result stored in the database and published to NATS.
Suggested fix: Log the full stderr server-side and return only a summary:
docker-compose.yml
[IMPORTANT] Lines 29, 55, 108, 124: gravity-pm, event-engine, Prometheus, and Grafana all bind ports to
0.0.0.0, while Valkey (line 76) and NATS (line 92) correctly bind to127.0.0.1. This is inconsistent — the application services and monitoring are exposed to all network interfaces.Suggested fix: Change to
127.0.0.1:${PORT}:portfor all four services unless external access is intentional, in which case document why..forgejo/workflows/ci.yml
[IMPORTANT] Line 33: The golangci-lint install script is fetched from the unpinned HEAD branch of the repository. While the binary version is pinned (
v2.10.1), the install script itself is not. A compromise of the repo's default branch could inject malicious code.Suggested fix: Pin the script URL to the release tag:
apps/gravity-pm/internal/handler/work_packages.go
[IMPORTANT] Lines 96-109:
CreateWorkPackagecontains business logic for defaulting Type, Status, and Priority. If another entry point (webhook, CLI, import) creates work packages, these defaults must be duplicated.Suggested fix: Move default-assignment logic into the store's
CreateWorkPackageTxor a model constructor (model.NewWorkPackage()).apps/gravity-pm/internal/handler/auth_wiring_test.go
[IMPORTANT] Lines 36-66: Auth wiring tests cover admin settings and theme routes but do NOT cover the admin-protected project deletion routes:
DELETE /projects/{projectID}andDELETE /projects/{projectID}/purge(handler.go lines 38-39).Suggested fix: Add entries to both
TestAdminRoutes_UnauthenticatedandTestAdminRoutes_NonAdmin:apps/gravity-pm/internal/web/setup.go
[IMPORTANT] Lines 109, 131-132, 166, 188: Database and OIDC error messages are passed directly to the rendered page (e.g.,
"failed to save settings: " + err.Error()). These can contain table names, constraint names, hostnames, or TLS certificate details.Suggested fix: Log full errors with
slog.Errorand render a generic message:"Failed to save settings. Check server logs for details."apps/gravity-pm/internal/store/store.go
[IMPORTANT] Lines 107-110:
scanUserhelper exists butGetUserandGetUserByExternalIDmanually inline the same 13-fieldScancall instead of using it. If a column is added topm_users, three locations must be updated in lockstep.Suggested fix: Refactor
GetUserandGetUserByExternalIDto usescanUser, matching the pattern already used inListUsers.apps/event-engine/internal/jobqueue/processor.go
HUMAN REVIEW: Lines 89-92, 174: When dispatch fails,
handleFailureincrements the DB retry count viaIncrementJobRetries, and then the returned error causes NATS to NAK the message for redelivery. This creates ambiguity about whether the DB retry count or NATS delivery count is the authoritative retry counter. The interaction between these two retry mechanisms needs clarification and possibly a single source of truth.apps/event-engine/internal/store/jobs.go
[IMPORTANT] Lines 199-220:
ReapStaleJobstransitions stale jobs to"failed"and increments retries, but does not check whetherretries >= max_retries. A job that has exhausted retries should be marked"dead"directly rather than cycling through the processor again.Suggested fix:
.env.example
HUMAN REVIEW: Lines 5-9: Required secrets all have the placeholder value
"changeme". While this is common for example files, users who copy.env.exampleto.envand forget to change values will run with well-known default secrets. Thedocker-compose.ymluses${VAR:?}syntax, but"changeme"satisfies that check. Consider leaving required values empty with generation instructions (e.g.,# REQUIRED — generate with: openssl rand -hex 32).Minor Findings (15 total — summarized)
ReadHeaderTimeout/IdleTimeoutonhttp.Server— Slowloris risklog.Fatalandslog— PR claims slog standardizationsanitizeFieldhas no length limit on "short identifiers"claimJobperforms two UPDATEs without a transaction — crash leaves orphaned "claimed" statejson.Marshalerror silently ignoredListWorkPackagesis 87 lines longUpdateWorkPackageallows TOCTOU racebuildUpdateSets(4 cases) — no tests for sorted ordering, startParam offset, quoting, Tx methods, or cascade logicqueryInthas no upper-bound —?limit=999999999hits the DB/storelacks trailing slash — could match/storefooVerifyBearerTokenandCallbackHandlerNitpick Findings (10 total — summarized)
setupRouterinstead ofmain()buildUpdateSetsdouble-quoting not documentedmap[string]boolinstead of idiomaticmap[string]struct{}gpm_oauth_vsgravity_session— inconsistent namingIsDevEnvironmentname implies env inspection but just does string comparisongenerateRandomHexininit()withAdminused to inject non-admin users — misleading nameint64(settings.ID)cast — AppSettings.ID should be int64 like all other modelsPR Review
Verdict: Acceptable
Updated review — previous review posted on 2026-02-23T05:59:24Z. This review was conducted from a fresh context against the current branch state (26 commits, 37 files, +1753/-406 lines). Focuses on findings not previously identified in the prior 1 review comment.
Findings
Test Results
Human Review Items
0 items flagged for human review.
Key Findings
UpdateAuthSettingsdoes not callValidateIssuerURL— settings.go:129DeleteProject/PurgeProjectread project state outside transaction — projects.go:99, 152PurgeProjectdoes not emit a domain event — projects.go:141(Grouped by file below)
apps/gravity-pm/internal/handler/settings.go
[IMPORTANT] Line 129:
UpdateAuthSettingsacceptsoidc_issuer_urlin the allowed fields map (line 137) but never callsauth.ValidateIssuerURL()before saving to the database. The setup wizard (setup.go:158) and the verify endpoint (settings.go:214) both validate, but the PATCH save path does not.The prior review stated "ValidateIssuerURL is enforced at the HTTP handler boundary (settings.go, setup.go)" — this is incorrect for
UpdateAuthSettings. While the immediate SSRF risk is limited becauseOIDCManager.Init()reads from the credentials file rather than the database, this is a defense-in-depth gap: an unvalidated URL is persisted topm_app_settingsand displayed in the admin UI. If any future code path readsoidc_issuer_urlfrom the database for HTTP requests, it becomes directly exploitable.Suggested fix:
apps/gravity-pm/internal/handler/projects.go
[IMPORTANT] Lines 99, 152: Both
DeleteProjectandPurgeProjectread the project viah.store.GetProject(ctx, id)from the connection pool outside the transaction. The transaction is opened later (line 110 / 163). Between the read and the transaction, the project can be modified or deleted by a concurrent request, causing the event payload at line 125 (evt.OldValues = structToValues(existing)) to contain stale state. Additionally,PurgeProjectat line 152 checks existence outside the transaction, then hard-deletes inside it — a concurrent soft-delete could cause the purge to silently succeed on a row the admin no longer expects.Compare with
UpdateWorkPackageandDeleteWorkPackageinwork_packages.gowhich correctly useGetWorkPackageForUpdateTxinside the transaction withFOR UPDATElocking.Suggested fix: Move the project read inside the transaction:
[IMPORTANT] Lines 141-184:
PurgeProjectpermanently hard-deletes a project and all its data viaON DELETE CASCADE, but does not publish any domain event.DeleteProject(soft delete) correctly publishes aProjectDeletedevent (line 124), but the irreversible purge operation has no audit trail in the event log. This means there is no record of data destruction for compliance or debugging purposes.Suggested fix:
apps/gravity-pm/internal/handler/work_packages.go
[MINOR] Lines 238-288:
DeleteWorkPackageproceeds to publish aWorkPackageDeletedevent even when the target work package is already soft-deleted.GetWorkPackageForUpdateTx(tx.go:72) returns(nil, nil)for deleted rows, but the handler only checkserr != nil(line 257). Whenoldisnil, the handler still callsDeleteWorkPackageTx(a no-op), then publishes an event withproject_id = 0(line 267, sinceprojectIDdefaults to zero). This produces a spurious event that could confuse downstream consumers.Suggested fix: Return 204 early when
oldis nil (WP not found or already deleted):apps/event-engine/internal/store/jobs.go
[MINOR] Line 312:
buildUpdateSetssilently drops columns not inallowedJobColumns. Compare with gravity-pm'sbuildUpdateSetsatstore.go:803which returns an error for unknown columns. This inconsistency means bugs in event-engine callers (passing a misspelled or wrong column name) would be silently ignored rather than surfaced.Suggested fix: Return an error for unknown columns, matching gravity-pm:
apps/gravity-pm/cmd/server/main.go
[MINOR] Line 246:
http.Serveris created with onlyAddrandHandler— missingReadHeaderTimeoutandIdleTimeout. This is the same issue identified in the prior review for event-engine (main.go:78). Both servers should set timeouts to mitigate Slowloris-style attacks.Suggested fix:
Previously Identified
47 findings from the prior review (2026-02-23) overlap with areas already identified. These are not repeated as new findings.
From prior review (5 blocking, 17 important, 15 minor, 10 nitpick, 3 human review):
</user_content>delimiter injection — sanitize.goSee the prior review for the full list.
Resolves PR#10 review findings across both review rounds. Blocking: - tx.go: add deleted_at IS NULL filter to UpdateVersionTx, UpdateWikiPageTx, UpdateThemeTx; empty-update fallback now reads via caller's tx - manager.go: validate OIDC issuer URL in Init() to prevent SSRF on hot-reload Important: - sanitize.go: escape </user_content> closing delimiter in body - settings.go: redact oidc_client_secret from event log, add ValidateIssuerURL to UpdateAuthSettings, eliminate TOCTOU in response construction - issuer_url.go: handle IPv4-mapped IPv6 in metadata IP check - claude.go: log stderr server-side, return generic error - projects.go: move project read inside tx with FOR UPDATE, emit audit event from PurgeProject - handler.go: fix chi route shadowing for DELETE /projects/{id} - auth_wiring_test.go: add DELETE project routes to test coverage - docker-compose.yml: bind all service ports to 127.0.0.1 - ci.yml: pin golangci-lint install script to release tag - jobs.go: ReapStaleJobs marks exhausted-retry jobs as dead directly - main.go (both): add ReadHeaderTimeout and IdleTimeout to http.Server CI pipeline: - settings_test.go: add missing allowHTTP arg to handler.New() - 001_jobs_table.sql: define update_updated_at() function for isolated CI OP#1773 OP#1766 OP#1767 OP#1768 OP#1769 OP#1770 OP#1771 OP#1772 OP#1774 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>PR Review Followup
Responding to reviews posted on 2026-02-23T05:59:24Z and 2026-02-23T06:45:23Z.
Triage Summary
Fixes Applied
All blocking and important findings resolved in commit
2fa79fe:Blocking:
AND deleted_at IS NULLto UpdateVersionTx, UpdateWikiPageTx, UpdateThemeTxtxinstead of pool-based methodsOIDCManager.Init()Important:
4. [IMPORTANT]
</user_content>delimiter injection — escape closing tag insanitizeBody, added testNewValues::ffff:169.254.169.254handling, added IPv6 and exotic scheme testspurged: trueCASE WHEN retries + 1 >= max_retries THEN 'dead' ELSE 'failed' ENDCI Pipeline:
17. settings_test.go: added missing
allowHTTParg tohandler.New()(lint + test-gravity-pm failure)18. event-engine migration: added
CREATE OR REPLACE FUNCTION update_updated_at()for isolated CI database (test-event-engine failure)- OP#1773: fix: CI pipeline — settings_test.go signature and event-engine migration
Deferred to v1.20
28 minor/nitpick items tracked in OP#1775 for the next version. Covers: mixed log.Fatal/slog, sanitizeField length limit, claimJob non-transactional updates, ListWorkPackages length, queryInt upper bound, CI grep pattern, duplicated claims extraction, missing test coverage, and 10 nitpick code style items.
Human Review Items (Unchanged)
These items require human judgment and were not auto-triaged:
"changeme"to empty values with generation instructionsNot Fixed (Deferred)
The following important findings were not auto-fixed and deferred to later work:
Test Results
This followup was generated by the pr-followup skill.
Implementation Complete — Ready for Merge
All v1.19.2 work packages are closed (34/34 tasks). This comment summarizes work done after the PR followup triage.
Commits Since Followup (8 commits)
bee9c22daf05c0b5a127bjob_statusenum in ReapStaleJobs1e5d1432972074actor_typeto prevent concurrent cleanup collisione1f487421eb010Bug Fixes
::job_statuscast. All 3 reaper tests now pass.actor_type='test'for cleanup. When CI runs packages concurrently, one package's cleanup deletes the other's test data. Fixed by namespacing store tests to'store-test'.Test Coverage Improvements
CI Coverage Gate
MIN_COVERAGEfrom 22% to 40%*_templ.gofiltering for gravity-pm — generated code inflated denominator without testable logicWork Packages Closed
Epic OP#1776 (test coverage, 21 SP):
Deferred items (OP#1769, OP#1771, OP#1772): Closed with completed items committed; deferred sub-items tracked as future tech debt.
Status