fix(msteams): bound federated certificate file reads#109864
fix(msteams): bound federated certificate file reads#109864xydt-juyaohui wants to merge 2 commits into
Conversation
openclaw-barnacle
Bot
added
channel: msteams
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 19, 2026, 9:52 PM ET / July 20, 2026, 01:52 UTC. Summary PR surface: Source +13, Tests +30. Total +43 across 2 files. Reproducibility: no. the source clearly shows the former unbounded synchronous read, but this review has no executed current-head filesystem reproduction or live oversized-file trace. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the shared bounded-reader approach, add an actual filesystem regression that proves both a normal PEM and a file over the 1 MiB limit, and attach redacted after-fix terminal or log evidence before merge. Do we have a high-confidence way to reproduce the issue? No; the source clearly shows the former unbounded synchronous read, but this review has no executed current-head filesystem reproduction or live oversized-file trace. Is this the best way to solve the issue? Yes, reusing the shared bounded secret-file reader is the narrowest maintainable fix; it needs actual boundary coverage and redacted real behavior proof rather than another mocked-only assertion. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 27f05c8993fb. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +13, Tests +30. Total +43 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles)
|
3d8c3f4 to
78fd737
Compare
CI status noteThis change only touches
The remaining CI failures are main-wide and unrelated to this change: 1. 2. 3. Happy to rebase once main's lint/memory baseline settles. The sibling sweep this builds on is #109260 (which itself introduced the |
createFederatedApp read the certificate path with an unbounded fs.readFileSync, so a misconfigured or oversized certificate file could trigger an unbounded read at credential-load time. Adopt tryReadSecretFileSync (the same helper the credential-reader sweep in openclaw#109260 applied to anthropic-vertex / google / gh-read / mantis) with a 1 MiB maxBytes bound so oversized certificate files are rejected at the size check rather than read whole. Missing or empty files surface as the existing "Failed to read certificate file" error. Update the sdk tests to mock tryReadSecretFileSync and add a regression case asserting an oversized file (helper returns undefined) fails the load with the bound applied.
78fd737 to
b77f3dc
Compare
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
Update after rebasing onto main (e0478e2)Rebased to include #109826 (test/script typecheck repair), #109845 (web push migration lint), and #109863 (CLI startup metadata cache reuse).
Leaving this for maintainer review - the change itself is green locally (vitest 19/19, oxlint, tsgo) and the remaining failures are main-wide baseline issues. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
1 participant
What Problem This Solves
createFederatedApp(inextensions/msteams/src/sdk.ts) read the federated certificate path with an unboundedfs.readFileSync(creds.certificatePath, "utf-8"), so a misconfigured or oversized certificate file could trigger an unbounded read at credential-load time.PR #109260 ran a credential-reader sweep that adopted
tryReadSecretFileSync(with amaxBytesbound) across anthropic-vertex, google, gh-read, and mantis readers, but the MS Teams federated certificate read was missed. This applies the same helper there with a 1 MiB bound, so an oversized certificate file is rejected at the size check instead of being read whole. Missing or empty files still surface as the existing "Failed to read certificate file" error.Evidence
fs.readFileSyncwithtryReadSecretFileSync(..., ``maxBytes: 1_048_576, rejectHardlinks: false)`, mirroring the fix: reject oversized credential files in remaining readers #109260 sweep shape (e.g. anthropic-vertex/region.ts).tryReadSecretFileSyncreturnsundefinedfor missing or over-limit files (its {BT}statSync().size > maxBytes{BT} guard), so the load path now throws "Failed to read certificate file ... file is empty or missing" in that case instead of proceeding with an empty/oversized key.extensions/msteams/src/sdk.test.tsis green: 19 passed (19).undefined(the over-limit path) and asserts the load fails andtryReadSecretFileSyncis called with the bound.Sibling of #109260 (same tryReadSecretFileSync helper, same maxBytes-bound sweep); applies it to the MS Teams federated certificate read that the original sweep missed.