SEZ9 commented on PR #11734:
URL: https://github.com/apache/seatunnel/pull/11734#issuecomment-5842995250
Thanks for the update on `a986eb165c`. I read through the new "Normative
Rules" section (R1–R5) and the explicit create-before-capture ordering
statement — that tightens the design considerably compared with `e944f1e3`, and
the decision tables make the contract much easier to verify.
That said, the four security points I raised on
`docs/en/engines/zeta/task-failure-history.md` in my previous review still
appear to be open in the current head, and I could not find them covered by
R1–R5:
1. **HIGH – unauthenticated exposure of raw exception messages / stack
traces via `/job-info/{jobId}/failures`.** The contract still needs to state
the authentication/authorization expectation for this endpoint and a redaction
requirement for the persisted `errorMsg`/stack-trace payload before it is
returned to API consumers. If the intent is to inherit whatever protection the
existing `JobInfoServlet` routes have, please say so explicitly and call out
that raw stack traces are a new class of data compared with the existing single
`errorMsg` field.
2. **MEDIUM – unredacted failure payloads persisted in the HA-backed
Hazelcast IMap.** The per-map ownership/fence/expiry table in R4 is a good
place to add a sanitization rule (what is stripped before write, and a size cap
on the stored message/stack trace), so secrets that leak into an exception
message are not replicated across cluster state for the full retention window.
3. **MEDIUM – worker `host:port` in the REST response.** Please either make
this field optional/configurable, or add a rationale in the doc for why cluster
topology must be disclosed to every API consumer. Exposing it only to the same
audience that can already list cluster members would be an acceptable
resolution if documented.
4. **LOW – input validation for the servlet routing extension and `limit`.**
The doc should spell out the accepted range/default for `limit`, the behaviour
on non-numeric or negative values, and how the `{jobId}/failures` path segment
is parsed and rejected when malformed, so the implementation has a normative
rule to follow rather than an implicit one.
Concrete asks before I can approve:
- Add a "Security" (or equivalent) normative section addressing points 1–3
with the same table style as R1–R5.
- Add the `limit`/path validation rules from point 4, ideally as an
extension of R5.
Happy to take another look as soon as those land.
<!-- streview-comment:1329 -->
--
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]
