Ashfaqbs commented on issue #1004:
URL: https://github.com/apache/flink-agents/issues/1004#issuecomment-5518972068
Confirmed still current on `main` — reproduced exactly:
`BashValidator.validate("echo hi > /tmp/x", List.of("echo"), List.of(), null)`
returns `Optional.empty()` (allowed) even with an empty `allowedScriptDirs`,
because `walk()`'s recursion visits `file_redirect` nodes (they're in
`ALLOWED_NAMED`, so the construct itself passes) but only `command` nodes ever
get their content validated — the redirect target text is never inspected.
`BashValidatorTest.redirectAllowed()` and the Python equivalent both currently
assert this as the expected/passing behavior, so they'd need updating alongside
any fix. Same gap for `variable_assignment` — its value is likewise never
validated, so `PATH=/tmp/evil echo hi` (with `echo` allowed) passes unchanged
today.
On the redirect half of this issue, I'd lean toward the second option over
"reject by default": a distinct `allowed_write_dirs` parameter (separate from
`allowed_script_dirs`, mirroring the "executable directories should not
implicitly become writable directories" concern already in the issue),
defaulting to empty — so out of the box every redirect is rejected (since
nothing is in the empty default), and an operator opts a directory into
writability explicitly rather than it happening implicitly via the
execute-allowlist. Concretely:
- Add `allowedWriteDirs`/`allowed_write_dirs` alongside
`allowed_commands`/`allowed_script_dirs` in `BashTool`'s parameter surface
(Java + Python), threaded into `BashValidator.validate`/`validate_command`.
- In `walk`, when visiting a `file_redirect` node, extract its target (the
`word`/`string` child after the redirect operator) and validate it the same way
`validateCommand` validates the executable — against `allowedWriteDirs` via the
existing `isUnderAllowedDirs` helper, rather than `allowedScriptDirs`.
- Update `redirectAllowed()` (and the Python equivalent) to pass an explicit
`allowedWriteDirs` containing `/tmp`, and add a new test asserting a redirect
to a path outside it is now rejected — matching this issue's own repro.
- Materialized skill directories would need to *not* be auto-added to this
new list (only to `allowed_script_dirs`, as today), so skill-provided scripts
stay executable-only, not writable, by default.
The variable-assignment / environment-hardening half (`PATH`, `BASH_ENV`,
dynamic-loader vars) and the skill-directory-immutability item feel like they
warrant their own follow-up rather than folding into the same PR — different
validation surface (assignment *values*, not redirect targets) and a different
enforcement mechanism (denylist of variable names vs. an allowlist of
directories). Happy to open a PR for the redirect half along these lines if
that direction sounds right, and split a second issue for the
environment-assignment half — or take a different shape if you'd rather keep it
together or go with "reject all redirects, no config" instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
