Skip to content

credential: address the four remaining findings from the PR #127 review#138

Open
congwang-mk wants to merge 1 commit into
mainfrom
fix/credential-followups
Open

credential: address the four remaining findings from the PR #127 review#138
congwang-mk wants to merge 1 commit into
mainfrom
fix/credential-followups

Conversation

@congwang-mk

Copy link
Copy Markdown
Contributor

Follow-up to #127, addressing the four findings that remained after the re-review:

  1. Seekable stdin rejected for fd:0. Stdin survives into the child (unlike fds above 2, which are closed post-fork), and a dup shares the file offset, so a < secret.txt redirect would let the child lseek back and re-read the secret even after the supervisor drained it. load_secret now refuses a rewindable fd 0 and points at the pipe pattern; pipes/sockets/ttys (ESPIPE) stay allowed, and seekable sources on fds above 2 are unaffected.

  2. Header names validated at rule-parse time. A malformed header:<name> / apikey:<name> previously surfaced only as a per-request 502 the first time the rule fired; parse_auth now runs HeaderName::from_bytes so a typo fails the build like every other spec error.

  3. File-overlap warning honors fs-deny. The warning suggested "add an fs-deny for it" but did not consult fs_denied, so following the advice still warned on every run. The overlap check (extracted into a testable exposing_grant helper) now suppresses the warning when a deny covers the secret file.

  4. Query param names compared percent-decoded. A child spelling the target param with stray percent-encoding (%6Bey= for key=) evaded the Replace de-dup filter, leaving its pair alongside the injected one. Param names are now decoded before comparison, for both the Replace filter and the AddOnly presence check.

Each fix comes with unit tests, including a stdin-swap test that exercises both the file-redirect rejection and the pipe acceptance on the real fd 0.

🤖 Generated with Claude Code

…ny, match decoded query params

Signed-off-by: Cong Wang <cwang@multikernel.io>
@congwang-mk congwang-mk force-pushed the fix/credential-followups branch from a50eaf0 to 6ede13b Compare July 12, 2026 04:21
@congwang-mk

congwang-mk commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@dzerik Please take a look. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant