Conversation
… actions - Prevent XSS by sanitizing Content-Type in API gateway responses - Add SQL identifier validation in shared ResourceStore constructor - Run containers as non-root user in Dockerfile and Dockerfile.dev - Pin all GitHub Actions to commit SHAs in cd, lint, release-drafter, smithy-sync - Bump Next.js from 16.2.3 to 16.2.4
Contributor
Reviewer's GuideThis PR hardens security across the service by validating SQL identifiers in the shared store, sanitizing API gateway Content-Type headers to avoid serving HTML, running containers as a non-root user with a guarded entrypoint, pinning GitHub Actions to commit SHAs, and updating Next.js to a patched version. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
dependencies
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
validateIdentifierhelper panics on invalid input and rejects anything beyond[a-zA-Z0-9_]; consider returning an error fromNewResourceStore(or at least narrowing its visibility) so unexpected input doesn’t crash the process and to allow for more flexible identifier formats if needed (e.g.,.for schemas). - In
ServiceRouter.ServeHTTP, it might be safer tostrings.TrimSpacethe computedContent-Typebefore checkingstrings.HasPrefix(strings.ToLower(ct), "text/html")to avoid bypasses via leading whitespace. - The Next.js dependency was changed from an exact version to a caret range (
16.2.3→^16.2.4), which will allow future minor/patch upgrades; if the intent is a tightly controlled security bump, consider keeping an exact version pin instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `validateIdentifier` helper panics on invalid input and rejects anything beyond `[a-zA-Z0-9_]`; consider returning an error from `NewResourceStore` (or at least narrowing its visibility) so unexpected input doesn’t crash the process and to allow for more flexible identifier formats if needed (e.g., `.` for schemas).
- In `ServiceRouter.ServeHTTP`, it might be safer to `strings.TrimSpace` the computed `Content-Type` before checking `strings.HasPrefix(strings.ToLower(ct), "text/html")` to avoid bypasses via leading whitespace.
- The Next.js dependency was changed from an exact version to a caret range (`16.2.3` → `^16.2.4`), which will allow future minor/patch upgrades; if the intent is a tightly controlled security bump, consider keeping an exact version pin instead.
## Individual Comments
### Comment 1
<location path="web/package.json" line_range="17" />
<code_context>
"clsx": "^2.1.1",
"lucide-react": "^1.7.0",
- "next": "16.2.3",
+ "next": "^16.2.4",
"react": "19.2.4",
"react-dom": "19.2.4",
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a caret range for Next may introduce unanticipated upgrades
Switching from a pinned `16.2.3` to `^16.2.4` means any `16.x` release can be installed, including via local installs or partial lockfile updates, which can introduce unexpected behavior in a core framework like Next. If you prefer stability over automatic minor/patch updates, consider keeping Next pinned and updating it manually instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Change validateIdentifier from panic to returning error in NewResourceStore - Add TrimSpace to Content-Type before HTML check to prevent whitespace bypass - Revert Next.js to exact version pin (16.2.4) instead of caret range
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ServiceRouter.ServeHTTP, consider usingmime.ParseMediaTypeto parse and compare the media type instead ofstrings.HasPrefixon the raw header value so thattext/htmldetection is robust to parameters and odd casing/spacing without accidentally matching types liketext/htmlx. - The
validateIdentifierlogic will fail ifcolscontains a trailing comma or patterns like*that some SQL helpers use; it may be worth explicitly documenting/guarding these cases or normalizingcols(e.g., filtering out empty segments) to avoid unexpected runtime errors for existing callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ServiceRouter.ServeHTTP`, consider using `mime.ParseMediaType` to parse and compare the media type instead of `strings.HasPrefix` on the raw header value so that `text/html` detection is robust to parameters and odd casing/spacing without accidentally matching types like `text/htmlx`.
- The `validateIdentifier` logic will fail if `cols` contains a trailing comma or patterns like `*` that some SQL helpers use; it may be worth explicitly documenting/guarding these cases or normalizing `cols` (e.g., filtering out empty segments) to avoid unexpected runtime errors for existing callers.
## Individual Comments
### Comment 1
<location path="docker/Dockerfile" line_range="24-27" />
<code_context>
# Stage 3: Runtime
FROM alpine:3.20
RUN apk add --no-cache sqlite-libs ca-certificates
+RUN adduser -D -H -h /app appuser
WORKDIR /app
COPY --from=go-builder /devcloud /app/devcloud
</code_context>
<issue_to_address>
**issue:** Non-root user may lack write permissions for `/app/data` at runtime.
Because the container now runs as `appuser`, only files owned at build time are guaranteed to be writable. Volumes declared with `VOLUME /app/data` are created at runtime and usually owned by root, so `appuser` may not be able to write there. If the app writes to `/app/data`, either handle `chown`/`chmod` for that path in an entrypoint/init step, or document that the volume must be created with appropriate ownership/permissions when running the container.
</issue_to_address>
### Comment 2
<location path="internal/shared/store_test.go" line_range="41-43" />
<code_context>
require.NoError(t, err)
t.Cleanup(func() { _ = db.Close() })
- return NewResourceStore[testItem](db, "items", "id", "id, name", testScanner)
+ rs, err := NewResourceStore[testItem](db, "items", "id", "id, name", testScanner)
+ require.NoError(t, err)
+ return rs
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative tests for `NewResourceStore` alongside this positive path.
To better cover the hardening, please also add tests that call `NewResourceStore` with invalid identifiers and assert that an error is returned (and, if practical, that the error message is as expected). Keeping these alongside the happy-path tests will ensure the constructor’s failure modes stay well covered.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Use mime.ParseMediaType for robust text/html detection in gateway - Add entrypoint.sh to handle /app/data volume permissions at runtime - Skip empty segments in validateIdentifier to tolerate trailing commas - Add negative tests for NewResourceStore with invalid identifiers
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the runtime Dockerfile, the
COPYlines fordevcloud,codegen,web/out,devcloud.yaml,smithy-models, andtemplatesare duplicated; consider removing the duplicates to avoid confusion and unnecessary rebuild work. - The container now runs as
appuser, butdocker/entrypoint.shcallschown appuser:appuser /app/data, which will fail when executed as a non-root user; you may want to either perform thechownin the Dockerfile as root or drop thechownfrom the entrypoint.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the runtime Dockerfile, the `COPY` lines for `devcloud`, `codegen`, `web/out`, `devcloud.yaml`, `smithy-models`, and `templates` are duplicated; consider removing the duplicates to avoid confusion and unnecessary rebuild work.
- The container now runs as `appuser`, but `docker/entrypoint.sh` calls `chown appuser:appuser /app/data`, which will fail when executed as a non-root user; you may want to either perform the `chown` in the Dockerfile as root or drop the `chown` from the entrypoint.
## Individual Comments
### Comment 1
<location path="docker/Dockerfile" line_range="26-27" />
<code_context>
RUN apk add --no-cache sqlite-libs ca-certificates
+RUN adduser -D -H -h /app appuser
WORKDIR /app
COPY --from=go-builder /devcloud /app/devcloud
COPY --from=go-builder /codegen /app/codegen
</code_context>
<issue_to_address>
**issue:** The COPY instructions for binaries, config, and assets are duplicated and can be consolidated.
The block copying `/devcloud`, `/codegen`, `web/out`, `devcloud.yaml`, `smithy-models/`, and `templates/` is repeated in the final stage, which unnecessarily increases image size and complicates maintenance. Unless there is a deliberate layering requirement, you can remove the duplicate COPY block and keep a single instance.
</issue_to_address>
### Comment 2
<location path="docker/entrypoint.sh" line_range="6" />
<code_context>
+
+# Ensure the data directory exists and is writable by appuser.
+mkdir -p /app/data
+chown appuser:appuser /app/data
+
+exec "$@"
</code_context>
<issue_to_address>
**issue (bug_risk):** Running `chown` as the unprivileged `appuser` will likely fail, especially with bind mounts.
Since the Dockerfile switches to `USER appuser`, this `chown` runs as a non-root user and will usually fail with `EPERM` (especially when `/app/data` is a root-owned bind mount). With `set -e` enabled, that will terminate the container at startup. You could either move this `chown` into the Dockerfile before dropping privileges, run the entrypoint as root and then `su-exec`/`gosu` to `appuser`, or drop the `chown` and rely on the existing `/app` ownership if that’s sufficient.
</issue_to_address>
### Comment 3
<location path="internal/shared/store_test.go" line_range="149-153" />
<code_context>
+ assert.ErrorContains(t, err, "empty table identifier")
+}
+
+func TestNewResourceStore_TrailingComma(t *testing.T) {
+ db := newTestDB(t)
+ rs, err := NewResourceStore[testItem](db, "items", "id", "id, name,", testScanner)
+ require.NoError(t, err)
+ require.NotNil(t, rs)
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider tests for valid identifiers (underscores/digits) to guard against over-restrictive changes
Current tests only cover rejection of invalid identifiers. Please also add a few positive cases (e.g. `items_v2`, `item2`, `user_profile_01`) to confirm underscores and digits remain accepted and to catch regressions if `validateIdentifier` is tightened in the future.
Suggested implementation:
```golang
func TestNewResourceStore_EmptyTable(t *testing.T) {
db := newTestDB(t)
_, err := NewResourceStore[testItem](db, "", "id", "id", testScanner)
assert.ErrorContains(t, err, "empty table identifier")
}
func TestNewResourceStore_ValidIdentifiers(t *testing.T) {
db := newTestDB(t)
tests := []struct {
name string
tableName string
primary string
cols string
}{
{
name: "table_with_version_suffix",
tableName: "items_v2",
primary: "id1",
cols: "id1, name, created_at",
},
{
name: "table_with_digit_suffix",
tableName: "item2",
primary: "id2",
cols: "id2, description, updated_at",
},
{
name: "table_with_underscores_and_digits",
tableName: "user_profile_01",
primary: "user_id_01",
cols: "user_id_01, profile_name, last_login_2",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
rs, err := NewResourceStore[testItem](db, tc.tableName, tc.primary, tc.cols, testScanner)
require.NoError(t, err)
require.NotNil(t, rs)
})
}
}
func TestResourceStore_GetNotFound(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, 1, n)
}
```
If `NewResourceStore` validates table/column identifiers against the actual database schema (rather than just using `validateIdentifier`), you may need to ensure the test migrations in `testMigrations` create tables/columns matching the names used in the new test cases. In that case, either:
1) Add corresponding tables/columns (`items_v2`, `item2`, `user_profile_01`, etc.) to `testMigrations`, or
2) Adjust the test case identifiers to match whatever tables/columns already exist in your test schema while still covering underscores and digits (e.g. `items_v2`, `items2`, `user_profile_01` using existing columns).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Remove duplicate COPY instructions in Dockerfile - Fix entrypoint to run as root with su-exec for privilege drop - Add positive identifier validation tests (underscores, digits, trailing comma)
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
validateIdentifierlogic only permits ASCII letters/digits/underscores, which will reject otherwise valid SQL identifiers (e.g., schema-qualified names likeschema.table, quoted names, or names with dashes); if those are possible now or in the future, consider broadening the allowed syntax or documenting the constraint at the call sites. - By unconditionally normalizing
text/htmlresponses totext/plainin the gateway, any API endpoints that intentionally return HTML (e.g., human-facing error pages or debug views) will now be forced to plain text; consider adding an explicit opt-out or path/hostname-based allowlist for trusted HTML responses if that use case exists. - In the Docker image,
/appis chowned recursively at build time and/app/datais chowned again on container start; if the directory tree grows large or is a mounted volume this double chown could become expensive, so you might restrict ownership changes to just the data directory or rely solely on build-time ownership where possible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `validateIdentifier` logic only permits ASCII letters/digits/underscores, which will reject otherwise valid SQL identifiers (e.g., schema-qualified names like `schema.table`, quoted names, or names with dashes); if those are possible now or in the future, consider broadening the allowed syntax or documenting the constraint at the call sites.
- By unconditionally normalizing `text/html` responses to `text/plain` in the gateway, any API endpoints that intentionally return HTML (e.g., human-facing error pages or debug views) will now be forced to plain text; consider adding an explicit opt-out or path/hostname-based allowlist for trusted HTML responses if that use case exists.
- In the Docker image, `/app` is chowned recursively at build time and `/app/data` is chowned again on container start; if the directory tree grows large or is a mounted volume this double chown could become expensive, so you might restrict ownership changes to just the data directory or rely solely on build-time ownership where possible.
## Individual Comments
### Comment 1
<location path="internal/gateway/router.go" line_range="56-58" />
<code_context>
+ if ct == "" {
+ ct = "application/octet-stream"
+ }
+ // Prevent XSS: never serve API responses as text/html.
+ ct = strings.TrimSpace(ct)
+ if mediaType, _, parseErr := mime.ParseMediaType(ct); parseErr == nil && strings.EqualFold(mediaType, "text/html") {
+ ct = "text/plain; charset=utf-8"
+ }
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Content-Type XSS protection can be bypassed if upstream sets a comma-separated list of media types.
This guard only works when `mime.ParseMediaType(ct)` succeeds. If an upstream sets something like `"text/html; charset=utf-8, text/plain"`, parsing will fail and the fallback won’t apply, while a browser may still treat the response as HTML based on the first type. To harden this, consider splitting on `","` first and running the `ParseMediaType` / `text/html` check on the first (or all) entries, then normalizing to a safe value if any resolve to HTML-like types.
Suggested implementation:
```golang
}
ct := resp.ContentType
if ct == "" {
ct = w.Header().Get("Content-Type")
}
if ct == "" {
ct = "application/octet-stream"
}
// Prevent XSS: never serve API responses as text/html.
// Harden against comma-separated lists of media types by inspecting each entry.
ct = strings.TrimSpace(ct)
if ct != "" {
parts := strings.Split(ct, ",")
htmlLike := false
for _, p := range parts {
p = strings.TrimSpace(p)
if p == "" {
continue
}
if mediaType, _, err := mime.ParseMediaType(p); err == nil && strings.EqualFold(mediaType, "text/html") {
htmlLike = true
break
}
}
if htmlLike {
// Normalize any HTML-like content types to a safe, non-HTML type.
ct = "text/plain; charset=utf-8"
} else {
// Normalize Content-Type to the first media type.
ct = strings.TrimSpace(parts[0])
}
}
```
To compile, you also need to:
1. Ensure `internal/gateway/router.go` imports the packages:
```go
import (
"mime"
"strings"
// existing imports...
)
```
If there is already an `import (` block, add `"mime"` and `"strings"` inside it without duplicating existing imports.
2. If this function later uses `ct` to set the `Content-Type` header or to write the response, leave that logic unchanged so it benefits from the stricter normalization.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Split on commas and inspect each entry individually so that values like "text/html, text/plain" cannot bypass the HTML sanitization.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
NewResourceStore/validateIdentifier,tableandidColare validated without trimming whitespace (unlikecols), so a config value with leading/trailing spaces will be rejected rather than normalized; considerstrings.TrimSpaceon those inputs before validation to align behavior and avoid surprising failures. - The
docker/entrypoint.shunconditionallychowns/app/dataon every container start, which can be slow on large or mounted volumes; you might gate this on current ownership (e.g.,if [ "$(stat -c %U /app/data)" != "appuser" ]; then chown ...; fi) or adjust the volume/ Dockerfile so it’s only needed once.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `NewResourceStore`/`validateIdentifier`, `table` and `idCol` are validated without trimming whitespace (unlike `cols`), so a config value with leading/trailing spaces will be rejected rather than normalized; consider `strings.TrimSpace` on those inputs before validation to align behavior and avoid surprising failures.
- The `docker/entrypoint.sh` unconditionally `chown`s `/app/data` on every container start, which can be slow on large or mounted volumes; you might gate this on current ownership (e.g., `if [ "$(stat -c %U /app/data)" != "appuser" ]; then chown ...; fi`) or adjust the volume/ Dockerfile so it’s only needed once.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…t chown - TrimSpace table and idCol in NewResourceStore to match cols behavior - Only chown /app/data if not already owned by appuser
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docker/entrypoint.sh" line_range="6" />
<code_context>
+
+# Ensure the data directory exists and is writable by appuser.
+mkdir -p /app/data
+if [ "$(stat -c %U /app/data 2>/dev/null)" != "appuser" ]; then
+ chown appuser:appuser /app/data
+fi
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `set -e` with `stat` in command substitution can cause unexpected exits.
With `set -e` enabled, a non‑zero exit from `stat` (e.g., due to environment differences or a different `stat` implementation) will terminate the script before the `if` check, even with stderr redirected. To make this fail open instead of aborting, consider temporarily disabling `set -e` around `stat`, or use something like:
```sh
owner=$(stat -c %U /app/data 2>/dev/null || echo "")
if [ "$owner" != "appuser" ]; then
chown appuser:appuser /app/data
fi
```
</issue_to_address>
### Comment 2
<location path="internal/shared/store_test.go" line_range="159-183" />
<code_context>
+ require.NotNil(t, rs)
+}
+
+func TestNewResourceStore_ValidIdentifiers(t *testing.T) {
+ db := newTestDB(t)
+
+ tests := []struct {
+ name string
+ tableName string
+ primary string
+ cols string
+ }{
+ {
+ name: "underscores",
+ tableName: "items",
+ primary: "id",
+ cols: "id, item_name, created_at",
+ },
+ {
+ name: "digits_in_identifiers",
+ tableName: "items",
+ primary: "id",
+ cols: "id, name2, col3_v1",
+ },
+ {
+ name: "trailing_comma",
+ tableName: "items",
+ primary: "id",
+ cols: "id, name,",
+ },
+ }
+
+ for _, tc := range tests {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend valid identifier table tests to cover whitespace and leading-comma cases
To better exercise the new normalization/parsing behavior, please add cases with surrounding whitespace in table/column identifiers (e.g. `" items "`, `" id "`, `" id, name "`) and a case with a leading comma in `cols` (e.g. `", id, name"`) to verify empty leading segments are ignored and valid segments are accepted.
```suggestion
tests := []struct {
name string
tableName string
primary string
cols string
}{
{
name: "underscores",
tableName: "items",
primary: "id",
cols: "id, item_name, created_at",
},
{
name: "digits_in_identifiers",
tableName: "items",
primary: "id",
cols: "id, name2, col3_v1",
},
{
name: "trailing_comma",
tableName: "items",
primary: "id",
cols: "id, name,",
},
{
name: "whitespace_in_identifiers",
tableName: " items ",
primary: " id ",
cols: " id, name ",
},
{
name: "leading_comma_in_cols",
tableName: "items",
primary: "id",
cols: ", id, name",
},
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+159
to
+183
| tests := []struct { | ||
| name string | ||
| tableName string | ||
| primary string | ||
| cols string | ||
| }{ | ||
| { | ||
| name: "underscores", | ||
| tableName: "items", | ||
| primary: "id", | ||
| cols: "id, item_name, created_at", | ||
| }, | ||
| { | ||
| name: "digits_in_identifiers", | ||
| tableName: "items", | ||
| primary: "id", | ||
| cols: "id, name2, col3_v1", | ||
| }, | ||
| { | ||
| name: "trailing_comma", | ||
| tableName: "items", | ||
| primary: "id", | ||
| cols: "id, name,", | ||
| }, | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend valid identifier table tests to cover whitespace and leading-comma cases
To better exercise the new normalization/parsing behavior, please add cases with surrounding whitespace in table/column identifiers (e.g. " items ", " id ", " id, name ") and a case with a leading comma in cols (e.g. ", id, name") to verify empty leading segments are ignored and valid segments are accepted.
Suggested change
| tests := []struct { | |
| name string | |
| tableName string | |
| primary string | |
| cols string | |
| }{ | |
| { | |
| name: "underscores", | |
| tableName: "items", | |
| primary: "id", | |
| cols: "id, item_name, created_at", | |
| }, | |
| { | |
| name: "digits_in_identifiers", | |
| tableName: "items", | |
| primary: "id", | |
| cols: "id, name2, col3_v1", | |
| }, | |
| { | |
| name: "trailing_comma", | |
| tableName: "items", | |
| primary: "id", | |
| cols: "id, name,", | |
| }, | |
| } | |
| tests := []struct { | |
| name string | |
| tableName string | |
| primary string | |
| cols string | |
| }{ | |
| { | |
| name: "underscores", | |
| tableName: "items", | |
| primary: "id", | |
| cols: "id, item_name, created_at", | |
| }, | |
| { | |
| name: "digits_in_identifiers", | |
| tableName: "items", | |
| primary: "id", | |
| cols: "id, name2, col3_v1", | |
| }, | |
| { | |
| name: "trailing_comma", | |
| tableName: "items", | |
| primary: "id", | |
| cols: "id, name,", | |
| }, | |
| { | |
| name: "whitespace_in_identifiers", | |
| tableName: " items ", | |
| primary: " id ", | |
| cols: " id, name ", | |
| }, | |
| { | |
| name: "leading_comma_in_cols", | |
| tableName: "items", | |
| primary: "id", | |
| cols: ", id, name", | |
| }, | |
| } |
…t cases - Use || echo "" fallback for stat to prevent set -e from aborting - Add test cases for whitespace-padded identifiers and leading commas in cols
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
NewResourceStore, you validate and trim individual column identifiers but keep the originalcolsstring (including whitespace/leading commas); consider reconstructing a normalized, validatedcolsstring from the parsed identifiers to avoid subtle SQL issues from caller-provided formatting. - Now that
NewResourceStorereturns an error, you might want to add a small helper (e.g.,MustNewResourceStore) for call sites that should treat misconfiguration as fatal, to keep call sites concise while still centralizing the validation logic. - After pinning GitHub Actions to SHAs, it can be helpful to add inline comments with the corresponding version tags (e.g.,
# v6) so future updates are easier to manage without having to look up each commit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `NewResourceStore`, you validate and trim individual column identifiers but keep the original `cols` string (including whitespace/leading commas); consider reconstructing a normalized, validated `cols` string from the parsed identifiers to avoid subtle SQL issues from caller-provided formatting.
- Now that `NewResourceStore` returns an error, you might want to add a small helper (e.g., `MustNewResourceStore`) for call sites that should treat misconfiguration as fatal, to keep call sites concise while still centralizing the validation logic.
- After pinning GitHub Actions to SHAs, it can be helpful to add inline comments with the corresponding version tags (e.g., `# v6`) so future updates are easier to manage without having to look up each commit.
## Individual Comments
### Comment 1
<location path="internal/gateway/router.go" line_range="56-70" />
<code_context>
+ // Prevent XSS: this gateway serves AWS API responses only (JSON/XML),
+ // never user-facing HTML. Sanitize any attempt to serve text/html.
+ ct = strings.TrimSpace(ct)
+ htmlLike := false
+ for _, p := range strings.Split(ct, ",") {
+ p = strings.TrimSpace(p)
+ if p == "" {
+ continue
+ }
+ if mediaType, _, parseErr := mime.ParseMediaType(p); parseErr == nil && strings.EqualFold(mediaType, "text/html") {
+ htmlLike = true
+ break
</code_context>
<issue_to_address>
**🚨 suggestion (security):** HTML sanitization only handles `text/html` and may miss other HTML-capable media types
Right now only `text/html` is treated as HTML-like, but browsers may also render HTML for types like `application/xhtml+xml` or custom `text/*+html`. If an attacker can influence the `Content-Type`, they could bypass this check while still getting HTML rendered. Consider extending the detection to cover `text/html`, `application/xhtml+xml`, and possibly `*+html` suffixes, while still disallowing user-facing HTML and only permitting the expected JSON/XML responses.
```suggestion
// Prevent XSS: this gateway serves AWS API responses only (JSON/XML),
// never user-facing HTML. Sanitize any attempt to serve HTML-like content types.
ct = strings.TrimSpace(ct)
htmlLike := false
for _, p := range strings.Split(ct, ",") {
p = strings.TrimSpace(p)
if p == "" {
continue
}
mediaType, _, parseErr := mime.ParseMediaType(p)
if parseErr != nil {
continue
}
mtLower := strings.ToLower(mediaType)
if mtLower == "text/html" ||
mtLower == "application/xhtml+xml" ||
strings.HasSuffix(mtLower, "+html") {
htmlLike = true
break
}
}
if htmlLike {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Reconstruct cols string from validated identifiers to strip whitespace/commas - Extend Content-Type XSS guard to cover application/xhtml+xml and *+html suffixes - Add inline version tag comments (e.g. # v6) to all pinned action SHAs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Content-Typein API gateway to never servetext/html, preventing browser script execution on API responsesNewResourceStoreto reject non-identifier characters in table/column namesappuserin bothDockerfileandDockerfile.devTest plan
go build ./...passesgo test ./internal/gateway/... ./internal/shared/...passesnpm auditreports 0 vulnerabilities in web/