[krisztian-gajdar]

Summary

Follow-ups to #760 from its independent review (#760 (comment)). All four findings were non-blocking.

1. Each temporary token link counts once (packages/sie_server/src/sie_server/core/server_auth.py, _drop_temporary_links)

• Before: the check compared the token's link count with the number of scan results. A listing that returned the staged entry twice made auth-token, a foreign link and one staged link look like the token plus two temporary links, so the token was accepted with the foreign link in place.
• Now: the count is compared with the distinct temporary link names plus one: len({entry.name for entry in linked}) + 1.
• POSIX does not repeat an unchanged directory entry, so this is hardening.

2. Test precision (packages/sie_server/tests/core/test_server_auth.py)

• test_a_link_count_that_keeps_changing_is_refused_after_a_few_scans asserts len(scans) == server_auth._LINK_SCAN_ATTEMPTS instead of a range.
• The _drop_temporary_links docstring no longer says a concurrent start may be adding its link. A start never adds a link to a token that a reader already has open.

3. Warning text (packages/sie_gateway/src/middleware/auth.rs)

• Before: the repeated-Authorization warning said "The gateway used the first value". When the first Authorization value is empty, x-api-key decides, so that was wrong.
• Now: it says "The gateway ignored the later Authorization values", which holds whichever header decided. The authorization_values_disagree docstring says the same.
• The request still gets the warning: 0.13.0 refuses it like any other repeat whose values disagree. The once-per-process flag and the post-authentication placement are unchanged.

4. Warning-test cases (middleware_warns_only_for_a_repeat_that_passes_authentication)

• New refusals: the 403 "Admin token not configured", and missing, unreadable and conflicting x-api-key requests. Each sends Authorization values that disagree.
• Each case checks the refusal's status and message, that it logs no warning, and that a passing request on the same router then logs exactly one, with the new text. The x-api-key cases pass with an empty first Authorization value, the case in item 3.

New test

• TestPersistedToken::test_a_temporary_link_listed_twice_by_the_scan_counts_once, with the scan yielding each entry twice:
  • [with-foreign-link]: refused, and the foreign and staged links stay.
  • [temporary-only]: accepted, and the staged link is removed.

Mutation checks

Each mutation was applied on its own and caught:

• comparing with len(linked) + 1: both new parameters fail ([with-foreign-link] with DID NOT RAISE, [temporary-only] with a refusal);
• scanning _LINK_SCAN_ATTEMPTS + 1 or _LINK_SCAN_ATTEMPTS - 1 times: the scan-count test fails;
• the old warning text: the warning test fails;
• logging the warning in the Missing, Unreadable or Conflicting credential branch, or before the "Admin token not configured" refusal: the warning test fails on that case.

Validation

• mise run lint and mise run typecheck: clean.
• mise run test -- packages/sie_server/tests/core/test_server_auth.py: 153 passed. mise run test -- packages/sie_server/tests/test_cli_auth.py: 58 passed.
• mise run gateway-fmt -- --check, mise run gateway-check and mise run gateway-clippy: clean.
• mise run gateway-test, default and cloud-storage features, with NATS: all passed.

Review

Items 1 and 3 change authentication code, so this gets an independent adversarial review before merge.

Summary by CodeRabbit

• Bug Fixes
  • Repeated Authorization headers now receive a warning that accurately describes how later values are handled.
  • Temporary token links are counted distinctly during cleanup, preventing duplicate scan entries from being mistaken for separate links.
  • Token files with temporary links can be read and cleaned up correctly; unrelated links remain in place, and conflicting credentials continue to be refused.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Team
• Run ID: 72292ec4-8339-4c13-a8fb-bc6cee6183ae

📥 Commits

Reviewing files that changed from the base of the PR and between 3186eb0 and 1c9c44d.

📒 Files selected for processing: 3

• packages/sie_gateway/src/middleware/auth.rs
• packages/sie_server/src/sie_server/core/server_auth.py
• packages/sie_server/tests/core/test_server_auth.py

📝 Walkthrough

The gateway middleware updates repeated-Authorization warning wording and tests warning behavior across credential outcomes. Server authentication now counts distinct temporary-link entry names, with tests for duplicate scan entries and retry limits.

Changes: Gateway authentication warning behavior

Layer / File(s)	Summary
Repeated-Authorization warning and authentication tests packages/sie_gateway/src/middleware/auth.rs	The warning and comment now say later Authorization values are ignored. Tests cover accepted and refused credential cases, check refusal responses, and verify that warnings are emitted only for requests that pass authentication.

Temporary-link counting

Layer / File(s)	Summary
Distinct temporary-link counting and tests packages/sie_server/src/sie_server/core/server_auth.py, packages/sie_server/tests/core/test_server_auth.py	The temporary-link count uses distinct entry names. Tests cover duplicate scan entries, cleanup or preservation of links, and the exact scan retry limit.

Suggested reviewers: svonava

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 1c9c4

No merge-blocking issue is identified in the warning or temporary-link changes; merge after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies both main changes: counting each temporary token link once and correcting the repeated-Authorization warning.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[krisztian-gajdar]

Independent adversarial review, run 1 (agent-generated)

This change touches token authentication in the server and the gateway, so it follows the authentication and authorization review gate.

Review record

• Reviewer session id: a7c5922bb918a18ce. A fresh agent, started without the authoring session's context: not a fork, not a context-inheriting subagent. No memory or instruction entry it loaded mentions this PR.
• Credentials: the brief forbids any write action, including gh, pushing and comments. The reviewer worked from a local copy of the whole diff and a detached checkout of the head. The platform does not strip credentials from an agent's environment, so this limit rests on its instructions.
• Reviewed head: 1c9c44d898090f1792a85d42a42412943dd0c4c8.
• Reviewed range: origin/main...1c9c44d8, merge base 3186eb09011be9bffd18378577ff3a94a59d8a60. This is the whole PR diff.
• Scope limits and premises from the author: none. The brief gives the PR title only and states "no scope limit".
• Abuse cases, test evidence and findings: in the reviewer's report below.
• Disposition:
  • Blocking findings: 0.
  • Findings 1 to 5 are non-blocking. The reviewer marks 1, 3 and 4 as pre-existing and 2 as a documentation sibling. All five are fixed in one follow-up pull request, which gets its own independent adversarial review. This PR links it when it opens.
• Runs started for this PR: this one.

Reviewer prompt, verbatim

The agent was started with this prompt:

Your complete brief is the file /private/tmp/claude-501/-Users-krisztiangajdar-Documents-projects-sie--claude-worktrees-sie-coding-capability-demo-146ebd/8340c74b-6a69-47aa-9745-d26f702c0537/scratchpad/review-811/prompt.md. Read it first and follow it exactly; it is your whole task. Return your report in the format it specifies.

The brief file, verbatim:

You are an independent adversarial security reviewer for a pull request in the public repository superlinked/sie, titled "fix: count each temporary token link once and correct the repeated-Authorization warning". You start with no context from the session that wrote it. Review the change as an attacker would and report what you find. Make no write action of any kind: do not use gh, do not push, do not comment, do not post to any service, do not edit files outside a scratch copy. You have no premise from the author beyond the title, and no scope limit. You may report on anything in the diff and on any code it changes the behaviour of.

Inputs:
- The whole PR diff, origin/main...head: /private/tmp/claude-501/-Users-krisztiangajdar-Documents-projects-sie--claude-worktrees-sie-coding-capability-demo-146ebd/8340c74b-6a69-47aa-9745-d26f702c0537/scratchpad/review-811/pr811.diff
- Base (merge base with main): 3186eb09011be9bffd18378577ff3a94a59d8a60. Head: 1c9c44d898090f1792a85d42a42412943dd0c4c8.
- A detached, read-only checkout of the head: /private/tmp/claude-501/-Users-krisztiangajdar-Documents-projects-sie--claude-worktrees-sie-coding-capability-demo-146ebd/8340c74b-6a69-47aa-9745-d26f702c0537/scratchpad/review-811/tree . Read any file there. You may run its tests locally (for example `cd <tree> && mise trust && mise run test -- packages/sie_server/tests/core/test_server_auth.py`, and `mise run gateway-test` for the gateway; a gateway build is large, so set `CARGO_PROFILE_DEV_DEBUG=0` and check free disk with `df -h /` first, stopping if under 15 GB). You may write throwaway scripts under /private/tmp/claude-501/-Users-krisztiangajdar-Documents-projects-sie--claude-worktrees-sie-coding-capability-demo-146ebd/8340c74b-6a69-47aa-9745-d26f702c0537/scratchpad/review-811/scratch only. No network calls beyond what the test suite itself does locally, and no calls to any hosted SIE endpoint.

What the change touches: the single server's check that its persisted auth token file has no hard links other than temporary links a concurrent start created (`packages/sie_server/src/sie_server/core/server_auth.py`, with `owned_files` in `core/data_dir.py`), and the Rust gateway's authentication middleware where a request repeats the `Authorization` header (`packages/sie_gateway/src/middleware/auth.rs`).

Probe at least:
1. The token file's link check: can a hard link to the token that is not a temporary link a start created (in the same directory or anywhere else on the filesystem) be counted as temporary, hidden, or left in place while the token is accepted? Consider names that match the temporary-name pattern, duplicate or case-variant names, entries that change type or inode between the scan and the unlink, symbolic links, a file owned by another user, a concurrent attacker adding and removing links during the scan so the counts balance, and the number of scan attempts. Can the cleanup unlink anything it should not?
2. The repeated-Authorization handling: which credential the gateway actually authenticates with for every shape of repeated header (an empty, whitespace-only, unreadable or non-Bearer first value; differing case of the header name; `Authorization` together with one or more `x-api-key` headers), and whether the warning's statement is true for every request that passes authentication with it. Can a caller make the gateway authenticate with a value a proxy or an upstream log did not see, or the reverse?
3. Fail-open paths: errors and refusals on each path (missing, invalid, unreadable or conflicting credentials; admin routes with and without an admin token configured) and whether any refusal path still emits the warning or reveals which value was used.
4. Logging: can a caller inject content into the log line, flood it, or suppress the one-time warning for a later real case; does the once-per-process flag introduce any race that matters.
5. Test quality: do the new tests drive the real code paths or stub the logic under test; are there untested branches that matter.

Checklist from docs/security-review-workflow.md (Authentication and authorization): probe missing, malformed, expired, revoked, replayed and foreign credentials; cross-tenant access; privilege escalation; and fail-open paths as applicable. Security fixes also require a sibling sweep: say whether the change leaves any sibling of the same shape unfixed that you can find.

Report format (it is posted on the PR unedited, labelled agent-generated):
- A first line "Agent-generated adversarial review".
- Reviewed head and range.
- Abuse cases considered.
- Test evidence (what you ran and the results).
- Findings, each with severity (blocking / non-blocking), file:line, a concrete failing input or sequence, and a suggested fix. Say "no findings" for a probe area that held.
- A final verdict line: "Blocking findings: N".

Reviewer report, unedited

Agent-generated adversarial review

Reviewed head and range

• Head: 1c9c44d
• Range: 3186eb0...1c9c44d (commits e3a57f4 and 1c9c44d)
• Files: packages/sie_server/src/sie_server/core/server_auth.py, packages/sie_server/tests/core/test_server_auth.py, packages/sie_gateway/src/middleware/auth.rs
• Also read the code whose behaviour these change: core/data_dir.py (owned_files, remove_stale_files), the gateway audit middleware, the request_token_id callers in handlers/proxy.rs, and the OpenAI-compatible inner-request header copies (proxy, audio, messages, decisions, systemone).

Abuse cases considered

1. Token link check. I tried to get a foreign hard link counted as temporary, hidden, or left in place while the token is accepted:
   • entries listed twice, and case-variant names (uppercase hex)
   • a directory named like a temporary link, and symbolic links
   • a link in another directory
   • a temporary link renamed during the scan, and a link added before and removed after the scan so the counts balance
   • an entry swapped between the lstat and the unlink
   • a token owned by another user, the bound on scan attempts, and the privacy of the token directory itself
2. Repeated Authorization. I checked which credential authenticates for each shape:
   • the first value empty, blank, Bearer with no token, Basic ..., or unreadable
   • an unreadable later value
   • the header name in mixed case (AUTHORIZATION, X-Api-Key)
   • one comma-joined value
   • Authorization together with one or more agreeing, conflicting or unreadable x-api-key headers
   • each of these on regular and admin routes, with and without an admin token configured
3. Fail-open paths. Missing, invalid, unreadable and conflicting credentials, Admin token required, Admin token not configured, and misconfigured auth. For each I checked whether a refusal logs or spends the warning, or reveals which value was compared.
4. Logging. Log injection, flooding, spending the one-time warning, and the ordering of the once-per-process flag.
5. Test quality. Whether the new tests drive the real code, mutation of the fix, and untested branches.

Test evidence

• Server suite. packages/sie_server/tests/core/test_server_auth.py at head: 153 passed. It ran in a local Python 3.12 environment with the head's packages/sie_server/src first on PYTHONPATH.

• Mutation. Restoring len(linked) at server_auth.py:293 makes both cases of test_a_temporary_link_listed_twice_by_the_scan_counts_once fail (2 failed). So the new test tells the fix apart from the old code.

• Link probes. These used real hard links and the real fstat. owned_files was wrapped only to inject a concurrent action at a fixed point.

  Probe	Head	Base
  C: listing repeats the temporary link, foreign link in another directory	refused	ACCEPTED
  F: directory named like a temporary link	refused	refused
  G: uppercase-hex temporary-looking link	refused	refused
  A: temporary link renamed during the scan, foreign link present	ACCEPTED	ACCEPTED
  B: temporary link added before and removed after the listing, foreign link present	ACCEPTED	ACCEPTED
  E: entry replaced between lstat and unlink	cleanup unlinked the replacement	same

• Gateway suite. cargo +1.98.1 test --offline -p sie-gateway --lib middleware::auth at head: 51 passed.

• Gateway probe matrix. 14 header shapes went through the real AuthLayer with a capturing subscriber, using tokens tok-AAAA, tok-BBBB, tok-CCCC and admin admin-ZZZZ:

  • Warns: shapes e (two tokens), f (token then unreadable) and h (blank first, two different later tokens, x-api-key). Each returned 200 and logged the warning once.
  • Refused, no warning: unreadable first, Bearer first, Basic first, comma-joined, and a blank first with no x-api-key. Each returned 401.
  • Passes silently: a blank first value, one later token and an x-api-key returned 200 authenticated as ****BBBB, with disagree=false and no warning (finding 1).

• Over the wire. axum::serve on 127.0.0.1 with raw HTTP/1.1 confirmed the same results, including mixed-case header names.

• Server comparison. The same request against sie-server authenticates as tok-AAAA. request_credentials returns {b'tok-AAAA'}, and the server returns 401 when only tok-BBBB is configured.

• Candidate fix. A recount after cleanup (finding 3) refuses probes A, B and E, keeps C refused, and the 153 server tests still pass.

Findings

1. Non-blocking (pre-existing, not introduced here). packages/sie_gateway/src/middleware/auth.rs:311 (extract_request_credential reads only the first Authorization value) and auth.rs:351 (authorization_values_disagree).

   • Input: Authorization: (empty or blank), Authorization: Bearer tok-AAAA, x-api-key: tok-BBBB.
   • Result: the gateway answers 200 authenticated as tok-BBBB, with no warning. The request is also outside the planned 0.13.0 refusal, because authorization_values_disagree returns false for it.
   • Admin routes: with x-api-key: admin-ZZZZ and a user token in the second Authorization value, POST /v1/pools succeeds.
   • Why it matters: sie-server authenticates the same request as tok-AAAA. Any proxy or log that applies the documented rule ("when both are sent, Authorization is used", COMPATIBILITY.md:251-252) by reading the first non-empty Authorization value attributes the request to a credential the gateway never checked. COMPATIBILITY.md:254 names a deployment that relies on that rule: a proxy that adds an x-api-key of its own. If such a proxy admits a client because of a later Authorization value, the gateway authenticates the client with the proxy's key.
   • Without x-api-key: the same shape is refused with 401, so it fails closed.
   • Suggested fix: treat "the first Authorization value carries no token and a later value does" as a repeated-Authorization case. Make authorization_values_disagree (or a sibling predicate) true for it, so it warns now and is refused from 0.13.0. The new warning text is already accurate for this shape. Keep which header authenticates unchanged in this release.

2. Non-blocking (sibling sweep, documentation). These still make the claim this PR corrected in the log line:

   • COMPATIBILITY.md:256 and :276
   • SECURITY.md:67-69
   • packages/sie_gateway/README.md:96
   • deploy/helm/sie-cluster/README.md:1566-1568
   • packages/sie_gateway/docs/architecture-guide.md:519

   They say a repeated Authorization header "uses its first value", or that when both headers are sent Authorization is used.

   • Input: Authorization:, Authorization: Bearer user, Authorization: Bearer bogus, x-api-key: user. It returns 200 via x-api-key, not via the first value.
   • Suggested fix: reuse the log line's wording: the later Authorization values are ignored, and when the first one carries no token, x-api-key decides.

3. Non-blocking (pre-existing, same shape as the fixed bug). packages/sie_server/src/sie_server/core/server_auth.py:293-299. Removing duplicate names fixes repeated listings. But the scan can still report more temporary links than ever exist together, and that path needs write access to the token directory.

   • Probe A: a token link foreign, plus a temporary link renamed from .auth-token.sie-server-aaaa... to ...bbbb... while the scan runs. The token is accepted and foreign stays (nlink 2).
   • Probe B: a temporary link created before and removed after the listing, with a foreign link present. The token is accepted.
   • Probe E: the cleanup unlinks whatever replaced the entry after its lstat. Only temporary-pattern names inside the token directory are exposed to this.
   • Suggested fix: after the unlink loop, fstat again and accept only when st_nlink == 1. Keep the count rule only when every unlink failed with EACCES or EROFS (read-only directory). Add a rename-during-scan test.

4. Non-blocking (pre-existing, outside the diff, but the precondition for finding 3). server_auth.py:217-228. check_private_directory checks only mode bits.

   • Input (macOS): a 0700 directory with the ACL entry everyone allow add_file,add_subdirectory,delete_child passes the check (verified locally). Another account could then add, rename or delete entries in the token directory.
   • Linux: POSIX ACLs show up in the group bits, so the existing check covers them.
   • Suggested fix: refuse a token directory that carries an extended ACL on macOS, or document that it must not have one.

5. Non-blocking (test quality). In auth.rs at lines 1363, 1370 and 1380, the passing cases send x-api-key: user while the second Authorization value is also Bearer user. A 200 alone therefore does not show which header authenticated. Only the "missing x-api-key" refusal at line 1356 pins it, indirectly. No test covers the shape in finding 1.

   • Suggested fix: add a passing case whose x-api-key differs from every Authorization value, and assert request_token_id.

Probe areas that held (no findings)

• Link check:
  • Repeated listings are now counted once (probe C).
  • Uppercase or other off-pattern names and directories are treated as foreign and refused.
  • Symbolic links are excluded through lstat.
  • A token owned by another user gets no temporary links counted, so nothing is unlinked, and it is refused.
  • Links in other directories are never counted.
  • Scans are bounded at 3.
  • The docstring change ("still removing its link") is accurate: once auth-token exists, a start only ever links its own new inode, so the only legitimate change to the token's link count is a removal.
• Warning accuracy: the new statement is true for every request that passes and logs it. Either the first value carried the token and the later ones were ignored, or the first was blank, x-api-key decided, and every Authorization value was ignored.
• Fail-open: none found. Every refusal returns before warn_on_repeated_authorization. Response bodies are static and do not reveal which value was compared.
• Logging:
  • The message is static, with no request data, so there is no injection.
  • It is logged once per process. There is a single AuthLayer, at server.rs:665.
  • swap guarantees a single log line, and Relaxed ordering is sufficient.
  • Any holder of a valid token can spend the warning. This is by design and unchanged by this PR. Refused requests cannot spend it.
• Gateway siblings: the audit middleware, the proxy's token id and the OpenAI-compatible inner requests all read credentials through extract_request_credential. The inner requests copy every value in order and do not re-authenticate.
• Server siblings: remove_stale_files is the only other link-count user, and it does not count entries.

Blocking findings: 0

[krisztian-gajdar]

The non-blocking findings 1 to 5 from the review above are fixed in #818, which gets its own independent adversarial review before it merges.