chore: improve JSON schema + remove default forbid list
#208
Conversation
Remove default forbid array
nickdnk
added
documentation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🔇 Additional comments (4)go.mod (3)
The update from v2.20.2 to v2.21.0 follows semantic versioning and, being a testing framework, shouldn't affect production code.
The indirect dependency update to a newer development snapshot of pprof looks safe. Let's verify there are no breaking changes: ✅ Verification successfulDependency update to pprof is safe The commits between versions only contain GitHub Actions workflow updates (checkout and setup-go actions), with no code changes that could affect runtime behavior. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any major changes in the pprof dependency between versions
# Get the commit messages between the two versions to check for breaking changes
git fetch https://github.com/google/pprof.git
echo "Checking commits between 598669927662 and d1b30febd7db:"
git -C $(git rev-parse --show-toplevel) log --oneline 598669927662..d1b30febd7db
Length of output: 467 Line range hint The PR objectives mention changes to JSON schema and removal of default forbid list, but these changes are not visible in the provided files. Let's check for the relevant files: ✅ Verification successfulChanges to JSON schema and forbid list are present and verified. The review comment's concerns have been addressed as both changes are present in the codebase:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Find schema and configuration related files that might contain the mentioned changes
# Look for JSON schema files
echo "JSON schema files:"
fd -e json -e jsonc --type f schema
# Look for Go files that might contain the forbid list logic
echo -e "\nGo files potentially containing forbid list:"
rg -l "(?i)forbid.*list|uploads.*forbid" -g "*.go"
Length of output: 329 Script: #!/bin/bash
# Check the content of schema.json and relevant Go files for forbid list and schema changes
echo "=== Content of schema.json ==="
cat schema.json
echo -e "\n=== Checking forbid list in config/uploads.go ==="
cat config/uploads.go
echo -e "\n=== Checking forbid list in handler/uploads.go ==="
cat handler/uploads.go
echo -e "\n=== Checking forbid list in handler/request.go ==="
cat handler/request.go
echo -e "\n=== Checking forbid list in handler/handler.go ==="
cat handler/handler.go
Length of output: 33515 tests/go.mod (1)
The indirect dependency updates follow semantic versioning and appear to be routine maintenance updates:
Let's verify that the test suite passes with these updates: Also applies to: 54-54, 73-73, 104-104, 123-123 ✅ Verification successfulDependency updates are safe to proceed The verification shows that none of the updated indirect dependencies (fsnotify, pprof, ginkgo, fasthttp, temporal) are directly imported in any test files. Since these are indirect dependencies and not actively used in the test code, the version bumps are low-risk changes that follow semantic versioning principles. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if there are any test failures in the git history after dependency updates
# Look for test failures in recent commits
git log -p --grep="test.*fail" --since="1 week ago"
Length of output: 53 Script: #!/bin/bash
# Let's verify if the test files import any of the updated packages
# Search for imports of updated packages in test files
echo "=== Searching for fsnotify imports in tests ==="
rg "github.com/fsnotify/fsnotify" -t go --glob "**/*_test.go"
echo -e "\n=== Searching for pprof imports in tests ==="
rg "github.com/google/pprof" -t go --glob "**/*_test.go"
echo -e "\n=== Searching for ginkgo imports in tests ==="
rg "github.com/onsi/ginkgo/v2" -t go --glob "**/*_test.go"
echo -e "\n=== Searching for fasthttp imports in tests ==="
rg "github.com/valyala/fasthttp" -t go --glob "**/*_test.go"
echo -e "\n=== Searching for temporal imports in tests ==="
rg "go.temporal.io/sdk" -t go --glob "**/*_test.go"
# Also check if there are any go.sum changes that might indicate breaking changes
echo -e "\n=== Checking go.sum changes ==="
git diff tests/go.sum
Length of output: 932 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
schema.json (1)
248-249: Consider keeping ".exe" in the forbid examples.While adding ".sh" and ".go" as examples is good, removing ".exe" from the examples might lead developers to overlook a common security concern. Executable files should typically be blocked in upload directories.
Consider updating the examples to include both:
".php", ".sh", ".go", +".exe"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
config/uploads.go(0 hunks)schema.json(11 hunks)
💤 Files with no reviewable changes (1)
- config/uploads.go
🔇 Additional comments (4)
schema.json (4)
10-10: LGTM! Description improvements enhance clarity.
The updates to field descriptions provide better context and accurately reflect the behavior of each configuration option. The removal of the "required" constraint for the address field is correct since it's optional when using FCGI or HTTP3.
Also applies to: 19-19, 26-26, 32-32, 42-42, 60-60
292-292: LGTM! Improved SSL configuration clarity.
The updated descriptions clearly explain the mutual exclusivity between manual SSL configuration (key/cert) and ACME, helping prevent configuration errors.
Also applies to: 355-355, 363-363, 371-371
396-396: LGTM! Enhanced FCGI configuration documentation.
The updates provide clearer guidance on FCGI configuration with comprehensive examples, including Unix socket support.
Also applies to: 401-408
427-427: LGTM! Clarified HTTP2 streams configuration.
The description now clearly states the default behavior when the value is omitted or set to zero.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
schema.json (1)
231-231: Security enhancement: Explicit forbidden file typesGood changes that:
- Remove the confusing default forbid list (aligns with PR objectives)
- Explicitly forbid potentially dangerous file types (.sh, .go)
Consider also forbidding other potentially dangerous extensions like .bash, .zsh, .pl, .py to prevent execution of arbitrary scripts.
Also applies to: 248-249
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
config/uploads.go(0 hunks)schema.json(11 hunks)
💤 Files with no reviewable changes (1)
- config/uploads.go
🔇 Additional comments (5)
schema.json (5)
10-10: LGTM: Description improvements enhance clarity
The updates to property descriptions provide better clarity about:
- Optional nature of the address field (aligns with PR objectives)
- Default values and behaviors
- Conditions under which properties apply
Also applies to: 19-19, 26-26, 32-32, 42-42, 60-60
292-292: LGTM: Better clarity on SSL configuration options
The updated descriptions clearly indicate the mutual exclusivity between ACME and manual certificate configuration, preventing potential misconfigurations.
Also applies to: 355-355, 363-363, 371-371
396-396: LGTM: FCGI configuration clarity improved
The updates provide better clarity about:
- Optional nature of FCGI support
- Various supported address formats including Unix sockets
Also applies to: 401-408
427-427: LGTM: HTTP/2 defaults clarified
The description now clearly states the default value for max_concurrent_streams when omitted or zero.
Line range hint 1-479: Verify schema validity
Let's ensure the schema is valid according to JSON Schema draft 2019-09.
✅ Verification successful
Schema validation successful
The schema validation checks reveal:
- All enum definitions are properly structured with string values and valid options
- Type definitions are consistent across the schema
- The schema follows JSON Schema draft 2019-09 format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Validate the JSON schema against draft 2019-09
# Expected result: No errors in schema validation
# Check if the schema is valid JSON
if ! jq empty schema.json; then
echo "Error: Invalid JSON syntax"
exit 1
fi
# Check if the schema follows draft 2019-09 rules
# Note: This is a basic check. A full validator would be better but isn't available in the sandbox
if ! jq '.["$schema"] == "https://json-schema.org/draft/2019-09/schema"' schema.json > /dev/null; then
echo "Error: Schema doesn't declare draft 2019-09"
exit 1
fi
# Check for common schema issues
jq '
def check_descriptions:
.. | select(type == "object") |
select(has("description") and (.description | type != "string")) |
"Invalid description type";
def check_examples:
.. | select(type == "object") |
select(has("examples") and (.examples | type != "array")) |
"Invalid examples type";
check_descriptions, check_examples
' schema.json
Length of output: 481
Script:
#!/bin/bash
# Let's perform more specific schema validation checks
# Check for required fields in objects that need them
echo "=== Checking required fields ==="
jq -r '
paths as $p |
select(getpath($p) | type == "object" and has("required")) |
[$p[], .required[]] | join("/")
' schema.json
# Check for proper type definitions
echo -e "\n=== Checking type definitions ==="
jq -r '
paths as $p |
select(getpath($p) | type == "object" and has("type")) |
[$p[], .type] | join(": ")
' schema.json
# Check for proper enum values
echo -e "\n=== Checking enum definitions ==="
jq -r '
paths as $p |
select(getpath($p) | type == "object" and has("enum")) |
[$p[], (.enum | tostring)] | join(": ")
' schema.json
# Check for proper references
echo -e "\n=== Checking $ref definitions ==="
jq -r '
paths as $p |
select(getpath($p) | type == "string" and startswith("#")) |
. as $ref |
$p | join("/")
' schema.json
Length of output: 4051
Script:
#!/bin/bash
# Let's verify the schema references and enum values more thoroughly
# Check if all internal references are valid
echo "=== Checking internal references ==="
jq -r '
def collect_refs:
. as $root |
[paths(.$ref?) |
select(getpath(.) | startswith("#")) as $ref |
{
"path": (. | join("/")),
"ref": getpath(.)
}] |
map(. as $item |
($item.ref | split("/")[1:] | reduce .[] as $seg (""; . + "/" + $seg)) as $normalized_ref |
if $root | has($normalized_ref[1:]) then
$item.path + ": OK"
else
$item.path + ": INVALID REF - " + $item.ref
end
)[];
collect_refs
' schema.json
# Validate enum values in detail
echo -e "\n=== Checking enum values in detail ==="
jq -r '
paths(select(type == "object" and has("enum"))) as $p |
{
"path": $p | join("/"),
"type": getpath($p).type,
"enum": getpath($p).enum
} | "Path: \(.path)\nType: \(.type)\nEnum values: \(.enum | tostring)\n"
' schema.json
# Check for any inconsistencies in type definitions
echo -e "\n=== Checking type consistency ==="
jq -r '
paths(select(type == "object" and has("type"))) as $p |
{
"path": $p | join("/"),
"type": getpath($p).type,
"has_items": has("items"),
"has_properties": has("properties")
} | select(
(.type == "array" and (has_items | not)) or
(.type == "object" and (has_properties | not))
) | "Potential issue at \(.path): type=\(.type)"
' schema.json
Length of output: 2327
Signed-off-by: Valery Piashchynski <[email protected]>
rustatian
changed the title
JSON schema + remove default forbid list
Remove default forbid array
Reason for This PR
addresswas specified as required in the JSON schema. It is not required, as you can use FCGI or HTTP3 instead.The default "forbid" list would be populated even if you specified that all files are allowed (empty list). This is counter-intuitive, especially since the current default looks more like an example than a thorough list.
Description of Changes
Made
addressnon-required in JSON schema.Minor grammar cleanup in JSON schema.
Removed default list of forbidden files.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
uploadsschema to modify disallowed file extensions and set default values for allowed files.Bug Fixes
Forbidslice, ensuring it remains empty if no values are specified.Documentation
schema.jsonfor better clarity on property functionalities and requirements.