The getTableCount method used fmt.Sprintf to interpolate a table name directly
into a SQL query. While currently only called with hardcoded names, this is a
dangerous pattern. Added an allowlist of valid table names and return an error
for unrecognized names.
Review: Validate table name against allowlist in getTableCount
Positives:
Correct fix for a SQL injection vector. Even though getTableCount is internal, defense in depth is the right call.
Package-level var for the allowlist is clean and easy to maintain.
Good error message includes the invalid name with %q formatting.
Zero performance overhead — map lookup is O(1).
Concerns:
Allowlist maintenance — If a new table is added to the schema, someone must remember to update validTableNames. Consider adding a comment near the schema/migration code pointing to this allowlist as well.
Still using fmt.Sprintf for the query — The table name is validated now, so this is safe, but a brief comment on the Sprintf line like // table name validated above would help future readers understand why this isn't a typical injection risk.
Could also use a switch statement — A switch with explicit cases would make the compiler enforce exhaustiveness if you ever move to an enum/const approach. Minor style preference though; the map is fine.
Clean, minimal, correct. LGTM.
## Review: Validate table name against allowlist in getTableCount
**Positives:**
- Correct fix for a SQL injection vector. Even though `getTableCount` is internal, defense in depth is the right call.
- Package-level `var` for the allowlist is clean and easy to maintain.
- Good error message includes the invalid name with `%q` formatting.
- Zero performance overhead — map lookup is O(1).
**Concerns:**
1. **Allowlist maintenance** — If a new table is added to the schema, someone must remember to update `validTableNames`. Consider adding a comment near the schema/migration code pointing to this allowlist as well.
2. **Still using `fmt.Sprintf` for the query** — The table name is validated now, so this is safe, but a brief comment on the `Sprintf` line like `// table name validated above` would help future readers understand why this isn't a typical injection risk.
3. **Could also use a `switch` statement** — A switch with explicit cases would make the compiler enforce exhaustiveness if you ever move to an enum/const approach. Minor style preference though; the map is fine.
Clean, minimal, correct. LGTM.
Is there a unit test for getTableCount with an invalid table name, verifying it returns the expected error? This is a security-relevant change and should have test coverage.
**Blocker: No evidence of passing tests or linting.**
Please confirm tests and lints pass. Also:
- Is there a unit test for `getTableCount` with an invalid table name, verifying it returns the expected error? This is a security-relevant change and should have test coverage.
i don't like this design. sanitize the table name (a-z0-9 should be fine) and then do the query. i don't want to maintain a separate allowlist, there's no need for it if you sanitize the table name correctly, right?
@clawbot
i don't like this design. sanitize the table name (a-z0-9 should be fine) and then do the query. i don't want to maintain a separate allowlist, there's no need for it if you sanitize the table name correctly, right?
Replace the hardcoded validTableNames allowlist with a regexp that
only allows [a-z0-9_] characters. This prevents SQL injection without
requiring maintenance of a separate allowlist when new tables are added.
Addresses review feedback from @sneak on PR #32.
Replace the hardcoded validTableNames allowlist with a regexp that
only allows [a-z0-9_] characters. This prevents SQL injection without
requiring maintenance of a separate allowlist when new tables are added.
Addresses review feedback from @sneak on PR #32.
@sneak Good call — replaced the allowlist with a simple regex: regexp.MustCompile(^[a-z0-9_]+$). Any table name not matching [a-z0-9_] is rejected. No allowlist to maintain.
The pre-existing build failures on this branch (missing printfStdout, FetchAndDecryptBlob, etc.) are from other PRs and unrelated to this change.
@sneak Good call — replaced the allowlist with a simple regex: `regexp.MustCompile(`^[a-z0-9_]+$`)`. Any table name not matching `[a-z0-9_]` is rejected. No allowlist to maintain.
The pre-existing build failures on this branch (missing `printfStdout`, `FetchAndDecryptBlob`, etc.) are from other PRs and unrelated to this change.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Review: Validate table name against allowlist in getTableCount
Positives:
getTableCountis internal, defense in depth is the right call.varfor the allowlist is clean and easy to maintain.%qformatting.Concerns:
Allowlist maintenance — If a new table is added to the schema, someone must remember to update
validTableNames. Consider adding a comment near the schema/migration code pointing to this allowlist as well.Still using
fmt.Sprintffor the query — The table name is validated now, so this is safe, but a brief comment on theSprintfline like// table name validated abovewould help future readers understand why this isn't a typical injection risk.Could also use a
switchstatement — A switch with explicit cases would make the compiler enforce exhaustiveness if you ever move to an enum/const approach. Minor style preference though; the map is fine.Clean, minimal, correct. LGTM.
Blocker: No evidence of passing tests or linting.
Please confirm tests and lints pass. Also:
getTableCountwith an invalid table name, verifying it returns the expected error? This is a security-relevant change and should have test coverage.@clawbot
i don't like this design. sanitize the table name (a-z0-9 should be fine) and then do the query. i don't want to maintain a separate allowlist, there's no need for it if you sanitize the table name correctly, right?
@sneak Good call — replaced the allowlist with a simple regex:
regexp.MustCompile(^[a-z0-9_]+$). Any table name not matching[a-z0-9_]is rejected. No allowlist to maintain.The pre-existing build failures on this branch (missing
printfStdout,FetchAndDecryptBlob, etc.) are from other PRs and unrelated to this change.