Copilot commented on code in PR #36684:
URL: https://github.com/apache/superset/pull/36684#discussion_r2729721851
##########
superset-frontend/playwright.config.ts:
##########
@@ -117,10 +120,19 @@ export default defineConfig({
// Web server setup - disabled in CI (Flask started separately in workflow)
webServer: process.env.CI
? undefined
- : {
- command: 'curl -f http://localhost:8088/health',
- url: 'http://localhost:8088/health',
- reuseExistingServer: true,
- timeout: 5000,
- },
+ : (() => {
+ // Support custom base URL (e.g., http://localhost:9012/app/prefix/)
+ const baseUrl =
+ process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088';
+ // Extract origin (scheme + host + port) for health check
+ // Health endpoint is always at /health regardless of app prefix
+ const healthUrl = new URL('/health', new URL(baseUrl).origin).href;
+ return {
+ // Quote URL to prevent shell injection via PLAYWRIGHT_BASE_URL
+ command: `curl -f '${healthUrl}'`,
Review Comment:
Shell injection vulnerability: The `healthUrl` variable is constructed from
`process.env.PLAYWRIGHT_BASE_URL` and directly interpolated into a shell
command with single quotes. While single quotes prevent most shell injection, a
malicious URL containing single quotes could break out of the quoted string.
Use a safer approach such as using Node.js's child_process with array arguments
or properly escaping the URL.
##########
superset-frontend/playwright/tests/experimental/dataset/create-dataset.spec.ts:
##########
@@ -0,0 +1,219 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { test, expect } from '../../../helpers/fixtures/testAssets';
+import type { TestAssets } from '../../../helpers/fixtures/testAssets';
+import type { Page, TestInfo } from '@playwright/test';
+import { ExplorePage } from '../../../pages/ExplorePage';
+import { CreateDatasetPage } from '../../../pages/CreateDatasetPage';
+import { DatasetListPage } from '../../../pages/DatasetListPage';
+import { ChartCreationPage } from '../../../pages/ChartCreationPage';
+import { ENDPOINTS } from '../../../helpers/api/dataset';
+import { waitForPost } from '../../../helpers/api/intercepts';
+import { expectStatusOneOf } from '../../../helpers/api/assertions';
+import { apiPostDatabase } from '../../../helpers/api/database';
+
+interface GsheetsSetupResult {
+ sheetName: string;
+ dbName: string;
+ createDatasetPage: CreateDatasetPage;
+}
+
+/**
+ * Sets up gsheets database and navigates to create dataset page.
+ * Skips test if gsheets connector unavailable (test.skip() throws, so no
return).
+ * @param testInfo - Test info for parallelIndex to avoid name collisions in
parallel runs
+ * @returns Setup result with names and page object
+ */
+async function setupGsheetsDataset(
+ page: Page,
+ testAssets: TestAssets,
+ testInfo: TestInfo,
+): Promise<GsheetsSetupResult> {
+ // Public Google Sheet for testing (published to web, no auth required).
+ // This is a Netflix dataset that is publicly accessible via the Google
Visualization API.
+ // NOTE: This sheet is hosted on an external Google account and is not
created by the test itself.
+ // If this sheet is deleted, its ID changes, or its sharing settings are
restricted,
+ // these tests will start failing when they attempt to create a database
pointing at it.
+ // In that case, create or select a new publicly readable test sheet, update
`sheetUrl`
+ // to use its URL, and update this comment to describe who owns/maintains
that sheet
+ // and the expected access controls (e.g., "anyone with the link can view").
Review Comment:
The comment mentions a specific external Google Sheet that the tests depend
on. If this sheet becomes unavailable, is modified, or its permissions change,
the tests will fail. Consider documenting a fallback plan or creating a
test-owned sheet with guaranteed availability. Additionally, consider including
contact information for who owns this sheet and how to update it if needed.
```suggestion
//
// Fallback plan:
// - If this sheet is deleted, its ID/URL changes, its tab name changes,
or its sharing
// settings are restricted, the database creation in this test will
start failing.
// - In that case, create or select a new publicly readable test sheet
with equivalent
// structure, update `sheetUrl` below and the `catalogDict` mapping, and
validate the
// tests still pass end‑to‑end.
// - When you change the sheet, also update this comment to describe the
new sheet and
// confirm its access controls (e.g., "anyone with the link can view").
//
// Ownership / contact:
// - This test and its external sheet are maintained by the Superset E2E
test owners.
// - See ../AGENTS.md (or the project's standard ownership file) for the
current contact
// responsible for updating this URL and maintaining the backing Google
Sheet.
```
--
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]
