Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions .github/workflows/fuzz.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,45 @@ on:
workflow_dispatch: # Allow manual trigger

jobs:
fuzz:
name: Fuzz Tests
security-fuzz:
name: Security Fuzz
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
fuzz-target:
- FuzzPathValidation
- FuzzPathValidationWithSymlinks
- FuzzCommandValidation
- FuzzURLValidation
include:
# internal/security/ — core validators
- target: FuzzPathValidation
package: ./internal/security/
- target: FuzzPathValidationWithSymlinks
package: ./internal/security/
- target: FuzzCommandValidation
package: ./internal/security/
- target: FuzzURLValidation
package: ./internal/security/
- target: FuzzSafeDialContext
package: ./internal/security/
# internal/tools/ — tool-level security
- target: FuzzPathTraversal
package: ./internal/tools/
- target: FuzzSSRFBypass
package: ./internal/tools/
- target: FuzzCommandInjection
package: ./internal/tools/
- target: FuzzContainsInjection
package: ./internal/tools/
- target: FuzzEnvVarBypass
package: ./internal/tools/
# internal/memory/ — secret detection
- target: FuzzContainsSecrets
package: ./internal/memory/
# internal/api/ — auth/CSRF
- target: FuzzCheckCSRF
package: ./internal/api/
- target: FuzzCheckPreSessionCSRF
package: ./internal/api/
- target: FuzzVerifySignedUID
package: ./internal/api/
steps:
- name: Checkout code
uses: actions/checkout@v4
Expand All @@ -27,5 +55,5 @@ jobs:
with:
go-version: "1.25"

- name: Run ${{ matrix.fuzz-target }}
run: go test -fuzz=${{ matrix.fuzz-target }} -fuzztime=30s ./internal/security/
- name: Run ${{ matrix.target }}
run: go test -fuzz=${{ matrix.target }} -fuzztime=30s ${{ matrix.package }}
41 changes: 41 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
.PHONY: build vet lint test test-race test-integration test-fuzz verify clean

# Build binary
build:
go build -o koopa ./

# Static analysis
vet:
go vet ./...

# Lint (matches CI: golangci-lint v2.7.1)
lint:
golangci-lint run ./...

# Unit tests (fast, no database required)
test:
go test -short ./...

# Unit tests with race detector (matches CI)
test-race:
go test -short -race ./...

# Integration tests (requires PostgreSQL with pgvector)
test-integration:
go test -tags=integration -race -timeout 15m ./...

# Run security fuzz targets for 30s each
test-fuzz:
go test -fuzz=FuzzPathValidation -fuzztime=30s ./internal/security/
go test -fuzz=FuzzCommandValidation -fuzztime=30s ./internal/security/
go test -fuzz=FuzzURLValidation -fuzztime=30s ./internal/security/
go test -fuzz=FuzzSafeDialContext -fuzztime=30s ./internal/security/

# Full verification chain (matches /verify skill)
# Stop at first failure.
verify: build vet lint test-race

# Remove build artifacts
clean:
rm -f koopa
go clean -testcache
6 changes: 4 additions & 2 deletions cmd/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,16 @@ func runServe() error {

flow := chat.NewFlow(a.Genkit, agent)

apiServer, err := api.NewServer(api.ServerConfig{
apiServer, err := api.NewServer(ctx, api.ServerConfig{
Logger: logger,
ChatAgent: agent,
ChatFlow: flow,
SessionStore: a.SessionStore,
MemoryStore: a.MemoryStore,
Pool: a.DBPool,
CSRFSecret: []byte(cfg.HMACSecret),

Choose a reason for hiding this comment

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

Security: CSRF Secret Handling

The CSRF secret is set as []byte(cfg.HMACSecret). If cfg.HMACSecret is not validated for sufficient entropy and length, this could compromise CSRF protection. Ensure that cfg.HMACSecret is at least 32 bytes of cryptographically secure random data. If not, add validation logic in the configuration loading or validation phase to enforce this requirement.

Recommended solution:

if len(cfg.HMACSecret) < 32 {
    return fmt.Errorf("HMACSecret must be at least 32 bytes for CSRF protection")
}

CORSOrigins: cfg.CORSOrigins,
IsDev: cfg.PostgresSSLMode == "disable",
IsDev: cfg.DevMode,
TrustProxy: cfg.TrustProxy,
RateBurst: parseRateBurst(),
})
Expand Down
18 changes: 9 additions & 9 deletions db/migrations/000001_init_schema.up.sql
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ CREATE TABLE IF NOT EXISTS messages (
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),

CONSTRAINT unique_message_sequence UNIQUE (session_id, sequence_number),
CONSTRAINT message_role_check CHECK (role IN ('user', 'assistant', 'system', 'tool'))
CONSTRAINT message_role_check CHECK (role IN ('user', 'assistant', 'system', 'tool', 'model'))
);

-- ============================================================================
Expand Down Expand Up @@ -87,27 +87,27 @@ CREATE TABLE IF NOT EXISTS memories (
GENERATED ALWAYS AS (to_tsvector('english', content)) STORED
);

CREATE INDEX idx_memories_embedding ON memories
CREATE INDEX IF NOT EXISTS idx_memories_embedding ON memories
USING hnsw (embedding vector_cosine_ops)
WITH (m = 16, ef_construction = 64);

CREATE INDEX idx_memories_owner ON memories(owner_id);
CREATE INDEX IF NOT EXISTS idx_memories_owner ON memories(owner_id);

CREATE INDEX idx_memories_owner_active_category
CREATE INDEX IF NOT EXISTS idx_memories_owner_active_category
ON memories(owner_id, active, category);

CREATE UNIQUE INDEX idx_memories_owner_content_unique
CREATE UNIQUE INDEX IF NOT EXISTS idx_memories_owner_content_unique
ON memories(owner_id, md5(content)) WHERE active = true;
Comment on lines +99 to 100

Choose a reason for hiding this comment

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

Potential collision risk with md5(content) for uniqueness

The unique index on md5(content) (lines 99-100) is used to enforce deduplication of active memories per owner. However, MD5 is not collision-resistant, and while collisions are rare, they are possible. For critical deduplication, consider using a stronger hash function such as SHA256, which is available in PostgreSQL via the pgcrypto extension:

CREATE UNIQUE INDEX IF NOT EXISTS idx_memories_owner_content_unique
ON memories(owner_id, encode(digest(content, 'sha256'), 'hex')) WHERE active = true;

This reduces the risk of accidental duplicates due to hash collisions.


CREATE INDEX idx_memories_search_text ON memories USING gin (search_text);
CREATE INDEX IF NOT EXISTS idx_memories_search_text ON memories USING gin (search_text);

CREATE INDEX idx_memories_decay_candidates
CREATE INDEX IF NOT EXISTS idx_memories_decay_candidates
ON memories (owner_id, updated_at)
WHERE active = true AND superseded_by IS NULL;

CREATE INDEX idx_memories_superseded_by ON memories (superseded_by)
CREATE INDEX IF NOT EXISTS idx_memories_superseded_by ON memories (superseded_by)
WHERE superseded_by IS NOT NULL;

CREATE INDEX idx_memories_expires_at
CREATE INDEX IF NOT EXISTS idx_memories_expires_at
ON memories (expires_at)
WHERE expires_at IS NOT NULL AND active = true;
3 changes: 3 additions & 0 deletions db/migrations/000002_messages_fts.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
DROP INDEX IF EXISTS idx_messages_search_text;
ALTER TABLE messages DROP COLUMN IF EXISTS search_text;
ALTER TABLE messages DROP COLUMN IF EXISTS text_content;
Comment on lines +2 to +3

Choose a reason for hiding this comment

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

Irreversible Data Loss Risk: Dropping the search_text and text_content columns will permanently remove all data stored in them. Ensure that this migration is only run when data loss is acceptable, or consider backing up the data before executing.

Recommended Solution: Add a warning or backup step before running this migration, or wrap the migration in a transaction to ensure atomicity.

Comment on lines +1 to +3

Choose a reason for hiding this comment

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

Potential Dependency Conflicts: Dropping columns and indexes without checking for dependencies (such as triggers, views, or application code) may cause runtime errors or inconsistencies.

Recommended Solution: Verify that no other database objects or application logic depend on these columns or index before executing the migration.

19 changes: 19 additions & 0 deletions db/migrations/000002_messages_fts.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-- Add text_content for full-text search on messages.
-- content is JSONB ([]*ai.Part), not directly searchable.
-- text_content is application-maintained, populated in AddMessage.
ALTER TABLE messages ADD COLUMN IF NOT EXISTS text_content TEXT;

-- Generated tsvector for FTS.
-- to_tsvector handles NULL natively (returns empty tsvector), no COALESCE needed.
ALTER TABLE messages ADD COLUMN IF NOT EXISTS search_text tsvector
GENERATED ALWAYS AS (to_tsvector('english', text_content)) STORED;

-- GIN index for fast full-text search.
CREATE INDEX IF NOT EXISTS idx_messages_search_text ON messages USING gin(search_text);

-- Backfill existing messages: extract text from JSONB parts.
UPDATE messages SET text_content = (
SELECT string_agg(elem->>'text', ' ')
FROM jsonb_array_elements(content) AS elem
WHERE elem->>'text' IS NOT NULL AND elem->>'text' != ''
) WHERE text_content IS NULL;
1 change: 1 addition & 0 deletions db/migrations/000003_messages_session_idx.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP INDEX IF EXISTS idx_messages_session_id;
4 changes: 4 additions & 0 deletions db/migrations/000003_messages_session_idx.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- Add index on messages.session_id for JOIN performance.
-- PostgreSQL does NOT auto-create indexes on FK referencing columns.
-- SearchMessages and CountMessages both JOIN on m.session_id = s.id.
CREATE INDEX IF NOT EXISTS idx_messages_session_id ON messages(session_id);
3 changes: 3 additions & 0 deletions db/migrations/000004_trigram_search.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
DROP INDEX IF EXISTS idx_memories_content_trgm;
DROP INDEX IF EXISTS idx_messages_text_content_trgm;
-- Do not drop pg_trgm extension; other schemas may use it.
17 changes: 17 additions & 0 deletions db/migrations/000004_trigram_search.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
-- Enable pg_trgm extension for trigram-based text search (CJK support).
CREATE EXTENSION IF NOT EXISTS pg_trgm;

-- CONCURRENTLY avoids locking the table during index creation.
-- NOTE: CONCURRENTLY cannot run inside a transaction block.
-- golang-migrate runs each file in a transaction by default;
-- the operator must run this migration manually with:
-- psql -f 000004_trigram_search.up.sql
-- or disable transactions in the migration tool.
Comment on lines +5 to +9

Choose a reason for hiding this comment

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

Migration Execution Risk:
The migration requires manual execution due to the use of CONCURRENTLY, which cannot run inside a transaction block. If the operator overlooks this requirement, the migration will fail, potentially leaving the database in an inconsistent state.

Recommendation:
Consider splitting the migration into two files: one for enabling the extension (which can run in a transaction), and another for index creation (to be run manually or with transaction disabled). Alternatively, provide automated checks or scripts to ensure proper execution.


-- GIN trigram index on messages.text_content for ILIKE fallback search.
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_messages_text_content_trgm
ON messages USING gin (text_content gin_trgm_ops);

-- GIN trigram index on memories.content for similarity() scoring.
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_memories_content_trgm
ON memories USING gin (content gin_trgm_ops);
Comment on lines +12 to +17

Choose a reason for hiding this comment

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

Missing Existence Checks:
The migration does not check for the existence of the messages and memories tables or their respective columns before attempting to create indexes. If these tables or columns are missing or renamed, the migration will fail.

Recommendation:
Add conditional logic or pre-checks to ensure the tables and columns exist before index creation, or document the dependency clearly to prevent errors.

8 changes: 4 additions & 4 deletions db/queries/sessions.sql
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
-- name: CreateSession :one
INSERT INTO sessions (title, owner_id)
VALUES ($1, sqlc.arg(owner_id))
Comment on lines 5 to 6

Choose a reason for hiding this comment

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

Inconsistent parameter usage in CreateSession query

The CreateSession query mixes positional parameters ($1) and named parameters (sqlc.arg(owner_id)). This inconsistency can lead to confusion or runtime errors, especially with code generation tools like sqlc that expect consistent parameter notation. For maintainability and to avoid subtle bugs, use either all positional or all named parameters. For example:

INSERT INTO sessions (title, owner_id)
VALUES (sqlc.arg(title), sqlc.arg(owner_id))
RETURNING id, title, owner_id, created_at, updated_at;

Or, if using positional parameters, ensure all are positional.

RETURNING *;
RETURNING id, title, owner_id, created_at, updated_at;

-- name: Session :one
SELECT id, title, owner_id, created_at, updated_at
Expand All @@ -26,7 +26,7 @@ SELECT id, title, owner_id, created_at, updated_at
FROM sessions
WHERE id = sqlc.arg(session_id) AND owner_id = sqlc.arg(owner_id);

-- name: UpdateSessionUpdatedAt :exec
-- name: UpdateSessionUpdatedAt :execrows
UPDATE sessions
SET updated_at = NOW()
WHERE id = sqlc.arg(session_id);
Expand All @@ -44,8 +44,8 @@ WHERE id = $1;

-- name: AddMessage :exec
-- Add a message to a session
INSERT INTO messages (session_id, role, content, sequence_number)
VALUES ($1, $2, $3, $4);
INSERT INTO messages (session_id, role, content, sequence_number, text_content)
VALUES ($1, $2, $3, $4, $5);

-- name: Messages :many
-- Get all messages for a session ordered by sequence
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ require (
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.38.0
go.opentelemetry.io/otel/sdk v1.38.0
go.uber.org/goleak v1.3.0
golang.org/x/net v0.47.0
golang.org/x/time v0.14.0
google.golang.org/genai v1.41.0
)
Expand Down Expand Up @@ -168,7 +169,6 @@ require (
go.yaml.in/yaml/v3 v3.0.4 // indirect
golang.org/x/crypto v0.45.0 // indirect
golang.org/x/exp v0.0.0-20251023183803-a4bb9ffd2546 // indirect
golang.org/x/net v0.47.0 // indirect
golang.org/x/oauth2 v0.33.0 // indirect
golang.org/x/sync v0.18.0 // indirect
golang.org/x/sys v0.38.0 // indirect
Expand Down
Loading
Loading