Skip to content

Rewrite most of template queries to sqlc - #1454

Merged
jakubno merged 26 commits into
mainfrom
sqlc-rewrite-most-of-template-queries
Nov 12, 2025
Merged

jakubno merged 26 commits into
mainfrom
sqlc-rewrite-most-of-template-queries

Conversation

@jakubno

@jakubno jakubno commented Nov 7, 2025 •

Copy link
Copy Markdown
Member

Note

Switch template CRUD/build logic to sqlc-backed queries and adopt CleanTemplateID, adding new SQL/generated code and removing legacy helpers.

  • API Handlers:
    • Switch to sqlc queries for template access/control in template_delete, template_update, deprecated_template_start_build, and v2 build start; update concurrent-build checks, snapshot existence, build updates, and template deletion via queries.*.
    • Use id.CleanTemplateID instead of CleanEnvID across handlers (deprecated_template_request_build, sandbox_create, template_delete, template_update).
    • Improve access checks by team and handle not-found/forbidden via dberrors and new query results.
  • Template Manager:
    • Replace ent calls with sqlc (GetTemplateBuildWithTemplate, FinishTemplateBuild, UpdateEnvBuildStatus) in build status sync and finish paths.
  • Template Build Registration:
    • Validate alias with CleanTemplateID.
  • DB/SQL:
    • Add sqlc queries and generated code for template/build ops: concurrent builds, get template/build(s) by id/alias, update build, finish build, delete template, exists snapshots, update template, build status updates, and additional fetchers.
    • Remove legacy ent-based helpers (packages/shared/pkg/db/envs.go).
  • Utilities:
    • Rename/standardize ID cleaner to CleanTemplateID.

Written by Cursor Bugbot for commit 7cdb35f. This will update automatically on new commits. Configure here.

@jakubno jakubno added the improvement Improvement for current functionality label Nov 7, 2025
Comment thread packages/api/internal/handlers/template_delete.go
Comment thread packages/api/internal/handlers/template_update.go Outdated
Comment thread packages/api/internal/handlers/template_delete.go
@claude

This comment was marked as resolved.

@claude

This comment was marked as resolved.

@claude

This comment was marked as outdated.

@claude

This comment was marked as resolved.

@claude

This comment was marked as outdated.

@claude

This comment was marked as outdated.

@claude

claude Bot commented Nov 10, 2025

Copy link
Copy Markdown

Security & Bug Issues

Critical: Missing team_id validation in GetTemplateByIdOrAlias

packages/db/queries/templates/get_template.sql:1-7

The query lacks team_id in the WHERE clause, allowing users to access templates from any team if they know the template ID or alias:

SELECT e.* FROM "public"."envs" e
LEFT JOIN "public"."env_aliases" ea ON ea.env_id = e.id
WHERE (
  e.id = @template_id_or_alias OR
  ea.alias = @template_id_or_alias
);

This is used in template_update.go:48 before checking team ownership. An attacker could:

  1. Query GetTemplateByIdOrAlias with another team's template ID
  2. Get the template details including the correct team_id
  3. The subsequent team check at line 70 would correctly fail, but the template data was already leaked

Fix: Add AND e.team_id = @team_id to the WHERE clause.


Missing security check in UpdateTemplateBuild

packages/db/queries/builds/update_template.sql:1-8

The query updates build fields without validating:

  • Team ownership
  • Template ownership
  • Build-to-template relationship
UPDATE "public"."env_builds"
SET start_cmd = @start_cmd, ready_cmd = @ready_cmd, dockerfile = @dockerfile
WHERE id = @build_uuid;

Used in template_start_build_v2.go:123-127 after only checking the template exists, but not verifying the build belongs to that template or team.

Fix: Add AND env_id = @template_id and validate template ownership separately.


Potential N+1 query performance issue

packages/db/queries/builds/get_template_builds_by_id_or_alias.sql:1-9

The LEFT JOIN with env_aliases without an index hint may cause full table scans when querying by alias. The OR condition in WHERE prevents efficient index usage:

WHERE e.team_id = @team_id AND (
    e.id = @template_id_or_alias OR
    ea.alias = @template_id_or_alias
)

For large teams with many templates, this could cause performance degradation.

Consider: Splitting into two queries or ensuring proper indexes on env_aliases.alias.


Race condition in FinishTemplateBuild

packages/db/queries/builds/finish_template_build.sql:1-11

The UPDATE has no transaction isolation check. If two processes call this simultaneously with different envd_version values, the last write wins without validation:

UPDATE "public"."env_builds"
SET finished_at = NOW(), total_disk_size_mb = @total_disk_size_mb, 
    status = 'uploaded', envd_version = @envd_version
WHERE id = @build_id AND env_id = @env_id;

Consider: Adding a status check AND status != 'uploaded' to prevent double-finishing.

@jakubno
jakubno marked this pull request as ready for review November 10, 2025 14:03
Comment thread packages/db/queries/builds/get_template_builds_by_id_or_alias.sql

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/api/internal/handlers/template_delete.go
Comment thread packages/db/queries/templates/update_template.sql
Comment thread packages/db/queries/builds/get_template_builds_by_id_or_alias.sql
Comment thread packages/api/internal/handlers/template_update.go Outdated
Comment thread packages/api/internal/handlers/template_update.go Outdated
Comment thread packages/api/internal/handlers/template_delete.go
Comment thread packages/api/internal/handlers/template_delete.go
@claude

This comment was marked as resolved.

@claude

This comment was marked as resolved.

Comment thread packages/db/queries/templates/get_template.sql
@jakubno
jakubno requested a review from sitole November 11, 2025 10:22
Comment thread packages/api/internal/handlers/template_update.go Outdated
@jakubno
jakubno requested a review from sitole November 11, 2025 16:52
Comment thread packages/api/internal/handlers/template_update.go

@sitole sitole left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fell free to merge it if 7cdb35f#r2517570525 is considered resolved.

@jakubno
jakubno merged commit 2dfec90 into main Nov 12, 2025
49 of 50 checks passed
@jakubno
jakubno deleted the sqlc-rewrite-most-of-template-queries branch November 12, 2025 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement for current functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants