-
Notifications
You must be signed in to change notification settings - Fork 51
[PROD RELEASE V6] #1702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PROD RELEASE V6] #1702
Conversation
…cted payment logic
…er-v6 Feat/challenge reviewer v6
V6 -> develop
fix(PM-2609): show respective scorecard types for respective review type
fix(PM-2609): lint error
fix(PM-2609): get only active scorecards
fix(PM-2640): copilot url in WM
| ) | ||
| {/* Always show Review link for v5 challenges using v5 ID */} | ||
| <> | ||
| <a href={reviewUrl} target={'_blank'} rel='noopener noreferrer' onClick={onClick}>Review</a> |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
To fix this issue, we should ensure that the value used for the challenge.id in the URL is strictly sanitized and/or type-checked as a valid non-malicious identifier before using it in the anchor's href attribute.
The best fix is to:
- Coerce
challenge.idto a string containing only safe characters appropriate for an identifier. Since IDs are typically numbers, we should ensure only numeric data is passed through, e.g. usingNumber(challenge.id)or a more robust parsing such asparseInt. - Alternatively, explicitly encode the URI component portion containing
challenge.idviaencodeURIComponent, which ensures any unexpected input is safely escaped for use in the URL. - Apply this fix within
src/components/LegacyLinks/index.js, at the point wherereviewUrlis constructed.
No new methods are needed; you can use JavaScript's built-in encodeURIComponent.
-
Copy modified line R15
| @@ -12,7 +12,7 @@ | ||
| e.stopPropagation() | ||
| }, []) | ||
|
|
||
| const reviewUrl = `${REVIEW_APP_URL}/active-challenges/${challenge.id}/challenge-details` | ||
| const reviewUrl = `${REVIEW_APP_URL}/active-challenges/${encodeURIComponent(challenge.id)}/challenge-details` | ||
| return ( | ||
| <div className={styles.container}> | ||
| {/* Always show Review link for v5 challenges using v5 ID */} |
| name: Installation of install_deploysuite. | ||
| command: | | ||
| git clone --branch v1.4.15 https://github.com/topcoder-platform/tc-deploy-scripts ../buildscript | ||
| git clone --branch v1.4.17 https://github.com/topcoder-platform/tc-deploy-scripts ../buildscript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The branch version for tc-deploy-scripts has been updated from v1.4.15 to v1.4.17. Ensure that this new version is compatible with the rest of the deployment scripts and does not introduce breaking changes.
| ./awsconfiguration.sh $DEPLOY_ENV | ||
| ./buildenv.sh -e $DEPLOY_ENV -b ${LOGICAL_ENV}-${APPNAME}-buildvar | ||
| #./buildenv.sh -e $DEPLOY_ENV -b ${LOGICAL_ENV}-${APPNAME}-buildvar | ||
| source awsenvconf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The buildenv.sh script has been commented out and replaced with psvar-processor.sh. Verify that psvar-processor.sh provides all necessary environment configurations previously handled by buildenv.sh to avoid missing configurations.
| ./master_deploy.sh -d ECS -e $DEPLOY_ENV -t latest -s ${LOGICAL_ENV}-global-appvar,${LOGICAL_ENV}-${APPNAME}-appvar -i ${APPNAME} -p FARGATE | ||
| #./buildenv.sh -e $DEPLOY_ENV -b ${LOGICAL_ENV}-${APPNAME}-deployvar | ||
| #source buildenvvar | ||
| ./psvar-processor.sh -t appenv -p /config/${APPNAME}/deployvar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The buildenv.sh script has been commented out and replaced with psvar-processor.sh. Ensure that psvar-processor.sh correctly sets up the environment variables needed for deployment, as this change could affect the deployment process.
| ./psvar-processor.sh -t appenv -p /config/${APPNAME}/deployvar | ||
| source deployvar_env | ||
| #./master_deploy.sh -d ECS -e $DEPLOY_ENV -t latest -s ${LOGICAL_ENV}-global-appvar,${LOGICAL_ENV}-${APPNAME}-appvar -i ${APPNAME} -p FARGATE | ||
| ./master_deploy.sh -d ECS -e $DEPLOY_ENV -t latest -j /config/common/global-appvar,/config/${APPNAME}/appvar -i ${APPNAME} -p FARGATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The master_deploy.sh command now uses a different set of parameters for the -j option. Confirm that the new paths /config/common/global-appvar,/config/${APPNAME}/appvar are correct and accessible during deployment.
| ./awsconfiguration.sh $DEPLOY_ENV | ||
| ./buildenv.sh -e $DEPLOY_ENV -b ${LOGICAL_ENV}-${APPNAME}-buildvar | ||
| #./buildenv.sh -e $DEPLOY_ENV -b ${LOGICAL_ENV}-${APPNAME}-buildvar | ||
| ./psvar-processor.sh -t appenv -p /config/${APPNAME}/buildvar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The buildenv.sh script has been commented out in the smoke testing configuration. Ensure that psvar-processor.sh provides all necessary environment configurations previously handled by buildenv.sh to avoid missing configurations during smoke tests.
| filters: &filters-dev | ||
| branches: | ||
| only: ["develop", "PM-803_wm-regression-fixes", "PM-902_show-all-projects-on-challenge-page", "pm-1365"] | ||
| only: ["develop"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The branch filter for development builds has been changed to only include the develop branch. Ensure this aligns with the intended workflow and that no other branches should trigger development builds.
|
|
||
| permissions: | ||
| contents: read | ||
| security-events: write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ security]
The security-events: write permission is quite broad. Ensure that this level of access is necessary, as it could pose a security risk if not properly justified.
| jobs: | ||
| trivy-scan: | ||
| name: Use Trivy | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Consider using a stable or LTS version of Ubuntu (e.g., ubuntu-latest or ubuntu-22.04) to ensure long-term support and stability for the workflow.
| ignore-unfixed: true | ||
| format: "sarif" | ||
| output: "trivy-results.sarif" | ||
| severity: "CRITICAL,HIGH,UNKNOWN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The severity levels include UNKNOWN, which might lead to unexpected results. Ensure this is intentional and aligns with your security policy.
| output: "trivy-results.sarif" | ||
| severity: "CRITICAL,HIGH,UNKNOWN" | ||
| scanners: vuln,secret,misconfig,license | ||
| github-pat: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ security]
Ensure that the GITHUB_TOKEN has the necessary permissions and is securely managed, as it is used to authenticate the Trivy scanner.
| *.env | ||
| *.pem | ||
| *.vscode | ||
| *.pem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
The entry *.pem is duplicated. Consider removing the duplicate to maintain clarity and avoid confusion.
| *.pem | ||
| *.vscode | ||
| *.pem | ||
| *.vscode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
The entry *.vscode is duplicated. Consider removing the duplicate to maintain clarity and avoid confusion.
| @@ -1,5 +1,5 @@ | |||
| # Use the base image with Node.js | |||
| FROM node:12 | |||
| # Use Node.js 22 base image | |||
Check notice
Code scanning / Trivy
No HEALTHCHECK defined Low
Type: dockerfile
Vulnerability DS026
Severity: LOW
Message: Add HEALTHCHECK instruction in your Dockerfile
Link: DS026
| else | ||
| # Compare lockfile from container with local copy | ||
| cp "$LOCK_FILE_NAME" ".old-$LOCK_FILE_NAME" | ||
| docker cp "app:/$APP_NAME/$LOCK_FILE_NAME" "$LOCK_FILE_NAME" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider checking the exit status of docker cp to ensure the lockfile is copied successfully. This will help avoid silent failures if the copy operation fails.
| fi | ||
| if [ "$UPDATE_CACHE" -eq 1 ]; then | ||
| echo "Lockfile changed or node_modules missing; refreshing local node_modules from container..." | ||
| rm -rf node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[security]
The use of rm -rf node_modules is potentially dangerous as it will forcefully remove the directory without confirmation. Ensure that this is the intended behavior and that there are no unintended side effects.
| CHALLENGE_PHASES_URL: `${DEV_API_HOSTNAME}/v6/challenge-phases`, | ||
| CHALLENGE_TIMELINES_URL: `${DEV_API_HOSTNAME}/v6/challenge-timelines`, | ||
| COPILOTS_URL: 'https://copilots.topcoder-dev.com', | ||
| PROJECT_API_URL: `${DEV_API_HOSTNAME}/v5/projects`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The PROJECT_API_URL is still using the /v5 endpoint, while other similar URLs have been updated to /v6. Verify if this is intentional or if it should be updated to maintain consistency.
| PROJECT_API_URL: `${DEV_API_HOSTNAME}/v5/projects`, | ||
| GROUPS_API_URL: `${DEV_API_HOSTNAME}/v5/groups`, | ||
| GROUPS_API_URL: `${DEV_API_HOSTNAME}/v6/groups`, | ||
| TERMS_API_URL: `${DEV_API_HOSTNAME}/v5/terms`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The TERMS_API_URL is still using the /v5 endpoint, while other similar URLs have been updated to /v6. Verify if this is intentional or if it should be updated to maintain consistency.
| @@ -1,10 +1,18 @@ | |||
| module.exports = (() => { | |||
| let env = process.env.NODE_ENV || 'development' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
Consider using const instead of let for env since its value is not reassigned after initialization. This can prevent accidental reassignment and improve code clarity.
| } | ||
|
|
||
| // Support explicit local environment (mirrors platform-ui local setup) | ||
| const hostEnv = process.env.HOST_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
Consider using const instead of let for hostEnv since its value is not reassigned after initialization. This can prevent accidental reassignment and improve code clarity.
| const API_V5 = `${DEV_API_HOSTNAME}/v5` | ||
|
|
||
| // Local service endpoints mirror platform-ui local.env.ts overrides | ||
| const LOCAL_CHALLENGE_API = 'http://localhost:3000/v6' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[security]
Consider using https instead of http for local service endpoints to ensure encrypted communication, even in local development environments.
| CP_TRACK_ID: '9d6e0de8-df14-4c76-ba0a-a9a8cb03a4ea', | ||
| CHALLENGE_TYPE_ID: '927abff4-7af9-4145-8ba1-577c16e64e2e', | ||
| MARATHON_TYPE_ID: '929bc408-9cf2-4b3e-ba71-adfbf693046c', | ||
| SEGMENT_API_KEY: 'QBtLgV8vCiuRX1lDikbMjcoe9aCHkF6n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ security]
The SEGMENT_API_KEY is hardcoded in the file. Consider using environment variables to store sensitive information like API keys to avoid exposing them in the codebase.
| SUBMISSIONS_API_URL: `${PROD_API_HOSTNAME}/v5/submissions`, | ||
| SUBMISSIONS_API_URL: `${PROD_API_HOSTNAME}/v6/submissions`, | ||
| REVIEW_TYPE_API_URL: `${PROD_API_HOSTNAME}/v5/reviewTypes`, | ||
| REVIEWS_API_URL: `${PROD_API_HOSTNAME}/v5/reviews`, //update to use v6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The comment //update to use v6 suggests an intention to update REVIEWS_API_URL to use version 6, but the URL is still pointing to v5. Ensure the URL is updated to ${PROD_API_HOSTNAME}/v6/reviews if the API version change is intended.
| SUBMISSIONS_API_URL: `${PROD_API_HOSTNAME}/v6/submissions`, | ||
| REVIEW_TYPE_API_URL: `${PROD_API_HOSTNAME}/v5/reviewTypes`, | ||
| REVIEWS_API_URL: `${PROD_API_HOSTNAME}/v5/reviews`, //update to use v6 | ||
| SCORECARDS_API_URL: `${PROD_API_HOSTNAME}/v5/scorecards`, // update to use v6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The comment // update to use v6 indicates an intention to update SCORECARDS_API_URL to version 6, but the URL remains at v5. Verify and update the URL to ${PROD_API_HOSTNAME}/v6/scorecards if the change is required.
| REVIEW_TYPE_API_URL: `${PROD_API_HOSTNAME}/v5/reviewTypes`, | ||
| REVIEWS_API_URL: `${PROD_API_HOSTNAME}/v5/reviews`, //update to use v6 | ||
| SCORECARDS_API_URL: `${PROD_API_HOSTNAME}/v5/scorecards`, // update to use v6 | ||
| WORKFLOWS_API_URL: `${PROD_API_HOSTNAME}/v5/workflows`, // update to use v6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The comment // update to use v6 suggests WORKFLOWS_API_URL should be updated to v6, but it still points to v5. Confirm and update the URL to ${PROD_API_HOSTNAME}/v6/workflows if necessary.
| // Add /* filename */ comments to generated require()s in the output. | ||
| pathinfo: isEnvDevelopment, | ||
| // Ensure compatible hashing algorithm on modern Node versions | ||
| hashFunction: 'xxhash64', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
The change to use xxhash64 for hashFunction is beneficial for performance, but ensure compatibility across environments where the build will be deployed.
| minimizer: [ | ||
| // This is only used in production mode | ||
| new TerserPlugin({ | ||
| parallel: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Enabling parallel for TerserPlugin is a good performance improvement, but verify that it doesn't cause issues in environments with limited CPU resources.
| // This is only used in production mode | ||
| new TerserPlugin({ | ||
| parallel: true, | ||
| extractComments: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Disabling extractComments in TerserPlugin may affect legal compliance if the build includes third-party code with required license comments. Ensure this is acceptable for your use case.
| inject: true, | ||
| template: paths.appHtml | ||
| // Use raw template content to avoid child compilation issues | ||
| templateContent: fs.readFileSync(paths.appHtml, 'utf8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Switching to templateContent for HtmlWebpackPlugin may lead to issues if the HTML template contains dynamic content or requires processing. Ensure this change doesn't affect the expected output.
| }), | ||
| // Merge our app constants with the standard env variables into a single DefinePlugin | ||
| (() => { | ||
| const envMap = env.stringified && env.stringified['process.env'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The merging of environment variables and constants into a single DefinePlugin instance is a good approach for maintainability, but ensure that there are no conflicts between the keys in envMap and constMap.
| chunkFilename: 'static/css/[name].[contenthash:8].chunk.css' | ||
| }), | ||
| // Polyfill select Node core modules for browser bundles (Webpack 5) | ||
| new NodePolyfillPlugin(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Adding NodePolyfillPlugin is necessary for Webpack 5, but verify that it doesn't introduce unnecessary polyfills that could increase bundle size.
| performance: false | ||
| performance: false, | ||
| // When debugging, surface more internal logs from webpack itself | ||
| infrastructureLogging: WM_DEBUG ? { level: 'verbose' } : { level: 'warn' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
The use of WM_DEBUG for infrastructureLogging is a useful debugging feature, but ensure it's properly documented so developers know how to enable verbose logging.
|
|
||
| const protocol = process.env.HTTPS === 'true' ? 'https' : 'http' | ||
| const host = process.env.HOST || '0.0.0.0' | ||
| const WM_DEBUG = /^(1|true|on|yes)$/i.test(String(process.env.WM_DEBUG || '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The regular expression used to parse WM_DEBUG environment variable is case-insensitive and checks for multiple truthy values. Ensure this aligns with the intended use case, as it may lead to unexpected behavior if the environment variable is set to a non-standard truthy value.
| } | ||
| // Add /health endpoint in dev to mirror production server.js | ||
| try { | ||
| const healthCheck = require('topcoder-healthcheck-dropin') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The require statement for topcoder-healthcheck-dropin is inside a try block. If this module is not installed, it will fail silently unless WM_DEBUG is enabled. Consider handling this more explicitly to avoid missing the health check functionality in development.
| # Try to fetch the JSON from S3; fall back to local file if AWS CLI is missing or S3 copy fails | ||
| JSON_CONTENT="" | ||
| if command -v aws >/dev/null 2>&1; then | ||
| if JSON_CONTENT=$(aws s3 cp "${APPVAR_S3_URI}" - 2>/dev/null); then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The use of aws s3 cp without specifying a region or profile might lead to unexpected behavior if the default AWS configuration is not set correctly. Consider explicitly setting the region and profile if applicable.
|
|
||
| if command -v nvm >/dev/null 2>&1; then | ||
| echo "[work-manager] Using Node via nvm..." | ||
| nvm use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The command nvm use without specifying a version can lead to using an unintended Node.js version if multiple versions are installed. Consider specifying a version to ensure consistency.
| -n "${PROXY_HOST}" \ | ||
| -s "${PROXY_SOURCE_PORT}" \ | ||
| -t "${PROXY_TARGET_PORT}" \ | ||
| >"${PROXY_LOG_FILE}" 2>&1 & echo $! > "${PROXY_PID_FILE}" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The background process for local-ssl-proxy is started without checking if it starts successfully. Consider adding a check to ensure the proxy starts correctly and handle any errors.
| fi | ||
|
|
||
| echo "[work-manager] Starting app: ${RUNNER} run start:dev (PORT=${PORT})" | ||
| "${RUNNER}" run start:dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The command ${RUNNER} run start:dev assumes that the start:dev script is defined in the package.json. Ensure that this script exists to prevent runtime errors.
| # Use the base image with Node.js | ||
| FROM node:12 | ||
| # Use Node.js 22 base image | ||
| FROM node:22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The change from Node.js version 12 to 22 is significant. Ensure that all dependencies and the application itself are compatible with Node.js 22, as there may be breaking changes between these major versions.
| RUN chown -R appuser:appuser /challenge-engine-ui | ||
|
|
||
| # Enable corepack and prepare pnpm for installs (run as root) | ||
| RUN corepack enable && corepack prepare pnpm@9.12.0 --activate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The command corepack prepare pnpm@9.12.0 --activate is used to activate a specific version of pnpm. Verify that this version is compatible with your project dependencies and that no breaking changes have been introduced.
| RUN echo "BABEL ENV in Docker: $BABEL_ENV" | ||
| RUN npm install | ||
| RUN npm run lint | ||
| RUN pnpm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Switching from npm install to pnpm install can lead to differences in how dependencies are resolved and installed. Ensure that the project dependencies are correctly handled by pnpm, especially if there are any specific npm scripts or configurations that might not be compatible.
No description provided.