fix: use constant-time comparison for API key validation#33858
fix: use constant-time comparison for API key validation#33858xr843 wants to merge 4 commits intolanggenius:mainfrom
Conversation
dosubot
bot
added
the
size:M
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the application by addressing a critical vulnerability related to timing side-channel attacks. By standardizing on constant-time comparison for all sensitive credential validations, the system is now more resilient against sophisticated brute-force attempts. The changes are purely security-focused and do not introduce any functional modifications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-22 08:11:09.802016547 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-22 08:11:00.020032058 +0000
@@ -21,9 +21,9 @@
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
--> controllers/console/human_input_form.py:190:70
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
- --> controllers/console/init_validate.py:31:2
+ --> controllers/console/init_validate.py:32:2
ERROR Object of class `MissingRouter` has no attribute `post` [missing-attribute]
- --> controllers/console/init_validate.py:44:2
+ --> controllers/console/init_validate.py:45:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
--> controllers/console/ping.py:10:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great security enhancement, replacing standard string comparisons with constant-time hmac.compare_digest to prevent timing attacks on sensitive data like API keys. The changes are applied consistently across multiple files. However, I've found a few places where the new implementation could raise a TypeError if a None value is passed to hmac.compare_digest, which was handled by the original code. My review includes suggestions to add the necessary None checks to prevent these potential runtime errors.
| if not hmac.compare_digest(auth_token, dify_config.ADMIN_API_KEY): | ||
| raise Unauthorized("API key is invalid.") |
There was a problem hiding this comment.
dify_config.ADMIN_API_KEY could be None if not set in the environment, which would cause hmac.compare_digest to raise a TypeError. A check should be added to ensure dify_config.ADMIN_API_KEY is not None before calling compare_digest, similar to how it's handled in other parts of this pull request.
| if not hmac.compare_digest(auth_token, dify_config.ADMIN_API_KEY): | |
| raise Unauthorized("API key is invalid.") | |
| if not dify_config.ADMIN_API_KEY or not hmac.compare_digest(auth_token, dify_config.ADMIN_API_KEY): | |
| raise Unauthorized("API key is invalid.") |
There was a problem hiding this comment.
Thanks for the review! This is actually already guarded. Looking at lines 80-81:
if not dify_config.ADMIN_API_KEY:
raise Unauthorized("API key is invalid.")This early return ensures dify_config.ADMIN_API_KEY is truthy (non-None, non-empty) before we ever reach line 86. Additionally, line 86 itself has a redundant not dify_config.ADMIN_API_KEY or short-circuit guard:
if not dify_config.ADMIN_API_KEY or not hmac.compare_digest(auth_token, dify_config.ADMIN_API_KEY):So hmac.compare_digest is only called when both arguments are guaranteed to be non-None strings.
| if not hmac.compare_digest(payload.api_key, HIDDEN_VALUE): | ||
| extension_data_from_db.api_key = payload.api_key |
There was a problem hiding this comment.
payload.api_key could be None. The original code handled this, but hmac.compare_digest will raise a TypeError if one of its arguments is None. This change could cause a runtime error. Please add a None check to maintain the original behavior and prevent a crash.
| if not hmac.compare_digest(payload.api_key, HIDDEN_VALUE): | |
| extension_data_from_db.api_key = payload.api_key | |
| if payload.api_key is None or not hmac.compare_digest(payload.api_key, HIDDEN_VALUE): | |
| extension_data_from_db.api_key = payload.api_key |
There was a problem hiding this comment.
Good catch on the general principle, but this case is safe. Line 131:
if payload.api_key is None or not hmac.compare_digest(payload.api_key, HIDDEN_VALUE):The payload.api_key is None or short-circuits before hmac.compare_digest is ever called when api_key is None. The second operand HIDDEN_VALUE is a module-level constant string, so it's always non-None.
This preserves the original behavior: when api_key is None, the condition evaluates to True and the key gets updated.
| # Check if client_id is masked and unchanged | ||
| final_client_id = client_id | ||
| if existing_masked.get("client_id") and client_id == existing_masked["client_id"]: | ||
| if existing_masked.get("client_id") and hmac.compare_digest(client_id, existing_masked["client_id"]): |
There was a problem hiding this comment.
client_id could be None, which would cause hmac.compare_digest to raise a TypeError. The original code would correctly evaluate this as False. A None check should be added, similar to how client_secret is handled a few lines below.
| if existing_masked.get("client_id") and hmac.compare_digest(client_id, existing_masked["client_id"]): | |
| if existing_masked.get("client_id") and client_id is not None and hmac.compare_digest(client_id, existing_masked["client_id"]): |
There was a problem hiding this comment.
Already handled. Lines 797-799:
if (
existing_masked.get("client_id")
and client_id is not None
and hmac.compare_digest(client_id, existing_masked["client_id"])
):The client_id is not None check on line 798 explicitly guards against None before hmac.compare_digest is reached. The same pattern is applied for client_secret below (line 806). Both arguments are guaranteed non-None when compare_digest executes.
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-22 15:28:25.367870725 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-22 15:28:16.752719424 +0000
@@ -21,9 +21,9 @@
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
--> controllers/console/human_input_form.py:190:70
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
- --> controllers/console/init_validate.py:31:2
+ --> controllers/console/init_validate.py:32:2
ERROR Object of class `MissingRouter` has no attribute `post` [missing-attribute]
- --> controllers/console/init_validate.py:44:2
+ --> controllers/console/init_validate.py:45:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
--> controllers/console/ping.py:10:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
|
…ner_api_user_auth The previous commit changed db.session.get(EndUser, user_id) to db.session.query(EndUser).where(...).first(), which broke the existing unit test that patches db.session.get. Restore the original call pattern while keeping the hmac_compare_digest constant-time comparison. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-23 00:21:15.378046962 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-23 00:21:06.274071767 +0000
@@ -21,9 +21,9 @@
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
--> controllers/console/human_input_form.py:190:70
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
- --> controllers/console/init_validate.py:31:2
+ --> controllers/console/init_validate.py:32:2
ERROR Object of class `MissingRouter` has no attribute `post` [missing-attribute]
- --> controllers/console/init_validate.py:44:2
+ --> controllers/console/init_validate.py:45:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
--> controllers/console/ping.py:10:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
|
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-23 01:50:59.229572543 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-23 01:50:49.017515763 +0000
@@ -21,9 +21,9 @@
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
--> controllers/console/human_input_form.py:190:70
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
- --> controllers/console/init_validate.py:31:2
+ --> controllers/console/init_validate.py:32:2
ERROR Object of class `MissingRouter` has no attribute `post` [missing-attribute]
- --> controllers/console/init_validate.py:44:2
+ --> controllers/console/init_validate.py:45:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
--> controllers/console/ping.py:10:2
ERROR Object of class `MissingRouter` has no attribute `get` [missing-attribute]
|
|
Superseded by #33986 (rebased on latest main to resolve CI failures caused by upstream test issues). |
2 participants
Summary
Replace
==/!=operators withhmac.compare_digest()for all API key and secret token comparisons to prevent timing side-channel attacks.Closes #33763
Supersedes #33768 (rebased onto latest main to resolve conflicts)
Changes
api/controllers/console/admin.py: Usehmac.compare_digest()for admin API key comparisonapi/controllers/console/auth/oauth_server.py: Usehmac.compare_digest()for OAuth client secret comparison with None checksapi/controllers/console/extension.py: Usehmac.compare_digest()for API key comparison against hidden valueapi/controllers/console/init_validate.py: Usehmac.compare_digest()for init password comparison with None checkapi/controllers/inner_api/wraps.py: Usehmac.compare_digest()for all inner API key comparisons inbilling_inner_api_only(),enterprise_inner_api_only(),enterprise_inner_api_user_auth(), andplugin_inner_api_only()api/extensions/ext_login.py: Usehmac.compare_digest()foradmin_api_key == auth_tokencomparison inload_user_from_request()api/libs/token.py: Usehmac.compare_digest()forauth_token == dify_config.ADMIN_API_KEYcomparison incheck_csrf_token()api/services/tools/mcp_tools_manage_service.py: Usehmac.compare_digest()for client_id and client_secret comparisonsWhy
String comparison with
==short-circuits on the first mismatched character, leaking timing information proportional to the matching prefix length. An attacker can exploit this to brute-force API keys one character at a time.hmac.compare_digest()runs in constant time regardless of input, eliminating this side channel.Test plan
==/!=comparisons for API keys listed in the issue are replacedhmac.compare_digest()accepts twostrarguments (same type), which matches usage hereNoneguards beforehmac.compare_digest()calls where values could beNone(it raisesTypeErroronNone)