Add support for connected accounts#1422
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
adamjmcgrath
force-pushed
the
connected-accounts
branch
4 times, most recently
from
3ffdffd to
1b1981b
Compare
adamjmcgrath
force-pushed
the
connected-accounts
branch
8 times, most recently
from
d6be788 to
9ee3378
Compare
adamjmcgrath
force-pushed
the
connected-accounts
branch
2 times, most recently
from
8f02b6b to
58ba314
Compare
| }), | ||
| getDpopNonce: () => this.getDpopNonce(config.dpopNonceId), | ||
| setDpopNonce: nonce => this.setDpopNonce(nonce), | ||
| setDpopNonce: nonce => this.setDpopNonce(nonce, config.dpopNonceId), |
adamjmcgrath
force-pushed
the
connected-accounts
branch
2 times, most recently
from
4248327 to
a69e3f0
Compare
adamjmcgrath
force-pushed
the
connected-accounts
branch
from
a69e3f0 to
c3d94d2
Compare
|
Claude finished @adamjmcgrath's task —— View job Review Complete ✅I've reviewed the connected accounts PR and identified several security and code quality issues that need attention. Review Progress
Key Issues Found
The PR has good test coverage and documentation, but these security concerns should be addressed before merging. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
❌ Issues need to be addressed
This PR introduces connected accounts functionality with comprehensive test coverage and clear documentation. However, several security vulnerabilities and code quality issues require attention:
Security Concerns
- State validation bypass: The CSRF protection can potentially be circumvented by user-controlled input
- Information disclosure: Raw API response bodies are exposed in error messages
- Open redirect risk: Fallback to
window.location.originfor redirect URI - URL validation: Direct use of API-provided
connect_uriwithout domain validation
Recommendations
- Address the state validation security issue identified by GitHub Advanced Security
- Sanitize error messages to prevent information leakage
- Implement proper redirect URI validation and avoid dynamic origins
- Add domain validation for connect URIs
- Use enum values instead of string literals for type safety
The implementation follows good patterns with PKCE, proper error handling, and comprehensive testing, but the security issues should be resolved before merging.
| /** | ||
| * The type of response, for login it will be `code` | ||
| */ | ||
| response_type: ResponseType.Code; |
There was a problem hiding this comment.
Is this response_type field a required field for existing flows also ?
Will making it required may cascade into breaking typescript change for existing users ?
There was a problem hiding this comment.
This is the return type of a function, so I wouldn't have thought adding a property could be a breaking change. It's not caused any issues in auth0-react.
But I might be wrong, let me know if you can demonstrate how it would break upstream code and I can change it to fix.
@frederikprijck ☝️ you're better at this stuff than me 😁 do you think this might be a breaking change?
There was a problem hiding this comment.
The simulator looks fine, i will try to check in an example app setup also.
|
Overall the PR looks good to me, Thanks for getting it up. |
**Added** - Add support for connected accounts [\#1422](#1422) ([adamjmcgrath](https://github.com/adamjmcgrath)) **Fixed** - Fetcher should set DPoP nonce with nonce id [\#1431](#1431) ([adamjmcgrath](https://github.com/adamjmcgrath)) - fix: do not mark setAuthorizationHeader as async [\#1428](#1428) ([frederikprijck](https://github.com/frederikprijck))
### Changes Add support for Connected Accounts to SPA JS: - Bump SPA-JS to include auth0/auth0-spa-js#1422 - Add a new `connectAccountWithRedirect` method that redirects the application to the `/connect` endpoint on the auth server (similar mechanics to the logging in with the `/authorize` endpoint) - Update the playground to enabled testing Connected Accounts <img width="50%" height="50%" alt="image" src="https://github.com/user-attachments/assets/7aecbc87-5f1b-47ee-a868-0d7c05a3ba56" /> ### References https://auth0team.atlassian.net/browse/AGAI-157 ### Testing Manual testing steps and demo video are in ESD-52475 - [x] This change adds unit test coverage - [ ] This change adds integration test coverage (integration tests are not possible since oidc-provider does not support the proprietary Auth0 connect flow) - [x] This change has been tested on the latest version of the platform/language ### Checklist - [x] I have read the [Auth0 general contribution guidelines](https://github.com/auth0/open-source-template/blob/master/GENERAL-CONTRIBUTING.md) - [x] I have read the [Auth0 Code of Conduct](https://github.com/auth0/open-source-template/blob/master/CODE-OF-CONDUCT.md) - [x] All code quality tools/guidelines have been run/followed
3 participants
Changes
Add support for Connected Accounts to SPA JS:
connectAccountWithRedirectmethod that redirects the application to the/connectendpoint on the auth server (similar mechanics to the logging in with the/authorizeendpoint)/connect- similar to PAR and/completesimilar to token exchange)References
https://auth0team.atlassian.net/browse/AGAI-156
Testing
Manual testing steps and demo video are in ESD-52259
(integration tests are not possible since oidc-provider does not support the proprietary Auth0 connect flow)
Checklist