fix: add code_verifier to token exchange OpenAPI spec#4098
fix: add code_verifier to token exchange OpenAPI spec#4098chirag1807 wants to merge 2 commits intoory:masterfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds optional OAuth2 PKCE Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/swagger.json (1)
2177-2181: ⚡ Quick winAdd description for the
code_verifierparameter.The newly added
code_verifierparameter lacks a description field. Adding documentation would improve the generated SDK documentation and help API consumers understand this PKCE-related parameter.📝 Suggested enhancement
{ "type": "string", + "description": "OAuth 2.0 PKCE Code Verifier\n\nThe code verifier for the OAuth 2.0 PKCE flow. This parameter is required when the authorization request included a code_challenge. The code_verifier is a cryptographically random string using the characters [A-Z] / [a-z] / [0-9] / \"-\" / \".\" / \"_\" / \"~\", with a minimum length of 43 characters and a maximum length of 128 characters.", "name": "code_verifier", "in": "formData" }Optionally, you could also add format constraints per RFC 7636:
{ "type": "string", + "description": "OAuth 2.0 PKCE Code Verifier\n\nThe code verifier for the OAuth 2.0 PKCE flow. This parameter is required when the authorization request included a code_challenge.", + "minLength": 43, + "maxLength": 128, + "pattern": "^[A-Za-z0-9-._~]+$", "name": "code_verifier", "in": "formData" }Note: Format constraints may be enforced server-side, so the pattern/length constraints are optional depending on your validation strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger.json` around lines 2177 - 2181, Add a descriptive "description" field to the OpenAPI parameter named "code_verifier" so SDKs and docs explain its purpose (it's the PKCE code verifier used in OAuth 2.0 authorization flows). Update the parameter object for "code_verifier" (the parameter with "name": "code_verifier", "in": "formData") to include a concise description such as that it is the original high-entropy random string generated by the client for PKCE, and optionally add RFC 7636 constraints (e.g., allowed characters/pattern and min/max length) if you want schema-level validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/httpclient/docs/OAuth2API.md`:
- Line 1373: The markdown table row for codeVerifier is missing the trailing
cell separator; update the table row for "codeVerifier" (the row showing
**codeVerifier** | **string** | |) to include the fourth cell delimiter by
adding a trailing pipe so the row has four cells properly terminated, ensuring
the table aligns and fixes markdownlint MD055/MD056 warnings.
---
Nitpick comments:
In `@spec/swagger.json`:
- Around line 2177-2181: Add a descriptive "description" field to the OpenAPI
parameter named "code_verifier" so SDKs and docs explain its purpose (it's the
PKCE code verifier used in OAuth 2.0 authorization flows). Update the parameter
object for "code_verifier" (the parameter with "name": "code_verifier", "in":
"formData") to include a concise description such as that it is the original
high-entropy random string generated by the client for PKCE, and optionally add
RFC 7636 constraints (e.g., allowed characters/pattern and min/max length) if
you want schema-level validation.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 420f8f1b-1b5f-4aae-b8a1-f7cb49c517e0
📒 Files selected for processing (6)
internal/httpclient/api/openapi.yamlinternal/httpclient/api_o_auth2.gointernal/httpclient/docs/OAuth2API.mdoauth2/handler.gospec/api.jsonspec/swagger.json
| **grantType** | **string** | | | ||
| **clientId** | **string** | | | ||
| **code** | **string** | | | ||
| **codeVerifier** | **string** | | |
There was a problem hiding this comment.
Fix malformed Markdown table row for codeVerifier
Line 1373 is missing a properly terminated 4th cell, causing markdownlint MD055/MD056 warnings. This can break rendered docs/table alignment in some viewers.
Suggested fix
- **codeVerifier** | **string** | |
+ **codeVerifier** | **string** | | |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **codeVerifier** | **string** | | | |
| **codeVerifier** | **string** | | | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1373-1373: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
[warning] 1373-1373: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/httpclient/docs/OAuth2API.md` at line 1373, The markdown table row
for codeVerifier is missing the trailing cell separator; update the table row
for "codeVerifier" (the row showing **codeVerifier** | **string** | |) to
include the fourth cell delimiter by adding a trailing pipe so the row has four
cells properly terminated, ensuring the table aligns and fixes markdownlint
MD055/MD056 warnings.
chirag1807
changed the title
2 participants
This PR fixes a mismatch between the OAuth2 token endpoint implementation and the OpenAPI (Swagger) specification by adding the missing
code_verifierparameter to the token exchange request.Hydra already supports PKCE, but the
code_verifierfield was not included in theswagger:parameters oauth2TokenExchangedefinition. This caused generated SDKs and API documentation to be incomplete.Related issue(s)
oauth2: Missing code_verifier parameter in token exchange OpenAPI spec
Checklist
[ ] I have referenced an issue containing the design document if my changeN/A - updating existing featureintroduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.com) from the maintainers to push
the changes.
works.
[ ] I have added or changed the documentation.N/ASummary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation