alveifbklsiu259 commented on code in PR #32261: URL: https://github.com/apache/superset/pull/32261#discussion_r1959314270
########## scripts/check-type.js: ########## @@ -0,0 +1,220 @@ +#!/usr/bin/env node + +/** + * 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. + */ + +// @ts-check +const { exit } = require("node:process"); +const { join, dirname, normalize, sep } = require("node:path"); +const { readdir } = require("node:fs/promises"); +const { existsSync } = require("node:fs"); +const { chdir, cwd } = require("node:process"); +const { createRequire } = require("node:module"); + +const SUPERSET_ROOT = dirname(__dirname); +const PACKAGE_ARG_REGEX = /^package=/; +const EXCLUDE_DECLARATION_DIR_REGEX = /^excludeDeclarationDir=/; +const DECLARATION_FILE_REGEX = /\.d\.ts$/; + +void (async () => { + const args = process.argv.slice(2); + const { + matchedArgs: [packageArg, excludeDeclarationDirArg], + remainingArgs, + } = extractArgs(args, [PACKAGE_ARG_REGEX, EXCLUDE_DECLARATION_DIR_REGEX]); + + if (!packageArg) { + console.error("package is not specified"); + exit(1); + } + + const packageRootDir = getPackage(packageArg); + const updatedArgs = removePackageSegment(remainingArgs, packageRootDir); + const argsStr = updatedArgs.join(" "); + + const excludedDeclarationDirs = getExcludedDeclarationDirs( + excludeDeclarationDirArg + ); + let declarationFiles = await getFilesRecursively( + packageRootDir, + DECLARATION_FILE_REGEX, + excludedDeclarationDirs + ); + declarationFiles = removePackageSegment(declarationFiles, packageRootDir); + const declarationFilesStr = declarationFiles.join(" "); + + const packageRootDirAbsolute = join(SUPERSET_ROOT, packageRootDir); + const tsConfig = getTsConfig(packageRootDirAbsolute); + const command = `--noEmit --allowJs --composite false --project ${tsConfig} ${argsStr} ${declarationFilesStr}`; Review Comment: @korbit-ai I understand the concerns about command injection, but I'd like to provide some context on why sanitization might not be strictly necessary in this case. - Controlled Environment: The script is executed as part of a pre-commit hook in our version control. This controlled environment means that the inputs to the script are coming from a trusted source (i.e., the pre-commit configuration). These inputs include only the changed files and predefined arguments. - Trusted Source: Since the arguments are provided by the pre-commit hook configuration, the risk of malicious input is significantly minimized. The inputs are controlled by the developers working within our repository, which reduces the likelihood of encountering harmful characters. - Practical Implications: Adding sanitization to the script could impose unnecessary restrictions on file naming conventions. This might lead to confusion and hinder developer productivity, as it would require developers to avoid certain characters in file names to ensure the type-checking script works correctly. Additionally, we have similar validation for arguments. For instance, declaration files are ensured to be files that end with `.d.ts`, and only arguments predefined in `pre-commit-config.yaml` or files matching `^superset-frontend\/.*\.(js|jsx|ts|tsx)$` are passed to the script. -- 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-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
