Reject non-plaintext credential tokens before pool entry - #49
Conversation
Encrypted envelope objects in accessToken/refreshToken now fail fast in CredentialManager instead of leaking into upstream Authorization headers; files carrying them are kept out of the credential pool with a one-time audit failure event, and the existing import validation keeps rejecting them before persistence.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @maiphucgiang, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 3 days and 18 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's Guide通过统一的 token 类型守卫，在凭据会话使用和凭据池加载两个边界阻止非明文 token 泄漏到上游；非法文件被隔离并仅记录一次审计失败，同时新增覆盖导入、入池、重复扫描和文件替换场景的回归测试。 Sequence diagram for plaintext token validation and credential pool loadingsequenceDiagram
participant Pool as CredentialPool
participant Manager as CredentialManager
participant Auth as auth_oauth
participant Audit as AuditLog
participant Upstream as UpstreamAPI
Pool->>Manager: summary()
Manager->>Manager: _session()
Manager->>Auth: ensure_plaintext_tokens(auth)
alt accessToken or refreshToken is non-string
Auth-->>Manager: AuthTokenTypeError
Manager-->>Pool: reject credential
Pool->>Audit: _log(failure)
Pool-->>Pool: remember cid in _ignored_invalid
else plaintext tokens
Auth-->>Manager: validation succeeds
Manager-->>Pool: credential summary
Pool->>Upstream: use credential headers
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db586150d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
maiphucgiang
changed the title
validate_cred_data now refuses non-string values under accessToken, access_token, token, refreshToken and refresh_token before persistence, so an envelope refreshToken can no longer overwrite a healthy credential that the pool would later reject as unusable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6c6d3ecb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if cid not in self._ignored_invalid: | ||
| _log(f"[cred] 拒绝入池（入库校验失败：{error}）: {Path(cid).name}") | ||
| self._ignored_invalid.add(cid) |
There was a problem hiding this comment.
When scan mode observes an invalid credential being deleted and a later invalid credential reuses the same pathname, this process-lifetime set still contains cid, so the new rejection produces no audit log. Unlike _ignored_duplicates, prune() never removes nonexistent paths from _ignored_invalid; this also lets the set grow without bound in directories where invalid credential filenames churn. Prune this set alongside the duplicate-path set so suppression applies only to repeated scans of the same file.
Useful? React with 👍 / 👎.
prune() now drops _ignored_invalid entries for missing paths like the duplicate set, so a recreated invalid file at a reused path warns again and the set cannot grow without bound.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3bc729981
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def ensure_plaintext_tokens(auth: dict) -> None: | ||
| """Reject non-string token fields so encrypted envelope objects never reach upstream headers.""" | ||
| for field in ("accessToken", "refreshToken"): |
There was a problem hiding this comment.
Although import validation now checks aliases, the documented direct-copy path scans .info files without calling validation or normalization, and this runtime guard checks only canonical names. Consequently, a file with a plaintext accessToken and object-valued refresh_token is admitted to the pool; it serves until refresh, when _refresh_locked() reads the absent refreshToken and sends an empty refresh header instead of rejecting and auditing the envelope. Include access_token, token, and refresh_token here (or normalize scanned credentials before summary()).
Useful? React with 👍 / 👎.
A replaced invalid credential at a reused path can share inode and mtime with the file it replaced, so path-presence pruning alone missed it; suppression now compares a content digest, warns once per distinct file and stays silent across rescans of unchanged content.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a89a03925
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| manager = CredentialManager(Path(cid)) | ||
| try: | ||
| summary = manager.summary() | ||
| except auth_oauth.AuthTokenTypeError as error: |
There was a problem hiding this comment.
When an already-pooled .info file is externally replaced with an envelope credential, reload() takes the existing-entry branch, whose broad exception handler returns before this new rejection logic runs. The stale entry therefore remains indefinitely with its old UID and identity, produces no rejection log, and can cause find_by_uid() to reject a valid repair imported under another filename as an account conflict. Handle AuthTokenTypeError in the existing-entry branch too and remove or invalidate that entry.
Useful? React with 👍 / 👎.
The existing-entry reload branch now treats AuthTokenTypeError like a removal: the stale identity is dropped with its ledger, cooldown, sticky and sync state instead of lingering without a warning or blocking a valid repair under another filename.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4076b4a76c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # reusing the path warns again even without a scan while missing. | ||
| _log(f"[cred] 拒绝入池（入库校验失败：{error}）: {Path(cid).name}") | ||
| self._ignored_invalid[cid] = digest | ||
| continue |
There was a problem hiding this comment.
When the pool is constructed with --auth-file (scan=False) and a configured file contains an envelope token, this continue leaves the path out of _entries; _rescan() derives its paths solely from the remaining entries in that mode. Consequently, replacing the rejected file with a valid plaintext credential is never detected, and a sole credential remains unavailable until the process restarts. The same permanent loss occurs when an existing explicit entry is evicted by the new AuthTokenTypeError branch, so configured paths need to be tracked independently and retried.
Useful? React with 👍 / 👎.
Explicit --auth-file mode now remembers every configured path and rescans that list, so a file rejected for envelope tokens or an entry evicted after an external replacement re-enters the pool as soon as the file is repaired, without waiting for a process restart.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e301825fdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if cid not in self._configured: | ||
| self._configured.append(cid) |
There was a problem hiding this comment.
When running with --auth-file (scan=False), every path passed to reload() is now permanently treated as configured, including credentials added later through the admin/OAuth import paths. If such a dynamically added credential is deleted, remove_file() never removes it from _configured, so every request continues probing that tombstone and an unrelated external file recreated under the same name is silently admitted even though explicit mode is supposed to use only configured files. Over credential churn this list and the per-request filesystem work also grow without bound; retain constructor paths separately or remove non-explicit paths on intentional deletion.
Useful? React with 👍 / 👎.
1 participant
Changes
ensure_plaintext_tokens/AuthTokenTypeErrorinapp/auth_oauth.py:accessToken/refreshTokenmust be plaintext strings whenever present, with an explicit error pointing operators to OAuth login for encrypted envelope files.accessToken,access_token,token,refreshToken,refresh_token) invalidate_cred_data, so an envelope refreshToken or alias can no longer pass import validation and overwrite a healthy credential the pool would later reject.CredentialManager._session, so summary, header builds and refresh all fail fast instead of interpolating a non-string object intoAuthorizationand burning the credential on upstream 401s.CredentialPool.reloadskips them with a one-time[cred]audit failure event. Suppression is keyed to a content digest, so a different invalid credential reusing a path warns again while unchanged content stays silent, andprune()drops entries for vanished paths.--auth-filepaths after rejection or eviction: explicit mode remembers every configured path and rescans that list, so repairing the file re-enters the pool without a restart.Verification
tests/test_credential_runtime.pyandtests/test_auth_oauth.py: envelope tokens never enter the pool and log once per distinct content, an envelope-onlyrefreshTokenis rejected, a replaced file at a reused path warns again, a swapped pooled file evicts its entry and the next scan rejects it once, rejected and evicted explicit paths recover after repair, envelope values under every token field and alias are rejected before persistence while string aliases still pass, andensure_plaintext_tokensaccepts strings and missing fields and rejects dict/number/list/bool values.failureevent, repeat syncs do not re-admit it, and a different invalid credential at the same path warns again; importing a plaintextaccessTokenpaired with an enveloperefreshTokenreturns 400 and never reaches the pool; a pooled probe replaced by an envelope at the same path has its stale entry evicted on the next scan; zero-multiplierdeepseek-v4.1-flashchats return 200 against the real upstream throughout.