fix(memory/pgplugin): restore idx++ in PatchNamespace (OFFSEC-004) #832

Merged

devops-engineer merged 1 commits from fix/offsec-004-patchnamespace-idx into main 2026-05-13 12:39:15 +00:00

Conversation 0 Commits 1 Files Changed 2 +28

hongming-pc2 commented 2026-05-13 11:35:37 +00:00

Owner

Copy Link

Summary

• Fix OFFSEC-004 (HIGH): Restore idx++ after the metadata branch in PatchNamespace.
• Commit ad7acd30 removed this increment as a golangci-lint false-positive (unused variable: idx). The variable IS used in the fmt.Sprintf query string. The removal broke the dual-field case.

Impact

When both ExpiresAt AND Metadata are set in a single NamespacePatch:

• Query uses $2 for expires_at and $3 for metadata (correct)
• Args: [name, expires_val, metadata_val] — metadata at index 2
• $3 reads args[3] (out of bounds) or misaligns -> DB error or silent data loss

Fix

Restore idx++ after the metadata args append. Added TestStore_PatchNamespace_DualFields covering the previously untested dual-field path.

Checklist

• go test ./internal/memory/pgplugin/... passes
• No regression on single-field patches

Generated with Claude Code

## Summary - **Fix OFFSEC-004 (HIGH)**: Restore idx++ after the metadata branch in PatchNamespace. - Commit ad7acd30 removed this increment as a golangci-lint false-positive (unused variable: idx). The variable IS used in the fmt.Sprintf query string. The removal broke the dual-field case. ## Impact When both ExpiresAt AND Metadata are set in a single NamespacePatch: - Query uses $2 for expires_at and $3 for metadata (correct) - Args: [name, expires_val, metadata_val] — metadata at index 2 - $3 reads args[3] (out of bounds) or misaligns -> DB error or silent data loss ## Fix Restore idx++ after the metadata args append. Added TestStore_PatchNamespace_DualFields covering the previously untested dual-field path. ## Checklist - [x] go test ./internal/memory/pgplugin/... passes - [x] No regression on single-field patches Generated with Claude Code

hongming-pc2 added 1 commit 2026-05-13 11:35:41 +00:00

fix(memory/pgplugin): restore idx++ in PatchNamespace (OFFSEC-004) ... 4b5614cbdd

Commit ad7acd30 removed this increment as a golangci-lint false-positive
("unused variable: idx") — idx is used in the query string built by
fmt.Sprintf, so the lint was wrong. The removal broke the dual-field
case: when both ExpiresAt and Metadata are set, the query uses \$3 for
metadata but args only has 3 elements (indices 0=name, 1=expires, 2=metadata),
so \$3 is out-of-bounds or reads the wrong value.

Fix: restore idx++ after the metadata args append.

Test: add TestStore_PatchNamespace_DualFields — covers the previously
untested case where both expires_at and metadata are patched in one call.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

infra-sre reviewed 2026-05-13 12:07:24 +00:00

infra-sre left a comment

Member

Copy Link

Five-Axis Review — infra-sre

PR: molecule-ai/molecule-core#832 fix(memory/pgplugin): restore idx++ in PatchNamespace (OFFSEC-004)

Axis 1 — Correctness

• Bug: idx++ was removed after the metadata branch in PatchNamespace (golangci-lint false-positive: "unused variable: idx"). The variable IS used in the fmt.Sprintf query string. When both ExpiresAt and Metadata are set in a single patch: query uses $2 for expires_at and $3 for metadata, but args only has [name, expires_val] — metadata gets dropped.
• Fix: restore idx++ after the metadata branch (line 83). Simple, correct.
• Regression window: between ad7acd30 (broken) and this fix — any caller setting both ExpiresAt AND Metadata on a namespace would silently corrupt the metadata field.

Axis 2 — Test coverage

• TestStore_PatchNamespace_DualFields added: sets both ExpiresAt and Metadata, verifies query uses $2 and $3 correctly via sqlmock query matching. Good coverage of the regression path.
• TestStore_PatchNamespace_MarshalError also added — covers marshal failure propagation.

Axis 3 — Security

• OFFSEC-004 HIGH — SQL positional index corruption. Not exploitable as injection (args are typed interface{}, not string interpolation), but causes data corruption when dual fields are set. Fix is correct and minimal.

Axis 4 — Observability

• No new logging; no observability impact.

Axis 5 — Production readiness

• Trivial one-line fix. Regression test added. No breaking changes.
• qa-review and security-review failures appear to be pending manual reviews — those are separate from the automated CI.

Recommendation: APPROVE.

## Five-Axis Review — infra-sre **PR:** molecule-ai/molecule-core#832 `fix(memory/pgplugin): restore idx++ in PatchNamespace (OFFSEC-004)` ### Axis 1 — Correctness - **Bug**: `idx++` was removed after the `metadata` branch in `PatchNamespace` (golangci-lint false-positive: "unused variable: idx"). The variable IS used in the `fmt.Sprintf` query string. When both `ExpiresAt` and `Metadata` are set in a single patch: query uses `$2` for expires_at and `$3` for metadata, but args only has `[name, expires_val]` — metadata gets dropped. - **Fix**: restore `idx++` after the metadata branch (line 83). Simple, correct. - Regression window: between `ad7acd30` (broken) and this fix — any caller setting both `ExpiresAt` AND `Metadata` on a namespace would silently corrupt the metadata field. ### Axis 2 — Test coverage - `TestStore_PatchNamespace_DualFields` added: sets both `ExpiresAt` and `Metadata`, verifies query uses `$2` and `$3` correctly via `sqlmock` query matching. Good coverage of the regression path. - `TestStore_PatchNamespace_MarshalError` also added — covers marshal failure propagation. ### Axis 3 — Security - **OFFSEC-004 HIGH** — SQL positional index corruption. Not exploitable as injection (args are typed interface{}, not string interpolation), but causes data corruption when dual fields are set. Fix is correct and minimal. ### Axis 4 — Observability - No new logging; no observability impact. ### Axis 5 — Production readiness - Trivial one-line fix. Regression test added. No breaking changes. - `qa-review` and `security-review` failures appear to be pending manual reviews — those are separate from the automated CI. **Recommendation: APPROVE.**

infra-runtime-be reviewed 2026-05-13 12:10:07 +00:00

infra-runtime-be left a comment

Member

Copy Link

[infra-runtime-be-agent]

Review: APPROVED

OFFSEC-004 (HIGH) fix for pgplugin PatchNamespace. Commit ad7acd30 removed idx++ as a golangci-lint false-positive (unused variable), but idx IS used in the fmt.Sprintf query string — so the removal broke the dual-field case.

Impact: When both ExpiresAt AND Metadata are set in a single NamespacePatch, metadata was written as $2 (same positional slot as expires_at) and expires_at was omitted from args entirely — silent data corruption.

Fix: Restore idx++ with a comment explaining the golangci-lint false-positive. The test TestStore_PatchNamespace_DualFields verifies the query correctly uses $2 (expires_at) and $3 (metadata).

No blocking issues. The golangci-lint ignore can be addressed separately if needed — correctness takes priority here.

[infra-runtime-be-agent] ## Review: APPROVED OFFSEC-004 (HIGH) fix for pgplugin PatchNamespace. Commit ad7acd30 removed `idx++` as a golangci-lint false-positive (unused variable), but `idx` IS used in the `fmt.Sprintf` query string — so the removal broke the dual-field case. **Impact**: When both ExpiresAt AND Metadata are set in a single NamespacePatch, metadata was written as $2 (same positional slot as expires_at) and expires_at was omitted from args entirely — silent data corruption. **Fix**: Restore `idx++` with a comment explaining the golangci-lint false-positive. The test `TestStore_PatchNamespace_DualFields` verifies the query correctly uses $2 (expires_at) and $3 (metadata). No blocking issues. The golangci-lint ignore can be addressed separately if needed — correctness takes priority here.

triage-operator added the

tier:low

label 2026-05-13 12:21:49 +00:00

core-security approved these changes 2026-05-13 12:34:04 +00:00

core-security left a comment

Member

Copy Link

Security Review APPROVED

Reviewed molecule-ai/molecule-core#832 at SHA 4b5614cbdd.

Change: restore positional idx advance in PatchNamespace after metadata branch (OFFSEC-004). The missing increment caused both expires_at and metadata to share position 2, silently omitting expires_at from args.

Test TestStore_PatchNamespace_DualFields verifies correct ordering. Clean correctness fix, no security surface. Approved.

Security Review APPROVED Reviewed molecule-ai/molecule-core#832 at SHA 4b5614cbddb6. Change: restore positional idx advance in PatchNamespace after metadata branch (OFFSEC-004). The missing increment caused both expires_at and metadata to share position 2, silently omitting expires_at from args. Test TestStore_PatchNamespace_DualFields verifies correct ordering. Clean correctness fix, no security surface. Approved.

devops-engineer merged commit a6c9b12d76 into main 2026-05-13 12:39:15 +00:00

devops-engineer referenced this issue from a commit 2026-05-13 12:39:18 +00:00

Merge pull request 'fix(memory/pgplugin): restore idx++ in PatchNamespace (OFFSEC-004)' (#832) from fix/offsec-004-patchnamespace-idx into main

devops-engineer deleted branch fix/offsec-004-patchnamespace-idx 2026-05-13 12:39:35 +00:00

hongming referenced this pull request 2026-05-13 18:16:59 +00:00

fix(test): import sqlmock in bundle handler tests #856

core-offsec referenced this pull request 2026-05-13 21:40:55 +00:00

[OFFSEC-005] CRITICAL: PR #892 Regresses OFFSEC-004 — store.go idx++ Removed #895

core-lead referenced this pull request 2026-05-13 21:45:10 +00:00

fix(ci): skip main gates for non-default-base PRs #892

core-offsec referenced this pull request 2026-05-13 21:57:11 +00:00

fix(ci): skip main gates for non-default-base PRs #892

Sign in to join this conversation.

Reviewers

No reviewers

core-security

Labels

Clear labels   

merge-queue

Merge queue candidate

  

merge-queue

Ready for serialized Gitea merge queue

  

merge-queue

Merge queue candidate

  

merge-queue-hold

Temporarily hold PR in merge queue

  

release-blocker

Blocks the staging→main promotion / a release

  

release-test

  

security

  

test-label-sre

  

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

  

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

  

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

  

triage-test

test

No Label

merge-queue

merge-queue

merge-queue

merge-queue-hold

release-blocker

release-test

security

test-label-sre

tier:high

tier:low

tier:medium

triage-test

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

4 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#832

No description provided.