[pull] master from supabase:master #2
Conversation
![pull[bot]](https://avatars.githubusercontent.com/in/12910?s=60&v=4){kind=small}
## What kind of change does this PR introduce? * Add error codes to the password login flow, which fixes #1631 ## What is the current behavior? * Most errors in the password login flow are returned as `oauthError`, which doesn't have support for an error code as the struct conforms to the [oauth error response](https://datatracker.ietf.org/doc/html/rfc6749#section-5.2) specified in the RFC ## What is the new behavior? * Errors which were previously returned as an `oauthError` struct now return `badRequestError` instead with the following error code `invalid_login_credentials` * In certain cases, we can return existing error codes like `ErrorCodeUserBanned`, `ErrorCodeEmailNotConfirmed`, `ErrorCodePhoneNotConfirmed` or `ErrorCodeValidationFailed` * Some error messages are updated to provide more clarity on the underlying error Feel free to include screenshots if it includes visual changes. ## Additional context Add any other context or screenshots.
## What kind of change does this PR introduce? Bug fix ## What is the current behavior? A valid provider name gets invalidated. ## What is the new behavior? A possible valid provider name gets verified by regex correctly. ## Additional context `[^a]+` -> This regex will match any char except character `a` `^a$` -> This regex will match string have single char `a` `^[a-zA-Z0-9]+$` -> This regex will match any alphanumeric string. `^` denotes start of string and `$` denotes end of string. This pull request will address #1719
Custom SMS verification did not work if Twilio Verify was enabled. Furthermore, test OTP flow was misplaced.
## What kind of change does this PR introduce? * OAuth state is now signed with the same JWK that is used to sign the access tokens ## What is the current behavior? * currently, it's weird for the `GOTRUE_JWT_SECRET` to be set (other than it being a fallback option) just for the sake of signing the oauth state ## What is the new behavior? Feel free to include screenshots if it includes visual changes. ## Additional context Add any other context or screenshots.
🤖 I have created a release *beep* *boop* --- ## [2.159.0](v2.158.1...v2.159.0) (2024-08-21) ### Features * Vercel marketplace OIDC ([#1731](#1731)) ([a9ff361](a9ff361)) ### Bug Fixes * add error codes to password login flow ([#1721](#1721)) ([4351226](4351226)) * change phone constraint to per user ([#1713](#1713)) ([b9bc769](b9bc769)) * custom SMS does not work with Twilio Verify ([#1733](#1733)) ([dc2391d](dc2391d)) * ignore errors if transaction has closed already ([#1726](#1726)) ([53c11d1](53c11d1)) * redirect invalid state errors to site url ([#1722](#1722)) ([b2b1123](b2b1123)) * remove TOTP field for phone enroll response ([#1717](#1717)) ([4b04327](4b04327)) * use signing jwk to sign oauth state ([#1728](#1728)) ([66fd0c8](66fd0c8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughVersion 2.159.2 of the Supabase Auth library has been released, introducing several bug fixes, including enhancements for anonymous user password updates and the removal of server-side cookie token methods. The previous version, 2.159.1, focused on returning the OAuth identity during user creation. Version 2.159.0 added support for Vercel Marketplace OIDC and various improvements to error handling and user-specific constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant VercelMarketplace
User->>API: Initiate Authentication
API->>VercelMarketplace: Request Token
VercelMarketplace-->>API: Return Token
API->>User: Provide Access
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- CHANGELOG.md (1 hunks)
- internal/api/api.go (1 hunks)
- internal/api/errorcodes.go (1 hunks)
- internal/api/external.go (6 hunks)
- internal/api/external_oauth.go (3 hunks)
- internal/api/external_test.go (1 hunks)
- internal/api/jwks.go (2 hunks)
- internal/api/mfa.go (1 hunks)
- internal/api/provider/oidc.go (2 hunks)
- internal/api/provider/vercel_marketplace.go (1 hunks)
- internal/api/samlacs.go (2 hunks)
- internal/api/token.go (7 hunks)
- internal/api/token_oidc.go (1 hunks)
- internal/api/verify.go (2 hunks)
- internal/conf/configuration.go (2 hunks)
- internal/storage/dial.go (1 hunks)
- openapi.yaml (1 hunks)
Files skipped from review due to trivial changes (1)
- internal/api/api.go
Additional comments not posted (40)
internal/api/jwks.go (2)
6-9: Review the import statements.The import statements include necessary packages for JWT handling and configuration. Ensure that all imported packages are used in the code.
34-61: Ensure proper error handling and key management insignJwt.The function
signJwtis well-structured for signing JWTs. However, consider the following points:
- Ensure that
conf.GetSigningJwkandconf.GetSigningKeyhandle errors appropriately, especially for key retrieval.- Verify that the
signingMethodreturned byconf.GetSigningAlgis compatible with the key type.- Consider logging errors for better observability.
Ensure that the functions
conf.GetSigningJwk,conf.GetSigningKey, andconf.GetSigningAlgare correctly implemented and handle errors properly. Run the following script to verify their implementations:Verification successful
Error Handling and Compatibility in
signJwtVerifiedThe functions
conf.GetSigningJwk,conf.GetSigningKey, andconf.GetSigningAlgare implemented with appropriate error handling. The compatibility of the signing method with the key type is managed by checking the algorithm string and defaulting to a common method. No immediate issues were found in the implementations.
GetSigningJwkreturns an error if no signing key is found.GetSigningKeyhandles errors when extracting the raw key.GetSigningAlgdefaults to a common signing method if the key is nil or unrecognized.The review comment's concerns are addressed in the current implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementations of key retrieval and signing algorithm functions. # Test: Search for the implementation of `GetSigningJwk`. ast-grep --lang go --pattern 'func GetSigningJwk($_) ($_, error) { $$$ }' # Test: Search for the implementation of `GetSigningKey`. ast-grep --lang go --pattern 'func GetSigningKey($_) ($_, error) { $$$ }' # Test: Search for the implementation of `GetSigningAlg`. ast-grep --lang go --pattern 'func GetSigningAlg($_) $_ { $$$ }'Length of output: 2123
internal/api/provider/vercel_marketplace.go (4)
1-11: Review the import statements and package declaration.The import statements include necessary packages for OAuth and OIDC handling. Ensure that all imported packages are used in the code.
18-22: Ensure proper initialization ofvercelMarketplaceProvider.The struct
vercelMarketplaceProvideris initialized with OAuth2 configuration and OIDC provider. Ensure that theAPIPathis correctly set and used in the OAuth flow.
24-57: Validate OAuth configuration and scopes inNewVercelMarketplaceProvider.The function
NewVercelMarketplaceProviderinitializes a new provider with given configurations. Consider the following:
- Ensure
ext.ValidateOAuthcorrectly validates the OAuth configuration.- Verify that
chooseHostcorrectly determines the API path.- Consider handling empty or invalid scopes more robustly.
Ensure that the function
ext.ValidateOAuthandchooseHostare correctly implemented and handle errors properly. Run the following script to verify their implementations:
59-78: Ensure proper token exchange and user data retrieval inGetOAuthTokenandGetUserData.The functions
GetOAuthTokenandGetUserDatahandle token exchange and user data retrieval. Ensure that:
- The token exchange process handles errors and edge cases properly.
- The user data retrieval correctly parses and validates the ID token.
Ensure that the functions
ExchangeandParseIDTokenare correctly implemented and handle errors properly. Run the following script to verify their implementations:internal/api/external_oauth.go (2)
13-13: Review the import statements and package declaration.The import statements include necessary packages for OAuth handling and logging. Ensure that all imported packages are used in the code.
36-49: Enhance error handling and logging inloadFlowState.The function
loadFlowStatehas been refactored to improve error handling. Consider the following:
- Ensure that
a.loadExternalStateproperly handles errors and edge cases.- Consider logging errors for better observability and debugging.
- Verify that the constructed redirect URL is correctly formatted and includes all necessary error details.
Ensure that the function
a.loadExternalStateis correctly implemented and handles errors properly. Run the following script to verify its implementation:internal/storage/dial.go (1)
148-153: Improved error handling in transaction method.The addition of the check for
sql.ErrTxDoneenhances the robustness of the transaction handling by ensuring only relevant errors are propagated.Ensure that the usage of
sql.ErrTxDoneis consistent and correctly implemented throughout the codebase.Run the following script to verify the usage of
sql.ErrTxDone:Verification successful
Consistent usage of
sql.ErrTxDoneverified.The usage of
sql.ErrTxDoneis consistent and correct, as it appears only once in the codebase, specifically ininternal/storage/dial.go. No further inconsistencies were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `sql.ErrTxDone` in the codebase. # Test: Search for occurrences of `sql.ErrTxDone`. Expect: Consistent and correct usage. rg --type go 'sql.ErrTxDone'Length of output: 93
internal/api/errorcodes.go (1)
84-86: Enhanced error code clarity and functionality.The renaming of
ErrorCodeVerifiedFactorExiststoErrorCodeMFAVerifiedFactorExistsimproves clarity by associating the error with MFA. The addition ofErrorCodeInvalidCredentialsexpands error handling capabilities.internal/api/token_oidc.go (1)
83-87: Expanded provider handling ingetProvidermethod.The addition of the "vercel_marketplace" provider case broadens the applicability of the method, enhancing its functionality.
internal/api/external_test.go (1)
238-238: Refactor: UpdateredirectErrorsfunction call.The
redirectErrorsfunction is now called directly, reflecting its refactoring to a standalone function. Ensure that this change aligns with the intended design and that all necessary dependencies are correctly managed.Verify the function's usage and dependencies to ensure consistency across the codebase.
Verification successful
Verification Successful:
redirectErrorsFunction Usage is ConsistentThe
redirectErrorsfunction is correctly refactored as a standalone function and is used consistently across the codebase. The function's usage in bothinternal/api/external.goandinternal/api/external_test.goaligns with the intended design. No further changes are necessary.
- Files Involved:
internal/api/external.gointernal/api/external_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `redirectErrors` to ensure consistency. # Test: Search for the function usage. Expect: Occurrences of the standalone function. rg --type go -A 5 $'redirectErrors'Length of output: 1153
internal/api/provider/oidc.go (3)
64-65: Enhancement: Add support for Vercel Marketplace ID tokens.The
ParseIDTokenfunction now includes a case forIssuerVercelMarketplace, enabling support for Vercel Marketplace ID tokens. Ensure that this addition is thoroughly tested.
356-362: New Struct:VercelMarketplaceIDTokenClaims.The
VercelMarketplaceIDTokenClaimsstruct is introduced to handle claims specific to Vercel Marketplace ID tokens. This struct includes fields forUserEmail,UserName, andUserAvatarUrl.
364-388: New Function:parseVercelMarketplaceIDToken.The
parseVercelMarketplaceIDTokenfunction processes Vercel Marketplace ID tokens, extracting user data and populating theUserProvidedDatastructure. Ensure that this function is covered by unit tests to verify its correctness.internal/api/samlacs.go (2)
46-58: Refactor: Enhanced error handling inSamlAcs.The
SamlAcsmethod now delegates its core functionality tohandleSamlAcs, with improved error handling that constructs a URL with error details for redirection. This enhances client feedback during errors.
60-60: Refactor: IntroducehandleSamlAcsfor core ACS logic.The
handleSamlAcsmethod encapsulates the main logic for processing ACS requests, separating concerns and improving code readability and maintainability.internal/api/token.go (10)
94-94: Improved error handling for unsupported grant types.The use of
badRequestErrorwithErrorCodeInvalidCredentialsenhances clarity in error reporting.
134-134: Enhanced error handling for missing email or phone.The use of
badRequestErrorwithErrorCodeValidationFailedfor missing email or phone improves error specificity.
139-139: Improved error handling for invalid credentials.The use of
badRequestErrorwithErrorCodeInvalidCredentialsenhances clarity in error reporting.
145-145: Improved error handling for users without passwords.The use of
badRequestErrorwithErrorCodeInvalidCredentialsenhances clarity in error reporting.
149-149: Improved error handling for banned users.The use of
badRequestErrorwithErrorCodeUserBannedprovides clear feedback for banned users.
188-188: Improved error handling in password verification hook.The inline error check for
invokeHookstreamlines the code and improves readability.
201-201: Improved error handling for hook rejection.The use of
badRequestErrorwithErrorCodeInvalidCredentialsprovides clear feedback for hook rejections.
205-205: Improved error handling for invalid passwords.The use of
badRequestErrorwithErrorCodeInvalidCredentialsenhances clarity in error reporting.
209-211: Improved error handling for unconfirmed email or phone.The use of
badRequestErrorwith specific error codes for unconfirmed email or phone improves error specificity.
369-369: Simplified JWT signing process.The direct use of
signJwtenhances readability and reduces complexity.internal/api/external.go (6)
110-110: Improved JWT signing process.The use of
signJwtabstracts the signing process and improves maintainability.
141-141: RefactoredredirectErrorsfunction call.Removing the receiver suggests a shift towards a more functional approach, improving flexibility.
481-491: Enhanced handling of OAuth state parameter.The change to accept an HTTP request allows flexible handling of the state parameter, accommodating both POST and GET requests.
494-510: Refined error management in JWT parsing.Returning the context instead of nil improves control flow and error management.
588-589: Extended support for new providervercel_marketplace.The addition enhances the API's capabilities to integrate with additional services.
599-599: RefactoredredirectErrorsto a standalone function.This change improves flexibility and aligns with a more functional programming style.
internal/api/mfa.go (1)
106-106: Refined error code for verified phone factors.The update from
ErrorCodeVerifiedFactorExiststoErrorCodeMFAVerifiedFactorExistsimproves clarity and specificity in error categorization.internal/api/verify.go (2)
691-695: Verify the logic for handling test OTPs.The logic for handling test OTPs seems correct, but ensure that the
GetTestOTPfunction is properly implemented and tested.Run the following script to verify the
GetTestOTPfunction implementation:Verification successful
Verified: The logic for handling test OTPs is correctly implemented.
The
GetTestOTPfunction is properly implemented ininternal/conf/configuration.go. It checks the validity of the test OTP and returns it if valid. No issues were found with this logic.
- Implementation found in
internal/conf/configuration.golines 386-393.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of GetTestOTP function. # Test: Search for the function definition. Expect: A valid implementation. ast-grep --lang go --pattern 'func (c *SmsProviderConfiguration) GetTestOTP($_, $_) ($_ , $_) { $$$ }'Length of output: 657
Line range hint
706-711: Clarify the condition for Twilio verification.The condition for Twilio verification checks if the SMS hook is disabled and if Twilio is the provider. Ensure this logic aligns with the intended behavior.
Run the following script to verify the condition logic:
Verification successful
Condition for Twilio Verification is Correctly Implemented
The
IsTwilioVerifyProvidermethod correctly checks if Twilio is the provider by verifying if theProviderfield is set to"twilio_verify". This aligns with the intended behavior of the condition in the code snippet. No issues found with the logic.
- The condition checks if the SMS hook is disabled and if Twilio is the provider, which is consistent with the intended functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for Twilio verification condition. # Test: Search for the IsTwilioVerifyProvider method. Expect: A valid implementation. ast-grep --lang go --pattern 'func (t *SmsProviderConfiguration) IsTwilioVerifyProvider() bool { $$$ }'Length of output: 320
CHANGELOG.md (1)
3-19: Changelog updates look good.The changelog entries for version 2.159.0 are well-structured and provide a clear summary of the changes.
internal/conf/configuration.go (2)
736-744: Streamlined logic inApplyDefaultslooks good.The removal of
nilchecks in favor of slice length checks simplifies the logic and is a positive change.
325-325: Verify the usage of theVercelMarketplacefield.The addition of the
VercelMarketplacefield looks correct. Ensure that this field is properly utilized in the codebase.Run the following script to verify the usage of the
VercelMarketplacefield:Verification successful
The
VercelMarketplacefield is properly utilized in the codebase.The field is actively used across multiple files, indicating correct integration into the application's logic.
- Files with usage:
internal/api/token_oidc.gointernal/api/external.gointernal/api/provider/oidc.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the VercelMarketplace field. # Test: Search for the field usage. Expect: Proper utilization in the codebase. rg --type go 'VercelMarketplace'Length of output: 1825
openapi.yaml (1)
285-285: Review the impact of the tightened regex pattern.The regular expression pattern for the
providerparameter has been changed to only allow alphanumeric characters. This could impact clients that previously relied on the ability to include non-alphanumeric characters. Ensure that this change aligns with the intended use cases and update any relevant documentation or client code accordingly.
## What kind of change does this PR introduce? * When the oauth user is first created, the identity needs to be returned in the user object as well. ## What is the current behavior? Please link any relevant issues here. ## What is the new behavior? Feel free to include screenshots if it includes visual changes. ## Additional context Add any other context or screenshots.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- internal/api/external.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- internal/api/external.go
🤖 I have created a release *beep* *boop* --- ## [2.159.1](v2.159.0...v2.159.1) (2024-08-23) ### Bug Fixes * return oauth identity when user is created ([#1736](#1736)) ([60cfb60](60cfb60)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- internal/api/anonymous_test.go (5 hunks)
- internal/api/user.go (1 hunks)
Additional comments not posted (6)
internal/api/user.go (1)
105-108: Improve readability and clarity.The changes improve the clarity of the validation logic by directly enforcing the requirement for either an email or phone number when updating the password for anonymous users. This is a positive change.
The code changes are approved.
internal/api/anonymous_test.go (5)
83-83: Enhance test coverage.The addition of an extra phone number in the
TestOTPmap allows for more comprehensive testing of the SMS functionality.The code changes are approved.
109-124: Expand test scenarios.The new test cases for converting anonymous users to permanent users using either an email and password combination or a phone number and password combination improve the robustness of the test suite.
The code changes are approved.
161-164: Refine verification logic.The verification logic now checks if the user has a password set after the conversion, enhancing the validation process.
The code changes are approved.
174-174: Improve flexibility.The assertions for verifying the user's email and phone now reference the dynamic values from the test cases, rather than hardcoded values. This change ensures that the tests are more flexible and accurately reflect the input provided in each test case.
The code changes are approved.
200-204: Ensure accurate verification.The verification logic now accurately checks the user's email and phone against the dynamic values from the test cases, ensuring that the tests reflect the input provided during the test.
The code changes are approved.
## What kind of change does this PR introduce? Remove set cookie tokens and clear cookie token methods as it looks like they aren't done server side. Task: https://www.notion.so/supabase/team-auth-113cb19144c1419ea5fb1d600281d959?p=ff083dad758e44ef9b4e230804e0fee7&pm=s
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
internal/api/logout_test.go (1)
Verify cookie management in
TestLogoutSuccess.The
TestLogoutSuccessfunction no longer checks for cookies, which may reduce the robustness of the test in verifying session management during logout. Consider reintroducing cookie-checking logic to ensure proper session termination.
- Ensure that cookies are cleared or invalidated as part of the logout process.
Analysis chain
Line range hint
1-50: Verify the impact of removing cookie-checking logic.The removal of the cookie-checking logic simplifies the test but may affect the robustness of the logout functionality verification. Ensure that this change does not introduce any issues with user authentication or session persistence.
Run the following script to verify the impact of the removal of the cookie-checking logic:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing cookie-checking logic in `TestLogoutSuccess`. # Test: Search for the function usage. Expect: No issues related to user session management. rg --type go -A 5 $'TestLogoutSuccess'Length of output: 434
Script:
#!/bin/bash # Description: Retrieve the full implementation of the `TestLogoutSuccess` function to verify cookie handling. ast-grep --lang go --pattern $'func (ts *LogoutTestSuite) TestLogoutSuccess() { $$$ }'Length of output: 1349
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- example.env (1 hunks)
- internal/api/anonymous.go (1 hunks)
- internal/api/auth.go (1 hunks)
- internal/api/external.go (9 hunks)
- internal/api/logout.go (2 hunks)
- internal/api/logout_test.go (1 hunks)
- internal/api/mfa.go (3 hunks)
- internal/api/middleware.go (1 hunks)
- internal/api/samlacs.go (3 hunks)
- internal/api/signup.go (1 hunks)
- internal/api/token.go (11 hunks)
- internal/api/token_refresh.go (2 hunks)
- internal/api/verify.go (6 hunks)
- internal/conf/configuration.go (4 hunks)
Files skipped from review due to trivial changes (1)
- example.env
Additional comments not posted (22)
internal/api/anonymous.go (1)
Line range hint
1-50: Verify the impact of removing cookie-setting logic.The removal of the cookie-setting logic simplifies the function but may affect user session management. Ensure that this change does not introduce any issues with user authentication or session persistence.
Run the following script to verify the impact of the removal of the cookie-setting logic:
Verification successful
Removal of Cookie-Setting Logic Verified
The removal of the cookie-setting logic in the
SignupAnonymouslyfunction does not impact session management. The codebase does not use cookies for session handling, relying instead on database transactions and session IDs. No further issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing cookie-setting logic in `SignupAnonymously`. # Test: Search for the function usage. Expect: No issues related to user session management. rg --type go -A 5 $'SignupAnonymously'Length of output: 670
Script:
#!/bin/bash # Description: Search for session management and cookie-setting logic in the codebase. # Search for any cookie-setting logic in the codebase. rg --type go 'SetCookie' # Search for session management logic in the codebase. rg --type go 'session'Length of output: 21144
internal/api/logout.go (1)
Line range hint
1-50: Verify the impact of removing cookie-clearing logic.The removal of the cookie-clearing logic simplifies the function but may affect user session management. Ensure that this change does not introduce any issues with user authentication or session persistence.
Run the following script to verify the impact of the removal of the cookie-clearing logic:
internal/api/auth.go (1)
Line range hint
10-19: Verify the impact of removingclearCookieTokenscalls.The removal of
clearCookieTokenscalls changes the error handling behavior. Ensure that this change does not negatively impact cookie management during authentication failures.Run the following script to verify the impact of removing
clearCookieTokenscalls:Verification successful
No impact from the removal of
clearCookieTokens.The
clearCookieTokensfunction is not defined or used anywhere in the codebase, so its removal does not affect the current functionality or error handling related to cookie management.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `clearCookieTokens` calls. # Test: Search for the usage of `clearCookieTokens` method. Expect: No critical dependency on this method. rg --type go 'clearCookieTokens'Length of output: 32
Script:
#!/bin/bash # Description: Check for the definition and usage of `clearCookieTokens`. # Search for the definition of `clearCookieTokens`. rg --type go 'func clearCookieTokens' # If found, search for references to `clearCookieTokens`. rg --type go 'clearCookieTokens'Length of output: 72
internal/api/token_refresh.go (1)
Line range hint
16-17: Verify the impact of removingclearCookieTokensandsetCookieTokenscalls.The removal of
clearCookieTokensandsetCookieTokenscalls changes the token and cookie management behavior. Ensure that this change does not negatively impact token reuse and session management.Run the following script to verify the impact of removing
clearCookieTokensandsetCookieTokenscalls:Also applies to: 104-105
Verification successful
No impact from removing
clearCookieTokensandsetCookieTokenscalls.The methods
clearCookieTokensandsetCookieTokensare not defined or used anywhere in the codebase. Therefore, their removal from theRefreshTokenGrantfunction does not affect the codebase's functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `clearCookieTokens` and `setCookieTokens` calls. # Test: Search for the usage of `clearCookieTokens` and `setCookieTokens` methods. Expect: No critical dependency on these methods. rg --type go 'clearCookieTokens|setCookieTokens'Length of output: 50
Script:
#!/bin/bash # Search for the definition of `clearCookieTokens` method ast-grep --lang go --pattern 'func clearCookieTokens($$$) { $$$ }' # Search for the definition of `setCookieTokens` method ast-grep --lang go --pattern 'func setCookieTokens($$$) { $$$ }' # Search for the usage of `clearCookieTokens` and `setCookieTokens` methods rg 'clearCookieTokens|setCookieTokens' -A 2Length of output: 181
internal/api/middleware.go (1)
Line range hint
90-95: Verify the impact of removingclearCookieTokenscall.The removal of
clearCookieTokenscall changes the error handling behavior. Ensure that this change does not negatively impact cookie management during JWT parsing failures.Run the following script to verify the impact of removing
clearCookieTokenscall:internal/api/samlacs.go (2)
46-57: LGTM!The function is correctly delegating processing to
handleSamlAcsand includes improved error handling and redirection logic.The code changes are approved.
Line range hint
60-290: LGTM! But verify the impact of removing cookie-setting logic.The function is correctly implemented and handles various error scenarios. However, ensure that the removal of cookie-setting logic does not affect the authentication process.
The code changes are approved.
Run the following script to verify the impact of removing the cookie-setting logic:
internal/api/signup.go (1)
Line range hint
1-290: LGTM! But verify the impact of removing cookie-setting logic.The function is correctly implemented and handles user registration. However, ensure that the removal of cookie-setting logic does not affect the authentication process.
The code changes are approved.
Run the following script to verify the impact of removing the cookie-setting logic:
internal/api/token.go (3)
92-92: LGTM!The function is correctly implemented and uses a more specific error handling mechanism.
The code changes are approved.
Line range hint
132-209: LGTM!The function is correctly implemented and uses a more specific error handling mechanism.
The code changes are approved.
364-364: LGTM!The function is correctly implemented and uses a streamlined JWT signing process.
The code changes are approved.
internal/api/external.go (6)
110-110: LGTM!The encapsulation of the JWT signing process enhances modularity and flexibility.
The code changes are approved.
141-141: LGTM!Calling
redirectErrorsdirectly improves code readability and streamlines the error handling process.The code changes are approved.
337-337: LGTM!Appending the identity to the user's identities list ensures proper association.
The code changes are approved.
477-503: LGTM!The changes enhance flexibility in handling different types of requests and improve security by allowing for dynamic key resolution.
The code changes are approved.
584-585: LGTM!Adding support for the "vercel_marketplace" provider expands the functionality of the API.
The code changes are approved.
595-595: LGTM!Refactoring to remove the
APIreceiver promotes a more functional approach to error handling.The code changes are approved.
internal/api/mfa.go (1)
106-106: LGTM!The refined error code provides clearer context regarding the type of verification error encountered.
The code changes are approved.
internal/api/verify.go (1)
682-687: LGTM!The change provides more nuanced control over when OTPs are verified through Twilio.
The code changes are approved.
internal/conf/configuration.go (3)
320-320: Verify the usage of theVercelMarketplacefield.The addition of the
VercelMarketplacefield enhances the configuration options for OAuth providers. Ensure that the field is used correctly in the codebase.Run the following script to verify the usage:
Verification successful
The
VercelMarketplacefield is used correctly across the codebase.The field is integrated into the logic for handling OAuth providers, with references in conditionals, assignments, and function calls, indicating proper usage. No issues were found.
- Files and Usages:
internal/api/token_oidc.go: Used in conditionals and assignments.internal/api/external.go: Used in provider creation.internal/api/provider/vercel_marketplace.go: Used in token parsing and provider logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `VercelMarketplace` field. # Test: Search for the usage of the `VercelMarketplace` field. Expect: Correct usage in the codebase. rg --type go 'VercelMarketplace'Length of output: 1825
260-261: Verify the impact of removing theCookiestruct.The removal of the
Cookiestruct simplifies theGlobalConfigurationstruct. Ensure that no other parts of the codebase rely on theCookiestruct.Run the following script to verify the impact:
Verification successful
Removal of
Cookiestruct does not impact existing references.The removal of the
Cookiestruct fromGlobalConfigurationdoes not affect the existing references to cookies in the codebase, as these are related to specific functionalities in the API layer and are independent of the removed struct.
useCookieHeaderis used in CORS configuration and is unrelated to the removed struct.- HTTP-Only cookie handling in
internal/api/samlacs.gois also independent of the removed struct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `Cookie` struct. # Test: Search for the usage of the `Cookie` struct. Expect: No occurrences. rg --type go 'Cookie'Length of output: 298
Script:
#!/bin/bash # Description: Investigate the context of `useCookieHeader` and its usage. # Search for the definition and usage of `useCookieHeader`. rg --type go 'useCookieHeader' -A 5 # Check for any other references to cookies in the codebase. rg --type go 'cookie' -A 5Length of output: 1626
731-739: Verify the impact of removing conditional checks related to theCookiesettings.The removal of conditional checks related to the
Cookiesettings reflects the removal of theCookiestruct. Ensure that the changes do not affect the default settings for other configurations.Run the following script to verify the impact:
Verification successful
Removal of
Cookiesettings does not impact other configurationsThe removal of conditional checks related to the
Cookiesettings does not affect the default settings for other configurations. The existing tests usingApplyDefaultsdo not rely onCookiesettings, indicating that their removal is isolated and does not introduce issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing conditional checks related to the `Cookie` settings. # Test: Search for the usage of the `ApplyDefaults` method. Expect: No issues with default settings for other configurations. rg --type go 'ApplyDefaults'Length of output: 649
Script:
#!/bin/bash # Description: Check the content of test files that use `ApplyDefaults` to verify coverage of scenarios related to removed `Cookie` settings. # List of test files using ApplyDefaults test_files=( "internal/utilities/request_test.go" "internal/api/saml_test.go" "internal/api/verify_test.go" "internal/api/auth_test.go" "internal/conf/jwk_test.go" ) # Display the content of the test files for file in "${test_files[@]}"; do echo "Content of $file:" cat "$file" echo -e "\n\n" doneLength of output: 65105
## What kind of change does this PR introduce? The HookName is generated from the Postgres URI and used internally to invoke the hook. Context: #1734 (comment) Since it's for internal use, we don't expose it similar to what we do with [encryption key](https://github.com/supabase/auth/compare/j0/hide_hook_name?expand=1#diff-4c28cb40881781a1067b3b3681c43f805dab629f31f3c7614b0f781ffa096505L457) and other similar fields. This is fine since we don't marshal the extensibility point. ## Testing setup We don't marshal the struct so it should be fine but for additional sanity check ran this locally: ``` GOTRUE_HOOK_CUSTOM_ACCESS_TOKEN_ENABLED="true" GOTRUE_HOOK_CUSTOM_ACCESS_TOKEN_URI="pg-functions://postgres/public/custom_access_token_hook" ``` ``` create or replace function public.custom_access_token_hook(event jsonb) returns jsonb language plpgsql as $$ declare claims jsonb; begin -- Proceed only if the user is an admin claims := event->'claims'; -- Check if 'user_metadata' exists in claims if jsonb_typeof(claims->'user_metadata') is null then -- If 'user_metadata' does not exist, create an empty object claims := jsonb_set(claims, '{user_metadata}', '{}'); end if; -- Set a claim of 'admin' claims := jsonb_set(claims, '{user_metadata, admin}', 'true'); -- Update the 'claims' object in the original event event := jsonb_set(event, '{claims}', claims); -- Return the modified or original event return event; end; $$; grant execute on function public.custom_access_token_hook to supabase_auth_admin; revoke execute on function public.custom_access_token_hook from authenticated, anon, public; grant usage on schema public to supabase_auth_admin; ```
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- internal/conf/configuration.go (5 hunks)
Additional comments not posted (3)
internal/conf/configuration.go (3)
260-261: LGTM!The addition of
SAMLandCORSfields to theGlobalConfigurationstruct expands the configuration options available for SAML and CORS settings.The code changes are approved.
320-320: LGTM!The addition of
VercelMarketplacefield to theProviderConfigurationstruct enhances the configuration options available for OAuth providers, specifically for Vercel's marketplace services.The code changes are approved.
732-740: LGTM!The removal of cookie-related logic from the
ApplyDefaultsmethod reflects the removal of theCookiestruct from theGlobalConfigurationtype, simplifying the configuration structure.The code changes are approved.
🤖 I have created a release *beep* *boop* --- ## [2.159.2](v2.159.1...v2.159.2) (2024-08-28) ### Bug Fixes * allow anonymous user to update password ([#1739](#1739)) ([2d51956](2d51956)) * hide hook name ([#1743](#1743)) ([7e38f4c](7e38f4c)) * remove server side cookie token methods ([#1742](#1742)) ([c6efec4](c6efec4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
CHANGELOG.md (5)
3-3: Add a date for the version release.The changelog entry for version 2.159.2 is missing the release date. Adding the date will provide better context for the changes.
-## [2.159.2](https://github.com/supabase/auth/compare/v2.159.1...v2.159.2) (2024-08-28) +## [2.159.2](https://github.com/supabase/auth/compare/v2.159.1...v2.159.2) (2024-08-28)
6-10: Ensure consistency in bug fix descriptions.The descriptions for the bug fixes should be consistent in style and clarity. Consider rephrasing for better readability.
-* allow anonymous user to update password ([#1739](https://github.com/supabase/auth/issues/1739)) ([2d51956](https://github.com/supabase/auth/commit/2d519569d7b8540886d0a64bf3e561ef5f91eb63)) -* hide hook name ([#1743](https://github.com/supabase/auth/issues/1743)) ([7e38f4c](https://github.com/supabase/auth/commit/7e38f4cf37768fe2adf92bbd0723d1d521b3d74c)) -* remove server side cookie token methods ([#1742](https://github.com/supabase/auth/issues/1742)) ([c6efec4](https://github.com/supabase/auth/commit/c6efec4cbc950e01e1fd06d45ed821bd27c2ad08)) +* Allow anonymous users to update their passwords ([#1739](https://github.com/supabase/auth/issues/1739)) ([2d51956](https://github.com/supabase/auth/commit/2d519569d7b8540886d0a64bf3e561ef5f91eb63)) +* Hide hook names ([#1743](https://github.com/supabase/auth/issues/1743)) ([7e38f4c](https://github.com/supabase/auth/commit/7e38f4cf37768fe2adf92bbd0723d1d521b3d74c)) +* Remove server-side cookie token methods ([#1742](https://github.com/supabase/auth/issues/1742)) ([c6efec4](https://github.com/supabase/auth/commit/c6efec4cbc950e01e1fd06d45ed821bd27c2ad08))Tools
LanguageTool
[uncategorized] ~10-~10: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...8fe2adf92bbd0723d1d521b3d74c)) * remove server side cookie token methods ([supabase#1742](https://g...(EN_COMPOUND_ADJECTIVE_INTERNAL)
15-17: Ensure consistency in bug fix descriptions.The description for the bug fix should be consistent in style and clarity. Consider rephrasing for better readability.
-* return oauth identity when user is created ([#1736](https://github.com/supabase/auth/issues/1736)) ([60cfb60](https://github.com/supabase/auth/commit/60cfb6063afa574dfe4993df6b0e087d4df71309)) +* Return OAuth identity when a user is created ([#1736](https://github.com/supabase/auth/issues/1736)) ([60cfb60](https://github.com/supabase/auth/commit/60cfb6063afa574dfe4993df6b0e087d4df71309))
22-24: Ensure consistency in feature descriptions.The description for the feature should be consistent in style and clarity. Consider rephrasing for better readability.
-* Vercel marketplace OIDC ([#1731](https://github.com/supabase/auth/issues/1731)) ([a9ff361](https://github.com/supabase/auth/commit/a9ff3612196af4a228b53a8bfb9c11785bcfba8d)) +* Vercel Marketplace OIDC support ([#1731](https://github.com/supabase/auth/issues/1731)) ([a9ff361](https://github.com/supabase/auth/commit/a9ff3612196af4a228b53a8bfb9c11785bcfba8d))
27-35: Ensure consistency in bug fix descriptions.The descriptions for the bug fixes should be consistent in style and clarity. Consider rephrasing for better readability.
-* add error codes to password login flow ([#1721](https://github.com/supabase/auth/issues/1721)) ([4351226](https://github.com/supabase/auth/commit/435122627a0784f1c5cb76d7e08caa1f6259423b)) -* change phone constraint to per user ([#1713](https://github.com/supabase/auth/issues/1713)) ([b9bc769](https://github.com/supabase/auth/commit/b9bc769b93b6e700925fcbc1ebf8bf9678034205)) -* custom SMS does not work with Twilio Verify ([#1733](https://github.com/supabase/auth/issues/1733)) ([dc2391d](https://github.com/supabase/auth/commit/dc2391d15f2c0725710aa388cd32a18797e6769c)) -* ignore errors if transaction has closed already ([#1726](https://github.com/supabase/auth/issues/1726)) ([53c11d1](https://github.com/supabase/auth/commit/53c11d173a79ae5c004871b1b5840c6f9425a080)) -* redirect invalid state errors to site url ([#1722](https://github.com/supabase/auth/issues/1722)) ([b2b1123](https://github.com/supabase/auth/commit/b2b11239dc9f9bd3c85d76f6c23ee94beb3330bb)) -* remove TOTP field for phone enroll response ([#1717](https://github.com/supabase/auth/issues/1717)) ([4b04327](https://github.com/supabase/auth/commit/4b043275dd2d94600a8138d4ebf4638754ed926b)) -* use signing jwk to sign oauth state ([#1728](https://github.com/supabase/auth/issues/1728)) ([66fd0c8](https://github.com/supabase/auth/commit/66fd0c8434388bbff1e1bf02f40517aca0e9d339)) +* Add error codes to the password login flow ([#1721](https://github.com/supabase/auth/issues/1721)) ([4351226](https://github.com/supabase/auth/commit/435122627a0784f1c5cb76d7e08caa1f6259423b)) +* Change phone constraints to be user-specific ([#1713](https://github.com/supabase/auth/issues/1713)) ([b9bc769](https://github.com/supabase/auth/commit/b9bc769b93b6e700925fcbc1ebf8bf9678034205)) +* Fix custom SMS functionality with Twilio Verify ([#1733](https://github.com/supabase/auth/issues/1733)) ([dc2391d](https://github.com/supabase/auth/commit/dc2391d15f2c0725710aa388cd32a18797e6769c)) +* Ignore errors if the transaction has already closed ([#1726](https://github.com/supabase/auth/issues/1726)) ([53c11d1](https://github.com/supabase/auth/commit/53c11d173a79ae5c004871b1b5840c6f9425a080)) +* Redirect invalid state errors to the site URL ([#1722](https://github.com/supabase/auth/issues/1722)) ([b2b1123](https://github.com/supabase/auth/commit/b2b11239dc9f9bd3c85d76f6c23ee94beb3330bb)) +* Remove TOTP field for phone enrollment responses ([#1717](https://github.com/supabase/auth/issues/1717)) ([4b04327](https://github.com/supabase/auth/commit/4b043275dd2d94600a8138d4ebf4638754ed926b)) +* Use signing JWK to sign OAuth states ([#1728](https://github.com/supabase/auth/issues/1728)) ([66fd0c8](https://github.com/supabase/auth/commit/66fd0c8434388bbff1e1bf02f40517aca0e9d339))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional context used
LanguageTool
CHANGELOG.md
[uncategorized] ~10-~10: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...8fe2adf92bbd0723d1d521b3d74c)) * remove server side cookie token methods ([supabase#1742](https://g...(EN_COMPOUND_ADJECTIVE_INTERNAL)
## What kind of change does this PR introduce? * Fixes #1236 * Reduces the number of false positives arising from validation errors when counting rate limits for emails / sms sent * This change applies the shared rate limiter for email and phone functions before the actual email / sms is being sent out rather than at the start of the request * The `limitEmailOrPhoneSentHandler()` now initialises the rate limiters and sets it in the request context so we can subsequently retrieve it right before the email / sms is sent ## What is the current behavior? Please link any relevant issues here. ## What is the new behavior? Feel free to include screenshots if it includes visual changes. ## Additional context Add any other context or screenshots.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
internal/api/middleware_test.go (2)
224-225: Ensure proper error handling.The code correctly captures the context and checks for errors. However, consider adding more detailed error messages for better debugging.
- ctx, err := limiter(w, req) + ctx, err := limiter(w, req) + if err != nil { + t.Fatalf("Failed to limit request: %v", err) + }
408-414: Ensure proper error handling.The code correctly retrieves request parameters and checks for errors. However, consider adding more detailed error messages for better debugging.
- err := retrieveRequestParams(r, &requestBody) + err := retrieveRequestParams(r, &requestBody) + if err != nil { + t.Fatalf("Failed to retrieve request parameters: %v", err) + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- internal/api/context.go (3 hunks)
- internal/api/external.go (11 hunks)
- internal/api/identity.go (2 hunks)
- internal/api/invite.go (1 hunks)
- internal/api/magic_link.go (2 hunks)
- internal/api/mail.go (14 hunks)
- internal/api/middleware.go (3 hunks)
- internal/api/middleware_test.go (2 hunks)
- internal/api/phone.go (3 hunks)
- internal/api/reauthenticate.go (2 hunks)
- internal/api/recover.go (2 hunks)
- internal/api/resend.go (2 hunks)
- internal/api/signup.go (3 hunks)
- internal/api/user.go (3 hunks)
- internal/mailer/mailer.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- internal/api/external.go
Additional comments not posted (46)
internal/api/recover.go (1)
69-69: LGTM! But verify the impact of removing specific error handling.The function is correctly implemented. However, ensure that removing the specific handling for
MaxFrequencyLimitErrordoes not negatively impact the clients interpreting error responses.The code changes are approved.
Run the following script to verify the impact of removing specific error handling:
Verification successful
No impact from removing
MaxFrequencyLimitErrorhandling.The search did not find any definition or usage of
MaxFrequencyLimitErrorin the codebase, indicating that its removal from theRecoverfunction does not affect other parts of the code. The change simplifies error handling without negative consequences.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing specific error handling for `MaxFrequencyLimitError`. # Test: Search for the usage of `MaxFrequencyLimitError`. Expect: No critical dependencies on this specific error handling. rg --type go 'MaxFrequencyLimitError'Length of output: 37
Script:
#!/bin/bash # Description: Search for the definition and usage of `MaxFrequencyLimitError` in the codebase. # Search for the definition of `MaxFrequencyLimitError`. rg 'MaxFrequencyLimitError' --type go -A 5 # Search for any usage of `MaxFrequencyLimitError` in the codebase. rg 'MaxFrequencyLimitError' --type goLength of output: 80
internal/api/invite.go (1)
83-83: LGTM! But verify the impact of returning the original error.The function is correctly implemented. However, ensure that returning the original error instead of wrapping it in a custom internal server error message does not negatively impact the error handling clarity.
The code changes are approved.
Run the following script to verify the impact of returning the original error:
internal/mailer/mailer.go (1)
17-17: LGTM! But verify the impact of removing theSendmethod.The interface is correctly implemented. However, ensure that removing the
Sendmethod does not negatively impact any implementations that relied on this method.The code changes are approved.
Run the following script to verify the impact of removing the
Sendmethod:internal/api/reauthenticate.go (1)
1-1: Verify the impact of removing specific error handling for rate limit errors.The function now directly returns the error if one occurs, without specific handling for rate limit errors. This change simplifies the function but reduces the granularity of error responses. Ensure that clients can handle rate limit violations appropriately.
Run the following script to verify the impact on clients:
internal/api/magic_link.go (1)
143-143: Verify the impact of removing specific error handling forMaxFrequencyLimitError.The function now returns the error directly without additional context or custom handling. This change simplifies the function but reduces the granularity of error responses. Ensure that clients can interpret errors appropriately.
Run the following script to verify the impact on clients:
internal/api/resend.go (1)
145-145: Verify the impact of removing specific error handling forMaxFrequencyLimitError.The function now returns the error directly without additional context or custom handling. This change simplifies the function but reduces the granularity of error responses. Ensure that clients can interpret errors appropriately.
Run the following script to verify the impact on clients:
internal/api/identity.go (1)
135-135: LGTM! But verify the impact of error handling changes.The function is correctly implemented. However, ensure that the broader codebase correctly handles the errors returned by
sendConfirmation.The code changes are approved.
Run the following script to verify the impact of error handling changes:
Verification successful
Error handling change is consistent with the codebase
The error handling change in
identity.goaligns with the existing pattern used across the codebase for thesendConfirmationfunction. This consistency ensures that errors are handled uniformly, and the change should not introduce any new issues.
internal/api/signup.gointernal/api/resend.gointernal/api/external.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of error handling changes in `sendConfirmation`. # Test: Search for the function usage. Expect: Only occurrences of the new error handling. rg --type go -A 5 $'sendConfirmation'Length of output: 3517
internal/api/phone.go (2)
48-48: LGTM!The function is correctly implemented with rate limiting to prevent excessive SMS requests.
The code changes are approved.
89-97: Verify the correctness of rate limiting logic.Ensure that the rate limiting logic correctly prevents excessive SMS requests without causing unintended side effects.
Run the following script to verify the correctness of rate limiting logic:
internal/api/context.go (3)
252-254: LGTM!The function correctly adds the
SharedLimiterinstance to the context.The code changes are approved.
256-262: LGTM!The function correctly retrieves the
SharedLimiterinstance from the context.The code changes are approved.
247-250: LGTM!The struct correctly encapsulates the rate limiters.
The code changes are approved.
internal/api/user.go (2)
104-107: Improve validation logic for anonymous users.The validation logic for anonymous users has been updated to directly check if the password is being set without an accompanying email or phone number. This improves clarity and efficiency.
The code changes are approved.
228-228: Simplify error handling for email change requests.The error handling for sending email change requests has been simplified by returning the error directly without additional checks. This enhances clarity and reduces complexity.
The code changes are approved.
internal/api/middleware.go (1)
100-104: Simplify rate limiting logic.The structured request body and associated checks have been removed. The rate limiter is now stored directly in the request context using a
SharedLimiterstruct. This simplifies the rate-limiting process.The code changes are approved.
internal/api/signup.go (2)
247-247: Simplify error handling for confirmation email.The error handling for the confirmation email sending process has been simplified by directly returning the error from
a.sendConfirmation. This enhances clarity and reduces complexity.The code changes are approved.
277-277: Streamline error handling for repeated sign-up attempts.The handling of the
errvariable has been streamlined, with the check forMaxFrequencyLimitErrorremoved. This simplifies the control flow.The code changes are approved.
internal/api/middleware_test.go (4)
227-229: LGTM!The code correctly checks that the shared limiter is set in the request context.
The code changes are approved.
406-407: LGTM!The code correctly retrieves the limiter from the request context.
The code changes are approved.
415-422: LGTM!The code correctly applies rate limiting for email and sends an appropriate JSON error response if the limit is exceeded.
The code changes are approved.
424-431: LGTM!The code correctly applies rate limiting for phone and sends an appropriate JSON error response if the limit is exceeded.
The code changes are approved.
internal/api/mail.go (25)
8-8: LGTM!The import of
tollboothis necessary for the new rate limiting functionality.The code changes are approved.
26-26: LGTM!The variable
EmailRateLimitExceededis more descriptive and aligns with the updated error handling strategy.The code changes are approved.
319-324: LGTM!The function correctly includes rate limiting checks and improved error handling.
The code changes are approved.
327-328: LGTM!The function correctly updates the user record after sending the confirmation email.
The code changes are approved.
331-332: LGTM!The function correctly creates a one-time token for the confirmation email.
The code changes are approved.
350-355: LGTM!The function correctly includes rate limiting checks and improved error handling.
The code changes are approved.
361-366: LGTM!The function correctly updates the user record and creates a one-time token for the invite email.
The code changes are approved.
376-376: LGTM!The function correctly includes rate limiting checks.
The code changes are approved.
389-394: LGTM!The function correctly includes rate limiting checks and improved error handling.
The code changes are approved.
398-399: LGTM!The function correctly updates the user record after sending the recovery email.
The code changes are approved.
402-403: LGTM!The function correctly creates a one-time token for the recovery email.
The code changes are approved.
426-432: LGTM!The function correctly includes rate limiting checks and improved error handling.
The code changes are approved.
435-436: LGTM!The function correctly updates the user record after sending the reauthentication email.
The code changes are approved.
439-440: LGTM!The function correctly creates a one-time token for the reauthentication email.
The code changes are approved.
452-452: LGTM!The function correctly includes rate limiting checks.
The code changes are approved.
466-471: LGTM!The function correctly includes rate limiting checks and improved error handling.
The code changes are approved.
474-475: LGTM!The function correctly updates the user record after sending the magic link email.
The code changes are approved.
478-479: LGTM!The function correctly creates a one-time token for the magic link email.
The code changes are approved.
489-489: LGTM!The function correctly includes rate limiting checks.
The code changes are approved.
516-521: LGTM!The function correctly includes rate limiting checks and improved error handling.
The code changes are approved.
525-533: LGTM!The function correctly updates the user record after sending the email change email.
The code changes are approved.
537-538: LGTM!The function correctly creates a one-time token for the current email change.
The code changes are approved.
543-544: LGTM!The function correctly creates a one-time token for the new email change.
The code changes are approved.
566-566: LGTM!The function correctly returns a more informative error when the frequency limit is exceeded.
The code changes are approved.
577-589: LGTM!The function correctly includes rate limiting checks before sending the email.
The code changes are approved.
By setting the `GOTRUE_SAML_ALLOW_ENCRYPTED_ASSERTIONS` to `true` the SAML private key will be advertised as usable with encryption too. Encrypted assertions are fairly rare these days because: - They make it very hard to debug what's going on. - HTTPS is the default protocol on the web for over 10 years, including in intranets. **Why not use a separate key?** The underlying library [does not support it](https://pkg.go.dev/github.com/crewjam/saml@v0.4.14/samlsp#Options) and there are no significant cryptological issues using the same RSA key for signatures and encryption, especially in a limited setting like this.
Adds an option to disable magic links, as they are more prone to email sending abuse than other email authentication methods.
Adds support for authorized email addresses, useful when needing to restrict email sending to a handful of email addresses. In the Supabase platform use case, this can be used to allow sending of emails on new projects only to the project's owners or developers, reducing email abuse. To enable it, specify `GOTRUE_EXTERNAL_EMAIL_AUTHORIZED_ADDRESSES` as a comma-delimited string of email addresses. The addresses should be lowercased and without labels. Labels are supported so emails will be sent to `someone+test1@gmail.com` and `someone+test2@gmail.com` if and only if the `someone@gmail.com` address is added in the authorized list. Not a substitute for blocklists!
Deduplicate the `WaitForCleanup` function into one under `utilities`.
## What kind of change does this PR introduce? Use appropriate column name for Web Authn verification - it's fine to change this as it's currently behind the feature flags `MFA_WEB_AUTHN_*_ENABLED` and should have never been used.
Verify that rate limits such as "0/1h", "0/24h" work as intended. This is a tests only change to ensure this behavior is preserved in the future.
Closes #1827 by defaulting to `files:read` oAuth scope for Figma as per the issue.
This does not fix lower restrictions from being bypassed, but does help in the case the rate limit is explicitly set to 0. Co-authored-by: Chris Stockton <chris.stockton@supabase.io>
🤖 I have created a release *beep* *boop* --- ## [2.164.0](v2.163.2...v2.164.0) (2024-11-13) ### Features * return validation failed error if captcha request was not json ([#1815](#1815)) ([26d2e36](26d2e36)) ### Bug Fixes * add error codes to refresh token flow ([#1824](#1824)) ([4614dc5](4614dc5)) * add test coverage for rate limits with 0 permitted events ([#1834](#1834)) ([7c3cf26](7c3cf26)) * correct web authn aaguid column naming ([#1826](#1826)) ([0a589d0](0a589d0)) * default to files:read scope for Figma provider ([#1831](#1831)) ([9ce2857](9ce2857)) * improve error messaging for http hooks ([#1821](#1821)) ([fa020d0](fa020d0)) * make drop_uniqueness_constraint_on_phone idempotent ([#1817](#1817)) ([158e473](158e473)) * possible panic if refresh token has a null session_id ([#1822](#1822)) ([a7129df](a7129df)) * rate limits of 0 take precedence over MAILER_AUTO_CONFIRM ([#1837](#1837)) ([cb7894e](cb7894e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Uploads the release artifact to a Supabase owned S3 bucket for easy releases.
## What kind of change does this PR introduce? Response to an internal ticket, IP mismatch can show up when authenticating as well which might be confusing for users
## What kind of change does this PR introduce? * Return the error code for in the redirect URL instead of the status code
## What kind of change does this PR introduce? Edits an existing migration to fallback onto btree indexes when hash indexes are unavailable. This change allows auth to be compatible with the orioledb storage engine. At time of writing orioledb only support btree indexes. That issue is expected to be addressed by the end of Q1 - 2025. Once that support is added, both heap and orioledb projects will both use the code path for hash indexes. hash indexes are preferred because they are [safer from timing attacks](https://dba.stackexchange.com/questions/285739/prevent-timing-attacks-in-postgres) A reminder has been set for May 1st for us to - remove the fork in this migration - identify and update the index type (from btree to hash) on any orioledb-alpha projects that are still in existence
## What kind of change does this PR introduce? Update workflow to match other repositories and better handling of github.event format.
Reverts #1856. We plan to roll this out in the next version (v2.166.0) as per internal discussion
The goal is to only return an error when we have a very high confidence the email won't be deliverable. This is currently going to be added as a draft for the team to review. I haven't actually implemented any paths that call this or configuration around when it is activated. --------- Co-authored-by: Chris Stockton <chris.stockton@supabase.io>
🤖 I have created a release *beep* *boop* --- ## [2.165.0](v2.164.0...v2.165.0) (2024-12-05) ### Features * add email validation function to lower bounce rates ([#1845](#1845)) ([2c291f0](2c291f0)) * use embedded migrations for `migrate` command ([#1843](#1843)) ([e358da5](e358da5)) ### Bug Fixes * fallback on btree indexes when hash is unavailable ([#1856](#1856)) ([b33bc31](b33bc31)) * return the error code instead of status code ([#1855](#1855)) ([834a380](834a380)) * revert fallback on btree indexes when hash is unavailable ([#1858](#1858)) ([1c7202f](1c7202f)) * update ip mismatch error message ([#1849](#1849)) ([49fbbf0](49fbbf0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.26.0 to 0.31.0. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/golang/crypto/commit/b4f1988a35dee11ec3e05d6bf3e90b695fbd8909"><code>b4f1988</code></a> ssh: make the public key cache a 1-entry FIFO cache</li> <li><a href="https://github.com/golang/crypto/commit/7042ebcbe097f305ba3a93f9a22b4befa4b83d29"><code>7042ebc</code></a> openpgp/clearsign: just use rand.Reader in tests</li> <li><a href="https://github.com/golang/crypto/commit/3e90321ac7bcee3d924ed63ed3ad97be2079cb56"><code>3e90321</code></a> go.mod: update golang.org/x dependencies</li> <li><a href="https://github.com/golang/crypto/commit/8c4e668694ccbaa1be4785da7e7a40f2ef93152b"><code>8c4e668</code></a> x509roots/fallback: update bundle</li> <li><a href="https://github.com/golang/crypto/commit/6018723c74059e3b91c84268b212c2f6cdab1f64"><code>6018723</code></a> go.mod: update golang.org/x dependencies</li> <li><a href="https://github.com/golang/crypto/commit/71ed71b4faf97caafd1863fed003e9ac311f10ee"><code>71ed71b</code></a> README: don't recommend go get</li> <li><a href="https://github.com/golang/crypto/commit/750a45fe5e473d5afa193e9088f3d135e64eca26"><code>750a45f</code></a> sha3: add MarshalBinary, AppendBinary, and UnmarshalBinary</li> <li><a href="https://github.com/golang/crypto/commit/36b172546bd03a74c79e109ec84c599b672ea9e4"><code>36b1725</code></a> sha3: avoid trailing permutation</li> <li><a href="https://github.com/golang/crypto/commit/80ea76eb17c0c52f5d5d04e833d6aeb6b062d81d"><code>80ea76e</code></a> sha3: fix padding for long cSHAKE parameters</li> <li><a href="https://github.com/golang/crypto/commit/c17aa50fbd32393e5d52fa65ca51cbfff0a75aea"><code>c17aa50</code></a> sha3: avoid buffer copy</li> <li>Additional commits viewable in <a href="https://github.com/golang/crypto/compare/v0.26.0...v0.31.0">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/supabase/auth/network/alerts). </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
## What kind of change does this PR introduce? Placeholder solution. Need to: - [x] Manually test
## What kind of change does this PR introduce? * Log a message if the session is nil
This merges a small release hotfix committed into the releases branch back into master. --------- Co-authored-by: Kang Ming <kang.ming1996@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## What kind of change does this PR introduce? * Skip the cleanup function if the status returned from the `ResponseWriter` is a non-2xx code * Refactor the cleanup method to be passed as an interface into `databaseCleanup` to make it easier to test
🤖 I have created a release *beep* *boop* --- ## [2.166.0](v2.165.0...v2.166.0) (2024-12-23) ### Features * switch to googleapis/release-please-action, bump to 2.166.0 ([#1883](#1883)) ([11a312f](11a312f)) ### Bug Fixes * check if session is nil ([#1873](#1873)) ([fd82601](fd82601)) * email_verified field not being updated on signup confirmation ([#1868](#1868)) ([483463e](483463e)) * handle user banned error code ([#1851](#1851)) ([a6918f4](a6918f4)) * Revert "fix: revert fallback on btree indexes when hash is unavailable" ([#1859](#1859)) ([9fe5b1e](9fe5b1e)) * skip cleanup for non-2xx status ([#1877](#1877)) ([f572ced](f572ced)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Argon2 parsing and comparison is broken in multiple ways: 1. Incorrect comparison being done using `ConstantTimeCompare`. This Go API is awful as it returns 1 on _equality_ (unlike all other comparison APIs that return 0) so it was missed. 2. All Argon2 comparisons were producing incorrect derived keys due to the multiplication by 1024. The `argon2.Key` and `IDKey` accept *KiB* as arguments (not bytes!) which caused all hashes to always be incorrect. Tests didn't catch this as they only tested for the positive case (which passed with flying colors).
🤖 I have created a release *beep* *boop* --- ## [2.167.0](v2.166.0...v2.167.0) (2024-12-24) ### Features * fix argon2 parsing and comparison ([#1887](#1887)) ([9dbe6ef](9dbe6ef)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Coverage is higher, but it's showing the coverage of the old repo.
The `-coverpkg` option changed in Go 1.20 and now includes files not under test, which skew the actual coverage. Files not under test: - `/cmd` The commands, these run as part of the executable. - `/client` A non-official admin client that should be removed from the repo in the next major version. - `/docs` Will be removed in a follow up PR. - `/main.go` Main file.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )