[GitHub] [superset] jileeon commented on a diff in pull request #23344: feat(embedded): Add 'urlParams' option to pass query parameters to embedded dashboard(for SQL templating)

["jileeon (via GitHub)" <gi...@apache.org>]

jileeon commented on code in PR #23344:
URL: https://github.com/apache/superset/pull/23344#discussion_r1160371746


##########
superset-embedded-sdk/src/index.ts:
##########
@@ -116,6 +117,8 @@ export async function embedDashboard({
         + filterConfigKeys
           .map(key => DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY[key] + '=' + filterConfig[key]).join('&')
         : ""
+      let extraUrlParams = new URLSearchParams(dashboardUiConfig?.urlParams || window.location.search).toString()

Review Comment:
   @villebro, Yes it is right If an embedded dashboard can be accessed by public internet(not a restricted network).
   So, we should consider the possibility of passing arbitrary query params(like you said above). and your proposal about securing Embedded Dashboards using `allow_embedded` is necessary.
   
   So, Do you agree with terms below?
   1. Any query params can be passed to an embedded dashboard - It means that this merge request is quite reasonable for this term.
   2. Provides the feature you proposed above, So it can secure Embedded Dashboards from passing arbitrary query params. - One more thing, I want a feature flag to enable it globally because I'm using Superset for an internal service.
   
   Thank you for your deep consideration! :)



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org