fix: use constant-time comparison for API key validation#33768
Closed
xr843 wants to merge 8 commits intolanggenius:mainfrom
Closed
fix: use constant-time comparison for API key validation#33768xr843 wants to merge 8 commits intolanggenius:mainfrom
xr843 wants to merge 8 commits intolanggenius:mainfrom
Conversation
Replace == / != operators with hmac.compare_digest() for all API key and secret token comparisons to prevent timing side-channel attacks. Closes langgenius#33763 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
dosubot
bot
added
the
size:S
Contributor
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 implementing constant-time comparison for all API key and secret token validations. By switching from standard string comparison to 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a great security improvement, replacing direct string comparisons of secrets with the constant-time hmac.compare_digest to prevent timing side-channel attacks. The changes are applied consistently across several files. However, I've identified a potential issue where the new implementation could raise a TypeError if an API key is not configured (i.e., its value is None), which would lead to an unhandled exception. I've added comments with suggestions to make these checks more robust and prevent this regression.
api/controllers/inner_api/wraps.py
Outdated
| # get header 'X-Inner-Api-Key' | ||
| inner_api_key = request.headers.get("X-Inner-Api-Key") | ||
| if not inner_api_key or inner_api_key != dify_config.INNER_API_KEY: | ||
| if not inner_api_key or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY): |
Contributor
There was a problem hiding this comment.
While hmac.compare_digest is the correct function for secure comparison, this change could introduce a TypeError if dify_config.INNER_API_KEY is None, as hmac.compare_digest does not accept None for its arguments. This would cause an unhandled exception and a server error. To prevent this, please add a check to ensure dify_config.INNER_API_KEY is truthy before the comparison.
Suggested change
| if not inner_api_key or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY): | |
| if not inner_api_key or not dify_config.INNER_API_KEY or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY): |
api/controllers/inner_api/wraps.py
Outdated
| # get header 'X-Inner-Api-Key' | ||
| inner_api_key = request.headers.get("X-Inner-Api-Key") | ||
| if not inner_api_key or inner_api_key != dify_config.INNER_API_KEY: | ||
| if not inner_api_key or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY): |
Contributor
There was a problem hiding this comment.
This line has the same potential TypeError as noted earlier if dify_config.INNER_API_KEY is None. hmac.compare_digest will raise an exception if one of its arguments is None. Please add a check to ensure the key is a valid string before comparison.
Suggested change
| if not inner_api_key or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY): | |
| if not inner_api_key or not dify_config.INNER_API_KEY or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY): |
api/controllers/inner_api/wraps.py
Outdated
| # get header 'X-Inner-Api-Key' | ||
| inner_api_key = request.headers.get("X-Inner-Api-Key") | ||
| if not inner_api_key or inner_api_key != dify_config.INNER_API_KEY_FOR_PLUGIN: | ||
| if not inner_api_key or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY_FOR_PLUGIN): |
Contributor
There was a problem hiding this comment.
Similar to the other checks in this file, this could raise a TypeError if dify_config.INNER_API_KEY_FOR_PLUGIN is None. A check should be added to prevent a potential server error.
Suggested change
| if not inner_api_key or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY_FOR_PLUGIN): | |
| if not inner_api_key or not dify_config.INNER_API_KEY_FOR_PLUGIN or not hmac_compare_digest(inner_api_key, dify_config.INNER_API_KEY_FOR_PLUGIN): |
api/libs/token.py
Outdated
| if dify_config.ADMIN_API_KEY_ENABLE: | ||
| auth_token = extract_access_token(request) | ||
| if auth_token and auth_token == dify_config.ADMIN_API_KEY: | ||
| if auth_token and hmac.compare_digest(auth_token, dify_config.ADMIN_API_KEY): |
Contributor
There was a problem hiding this comment.
This comparison is vulnerable to a TypeError if dify_config.ADMIN_API_KEY is None. hmac.compare_digest requires both arguments to be strings or bytes. Please add a check for dify_config.ADMIN_API_KEY to prevent a potential server error, which would be a regression from the original code.
Suggested change
| if auth_token and hmac.compare_digest(auth_token, dify_config.ADMIN_API_KEY): | |
| if auth_token and dify_config.ADMIN_API_KEY and hmac.compare_digest(auth_token, dify_config.ADMIN_API_KEY): |
hmac.compare_digest() raises TypeError when either argument is None, which would happen if an API key config value is unset. Add truthiness checks before calling compare_digest to preserve the original behavior of safely rejecting when keys are not configured. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
Author
|
@themavik Thanks for the review! Good catch on line 75 — the logic does look counterintuitive. However, this is the pre-existing behavior that my PR preserves as a 1:1 equivalent replacement: # Before (original):
if signature_base64 != token:
return view(*args, **kwargs)
# After (this PR):
if not hmac_compare_digest(signature_base64, token):
return view(*args, **kwargs)The Whether that soft-auth pattern is the right design is a separate question, but it's outside the scope of this PR (which only replaces Also pushed a follow-up commit to guard against |
crazywoola
previously approved these changes
Mar 20, 2026
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-20 02:10:11.302452702 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-20 02:10:01.386425848 +0000
@@ -115,11 +115,11 @@
ERROR Pyrefly detected conflicting types while breaking a dependency cycle: `str | None` is not assignable to `None`. Adding explicit type annotations might possibly help. [bad-assignment]
--> core/prompt/utils/extract_thread_messages.py:11:5
ERROR Object of class `NoneType` has no attribute `data_source_type` [missing-attribute]
- --> core/rag/datasource/keyword/jieba/jieba.py:143:36
+ --> core/rag/datasource/keyword/jieba/jieba.py:137:36
ERROR Object of class `NoneType` has no attribute `keyword_table` [missing-attribute]
- --> core/rag/datasource/keyword/jieba/jieba.py:145:13
+ --> core/rag/datasource/keyword/jieba/jieba.py:139:13
ERROR Cannot index into `set[Any]` [bad-index]
- --> core/rag/datasource/keyword/jieba/jieba.py:158:29
+ --> core/rag/datasource/keyword/jieba/jieba.py:152:29
ERROR No matching overload found for function `list.__init__` called with arguments: (object) [no-matching-overload]
--> core/rag/datasource/keyword/jieba/jieba_keyword_table_handler.py:88:34
ERROR Class member `AnalyticdbVector.add_texts` overrides parent class `BaseVector` in an inconsistent manner [bad-override]
@@ -586,7 +586,7 @@
ERROR Argument `Literal['normal']` is not assignable to parameter `value` with type `AppStatus | SQLCoreOperations[AppStatus]` in function `sqlalchemy.orm.base.Mapped.__set__` [bad-argument-type]
--> tests/integration_tests/controllers/console/app/test_chat_message_permissions.py:30:22
ERROR Argument `Literal['normal']` is not assignable to parameter `value` with type `AppStatus | SQLCoreOperations[AppStatus]` in function `sqlalchemy.orm.base.Mapped.__set__` [bad-argument-type]
- --> tests/integration_tests/controllers/console/app/test_feedback_export_api.py:32:22
+ --> tests/integration_tests/controllers/console/app/test_feedback_export_api.py:31:22
ERROR Argument `Literal['normal']` is not assignable to parameter `value` with type `AppStatus | SQLCoreOperations[AppStatus]` in function `sqlalchemy.orm.base.Mapped.__set__` [bad-argument-type]
--> tests/integration_tests/controllers/console/app/test_model_config_permissions.py:28:22
ERROR Attribute `current_tenant_id` of class `Account` is a read-only property and cannot be set [read-only]
@@ -940,31 +940,31 @@
ERROR Argument `Literal['archive']` is not assignable to parameter `value` with type `SQLCoreOperations[TenantStatus] | TenantStatus` in function `sqlalchemy.orm.base.Mapped.__set__` [bad-argument-type]
--> tests/test_containers_integration_tests/services/test_account_service.py:3334:25
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:134:38
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:133:38
ERROR `str` is not assignable to attribute `agent_mode` with type `Never` [bad-assignment]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:138:47
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:137:47
ERROR Argument `Literal['account']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:249:29
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:248:29
ERROR Argument `Literal['account']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:275:29
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:274:29
ERROR Argument `Literal['account']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:564:29
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:563:29
ERROR Argument `str | None` is not assignable to parameter `created_by` with type `SQLCoreOperations[str] | str` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:565:24
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:564:24
ERROR Argument `Literal['account']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:799:29
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:798:29
ERROR Argument `str | None` is not assignable to parameter `created_by` with type `SQLCoreOperations[str] | str` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:800:24
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:799:24
ERROR Argument `Literal['account']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:894:29
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:893:29
ERROR Argument `Literal['account']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:963:29
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:962:29
ERROR Argument `str | None` is not assignable to parameter `created_by` with type `SQLCoreOperations[str] | str` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:964:24
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:963:24
ERROR Argument `Literal['account']` is not assignable to parameter `created_by_role` with type `CreatorUserRole | SQLCoreOperations[CreatorUserRole]` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:1004:29
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:1003:29
ERROR Argument `str | None` is not assignable to parameter `created_by` with type `SQLCoreOperations[str] | str` in function `models.model.MessageAgentThought.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_agent_service.py:1005:24
+ --> tests/test_containers_integration_tests/services/test_agent_service.py:1004:24
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
--> tests/test_containers_integration_tests/services/test_annotation_service.py:101:38
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
@@ -1168,13 +1168,13 @@
ERROR Argument `str` is not assignable to parameter `role` with type `SQLCoreOperations[TenantAccountRole] | TenantAccountRole` in function `models.account.TenantAccountJoin.__init__` [bad-argument-type]
--> tests/test_containers_integration_tests/services/test_human_input_delivery_test.py:32:18
ERROR Argument `Literal['active']` is not assignable to parameter `status` with type `AccountStatus | SQLCoreOperations[AccountStatus]` in function `models.account.Account.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_message_export_service.py:53:20
+ --> tests/test_containers_integration_tests/services/test_message_export_service.py:52:20
ERROR Argument `Literal['normal']` is not assignable to parameter `status` with type `SQLCoreOperations[TenantStatus] | TenantStatus` in function `models.account.Tenant.__init__` [bad-argument-type]
- --> tests/test_containers_integration_tests/services/test_message_export_service.py:58:63
+ --> tests/test_containers_integration_tests/services/test_message_export_service.py:57:63
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
- --> tests/test_containers_integration_tests/services/test_message_service.py:119:38
+ --> tests/test_containers_integration_tests/services/test_message_service.py:118:38
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
- --> tests/test_containers_integration_tests/services/test_message_service.py:122:81
+ --> tests/test_containers_integration_tests/services/test_message_service.py:121:81
ERROR Argument `Literal['active']` is not assignable to parameter `status` with type `AccountStatus | SQLCoreOperations[AccountStatus]` in function `models.account.Account.__init__` [bad-argument-type]
--> tests/test_containers_integration_tests/services/test_messages_clean_service.py:112:20
ERROR Argument `Literal['normal']` is not assignable to parameter `status` with type `SQLCoreOperations[TenantStatus] | TenantStatus` in function `models.account.Tenant.__init__` [bad-argument-type]
@@ -1417,6 +1417,28 @@
--> tests/test_containers_integration_tests/services/tools/test_mcp_tools_manage_service.py:63:20
ERROR Argument `Literal['normal']` is not assignable to parameter `status` with type `SQLCoreOperations[TenantStatus] | TenantStatus` in function `models.account.Tenant.__init__` [bad-argument-type]
--> tests/test_containers_integration_tests/services/tools/test_mcp_tools_manage_service.py:72:20
+ERROR Unexpected keyword argument `icon_dark` in function `models.tools.ApiToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:51:17
+ERROR Unexpected keyword argument `credentials` in function `models.tools.ApiToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:54:17
+ERROR Unexpected keyword argument `provider_type` in function `models.tools.ApiToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:55:17
+ERROR Unexpected keyword argument `description` in function `models.tools.BuiltinToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:60:17
+ERROR Unexpected keyword argument `icon` in function `models.tools.BuiltinToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:61:17
+ERROR Unexpected keyword argument `icon_dark` in function `models.tools.BuiltinToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:62:17
+ERROR Unexpected keyword argument `credentials` in function `models.tools.BuiltinToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:66:17
+ERROR Unexpected keyword argument `icon_dark` in function `models.tools.WorkflowToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:73:17
+ERROR Unexpected keyword argument `workflow_id` in function `models.tools.WorkflowToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:76:17
+ERROR Unexpected keyword argument `description` in function `models.tools.MCPToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:81:17
+ERROR Unexpected keyword argument `provider_icon` in function `models.tools.MCPToolProvider.__init__` [unexpected-keyword]
+ --> tests/test_containers_integration_tests/services/tools/test_tools_transform_service.py:82:17
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
--> tests/test_containers_integration_tests/services/tools/test_workflow_tools_manage_service.py:109:38
ERROR Object of class `NoneType` has no attribute `id` [missing-attribute]
@@ -2076,29 +2098,29 @@
ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]
--> tests/unit_tests/controllers/files/test_upload.py:170:26
ERROR Missing argument `tenant_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:171:44
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:168:44
ERROR Missing argument `user_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:171:44
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:168:44
ERROR Missing argument `tenant_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:188:31
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:185:31
ERROR Missing argument `user_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:188:31
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:185:31
ERROR Missing argument `tenant_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:203:35
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:200:35
ERROR Missing argument `user_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:203:35
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:200:35
ERROR Missing argument `tenant_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:225:44
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:222:44
ERROR Missing argument `user_model` in function `controllers.inner_api.plugin.wraps.decorated_view` [missing-argument]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:225:44
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:222:44
ERROR Argument `type[PluginTestPayload]` is not assignable to parameter `payload_type` with type `type[BaseModel]` in function `controllers.inner_api.plugin.wraps.plugin_data` [bad-argument-type]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:253:35
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:250:35
ERROR Argument `type[PluginTestPayload]` is not assignable to parameter `payload_type` with type `type[BaseModel]` in function `controllers.inner_api.plugin.wraps.plugin_data` [bad-argument-type]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:268:35
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:265:35
ERROR Argument `type[TestPluginData.test_should_raise_error_on_invalid_payload.InvalidPayload]` is not assignable to parameter `payload_type` with type `type[BaseModel]` in function `controllers.inner_api.plugin.wraps.plugin_data` [bad-argument-type]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:286:35
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:283:35
ERROR Argument `type[PluginTestPayload]` is not assignable to parameter `payload_type` with type `type[BaseModel]` in function `controllers.inner_api.plugin.wraps.plugin_data` [bad-argument-type]
- --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:299:35
+ --> tests/unit_tests/controllers/inner_api/plugin/test_plugin_wraps.py:296:35
ERROR `SimpleNamespace` is not assignable to attribute `db` with type `SQLAlchemy` [bad-assignment]
--> tests/unit_tests/controllers/mcp/test_mcp.py:19:17
ERROR `SimpleNamespace` is not assignable to attribute `mcp_ns` with type `Namespace` [bad-assignment]
@@ -2122,17 +2144,17 @@
ERROR Missing argument `session` in function `services.conversation_service.ConversationService.pagination_by_last_id` [missing-argument]
--> tests/unit_tests/controllers/service_api/app/test_conversation.py:383:59
ERROR Argument value `Literal[0]` violates Pydantic `ge` constraint `Literal[1]` for field `limit` [bad-argument-type]
- --> tests/unit_tests/controllers/service_api/app/test_message.py:94:63
+ --> tests/unit_tests/controllers/service_api/app/test_message.py:93:63
ERROR Argument value `Literal[101]` violates Pydantic `le` constraint `Literal[100]` for field `limit` [bad-argument-type]
- --> tests/unit_tests/controllers/service_api/app/test_message.py:100:63
+ --> tests/unit_tests/controllers/service_api/app/test_message.py:99:63
ERROR Argument `str | None` is not assignable to parameter `obj` with type `Sized` in function `len` [bad-argument-type]
- --> tests/unit_tests/controllers/service_api/app/test_message.py:138:20
+ --> tests/unit_tests/controllers/service_api/app/test_message.py:137:20
ERROR Argument value `Literal[0]` violates Pydantic `ge` constraint `Literal[1]` for field `page` [bad-argument-type]
- --> tests/unit_tests/controllers/service_api/app/test_message.py:170:31
+ --> tests/unit_tests/controllers/service_api/app/test_message.py:169:31
ERROR Argument value `Literal[0]` violates Pydantic `ge` constraint `Literal[1]` for field `limit` [bad-argument-type]
- --> tests/unit_tests/controllers/service_api/app/test_message.py:183:31
+ --> tests/unit_tests/controllers/service_api/app/test_message.py:182:31
ERROR Argument value `Literal[102]` violates Pydantic `le` constraint `Literal[101]` for field `limit` [bad-argument-type]
- --> tests/unit_tests/controllers/service_api/app/test_message.py:188:31
+ --> tests/unit_tests/controllers/service_api/app/test_message.py:187:31
ERROR Argument `list[dict[str, Any]] | None` is not assignable to parameter `obj` with type `Sized` in function `len` [bad-argument-type]
--> tests/unit_tests/controllers/service_api/app/test_workflow.py:92:20
ERROR Argument value `Literal[0]` violates Pydantic `ge` constraint `Literal[1]` for field `page` [bad-argument-type]
@@ -3956,6 +3978,20 @@
--> tests/unit_tests/core/prompt/test_simple_prompt_transform.py:410:19
ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `context_files` with type `list[File] | None` in function `core.prompt.simple_prompt_transform.SimplePromptTransform._get_last_user_message` [bad-argument-type]
--> tests/unit_tests/core/prompt/test_simple_prompt_transform.py:411:27
+ERROR Argument `None` is not assignable to parameter `process_rule` with type `dict[Unknown, Unknown]` in function `core.rag.cleaner.clean_processor.CleanProcessor.clean` [bad-argument-type]
+ --> tests/unit_tests/core/rag/cleaner/test_clean_processor.py:10:60
+ERROR Argument `None` is not assignable to parameter `process_rule` with type `dict[Unknown, Unknown]` in function `core.rag.cleaner.clean_processor.CleanProcessor.clean` [bad-argument-type]
+ --> tests/unit_tests/core/rag/cleaner/test_clean_processor.py:13:60
+ERROR Argument `None` is not assignable to parameter `process_rule` with type `dict[Unknown, Unknown]` in function `core.rag.cleaner.clean_processor.CleanProcessor.clean` [bad-argument-type]
+ --> tests/unit_tests/core/rag/cleaner/test_clean_processor.py:18:56
+ERROR Argument `None` is not assignable to parameter `process_rule` with type `dict[Unknown, Unknown]` in function `core.rag.cleaner.clean_processor.CleanProcessor.clean` [bad-argument-type]
+ --> tests/unit_tests/core/rag/cleaner/test_clean_processor.py:23:54
+ERROR Argument `None` is not assignable to parameter `process_rule` with type `dict[Unknown, Unknown]` in function `core.rag.cleaner.clean_processor.CleanProcessor.clean` [bad-argument-type]
+ --> tests/unit_tests/core/rag/cleaner/test_clean_processor.py:29:43
+ERROR Argument `None` is not assignable to parameter `process_rule` with type `dict[Unknown, Unknown]` in function `core.rag.cleaner.clean_processor.CleanProcessor.clean` [bad-argument-type]
+ --> tests/unit_tests/core/rag/cleaner/test_clean_processor.py:167:41
+ERROR Argument `None` is not assignable to parameter `process_rule` with type `dict[Unknown, Unknown]` in function `core.rag.cleaner.clean_processor.CleanProcessor.clean` [bad-argument-type]
+ --> tests/unit_tests/core/rag/cleaner/test_clean_processor.py:175:43
ERROR Argument `Literal['manhattan']` is not assignable to parameter `distance_function` with type `Literal['cosine', 'euclidean']` in function `core.rag.datasource.vdb.alibabacloud_mysql.alibabacloud_mysql_vector.AlibabaCloudMySQLVectorConfig.__init__` [bad-argument-type]
--> tests/unit_tests/core/rag/datasource/vdb/alibabacloud_mysql/test_alibabacloud_mysql_vector.py:682:35
ERROR Argument `list[str]` is not assignable to parameter `docs` with type `Sequence[Document]` in function `core.rag.docstore.dataset_docstore.DatasetDocumentStore.add_documents` [bad-argument-type]
@@ -4086,28 +4122,10 @@
--> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:3769:67
ERROR `None` is not subscriptable [unsupported-operation]
--> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4009:16
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.single_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4476:40
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.single_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4528:40
ERROR Argument `SimpleNamespace` is not assignable to parameter `metadata_condition` with type `MetadataCondition | None` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.single_retrieve` [bad-argument-type]
--> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4533:40
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.single_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4549:36
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.single_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4579:36
ERROR Argument `SimpleNamespace` is not assignable to parameter `metadata_condition` with type `MetadataCondition | None` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.single_retrieve` [bad-argument-type]
--> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4584:36
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.single_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4592:36
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.multiple_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4628:36
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.multiple_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4656:36
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.multiple_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4714:40
-ERROR Argument `list[SimpleNamespace]` is not assignable to parameter `available_datasets` with type `list[Dataset]` in function `core.rag.retrieval.dataset_retrieval.DatasetRetrieval.multiple_retrieve` [bad-argument-type]
- --> tests/unit_tests/core/rag/retrieval/test_dataset_retrieval.py:4758:44
ERROR Argument `Iterator[Any]` is not assignable to parameter `invoke_result` with type `Generator[Unknown]` in function `core.rag.retrieval.router.multi_dataset_react_route.ReactMultiDatasetRouter._handle_invoke_result` [bad-argument-type]
--> tests/unit_tests/core/rag/retrieval/test_multi_dataset_react_route.py:184:52
ERROR Argument `None` is not assignable to parameter `text` with type `str` in function `core.rag.splitter.text_splitter.RecursiveCharacterTextSplitter.split_text` [bad-argument-type]
|
Contributor
Pyrefly DiffNo changes detected. |
Member
|
Please fix the lint errors. |
Cover the additional vulnerable locations identified by @dosu: - api/controllers/console/admin.py: admin_required() API key check - api/controllers/console/init_validate.py: INIT_PASSWORD validation - api/controllers/console/auth/oauth_server.py: OAuth client_secret check All three now use hmac.compare_digest() with None guards to prevent both timing side-channel attacks and TypeError on unconfigured values. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
dosubot
bot
added
size:M
Contributor
Author
|
Added a follow-up commit covering the three additional locations identified by @dosu:
This PR now covers all known |
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-20 02:52:57.390909855 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-20 02:52:48.248927190 +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]
|
|
Great work on the comprehensive coverage! 🎉 However, the research identified two additional locations that might warrant consideration:
That said, these may be lower risk since they compare user input against a sentinel/masked value (to detect if a field was modified), rather than validating actual secrets for authentication. The timing information leaked would only reveal how much the input matches the placeholder, not a real credential. If you'd like to include them for completeness, they'd follow the same To reply, just mention @dosu. How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other
|
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-20 02:54:56.963234857 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-20 02:54:46.503269400 +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]
|
…sons Fix lint issues flagged by CI. Also add hmac.compare_digest() to two additional locations identified by @dosu for completeness: - extension.py: payload.api_key sentinel check - mcp_tools_manage_service.py: client_secret masked value check Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-21 08:08:06.348458701 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-21 08:07:57.224312936 +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]
|
Restructure the condition so that a successful signature match sets the user, instead of using a negated early-return on mismatch. This makes the intent clearer without changing behaviour: unverified requests still proceed without a user context. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
Author
|
@themavik Good catch on line 75 — the negated condition did make the intent confusing to read. I've restructured it in 99d1ff9 so the logic reads positively: if the signature matches, set the user; then always return. This preserves the existing behaviour (unverified requests still proceed without a user context, which is by design for this decorator — it's an optional user-injection middleware, not an access gate), while making the intent much clearer. Thanks for the review! |
Contributor
Author
|
@themavik Thanks for the review! Regarding line 75 in if hmac_compare_digest(signature_base64, token):
kwargs["user"] = db.session.query(EndUser).where(EndUser.id == user_id).first()
return view(*args, **kwargs)When the signature matches, we set The comparison logic itself is correct — it's equivalent to the original That said, whether this "optional auth" pattern is the right security design is a separate discussion that might be worth a follow-up issue. Also pushed a fix for |
Contributor
Author
|
@themavik Thanks for the catch! You're right — the original change inverted the logic in if hmac_compare_digest(signature_base64, token):
kwargs["user"] = db.session.query(EndUser).where(EndUser.id == user_id).first()
return view(*args, **kwargs)This is logically equivalent to the original |
4 tasks
Contributor
Author
|
Superseded by #33858 (rebased onto latest main to resolve merge conflicts). |
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
2 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.
Summary
Replace
==/!=operators withhmac.compare_digest()for all API key and secret token comparisons to prevent timing side-channel attacks.Closes #33763
Changes
api/libs/token.py: Usehmac.compare_digest()forauth_token == dify_config.ADMIN_API_KEYcomparison incheck_csrf_token()api/extensions/ext_login.py: Usehmac.compare_digest()foradmin_api_key == auth_tokencomparison inload_user_from_request()api/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()Why
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 here