korbit-ai[bot] commented on code in PR #31697:
URL: https://github.com/apache/superset/pull/31697#discussion_r1901669517
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -117,3 +124,36 @@ class CeleryConfig:
)
except ImportError:
logger.info("Using default Docker config...")
+
+
+AUTH_ROLE_PUBLIC = 'Public'
+PUBLIC_ROLE_LIKE = "Public" # i created guest role for anonymous view
dashboards
+
+from flask_appbuilder.security.manager import AUTH_OAUTH
+
+from custom_sso_security_manager import CustomSsoSecurityManager
+CUSTOM_SECURITY_MANAGER = CustomSsoSecurityManager
+
+AUTH_TYPE = AUTH_OAUTH
+AUTH_USER_REGISTRATION = True
+AUTH_USER_REGISTRATION_ROLE = 'Gamma'
+OAUTH_PROVIDERS = [
+ {
+ 'name': 'keycloak',
+ 'token_key': 'access_token',
+ 'icon': 'fa-key',
+ 'remote_app': {
+ 'client_id': 'superset',
+ 'client_secret': 'x0CT3VtUgPxVMRoCNl4zsrkvrhJqoifA',
+ 'api_base_url': 'http://10.47.188.15:8080/realms/edge/protocol/',
+
'jwks_uri':'http://10.47.188.15:8080/realms/edge/protocol/openid-connect/certs',
Review Comment:
### Remove hardcoded secrets from the codebase. <sub></sub>
<details>
<summary>Tell me more</summary>
​
The pull request introduces a security vulnerability by hardcoding sensitive
secrets like the Keycloak client_secret and URLs in the superset_config.py
file. Secrets should never be committed to source code as that exposes them if
the code is compromised.
To resolve this, remove the hardcoded secrets and instead retrieve them from
environment variables or a secure secrets store at runtime. This ensures the
secrets are not present in the codebase.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:c84656c5-a0fb-4192-a764-9222e7f9f25d -->
##########
docker/pythonpath_dev/custom_sso_security_manager.py:
##########
@@ -0,0 +1,43 @@
+#!/usr/bin/env python
+# custom_sso_security_manager.py
+
+import logging
+import json
+from superset.security import SupersetSecurityManager
+
+class CustomSsoSecurityManager(SupersetSecurityManager):
+
+ def oauth_user_info(self, provider, response=None):
+
+ if provider == 'keycloak':
+ me =
self.appbuilder.sm.oauth_remotes[provider].get('openid-connect/userinfo')
+ data = json.loads(me._content)
+ user = {
+ 'username' : data['preferred_username'],
+ 'name' : data['name'],
+ 'email' : data['email'],
+ 'first_name': data['given_name'],
+ 'last_name': data['family_name'],
+ 'roles': data['roles'],
+ 'is_active': True,
+ }
Review Comment:
### Lack of data validation and sanitization in `oauth_user_info` method.
<sub></sub>
<details>
<summary>Tell me more</summary>
​
The `oauth_user_info` method retrieves user data from the Keycloak
`/openid-connect/userinfo` endpoint and directly uses the returned fields like
`preferred_username`, `name`, `email` etc to construct a user object without
validating the data.
A malicious attacker could potentially inject special characters or scripts
in these fields to perform injection or XSS attacks when the data is used
downstream without escaping.
Make sure to properly validate and sanitize all the user fields before using
them.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:ccb5ea6e-317e-4951-a030-5ae28aad41f9 -->
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -117,3 +124,36 @@ class CeleryConfig:
)
except ImportError:
logger.info("Using default Docker config...")
+
+
+AUTH_ROLE_PUBLIC = 'Public'
+PUBLIC_ROLE_LIKE = "Public" # i created guest role for anonymous view
dashboards
+
+from flask_appbuilder.security.manager import AUTH_OAUTH
+
+from custom_sso_security_manager import CustomSsoSecurityManager
+CUSTOM_SECURITY_MANAGER = CustomSsoSecurityManager
+
+AUTH_TYPE = AUTH_OAUTH
+AUTH_USER_REGISTRATION = True
+AUTH_USER_REGISTRATION_ROLE = 'Gamma'
+OAUTH_PROVIDERS = [
+ {
+ 'name': 'keycloak',
+ 'token_key': 'access_token',
+ 'icon': 'fa-key',
+ 'remote_app': {
+ 'client_id': 'superset',
+ 'client_secret': 'x0CT3VtUgPxVMRoCNl4zsrkvrhJqoifA',
+ 'api_base_url': 'http://10.47.188.15:8080/realms/edge/protocol/',
+
'jwks_uri':'http://10.47.188.15:8080/realms/edge/protocol/openid-connect/certs',
Review Comment:
### Environment-specific Keycloak URLs <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Hard-coded IP address and port in Keycloak configuration URLs may cause
connectivity issues in different environments.
###### Why this matters
Using hard-coded IP addresses makes the configuration environment-specific
and will fail when deployed to different environments or if the Keycloak server
changes location.
###### Suggested change
Use environment variables for Keycloak host and port: 'api_base_url':
f'http://{os.getenv("KEYCLOAK_HOST")}:{os.getenv("KEYCLOAK_PORT")}/realms/edge/protocol/'
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:3693fab3-de21-442e-872c-8f3537c586b8 -->
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -97,12 +97,19 @@ class CeleryConfig:
CELERY_CONFIG = CeleryConfig
-FEATURE_FLAGS = {"ALERT_REPORTS": True}
-ALERT_REPORTS_NOTIFICATION_DRY_RUN = True
-WEBDRIVER_BASEURL = "http://superset:8088/"; # When using docker compose
baseurl should be http://superset_app:8088/ # noqa: E501
-# The base URL for the email report hyperlinks.
-WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL
-SQLLAB_CTAS_NO_LIMIT = True
+FEATURE_FLAGS = {
+ "DASHBOARD_RBAC": True,
+ "ENABLE_TEMPLATE_PROCESSING": True,
+ "DASHBOARD_NATIVE_FILTERS": True,
+ "DASHBOARD_CROSS_FILTERS": True,
+}
+OVERRIDE_HTTP_HEADERS = {'X-Frame-Options': 'ALLOWALL'}
+TALISMAN_ENABLED = False
+ENABLE_CORS = True
+HTTP_HEADERS={"X-Frame-Options":"ALLOWALL"}
+
+# Allow embedding of dashboards in iframes
+X_FRAME_OPTIONS = 'ALLOWALL' # ALLOWALL allows embedding from any origin, use
SAMEORIGIN for same domain
Review Comment:
### Security risk with allowed iframe embedding. <sub></sub>
<details>
<summary>Tell me more</summary>
​
The OVERRIDE_HTTP_HEADERS and HTTP_HEADERS settings allow embedding Superset
in iframes from any origin by setting X-Frame-Options: ALLOWALL. This
introduces a security risk of clickjacking attacks.
Please carefully evaluate if public embedding from any site is really
needed. If possible, restrict to allow embedding only from trusted domains
using X-Frame-Options: ALLOW-FROM https://trusted.com.
If any origin does need to be allowed, also add Content-Security-Policy
frame-ancestors * to prevent clickjacking. Additionally, consider alternative
architectures like server-side rendering of dashboards to avoid iframes
altogether.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:ae23faf8-de02-4e14-bd50-0cf1af2e5f16 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
