Fix: safe argument guard and remove redundant redis call#14060
Open
paulhuiseismic wants to merge 1 commit intoinfiniflow:mainfrom
Open
Fix: safe argument guard and remove redundant redis call#14060paulhuiseismic wants to merge 1 commit intoinfiniflow:mainfrom
paulhuiseismic wants to merge 1 commit intoinfiniflow:mainfrom
Conversation
dosubot
bot
added
the
size:S
Contributor
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/user_app.py`:
- Around line 1028-1034: The early-return branches in the reset-password flow
lack logging; add debug/info-level logs before each return to record why the
flow exited (e.g., missing args or unverified email). Specifically, in the
function handling reset-password where get_json_result is returned for missing
email/new_pwd/new_pwd2 and where REDIS_CONN.get(_verified_key(email)) is
checked, insert concise log statements (using the module/app logger) that
include the email (or masked email) and the reason ("missing parameters" or
"email not verified") immediately before those get_json_result returns so
failures are traceable in logs.
- Around line 1036-1039: The decrypt/base64 decode sequence for new passwords
(functions/variables: decrypt, new_pwd, new_pwd2, new_pwd_base64,
new_pwd_string, new_pwd2_string) can throw on malformed input; wrap the calls to
decrypt(...) and base64.b64decode(...) in a try/except that catches
decryption/decoding errors (e.g., ValueError, binascii.Error, custom decrypt
exceptions), validate the results are proper UTF-8 strings, and on failure
return a controlled 4xx response (argument/auth error) instead of letting the
exception bubble up; update the code paths that set
new_pwd_base64/new_pwd_string/new_pwd2_string to use these guarded conversions
and return a clear client error when validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beee1159-243b-45ca-aeea-3268d75be1a5
📒 Files selected for processing (1)
api/apps/user_app.py
Comment on lines
+1028
to
1034
| # Guard first, before any decryption that would crash on None values | ||
| if not all([email, new_pwd, new_pwd2]): | ||
| return get_json_result(data=False, code=RetCode.ARGUMENT_ERROR, message="email and passwords are required") | ||
|
|
||
| REDIS_CONN.get(_verified_key(email)) | ||
| # Single Redis call — removed the preceding redundant discarded call | ||
| if not REDIS_CONN.get(_verified_key(email)): | ||
| return get_json_result(data=False, code=RetCode.AUTHENTICATION_ERROR, message="email not verified") |
Contributor
There was a problem hiding this comment.
Add logging for new early-exit reset-password flows.
Line 1029 and Line 1033 return early without any trace logs, which makes reset failure diagnosis harder in production.
Suggested patch
# Guard first, before any decryption that would crash on None values
if not all([email, new_pwd, new_pwd2]):
+ logging.warning("forget_reset_password rejected: missing required fields")
return get_json_result(data=False, code=RetCode.ARGUMENT_ERROR, message="email and passwords are required")
# Single Redis call — removed the preceding redundant discarded call
- if not REDIS_CONN.get(_verified_key(email)):
+ if not REDIS_CONN.get(_verified_key(email)):
+ logging.info("forget_reset_password rejected: email not verified")
return get_json_result(data=False, code=RetCode.AUTHENTICATION_ERROR, message="email not verified")As per coding guidelines, **/*.py: Add logging for new flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/user_app.py` around lines 1028 - 1034, The early-return branches in
the reset-password flow lack logging; add debug/info-level logs before each
return to record why the flow exited (e.g., missing args or unverified email).
Specifically, in the function handling reset-password where get_json_result is
returned for missing email/new_pwd/new_pwd2 and where
REDIS_CONN.get(_verified_key(email)) is checked, insert concise log statements
(using the module/app logger) that include the email (or masked email) and the
reason ("missing parameters" or "email not verified") immediately before those
get_json_result returns so failures are traceable in logs.
Comment on lines
+1036
to
+1039
| new_pwd_base64 = decrypt(new_pwd) | ||
| new_pwd_string = base64.b64decode(new_pwd_base64).decode('utf-8') | ||
| new_pwd2_string = base64.b64decode(decrypt(new_pwd2)).decode('utf-8') | ||
|
|
Contributor
There was a problem hiding this comment.
Handle malformed encrypted/base64 password payloads to prevent 500s.
Line 1036-Line 1039 can raise on bad input and currently bubbles into an internal error response path. This endpoint should return a controlled argument/auth error instead.
Suggested patch
- new_pwd_base64 = decrypt(new_pwd)
- new_pwd_string = base64.b64decode(new_pwd_base64).decode('utf-8')
- new_pwd2_string = base64.b64decode(decrypt(new_pwd2)).decode('utf-8')
+ try:
+ new_pwd_base64 = decrypt(new_pwd)
+ new_pwd_string = base64.b64decode(new_pwd_base64, validate=True).decode("utf-8")
+ new_pwd2_string = base64.b64decode(decrypt(new_pwd2), validate=True).decode("utf-8")
+ except Exception:
+ logging.warning("forget_reset_password rejected: invalid encrypted password payload")
+ return get_json_result(data=False, code=RetCode.ARGUMENT_ERROR, message="invalid password payload")📝 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.
Suggested change
| new_pwd_base64 = decrypt(new_pwd) | |
| new_pwd_string = base64.b64decode(new_pwd_base64).decode('utf-8') | |
| new_pwd2_string = base64.b64decode(decrypt(new_pwd2)).decode('utf-8') | |
| try: | |
| new_pwd_base64 = decrypt(new_pwd) | |
| new_pwd_string = base64.b64decode(new_pwd_base64, validate=True).decode("utf-8") | |
| new_pwd2_string = base64.b64decode(decrypt(new_pwd2), validate=True).decode("utf-8") | |
| except Exception: | |
| logging.warning("forget_reset_password rejected: invalid encrypted password payload") | |
| return get_json_result(data=False, code=RetCode.ARGUMENT_ERROR, message="invalid password payload") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/user_app.py` around lines 1036 - 1039, The decrypt/base64 decode
sequence for new passwords (functions/variables: decrypt, new_pwd, new_pwd2,
new_pwd_base64, new_pwd_string, new_pwd2_string) can throw on malformed input;
wrap the calls to decrypt(...) and base64.b64decode(...) in a try/except that
catches decryption/decoding errors (e.g., ValueError, binascii.Error, custom
decrypt exceptions), validate the results are proper UTF-8 strings, and on
failure return a controlled 4xx response (argument/auth error) instead of
letting the exception bubble up; update the code paths that set
new_pwd_base64/new_pwd_string/new_pwd2_string to use these guarded conversions
and return a clear client error when validation fails.
Contributor
|
@paulhuiseismic |
Contributor
Author
|
Hi @xugangqiang this is just for code enhancement and should be no breaking change without regression issue. So would you like me to add the UT also? cc: @Magicbook1108 |
Contributor
|
Contributor
|
@paulhuiseismic |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Type of change