fix: OAuth2 token encoding and /auth/info route - #49
Conversation
Coding-Dev-Tools
commented
Aug 9, 2026
Coding-Dev-Tools
commented
- URL-encode 1Password Connect filter keys for special characters\n- Encode OAuth2 introspection tokens\n- Add /auth/info route and tests\n- Fix dead code in serve.py
…l characters OnePasswordStore.get() and delete() injected the key directly into the filter query parameter without URL encoding. Keys containing &, =, #, spaces, or quotes produced malformed URLs and failed to match. Fix: apply urllib.parse.quote(key, safe='') before embedding in the filter string, matching the pattern used by serve.py OAuth2 tokens. Added 2 regression tests covering get/delete with special-character keys.
🤖 Automated Code Review✅ Ruff Lint — No issues
|
![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: 343b88f353
ℹ️ 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".
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Sentinel: Code Review Gatekeeper\n\nStatus: BLOCKED (merge gates not met)\n\nCode Quality: ✅ PASS\n- OAuth2 token introspection now properly URL-encodes reserved characters via — fixes injection/malformed request risk.\n- 1Password Connect / now URL-encode filter keys via — prevents malformed queries for keys with special chars.\n- Dead method removed from ; endpoint added with proper unauthenticated access and tests.\n- Ruff formatting applied to test files.\n- All CI checks passing (3.11, 3.12, 3.13 + automated code review).\n\nSecurity: ✅ No secrets, credentials, or unsafe patterns detected. Encoding fixes are security-positive.\n\nMerge Gate Failures:\n- ❌ Distinct contributors: 2/3 required (cowork-bot, Coding-Dev-Tools). Need at least 1 more distinct agent author.\n- ❌ Reviewer approvals: 0/3 required. Codex left suggestions but no formal approval.\n- ❌ PR age: ~9 hours old — passes 6h minimum.\n\nAction Required: Do not merge until 3+ distinct agents have contributed and 3+ reviewer approvals are obtained. Code changes themselves are approved.
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Sentinel: Code Review Gatekeeper
Status: BLOCKED (merge gates not met)
Code Quality: ✅ PASS
- OAuth2 token introspection now properly URL-encodes reserved characters via
urlencode()— fixes injection/malformed request risk. - 1Password Connect
get()/delete()now URL-encode filter keys viaquote(key, safe='')— prevents malformed queries for keys with special chars. - Dead
_check_authmethod removed fromserve.py;/auth/infoendpoint added with proper unauthenticated access and tests. - Ruff formatting applied to test files.
- All CI checks passing (3.11, 3.12, 3.13 + automated code review).
Security: ✅ No secrets, credentials, or unsafe patterns detected. Encoding fixes are security-positive.
Merge Gate Failures:
- ❌ Distinct contributors: 2/3 required (cowork-bot, Coding-Dev-Tools). Need at least 1 more distinct agent author.
- ❌ Reviewer approvals: 0/3 required. Codex left suggestions but no formal approval.
- ✅ PR age: ~9 hours old — passes 6h minimum.
Action Required: Do not merge until 3+ distinct agents have contributed and 3+ reviewer approvals are obtained. Code changes themselves are approved.
…ad of discarding all
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0831ab7f50
ℹ️ 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".
Closes coverage gaps in BearerAuth, ApiKeyAuth, OAuth2Auth (userinfo, introspect, cache, scope/audience validation, error paths), MultiAuth fallback logic, and build_auth_from_env factory. 527 tests pass, ruff clean.
… automated code review
…nd; batch Doppler get_many
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd3be4f5b
ℹ️ 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 type(exc).__name__ == "InvalidPath": | ||
| return True | ||
| message = str(exc).lower() | ||
| return "404" in message or "not found" in message or "path" in message and "missing" in message |
There was a problem hiding this comment.
When a real Vault failure contains 404 for another reason—such as a connection error to vault.internal:4040 or a permission error whose requested path contains 404—this predicate classifies it as a missing secret. The new get, delete, and list_keys handlers then silently return None, False, or [] instead of surfacing the operational failure they are intended to preserve; inspect the exception type or structured HTTP status rather than searching the entire message for this substring.
Useful? React with 👍 / 👎.
…te_env_var rotate_all previously rewrote the .env file once per key via N sequential read-modify-write cycles: slow, and a crash mid-loop left the file partially rotated. New rotate_env_file() plans all replacements, applies them in one pass over the lines, and writes atomically (temp file + fsync + os.replace) so a crash can never leave a truncated/half-rotated .env. CLI rotate-all now uses it. 6 regression tests added.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d667c835f
ℹ️ 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".
| """ | ||
| tmp = path.with_name(f".{path.name}.rotate-tmp-{os.getpid()}") | ||
| try: | ||
| with open(tmp, "w") as f: |
There was a problem hiding this comment.
When the source .env is secured with mode 0600 and the process has a typical 0022 umask, opening this new temporary file creates it as 0644; os.replace() then installs those permissions on the rotated .env. A successful rotate-all therefore makes every newly generated secret readable by other local users, so copy the original file mode to the temporary file before replacing it.
Useful? React with 👍 / 👎.
| if m and m.group(1) in plan and m.group(1) not in seen: | ||
| key = m.group(1) | ||
| seen.add(key) |
There was a problem hiding this comment.
For a file containing the same key more than once, such as TOKEN=first followed by TOKEN=effective, dotenv_values() places one TOKEN in the plan but this seen condition rewrites only the first occurrence. The later occurrence remains unchanged and continues to be the effective value when the file is loaded, even though the command reports and audits a successful rotation.
Useful? React with 👍 / 👎.
| f.write(content) | ||
| f.flush() | ||
| os.fsync(f.fileno()) | ||
| os.replace(tmp, path) |
There was a problem hiding this comment.
When the configured environment file is a symlink, the preceding read follows the link but os.replace(tmp, path) replaces the symlink itself with a regular file. This leaves the original shared/generated target containing the old secrets and silently detaches this environment from future target updates; resolve the target before creating and replacing the temporary file, or otherwise preserve the link.
Useful? React with 👍 / 👎.
- _get_commit_meta: NUL (%x00) field separators so author names/subjects
containing '|' no longer silently drop every change of a commit; run the
lookup inside the target repo via cwd instead of the process CWD
- pass POSIX ('/') paths to git log/show so nested .env files resolve on
Windows (backslash pathspecs matched nothing -> empty history)
- add tests/test_history.py: meta parsing with '|' authors, mask behavior,
end-to-end history detection in a real temp repo incl. nested path
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
DopplerStore.get/get_many/set/delete/list_keys and OnePasswordStore._api_get/_api_post/delete were silently swallowing server errors (5xx, auth failures) by returning None/False instead of raising SecretStoreError. This matches the fix pattern already applied to VaultStore in a prior run: raise SecretStoreError on genuine errors while preserving None/False for "not found" (404) responses. Changes: - DopplerStore.get: raise SecretStoreError on non-404, non-200 status codes - DopplerStore.get_many: raise SecretStoreError on non-404, non-200 - DopplerStore.set: raise SecretStoreError on non-2xx (was False) - DopplerStore.delete: raise SecretStoreError on non-204/404 (was False for all non-204) - DopplerStore.list_keys: raise SecretStoreError on non-200 (was []) - OnePasswordStore._api_get: raise SecretStoreError on non-404, non-200 (was None) - OnePasswordStore._api_post: raise SecretStoreError on non-2xx (was False) - OnePasswordStore.delete: raise SecretStoreError on non-204/404 (was False for all non-204) - Add 10 regression tests covering error-raising for Doppler and OnePassword stores 536 tests pass, ruff clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc0c6b2cd2
ℹ️ 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".
| lines = env_file.read_text().split("\n") | ||
| seen: set[str] = set() | ||
| out_lines: list[str] = [] | ||
| key_line = re.compile(r"^\s*([A-Za-z_][A-Za-z0-9_]*)\s*=") |
There was a problem hiding this comment.
When a file contains a plain dotenv key outside shell-identifier syntax, such as SERVICE.API-KEY=old, dotenv_values() includes it in plan, but this regex never matches the line. rotate-all therefore counts and audits the key as rotated while leaving its old secret in place; the previous per-key implementation escaped and matched the complete key. Match the parsed keys rather than restricting replacements to [A-Za-z_][A-Za-z0-9_]*.
Useful? React with 👍 / 👎.
…o prevent .env file corruption on crash
rotate_env_var was writing the .env file via a plain open('w') call,
which is not crash-safe: a power loss, signal, or OOM kill mid-write
would leave the file truncated/corrupted, losing ALL environment
variables for that project. rotate_env_file already uses the module's
_atomic_write() (temp+fsync+os.replace) pattern, but rotate_env_var
missed it.
Fix: replace the plain open/w block with _atomic_write(). The same
atomic template is also backstopped by a regression test
(test_rotate_env_var_atomic_write) that verifies no temp files remain
and that the file content is correct after rotation.
Co-authored-by: cowork-bot <cowork-bot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eb7065c43
ℹ️ 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".
| """ | ||
| tmp = path.with_name(f".{path.name}.rotate-tmp-{os.getpid()}") | ||
| try: | ||
| with open(tmp, "w") as f: |
There was a problem hiding this comment.
When the environment file is in a shared sticky directory, another local user can pre-create the predictable .<name>.rotate-tmp-<pid> path as a symlink; this open(..., "w") follows it and overwrites any file writable by the rotating process before os.replace() installs the symlink as the environment file. Create a uniquely named temporary file atomically with exclusive/no-follow semantics instead of opening a PID-derived pathname.
Useful? React with 👍 / 👎.
1 participant
