Repository navigation
fix(security): stop an API key from sending every future transcript to a folder it picks - #533
Merged
Merged
Conversation
… key's export.dir and the export's `to` decide where every finished call's transcript and audio are written, yet an admin key could set them over the API, from another machine, and point them at a share it controls. Both are now taken only from the user on this computer: the app's own token on the desktop app's loopback listener (akou config set, akou export --to) and the desktop window. An ak_ key, the window's page in a browser and server mode get 400.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 47 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
export.dirdecides where every finished call goes: the Markdown, the event log and, withexport.audio: copy, the audio. An admin API key could set it withPATCH /v1/config, also from another machine through the network listener. It could point it at a share it controls, such as\\host\shareon Windows or/net/<host>on macOS. Every later call then went there, even after the key was revoked. On Windows each write also sends the user's NTLM hash to that host. ThetoofPOST /calls/{id}/exporthad the same gap for one call.webhook.urlis already file only for this reason.Fix
Only the user on this computer can now choose these folders. That means the app's own token on the desktop app's loopback listener, so
akou config set export.dirandakou export --to DIRwork as before. It also means the desktop window. Some callers now get 400: anak_key, the window's page in a browser, and any caller in server mode. A key can still export a call into the folder the user set. The rule is a newkeyWritable: falseflag in the settings schema, next toapiWritableandwindowWritable. The API reference, the configuration page, DESIGN and TRAPS (EXP-1, EXP-2) say this.Testing
tests/export-access.test.tsruns each refused caller through the real routes. Against main's code, 6 of its 10 tests fail. With the fix, all 10 pass. The positive controls are:export.dirandtoasr.threadstostill goes intoexport.dircheck:static, the settings-doc check and 13 related test files (193 tests, including the e2e suites that callPATCH /configand--towith the app token) pass.