codeant-ai-for-open-source[bot] commented on code in PR #36348:
URL: https://github.com/apache/superset/pull/36348#discussion_r2593713016
##########
docs/docs/configuration/databases.mdx:
##########
@@ -54,6 +54,7 @@ are compatible with Superset.
| [Ascend.io](/docs/configuration/databases#ascendio) |
`pip install impyla`
|
`ascend://{username}:{password}@{hostname}:{port}/{database}?auth_mechanism=PLAIN;use_ssl=true`
|
| [Azure MS SQL](/docs/configuration/databases#sql-server) |
`pip install pymssql`
|
`mssql+pymssql://UserName@presetSQL:[email protected]:1433/TestSchema`
|
| [ClickHouse](/docs/configuration/databases#clickhouse) |
`pip install clickhouse-connect`
| `clickhousedb://{username}:{password}@{hostname}:{port}/{database}`
|
+| [Cloudflare D1](/docs/configuration/databases#cloudflare-d1)
| `pip install superset-engine-d1`
|
`d1://{cloudflare_account_id}:{cloudflare_api_token}@{cloudflare_d1_database_id}`
|
Review Comment:
**Suggestion:** Documentation security issue: the new table row shows a full
example SQLAlchemy URI with the Cloudflare API token embedded, which encourages
pasting secrets into URIs (these often get logged or leaked). Replace the
connection-string cell with a link to the detailed section so the table doesn't
advertise embedding secrets and reduces chance of accidental token exposure.
[security]
**Severity Level:** Critical 🚨
```suggestion
| [Cloudflare D1](/docs/configuration/databases#cloudflare-d1)
| `pip install superset-engine-d1`
| [See detailed connection instructions below](#cloudflare-d1)
|
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Replacing the inline example URI in the summary table with a link reduces
the chance
that readers copy/paste URIs containing secrets into places where they might
be logged
or leaked. The existing cell contains only placeholders (no real secret),
but the change
is a sensible security-hardening and reduces duplication of the full example
that already
exists later in the document.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/configuration/databases.mdx
**Line:** 57:57
**Comment:**
*Security: Documentation security issue: the new table row shows a full
example SQLAlchemy URI with the Cloudflare API token embedded, which encourages
pasting secrets into URIs (these often get logged or leaked). Replace the
connection-string cell with a link to the detailed section so the table doesn't
advertise embedding secrets and reduces chance of accidental token exposure.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
docs/docs/configuration/databases.mdx:
##########
@@ -359,6 +360,20 @@ uses the default user without a password (and doesn't
encrypt the connection):
clickhousedb://localhost/default
```
+#### Cloudflare D1
+
+To use Cloudflare D1 with superset, install the
[superset-engine-d1](https://github.com/sqlalchemy-cf-d1/superset-engine-d1)
library.
+
+```
+pip install superset-engine-d1
+```
+
+The expected connection string is formatted as follows:
+
+```
+d1://{cloudflare_account_id}:{cloudflare_api_token}@{cloudflare_d1_database_id}
Review Comment:
**Suggestion:** Missing security and usability guidance in the added
Cloudflare D1 section: it shows an example URI embedding the API token without
warning about logging/leakage or the need to URL-encode special characters;
also it omits the common Docker workflow instruction to persist the driver in
images. Update the section to (1) warn not to embed API tokens in URIs and
prefer Secure Extra/env vars, (2) mention URL-encoding for tokens with special
characters, and (3) show how to add the package to
docker/requirements-local.txt so installs persist in container images.
[security]
**Severity Level:** Critical 🚨
```suggestion
To use Cloudflare D1 with Superset, install the
[superset-engine-d1](https://github.com/sqlalchemy-cf-d1/superset-engine-d1)
library.
```
pip install superset-engine-d1
```
If you're running Superset with Docker Compose, persist the dependency in
your image by adding the package to `./docker/requirements-local.txt`:
```bash
# Run from the repo root:
echo "superset-engine-d1" >> ./docker/requirements-local.txt
```
IMPORTANT SECURITY NOTE: Do not embed raw API tokens directly in SQLAlchemy
URIs in production — URIs can be logged, stored in histories, or leaked. Prefer
storing credentials in the "Secure Extra" field, environment variables, or
using Superset's secure credential mechanisms. If you must include a token in a
URI for testing, URL-encode any special characters in the token (for example,
replace '+' with '%2B', '@' with '%40', etc.).
The expected connection string (for local/testing examples only) is
formatted as follows:
```
d1://{cloudflare_account_id}:{cloudflare_api_token_urlencoded}@{cloudflare_d1_database_id}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The proposed additions fix real omissions: instructing how to persist the
dependency in Docker,
warning against embedding tokens in URIs (which can be logged), and advising
URL-encoding for special characters
are actionable, improve security posture, and match patterns already used
elsewhere in the docs. This is not
mere stylistic noise — it addresses usability and security concerns visible
in the new section.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/configuration/databases.mdx
**Line:** 365:374
**Comment:**
*Security: Missing security and usability guidance in the added
Cloudflare D1 section: it shows an example URI embedding the API token without
warning about logging/leakage or the need to URL-encode special characters;
also it omits the common Docker workflow instruction to persist the driver in
images. Update the section to (1) warn not to embed API tokens in URIs and
prefer Secure Extra/env vars, (2) mention URL-encoding for tokens with special
characters, and (3) show how to add the package to
docker/requirements-local.txt so installs persist in container images.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
