fix: gojq module review — permissions, collision, rename, upgrade#3451
fix: gojq module review — permissions, collision, rename, upgrade#3451
Conversation
- Fix generateRandomID fallback collision risk by adding UnixNano - Tighten payload file permissions from 0644 to 0600 - Rename walk(f) to walk_schema to avoid shadowing gojq built-in - Upgrade gojq v0.12.18 → v0.12.19 - Add single-output contract test for walk_schema filter - Update comments, logs, and README to reflect changes Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/b442d995-9a62-46f3-a6df-e5182255ef31 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Addresses findings from the gojq module review by tightening payload file permissions, reducing ID collision risk in a fallback path, avoiding naming collision with gojq’s built-in walk/1, and upgrading gojq.
Changes:
- Restrict saved payload file permissions from
0644to0600. - Rename custom jq function
walk→walk_schema(and update docs/comments) to avoid shadowing built-inwalk/1. - Upgrade dependencies
github.com/itchyny/gojqtov0.12.19(andtimefmt-gotov0.1.8), and add a test documenting the “single jq output” iterator contract.
Show a summary per file
| File | Description |
|---|---|
| internal/middleware/README.md | Updates jq filter documentation to use walk_schema and explains why. |
| internal/middleware/jqschema.go | Updates jq filter definition/name, hardens ID fallback, tightens payload file perms, bumps version references. |
| internal/middleware/jqschema_test.go | Adds a “single output” contract test and updates permissions expectation to 0600. |
| go.mod | Bumps gojq to v0.12.19 and timefmt-go indirect to v0.1.8. |
| go.sum | Updates module checksums for the upgraded dependencies. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/5 changed files
- Comments generated: 4
internal/middleware/jqschema.go
Outdated
| } | ||
|
|
||
| logger.LogInfo("payload", "Successfully saved large payload to filesystem: path=%s, size=%d bytes, permissions=0644", | ||
| logger.LogInfo("payload", "Successfully saved large payload to filesystem: path=%s, size=%d bytes, permissions=0600", |
There was a problem hiding this comment.
os.WriteFile only applies the provided file mode when the file is created; if payload.json already exists (e.g., from a prior run or an ID collision), its existing permissions are preserved. That can leave a world-readable file even after switching to 0600. Consider explicitly enforcing the mode after writing (e.g., chmod to 0600) and/or opening with flags that ensure the file is created with the intended permissions.
| logger.LogInfo("payload", "Successfully saved large payload to filesystem: path=%s, size=%d bytes, permissions=0600", | |
| if err := os.Chmod(filePath, 0600); err != nil { | |
| logger.LogError("payload", "Failed to enforce payload file permissions: path=%s, size=%d bytes, error=%v", | |
| filePath, payloadSize, err) | |
| return "", fmt.Errorf("failed to set payload file permissions: %w", err) | |
| } | |
| logger.LogInfo("payload", "Successfully saved large payload to filesystem: path=%s, size=%d bytes, enforcedPermissions=0600", |
internal/middleware/jqschema.go
Outdated
| logger.LogInfo("payload", "Successfully saved large payload to filesystem: path=%s, size=%d bytes, permissions=0600", | ||
| filePath, payloadSize) |
There was a problem hiding this comment.
The success log hardcodes permissions=0600, but the resulting mode can still differ (e.g., if the file already existed, or if the OS applies different semantics). Consider logging the actual stat.Mode().Perm() after the write so logs match reality.
See below for a potential fix:
// Verify file was written correctly and log the actual resulting mode
if stat, err := os.Stat(filePath); err != nil {
logger.LogWarn("payload", "Could not verify payload file after write: path=%s, error=%v", filePath, err)
logger.LogInfo("payload", "Successfully saved large payload to filesystem: path=%s, size=%d bytes",
filePath, payloadSize)
} else {
logger.LogInfo("payload", "Successfully saved large payload to filesystem: path=%s, size=%d bytes, permissions=%#o",
filePath, payloadSize, stat.Mode().Perm())
| for _, input := range inputs { | ||
| iter := jqSchemaCode.RunWithContext(context.Background(), input) | ||
|
|
There was a problem hiding this comment.
This test uses jqSchemaCode directly without asserting compilation succeeded first. If init-time compilation ever fails (or changes in the future), jqSchemaCode may be nil and this will panic rather than fail with a helpful assertion. Consider checking jqSchemaCompileErr/jqSchemaCode != nil up front (or calling applyJqSchema and validating the iterator behavior indirectly).
internal/middleware/jqschema.go
Outdated
| } | ||
|
|
||
| logMiddleware.Printf("Successfully compiled jq schema filter at init (gojq v0.12.18)") | ||
| logMiddleware.Printf("Successfully compiled jq schema filter at init (gojq v0.12.19)") |
There was a problem hiding this comment.
Hardcoding the gojq version in logs tends to become stale as dependencies are upgraded. Consider removing the version from this message (or sourcing it from build/module metadata) so the log stays accurate without needing manual updates.
| logMiddleware.Printf("Successfully compiled jq schema filter at init (gojq v0.12.19)") | |
| logMiddleware.Printf("Successfully compiled jq schema filter at init") |
- Add os.Chmod after WriteFile to enforce 0600 on pre-existing files - Log actual stat.Mode().Perm() instead of hardcoded permissions value - Guard contract test with jqSchemaCompileErr/jqSchemaCode nil checks - Remove hardcoded gojq version from init log message Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses findings from the Go Fan gojq module review.
Security
0644→0600— payloads may contain sensitive API responses; no reason for world-readableBug fix
generateRandomIDfallback collision — whencrypto/rand.Readfails, the fallback wasfmt.Sprintf("fallback-%d", os.Getpid())which is identical for every call in the same process. Addedtime.Now().UnixNano():Maintenance
def walk(f)→def walk_schema— avoids shadowing gojq's built-inwalk/1. Now a zero-arity recursive def since thefparameter was never meaningfully used (always called aswalk(.))github.com/itchyny/gojqv0.12.18 → v0.12.19Tests
TestApplyJqSchema_SingleOutputContract— asserts the jq filter iterator yields exactly one value across multiple input shapes, documenting the invariantTestPayloadStorage_FilePermissionsexpectation to0600Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build511967270/b514/launcher.test /tmp/go-build511967270/b514/launcher.test -test.testlogfile=/tmp/go-build511967270/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build511967270/b211/vet.cfg 6718451/b186/_pkg_.a ache/go/1.25.8/x64/src/vendor/golang.org/x/text/-Wl,--no-gc-sections x_amd64/vet . --gdwarf2 --64 x_amd64/vet(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build511967270/b496/config.test /tmp/go-build511967270/b496/config.test -test.testlogfile=/tmp/go-build511967270/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o ache/go/1.25.8/x64/src/net -trimpath x_amd64/vet -p net/url -lang=go1.25 x_amd64/vet(dns block)nonexistent.local/tmp/go-build511967270/b514/launcher.test /tmp/go-build511967270/b514/launcher.test -test.testlogfile=/tmp/go-build511967270/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build511967270/b211/vet.cfg 6718451/b186/_pkg_.a ache/go/1.25.8/x64/src/vendor/golang.org/x/text/-Wl,--no-gc-sections x_amd64/vet . --gdwarf2 --64 x_amd64/vet(dns block)slow.example.com/tmp/go-build511967270/b514/launcher.test /tmp/go-build511967270/b514/launcher.test -test.testlogfile=/tmp/go-build511967270/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build511967270/b211/vet.cfg 6718451/b186/_pkg_.a ache/go/1.25.8/x64/src/vendor/golang.org/x/text/-Wl,--no-gc-sections x_amd64/vet . --gdwarf2 --64 x_amd64/vet(dns block)this-host-does-not-exist-12345.com/tmp/go-build511967270/b523/mcp.test /tmp/go-build511967270/b523/mcp.test -test.testlogfile=/tmp/go-build511967270/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -plu�� g_.a -plugin-opt=/usrgithub.qkg1.top/github/gh-aw-mcpg/internal/launcher x_amd64/vet -plugin-opt=-pasbash rs/otlp/otlptrac/usr/bin/runc -plugin-opt=-pas--version x_amd64/vet -uns�� .cfg elemetry.io/otel-nolocalimports x_amd64/vet -m elf_x86_64 --hash-style=gnu--version x_amd64/vet(dns block)If you need me to access, download, or install something from one of these locations, you can either: