fix: Update OAuth authentication URL host to www domain & Bump version to 0.2.2 #30
Conversation
Change-Id: I96cda3ea6a200b85184177851af8b1e4ce27f21d
|
Warning Rate limit exceeded@hanzeINGH has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request represents a version increment for the Coze API Java SDK from version 0.2.1 to 0.2.2. The changes are consistent across multiple files, including the project's POM files, the OAuth client implementation, and an example authentication class. The version update is applied to the project's core artifacts, dependency references, and user agent versioning, ensuring a synchronized version across the project. Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #30 +/- ##
============================================
- Coverage 76.57% 76.37% -0.21%
Complexity 203 203
============================================
Files 41 41
Lines 1144 1147 +3
Branches 83 84 +1
============================================
Hits 876 876
- Misses 210 212 +2
- Partials 58 59 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -54,6 +56,7 @@ protected OAuthClient(OAuthBuilder<?> builder) { | |||
| this.clientSecret = builder.clientSecret; | |||
| this.clientID = builder.clientID; | |||
| this.baseURL = builder.baseURL; | |||
| this.wwwURL = Strings.replace(baseURL, "api.", "www."); | |||
![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 (2)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (1)
49-49: LGTM: Fixed OAuth URL construction using correct subdomain.Good fix for using www subdomain instead of api subdomain for OAuth URLs. However, consider using a URL manipulation library for more robust URL handling.
Consider using
java.net.URLfor URL manipulation instead of string replacement:-this.wwwURL = Strings.replace(baseURL, "api.", "www."); +java.net.URL url = new java.net.URL(baseURL); +this.wwwURL = new java.net.URL(url.getProtocol(), + url.getHost().replace("api.", "www."), + url.getPort(), + url.getFile()).toString();Also applies to: 59-59
example/src/main/java/example/auth/WebOAuthExample.java (1)
50-50: Security improvement: Added state parameter to OAuth flowGood addition of the state parameter to prevent CSRF attacks. This follows OAuth 2.0 best practices by maintaining state between the request and callback.
However, using a hardcoded "state" value reduces its effectiveness.
Consider generating a random state parameter for each request:
- String oauthURL = oauth.getOAuthURL(redirectURI, "state"); + String state = java.util.UUID.randomUUID().toString(); + String oauthURL = oauth.getOAuthURL(redirectURI, state); - oauthURL = oauth.getOAuthURL(redirectURI, "state", "workspaceID"); + oauthURL = oauth.getOAuthURL(redirectURI, state, "workspaceID");Also applies to: 56-56
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/pom.xml(1 hunks)api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java(4 hunks)api/src/main/java/com/coze/openapi/service/utils/UserAgentInterceptor.java(1 hunks)example/pom.xml(1 hunks)example/src/main/java/example/auth/WebOAuthExample.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/pom.xml
- api/src/main/java/com/coze/openapi/service/utils/UserAgentInterceptor.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (Java 11 on Windows)
🔇 Additional comments (4)
example/src/main/java/example/auth/WebOAuthExample.java (1)
50-50: Good security practice: Added state parameter to OAuth flow!Adding the state parameter is a security best practice that helps prevent CSRF attacks in OAuth flows. The state parameter should be a unique, non-guessable value that is validated when processing the callback.
Consider validating the state parameter in your callback handler. Here's a script to check if state validation is implemented:
Also applies to: 56-56
✅ Verification successful
Good security practice confirmed: State parameter correctly added to OAuth flow!
The state parameter is properly implemented in the OAuth URL generation. Since this is a client SDK, the actual validation of the state parameter should be implemented by applications using this library in their callback handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for state parameter validation in callback handlers rg -i "state.*equals|state.*match|validateState|verifyState" .Length of output: 64
Script:
#!/bin/bash # Search for OAuth callback handler implementations rg -i "callback|redirect.*uri|oauth.*callback" -A 5 . # Also search for specific endpoint mappings that might handle OAuth callbacks ast-grep --pattern 'class $_ { $$$ @$_Mapping($$$"/callback"$$$) $$$ }'Length of output: 49537
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (2)
127-127: LGTM: Consistent use of wwwURL for OAuth endpoints.The OAuth URL construction now correctly uses the www subdomain for both regular and workspace-specific authorization endpoints.
Also applies to: 129-129
127-127: Approve URL endpoint changesGood change to use wwwURL for OAuth endpoints, ensuring the correct domain is used for the authorization flow.
Also applies to: 129-129
example/pom.xml (1)
19-19: LGTM: Version bump for bug fix.Appropriate version increment (0.2.1 → 0.2.2) following semantic versioning for a bug fix.
| @@ -45,6 +46,7 @@ public abstract class OAuthClient { | |||
| protected final String clientSecret; | |||
| protected final String clientID; | |||
| protected final String baseURL; | |||
| protected final String wwwURL; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate wwwURL construction
While the URL domain separation is correct, the current implementation using simple string replacement could be fragile.
Consider adding validation and using URL parsing:
- this.wwwURL = Strings.replace(baseURL, "api.", "www.");
+ try {
+ java.net.URL url = new java.net.URL(baseURL);
+ String host = url.getHost().replace("api.", "www.");
+ this.wwwURL = new java.net.URL(url.getProtocol(), host, url.getPort(), "").toString();
+ } catch (java.net.MalformedURLException e) {
+ throw new IllegalArgumentException("Invalid base URL: " + baseURL, e);
+ }Also applies to: 59-59
Change-Id: I5fdea142e7f80c43ea9e78047ff0d1e1e38ef65a
fix code format Change-Id: I22bd59dcadd3777eb6ba3eb37ff8a739c469e751
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (1)
59-63:⚠️ Potential issueFix code formatting and improve URL handling.
- Fix the formatting violation:
- }else { + } else {
- The current implementation using simple string replacement could be fragile. Consider using URL parsing:
- if (builder.wwwURL != null) { - this.wwwURL = builder.wwwURL; - } else { - this.wwwURL = Strings.replace(baseURL, "api.", "www."); - } + if (builder.wwwURL != null) { + validateURL(builder.wwwURL); + this.wwwURL = builder.wwwURL; + } else { + try { + java.net.URL url = new java.net.URL(baseURL); + String host = url.getHost().replace("api.", "www."); + this.wwwURL = new java.net.URL(url.getProtocol(), host, url.getPort(), "").toString(); + } catch (java.net.MalformedURLException e) { + throw new IllegalArgumentException("Invalid base URL: " + baseURL, e); + } + }
🧹 Nitpick comments (2)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (2)
131-133: Improve URL path handling.The current URL construction using string concatenation could lead to malformed URLs if
wwwURLends with a slash. Consider usingjava.net.URLor a URL builder:- String uri = wwwURL + "/api/permission/oauth2/authorize"; - if (workspaceID != null) { - uri = wwwURL + String.format("/api/permission/oauth2/workspace_id/%s/authorize", workspaceID); - } + String path = workspaceID != null + ? String.format("/api/permission/oauth2/workspace_id/%s/authorize", workspaceID) + : "/api/permission/oauth2/authorize"; + try { + java.net.URL baseUrl = new java.net.URL(wwwURL); + uri = new java.net.URL(baseUrl, path).toString(); + } catch (java.net.MalformedURLException e) { + throw new IllegalArgumentException("Failed to construct OAuth URL", e); + }
260-260: Add URL validation in the builder.Consider adding validation in the
wwwURLsetter to fail fast if an invalid URL is provided:public T wwwURL(String wwwURL) { + try { + new java.net.URL(wwwURL); + } catch (java.net.MalformedURLException e) { + throw new IllegalArgumentException("Invalid www URL: " + wwwURL, e); + } this.wwwURL = wwwURL; return self(); }Also applies to: 286-290
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java
[error] 58-58: Code formatting violation: Incorrect spacing around 'else' keyword. Found 'else{' but expected 'else {'.
🔇 Additional comments (1)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (1)
49-49: LGTM: Field declaration is appropriate.The new
wwwURLfield follows the class's field declaration patterns with proper access level and immutability.
update Change-Id: I7bcbf5ac5d04881da5ed121c34f6c908759d19d2
Change-Id: I4c409c46bb5418912e1cb8ed5403a73e5b364b58
Change-Id: I34d5994b0e6d952139acd854dcb6f4f94a996075
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (1)
58-62: 🛠️ Refactor suggestionUse URL parsing for safer domain conversion.
The current string replacement approach could be fragile. Consider using proper URL parsing for safer domain conversion.
if (builder.wwwURL != null) { this.wwwURL = builder.wwwURL; } else { - this.wwwURL = builder.baseURL.replace("api.", "www."); + try { + java.net.URL url = new java.net.URL(builder.baseURL); + String host = url.getHost().replace("api.", "www."); + this.wwwURL = new java.net.URL(url.getProtocol(), host, url.getPort(), "").toString(); + } catch (java.net.MalformedURLException e) { + throw new IllegalArgumentException("Invalid base URL: " + builder.baseURL, e); + } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 59-59: api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java#L59
Added line #L59 was not covered by tests
🧹 Nitpick comments (1)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (1)
130-132: Improve URL path handling.The current URL construction could lead to malformed URLs if
wwwURLends with a slash. Consider usingjava.net.URLor a URL builder to properly handle path concatenation.- String uri = wwwURL + "/api/permission/oauth2/authorize"; + String baseUri = wwwURL.endsWith("/") ? wwwURL.substring(0, wwwURL.length() - 1) : wwwURL; + String uri = baseUri + "/api/permission/oauth2/authorize"; if (workspaceID != null) { - uri = wwwURL + String.format("/api/permission/oauth2/workspace_id/%s/authorize", workspaceID); + uri = baseUri + String.format("/api/permission/oauth2/workspace_id/%s/authorize", workspaceID); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java(5 hunks)example/src/main/java/example/auth/PKCEOAuthExample.java(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java
[warning] 59-59: api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java#L59
Added line #L59 was not covered by tests
[warning] 286-287: api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java#L286-L287
Added lines #L286 - L287 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (Java 11 on Windows)
🔇 Additional comments (1)
api/src/main/java/com/coze/openapi/service/auth/OAuthClient.java (1)
48-48: LGTM: Field declaration follows class conventions.The new
wwwURLfield is appropriately declared with matching protection level and immutability.
| @@ -45,7 +45,7 @@ public static void main(String[] args) { | |||
| to select the code_challenge_method. | |||
| * */ | |||
| GetPKCEAuthURLResp oauthURL = | |||
| oauth.genOAuthURL(redirectURI, "state", PKCEOAuthClient.CodeChallengeMethod.S256); | |||
| oauth.genOAuthURL(redirectURI, "states", PKCEOAuthClient.CodeChallengeMethod.S256); | |||
There was a problem hiding this comment.
Critical: Incorrect OAuth state parameter name breaks CSRF protection
The state parameter has been changed from "state" to "states", which deviates from the OAuth 2.0 specification. This is incorrect and could break the CSRF protection mechanism, as OAuth providers expect the standard parameter name "state".
Apply this fix to align with the OAuth 2.0 specification:
- oauth.genOAuthURL(redirectURI, "states", PKCEOAuthClient.CodeChallengeMethod.S256);
+ oauth.genOAuthURL(redirectURI, "state", PKCEOAuthClient.CodeChallengeMethod.S256);This is also consistent with the example shown in the comments at line 56.
📝 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.
| oauth.genOAuthURL(redirectURI, "states", PKCEOAuthClient.CodeChallengeMethod.S256); | |
| oauth.genOAuthURL(redirectURI, "state", PKCEOAuthClient.CodeChallengeMethod.S256); |
| @@ -251,6 +256,7 @@ | |||
| protected String clientID; | |||
| protected String clientSecret; | |||
| protected String baseURL; | |||
| protected String wwwURL; | |||
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for wwwURL builder methods
The codebase analysis confirms that wwwURL builder functionality lacks test coverage. Existing OAuth client tests demonstrate thorough testing patterns for other builder properties, but wwwURL-specific tests are missing. Please add test cases to verify:
- Setting custom wwwURL
- Fallback to baseURL conversion
- Invalid URL handling
🔗 Analysis chain
Add test coverage for wwwURL builder methods.
The new builder methods for wwwURL are not covered by tests. Please add test cases to verify:
- Setting custom wwwURL
- Fallback to baseURL conversion
- Invalid URL handling
Also applies to: 285-289
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing test coverage of wwwURL functionality
# Search for test files that might contain wwwURL-related tests
echo "Searching for existing test files..."
fd -e java -e kt test
# Search for wwwURL usage in test files
echo -e "\nSearching for wwwURL test coverage..."
rg -l "wwwURL" $(fd -e java -e kt test)
Length of output: 4911
Script:
#!/bin/bash
# Check for builder pattern usage in auth tests
echo "Checking builder usage in auth tests..."
rg -A 5 -B 5 "\.builder\(\)" api/src/test/java/com/coze/openapi/service/auth/
# Check for any indirect wwwURL testing through setters
echo -e "\nChecking for indirect wwwURL testing..."
rg -A 5 -B 5 "\.setWww" api/src/test/java/com/coze/openapi/service/auth/
Length of output: 338
Script:
#!/bin/bash
# Check test structure and initialization patterns
echo "Analyzing OAuthClient test implementations..."
rg -l "OAuthClient" api/src/test/java/com/coze/openapi/service/auth/ | xargs cat
# Broader search for builder-related patterns
echo -e "\nChecking for builder patterns..."
rg "new\s+.*Builder|builder\s*\(\)" api/src/test/java/com/coze/openapi/service/auth/
Length of output: 30942
chyroc
changed the title
chyroc
changed the title
Change-Id: I96cda3ea6a200b85184177851af8b1e4ce27f21d