fix(bundler): harden filepath.Join with SafeJoin for path-traversal protection#578
fix(bundler): harden filepath.Join with SafeJoin for path-traversal protection#578
Conversation
There was a problem hiding this comment.
A couple of minor issues.
Consider adding a regression test for the Helm external-data path.
| # | File | Line | Severity | Description |
|---|---|---|---|---|
| 1 | pkg/bundler/checksum/checksum.go |
88, 154 | minor | GetChecksumFilePath and CountEntries still use filepath.Join — no real traversal risk (constant second arg) but inconsistent with the hardening pattern. Converting would require a signature change to (string, error). |
| 2 | pkg/bundler/bundler.go |
~830 | minor | Unsafe data file path in attestation silently skipped (continue + Debug log) instead of failing. User opted into --attest, so silently omitting a file from attestation weakens integrity. Should at minimum log at Warn, or arguably fail like the other SafeJoin calls. |
|
Note: the double |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
the regression introduced by #575 needs to be fixed first.
yuanchen8911
dismissed
their stale review
regression in PR #575 should be fixed.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
, #575 adds the second copyDataFiles(dir) call in runDeployer() after Helm already copied the same external --data files in buildDeployer(). That needs to be fixed.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
#575 adds the second copyDataFiles(dir) call in runDeployer() after Helm already copied the same external --data files in buildDeployer(). That needs to be fixed.
lockwobr
force-pushed
the
feat/deployer-interface
branch
2 times, most recently
from
8e9c264 to
c93bfd9
Compare
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
SafeJoin hardening looks correct overall. The test for path traversal rejection in helm_test.go is solid. Two items from Mark's review still need addressing:
-
The attestation data file loop (former line ~830) now silently continues on SafeJoin failure with a Debug log, but the user opted into
--attest-- silently omitting a file weakens integrity. Should at minimum be Warn, arguably fail. -
GetChecksumFilePathandCountEntriesstill usefilepath.Join. The inline comments explaining why are fine, but worth confirming Mark is OK with that rationale.
Also note the dependency on #575 for the copyDataFiles flow -- merge order matters here.
lockwobr
force-pushed
the
feat/deployer-interface
branch
2 times, most recently
from
fa30a50 to
ec451a2
Compare
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@lockwobr this PR now has merge conflicts with |
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (3 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
LGTM. On the current rebased head, the previously discussed #575 issue is addressed for purposes of reviewing this PR, and I don’t have any remaining blocking findings on the SafeJoin hardening changes.
|
Confirmed on the rebased head that the copyDataFiles issue discussed from #575 is no longer a blocker for reviewing this PR. The SafeJoin hardening here looks good to me, and the PR LGTM from a code-review perspective. |
3 participants
Summary
Harden
filepath.Joincalls in bundler production code withdeployer.SafeJoinfor path-traversal protection, and excludegoconstfrom test file linting.Motivation / Context
CodeQL flagged several
filepath.Joincalls during PR #527 review where the second argument could originate from user input.SafeJoinvalidates that joined pathsstay within the base directory, preventing path traversal. The
goconstexclusion removes unnecessary ceremony — repeated test strings don't benefit from constantextraction.
Closes: #574
Related: #527
Branched off of: #575
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
Testing
# Commands run (prefer `make qualify` for non-trivial changes) make qualifyRisk Assessment
Rollout notes:
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info