feat(identity): add bounded GitHub OIDC transport - #201
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an injectable HTTPS transport for GitHub OIDC metadata and JWKS retrieval. It validates URLs and configuration, limits response reads, checks redirects and JSON objects, sanitizes failures, exports the transport, packages the module, and adds hermetic tests. ChangesGitHub OIDC transport
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GitHubOidcHttpsTransport
participant GitHubOIDCEndpoint
Caller->>GitHubOidcHttpsTransport: Request metadata or JWKS
GitHubOidcHttpsTransport->>GitHubOIDCEndpoint: Send bounded HTTPS GET
GitHubOIDCEndpoint-->>GitHubOidcHttpsTransport: Return response bytes
GitHubOidcHttpsTransport-->>Caller: Return JSON object or RuntimeError
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odoo_forge/identity_github/transport.py`:
- Around line 43-46: Update get_metadata and/or _validate_https_url to reject
issuer URLs containing a query component before appending
_OPENID_CONFIGURATION_PATH. Preserve acceptance of valid HTTPS issuers without
queries and ensure the existing URL validation error behavior is used.
- Around line 55-63: Move concrete urllib I/O out of _read_response in the
identity GitHub transport and introduce an injected, owned bounded HTTP port
that the core uses for fetching response bytes while retaining OIDC protocol and
response handling there. Implement the urllib-backed port in the adapter
package, preserving timeout and maximum-response-size enforcement. Update
tests/identity_github/test_transport.py lines 56-141 to use a fake port and
assert the port contract instead of patching urllib.request.urlopen; no direct
test change is required beyond this replacement.
- Around line 64-65: Update the exception handling in the GitHub OIDC transport
request flow to raise RuntimeError from None instead of chaining exc,
suppressing the underlying transport exception in formatted tracebacks. Add an
assertion in test_transport.py verifying the formatted traceback omits the
chained URLError details.
- Around line 33-41: Update the transport constructor validation around _timeout
and _max_response_bytes to reject non-finite timeout values and require
max_response_bytes to be a positive integer before assignment. Preserve the
existing ValueError behavior for invalid inputs, and add tests covering NaN,
infinity, and a fractional response limit such as 1.5.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a79ad67-19d7-4bf5-be07-4bf0e730ce71
📒 Files selected for processing (2)
src/odoo_forge/identity_github/transport.pytests/identity_github/test_transport.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/odoo_forge/identity_github/transport.py (2)
48-51:⚠️ Potential issue | 🟡 MinorReject empty query and fragment delimiters before building the discovery URL.
urlsplit()removes delimiters from its result fields, so?and#produce empty values. (docs.python.org)Therefore,
https://issuer.example?andhttps://issuer.example#pass the truthiness checks.get_metadatathen appends the discovery path to the raw delimiter and builds the wrong URL. Reject the delimiters or normalize the parsed URL before concatenation. Add both regression cases.Suggested validation change
- or parsed.fragment - or (not allow_query and parsed.query) + or "#" in url + or (not allow_query and "?" in url)Also applies to: 89-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odoo_forge/identity_github/transport.py` around lines 48 - 51, Update _validate_https_url, used by get_metadata, to reject issuers containing empty query or fragment delimiters (`?` or `#`) rather than relying only on urlsplit() field truthiness. Ensure URL normalization or validation occurs before get_metadata appends _OPENID_CONFIGURATION_PATH, and add regression coverage for both delimiter-only cases.Source: MCP tools
60-71:⚠️ Potential issue | 🟠 MajorMove concrete HTTPS I/O behind an owned port.
GitHubOidcHttpsTransport._read_responsestill performsurllib.request.urlopenI/O insidesrc/odoo_forge/**.GitHubOidcTransportdoes not inject an HTTP dependency. Move theurllibimplementation to an adapter package and inject an owned bounded HTTP port. Update the tests to fake that port.As per path instructions,
src/odoo_forge/**is the pure domain core and must not import adapter packages or perform direct network I/O; it may depend only on its own ports and Pydantic models.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odoo_forge/identity_github/transport.py` around lines 60 - 71, Refactor GitHubOidcHttpsTransport._read_response so the domain transport no longer imports or calls urllib.request.urlopen directly. Define or reuse an owned bounded HTTP port, inject it through GitHubOidcTransport, and move the concrete urllib implementation into the adapter package while preserving HTTPS validation, timeout/response-size limits, and existing failure behavior. Update the related tests to provide a fake implementation of the injected port.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/odoo_forge/identity_github/transport.py`:
- Around line 48-51: Update _validate_https_url, used by get_metadata, to reject
issuers containing empty query or fragment delimiters (`?` or `#`) rather than
relying only on urlsplit() field truthiness. Ensure URL normalization or
validation occurs before get_metadata appends _OPENID_CONFIGURATION_PATH, and
add regression coverage for both delimiter-only cases.
- Around line 60-71: Refactor GitHubOidcHttpsTransport._read_response so the
domain transport no longer imports or calls urllib.request.urlopen directly.
Define or reuse an owned bounded HTTP port, inject it through
GitHubOidcTransport, and move the concrete urllib implementation into the
adapter package while preserving HTTPS validation, timeout/response-size limits,
and existing failure behavior. Update the related tests to provide a fake
implementation of the injected port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 965bfbd7-29f2-4848-aed1-1ebda6995500
📒 Files selected for processing (2)
src/odoo_forge/identity_github/transport.pytests/identity_github/test_transport.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odoo_forge_identity_github/transport.py (1)
67-69: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate every redirect hop before following it.
urllib.request.urlopenfollows eachLocationbefore_read_responsechecksresponse.geturl(). An HTTPS URL can therefore redirect to HTTP and perform that request first. Reject every resolved redirect unless its scheme ishttps, retain the final check, and add a hermetic multi-hop redirect test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odoo_forge_identity_github/transport.py` around lines 67 - 69, Update the request flow around _read_response so every redirect hop is validated as HTTPS before urllib follows it, rather than checking only the final response URL. Preserve the existing final _validate_https_url check, and add a hermetic multi-hop redirect test covering rejection of any redirect resolving to HTTP.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odoo_forge_identity_github/transport.py`:
- Around line 99-100: Update the URL validation in get_jwks to reject a bare “#”
fragment delimiter unconditionally, while continuing to reject “?” only when
allow_query is false; add a regression test covering a JWKS URL ending in “#”.
---
Outside diff comments:
In `@src/odoo_forge_identity_github/transport.py`:
- Around line 67-69: Update the request flow around _read_response so every
redirect hop is validated as HTTPS before urllib follows it, rather than
checking only the final response URL. Preserve the existing final
_validate_https_url check, and add a hermetic multi-hop redirect test covering
rejection of any redirect resolving to HTTP.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c1bf330-be2f-4294-829c-0114901450fb
📒 Files selected for processing (4)
pyproject.tomlsrc/odoo_forge_identity_github/__init__.pysrc/odoo_forge_identity_github/transport.pytests/identity_github/test_transport.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odoo_forge_identity_github/transport.py`:
- Line 48: Update GitHubOidcHttpsTransport’s constructor to accept a typed
opener or bounded HTTP port, store and use that injected dependency instead of
calling urllib.request.build_opener in the transport, and construct the concrete
urllib opener only at the composition boundary. Update tests to pass a fake
dependency rather than patching urllib.request.build_opener.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 04038d9a-bb92-4b15-ba8b-bbd4640f26f6
📒 Files selected for processing (2)
src/odoo_forge_identity_github/transport.pytests/identity_github/test_transport.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/identity_github/test_transport.py`:
- Line 16: Refactor the redirect-rejection test to remove the
`_HttpsRedirectHandler` import and all direct calls to its private method.
Exercise the behavior through the public transport factory and transport API,
using a loopback HTTPS server if needed; mark the resulting test as integration
and deselected by default when it requires that server.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e6cbd4d-f433-4de9-8605-2ee4e37c0c72
📒 Files selected for processing (3)
src/odoo_forge_identity_github/__init__.pysrc/odoo_forge_identity_github/transport.pytests/identity_github/test_transport.py
2 participants
Closes #198
Type
Summary
Changes
src/odoo_forge/identity_github/transport.pytests/identity_github/test_transport.pyTest plan
uv run pytest tests/identity_github/test_transport.py— 7 passedChain context
Strategy: stacked to
main.main→ 📍 PR 1 transport → PR 2 provider → PR 3 JOSE validationThis PR starts with the transport boundary and ends with hermetic transport verification. PR 2 depends on this merge. Provider behavior and JOSE validation remain out of scope. Rollback removes these two files.
Contributor checklist
type:*labelSummary by CodeRabbit
New Features
Bug Fixes
Tests