fix(tests): '\$[' is read as the deprecated $[...] arithmetic form - #60
fix(tests): '\$[' is read as the deprecated $[...] arithmetic form#60hyperpolymath wants to merge 1 commit into
Conversation
security-tests.sh:72 embedded '\\$[{]?' in a double-quoted string. After bash processes the escape this leaves '$[', which is the obsolete arithmetic-expansion syntax, so the file failed to parse. Escaped so a literal dollar reaches the regex, preserving the intended ERE.
Found by an estate-wide shellcheck sweep of 5,111 tracked scripts across 375
repos: 11 files fail to PARSE (SC1073/SC1072). shellcheck stops analysing at
the failure, so everything after it in the file was never checked either.
Verified: shellcheck -S error reports 0 parse errors for the file(s) touched.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe security test updates the unsafe- ChangesSecurity test validation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized test-script fix restores parsing while preserving the intended regular expression, and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR successfully addresses the ShellCheck parsing error (SC1072/SC1073) caused by the deprecated arithmetic expansion syntax. While Codacy indicates the PR is up to standards, the internal logic of the 'eval' safety check in tests/security-tests.sh remains problematic.
The security check can be trivially bypassed by adding a trailing comment to an unsafe line, and the regex currently fails to detect common injection vectors such as positional parameters (
About this PR
- Although the parsing issue is fixed, the PR does not include an automated check or CI configuration to ensure the script remains ShellCheck-compliant in the future.
Test suggestions
- Verify the script passes ShellCheck analysis without syntax/parse errors.
- Verify the regex correctly identifies unsafe eval usage (e.g., 'eval
$VAR' or 'eval $ {VAR').
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify the script passes ShellCheck analysis without syntax/parse errors.
2. Verify the regex correctly identifies unsafe eval usage (e.g., 'eval $VAR' or 'eval ${VAR').
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
|
||
| # Should not have eval of user input without validation | ||
| test_assert "${script_name} doesn't have unsafe eval" "! grep -E 'eval .*\\$[{]?[a-zA-Z_]+' \"${script}\" | grep -qv '#'" | ||
| test_assert "${script_name} doesn't have unsafe eval" "! grep -E 'eval .*\\\$[{]?[a-zA-Z_]+' \"${script}\" | grep -qv '#'" |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The 'eval' safety check has two significant issues: 1) It is easily bypassed by trailing comments (e.g., 'eval $VAR # comment') because the current implementation discards any line containing a hash. 2) The regex '[a-zA-Z_]+' fails to detect positional parameters ($1, ! grep -E '^[[:space:]]*[^#]*eval .*\\\$[({]?[a-zA-Z0-9_*@?]'.
1 participant
security-tests.sh:72 embedded '\$[{]?' in a double-quoted string. After bash processes the escape this leaves '$[', which is the obsolete arithmetic-expansion syntax, so the file failed to parse. Escaped so a literal dollar reaches the regex, preserving the intended ERE.
Found by an estate-wide shellcheck sweep of 5,111 tracked scripts across 375 repos: 11 files fail to parse (
SC1073/SC1072). shellcheck stops analysing at the failure, so everything after it was never checked either.Verified:
shellcheck -S errorreports 0 parse errors for the file(s) touched.