Skip to content

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Nov 1, 2025

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Bug fix


Description

  • Update Python version to 3.13 in Base Dockerfile

  • Improve shell script error handling with set -e and trap

  • Refactor user creation to use Docker secrets mount

  • Fix chrome-for-testing make target naming inconsistency

  • Remove redundant setuptools dependency from pip install


Diagram Walkthrough

flowchart LR
  A["Base Dockerfile"] -->|"Update Python 3.13"| B["Python 3.13 Installation"]
  A -->|"Add secrets mount"| C["User Creation"]
  D["bootstrap.sh"] -->|"Add error handling"| E["set -e and trap"]
  D -->|"Fix targets"| F["chrome-for-testing_only"]
  G["GitHub Workflow"] -->|"Update test commands"| F
Loading

File Walkthrough

Relevant files
Enhancement
Dockerfile
Update Python to 3.13 and refactor user creation                 

Base/Dockerfile

  • Update ENVSUBST_VERSION from 1.4.6 to 1.4.7
  • Add PYTHON_VERSION=3.13 build argument
  • Add deadsnakes PPA for Python 3.13 installation
  • Refactor Python installation to use specific version with
    update-alternatives
  • Move user creation to separate RUN block using Docker secrets mount
  • Remove setuptools from pip install command
+28/-21 
Bug fix
bootstrap.sh
Improve error handling and fix make targets                           

tests/build-backward-compatible/bootstrap.sh

  • Add set -e flag to exit on command failures
  • Add error trap handler with line number reporting
  • Remove redundant error checking after make commands
  • Fix chrome-for-testing_only make target name
+6/-17   
release-chrome-for-testing-versions.yml
Update workflow test targets for chrome-for-testing           

.github/workflows/release-chrome-for-testing-versions.yml

  • Update test commands from test_chrome to test_chrome-for-testing
  • Update standalone test commands to use chrome-for-testing naming
+3/-3     

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 1, 2025

PR Compliance Guide 🔍

(Compliance updated until commit 3f1bbb1)

Below is a summary of compliance checks for this PR:

Security Compliance
Unpinned external repo

Description: Adding the deadsnakes PPA and importing a GPG key by ID via hkps without pinning specific
key fingerprint verification or package versions can enable supply-chain compromise if the
keyserver or PPA is tampered with.
Dockerfile [92-101]

Referred Code
RUN gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys F23C5A6CF475977595C89F51BA6932366A755776 \
    && gpg --export F23C5A6CF475977595C89F51BA6932366A755776 > /usr/share/keyrings/deadsnakes.pgp \
    && echo "deb [signed-by=/usr/share/keyrings/deadsnakes.pgp] https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu noble main" | tee /etc/apt/sources.list.d/deadsnakes.list \
    && apt-get -qqy update \
    && apt-get upgrade -yq \
    && apt-get -qqy --no-install-recommends install python${PYTHON_VERSION} python${PYTHON_VERSION}-venv \
    && update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 1 \
    && update-alternatives --install /usr/bin/python python /usr/bin/python${PYTHON_VERSION} 1 \
    && python3 -m ensurepip --upgrade \
    && python3 -m pip install --upgrade pip virtualenv --break-system-packages \
Potential secret exposure

Description: The ERR trap prints the failing line number and exit code but may expose sensitive
environment values if subsequent echo commands or traced output include secrets; consider
minimizing error output in CI logs.
bootstrap.sh [32-34]

Referred Code
# Trap errors and exit with the correct error code
trap 'exit_code=$?; echo "Error: Command failed with exit code $exit_code on line $LINENO"; exit $exit_code' ERR
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

🔴
Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Secret exposure risk: The Dockerfile reads a password from the SEL_PASSWD secret and immediately sets the user
password via chpasswd without any explicit measures to prevent it from appearing in image
layers or build logs.

Referred Code
RUN --mount=type=secret,id=SEL_PASSWD \
  groupadd ${SEL_GROUP} \
         --gid ${SEL_GID} \
  && useradd ${SEL_USER} \
         --create-home \
         --gid ${SEL_GID} \
         --shell /bin/bash \
         --uid ${SEL_UID} \
  && usermod -a -G sudo ${SEL_USER} \
  && echo 'ALL ALL = (ALL) NOPASSWD: ALL' >> /etc/sudoers \
  && echo "${SEL_USER}:$(cat /run/secrets/SEL_PASSWD)" | chpasswd
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logs: New shell error handling and build actions do not add any structured audit logging of
critical actions or outcomes beyond simple echo on error.

Referred Code
# Trap errors and exit with the correct error code
trap 'exit_code=$?; echo "Error: Command failed with exit code $exit_code on line $LINENO"; exit $exit_code' ERR

for CDP_VERSION in "${VERSION_LIST[@]}"; do
  python3 tests/build-backward-compatible/builder.py ${SELENIUM_VERSION} ${CDP_VERSION} ${BROWSER}
  export $(cat .env | xargs)
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Error detail exposure: The trap echoes command failure with exit code and line number which could expose internal
script structure if surfaced to end users.

Referred Code
# Trap errors and exit with the correct error code
trap 'exit_code=$?; echo "Error: Command failed with exit code $exit_code on line $LINENO"; exit $exit_code' ERR

for CDP_VERSION in "${VERSION_LIST[@]}"; do
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Key source trust: Importing GPG key from a public keyserver and adding an external PPA without signature
pinning beyond a single key may require additional verification to ensure secure supply
chain handling.

Referred Code
RUN gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys F23C5A6CF475977595C89F51BA6932366A755776 \
    && gpg --export F23C5A6CF475977595C89F51BA6932366A755776 > /usr/share/keyrings/deadsnakes.pgp \
    && echo "deb [signed-by=/usr/share/keyrings/deadsnakes.pgp] https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu noble main" | tee /etc/apt/sources.list.d/deadsnakes.list \
    && apt-get -qqy update \
    && apt-get upgrade -yq \
    && apt-get -qqy --no-install-recommends install python${PYTHON_VERSION} python${PYTHON_VERSION}-venv \
    && update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 1 \
    && update-alternatives --install /usr/bin/python python /usr/bin/python${PYTHON_VERSION} 1 \
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit 3f1bbb1
Security Compliance
Untrusted package source

Description: Adds deadsnakes PPA and imports a GPG key from a public keyserver at build time, which
increases supply-chain risk if the keyserver or PPA is compromised; consider pinning by
fingerprint and using a trusted, reproducible source.
Dockerfile [92-101]

Referred Code
RUN gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys F23C5A6CF475977595C89F51BA6932366A755776 \
    && gpg --export F23C5A6CF475977595C89F51BA6932366A755776 > /usr/share/keyrings/deadsnakes.pgp \
    && echo "deb [signed-by=/usr/share/keyrings/deadsnakes.pgp] https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu noble main" | tee /etc/apt/sources.list.d/deadsnakes.list \
    && apt-get -qqy update \
    && apt-get upgrade -yq \
    && apt-get -qqy --no-install-recommends install python${PYTHON_VERSION} python${PYTHON_VERSION}-venv \
    && update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 1 \
    && update-alternatives --install /usr/bin/python python /usr/bin/python${PYTHON_VERSION} 1 \
    && python3 -m ensurepip --upgrade \
    && python3 -m pip install --upgrade pip virtualenv --break-system-packages \
Excessive sudo privileges

Description: Writes a user password from a Docker secret and grants passwordless sudo to all users via
sudoers, which could allow privilege escalation inside containers if the secret or account
is misused.
Dockerfile [74-84]

Referred Code
RUN --mount=type=secret,id=SEL_PASSWD \
  groupadd ${SEL_GROUP} \
         --gid ${SEL_GID} \
  && useradd ${SEL_USER} \
         --create-home \
         --gid ${SEL_GID} \
         --shell /bin/bash \
         --uid ${SEL_UID} \
  && usermod -a -G sudo ${SEL_USER} \
  && echo 'ALL ALL = (ALL) NOPASSWD: ALL' >> /etc/sudoers \
  && echo "${SEL_USER}:$(cat /run/secrets/SEL_PASSWD)" | chpasswd
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logging: The added error trap and build steps do not record audit logs for critical actions or
include user/context details, leaving auditability unclear for these operations.

Referred Code
# Trap errors and exit with the correct error code
trap 'exit_code=$?; echo "Error: Command failed with exit code $exit_code on line $LINENO"; exit $exit_code' ERR

for CDP_VERSION in "${VERSION_LIST[@]}"; do
  python3 tests/build-backward-compatible/builder.py ${SELENIUM_VERSION} ${CDP_VERSION} ${BROWSER}
  export $(cat .env | xargs)
  if [ "${BROWSER}" = "all" ] || [ "${BROWSER}" = "firefox" ] && [ "${SKIP_BUILD}" = "false" ]; then
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Error detail exposure: The ERR trap echoes the failing line number and exit code to stdout, which may expose
internal execution details if logs are user-visible.

Referred Code
# Trap errors and exit with the correct error code
trap 'exit_code=$?; echo "Error: Command failed with exit code $exit_code on line $LINENO"; exit $exit_code' ERR
Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Secrets handling: The Dockerfile reads SEL_PASSWD from a secrets mount and sets the password, but there is
no explicit guarantee that this value is never echoed or logged elsewhere during build,
requiring verification of build logs.

Referred Code
RUN --mount=type=secret,id=SEL_PASSWD \
  groupadd ${SEL_GROUP} \
         --gid ${SEL_GID} \
  && useradd ${SEL_USER} \
         --create-home \
         --gid ${SEL_GID} \
         --shell /bin/bash \
         --uid ${SEL_UID} \
  && usermod -a -G sudo ${SEL_USER} \
  && echo 'ALL ALL = (ALL) NOPASSWD: ALL' >> /etc/sudoers \
  && echo "${SEL_USER}:$(cat /run/secrets/SEL_PASSWD)" | chpasswd
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unpinned repository: The addition of the deadsnakes PPA with a GPG key fetch at build time introduces
supply-chain risk without pinning by digest or verifying package checksums for Python
3.13.

Referred Code
RUN gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys F23C5A6CF475977595C89F51BA6932366A755776 \
    && gpg --export F23C5A6CF475977595C89F51BA6932366A755776 > /usr/share/keyrings/deadsnakes.pgp \
    && echo "deb [signed-by=/usr/share/keyrings/deadsnakes.pgp] https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu noble main" | tee /etc/apt/sources.list.d/deadsnakes.list \
    && apt-get -qqy update \
    && apt-get upgrade -yq \
    && apt-get -qqy --no-install-recommends install python${PYTHON_VERSION} python${PYTHON_VERSION}-venv \
    && update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 1 \
    && update-alternatives --install /usr/bin/python python /usr/bin/python${PYTHON_VERSION} 1 \
    && python3 -m ensurepip --upgrade \
    && python3 -m pip install --upgrade pip virtualenv --break-system-packages \
    && rm -rf /var/lib/apt/lists/* /var/cache/apt/* \
    && echo "source $VENV_PATH/bin/activate" >> /etc/bash.bashrc

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 1, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Re-evaluate the necessity of Python 3.13

Consider reverting to the default Python 3.12 provided by the Ubuntu Noble base
image. This would remove the need for the 'deadsnakes' PPA, simplifying the
Dockerfile and improving build stability.

Examples:

Base/Dockerfile [92-101]
RUN gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys F23C5A6CF475977595C89F51BA6932366A755776 \
    && gpg --export F23C5A6CF475977595C89F51BA6932366A755776 > /usr/share/keyrings/deadsnakes.pgp \
    && echo "deb [signed-by=/usr/share/keyrings/deadsnakes.pgp] https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu noble main" | tee /etc/apt/sources.list.d/deadsnakes.list \
    && apt-get -qqy update \
    && apt-get upgrade -yq \
    && apt-get -qqy --no-install-recommends install python${PYTHON_VERSION} python${PYTHON_VERSION}-venv \
    && update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 1 \
    && update-alternatives --install /usr/bin/python python /usr/bin/python${PYTHON_VERSION} 1 \
    && python3 -m ensurepip --upgrade \
    && python3 -m pip install --upgrade pip virtualenv --break-system-packages \

Solution Walkthrough:

Before:

# Base/Dockerfile
ARG PYTHON_VERSION=3.13
...
RUN gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys ...
    && gpg --export ... > /usr/share/keyrings/deadsnakes.pgp
    && echo "deb [signed-by=...] https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu noble main" | tee ...
    && apt-get -qqy update
    && apt-get -qqy --no-install-recommends install python${PYTHON_VERSION} python${PYTHON_VERSION}-venv
    && update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 1
    && python3 -m ensurepip --upgrade
    && python3 -m pip install --upgrade pip virtualenv --break-system-packages
    ...

After:

# Base/Dockerfile
# No PYTHON_VERSION ARG needed
...
RUN apt-get -qqy update
    && apt-get upgrade -yq
    && apt-get -qqy --no-install-recommends install \
       python3 python3-pip python3-venv
    && python3 -m ensurepip --upgrade
    && python3 -m pip install --upgrade pip virtualenv --break-system-packages
    ...
Suggestion importance[1-10]: 8

__

Why: This is a significant architectural suggestion that correctly questions adding an external PPA for a minor Python version bump, which increases complexity and fragility without clear justification.

Medium
Security
Improve security and reliability of key retrieval

Improve security and reliability by using the encrypted hkps protocol for GPG
key retrieval and adding a fallback keyserver. This mitigates man-in-the-middle
risks and makes the build process more resilient to keyserver downtime.

Base/Dockerfile [92-103]

-RUN gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys F23C5A6CF475977595C89F51BA6932366A755776 \
+RUN (gpg --keyserver hkps://keyserver.ubuntu.com --recv-keys F23C5A6CF475977595C89F51BA6932366A755776 || \
+     gpg --keyserver hkps://keys.openpgp.org --recv-keys F23C5A6CF475977595C89F51BA6932366A755776) \
     && gpg --export F23C5A6CF475977595C89F51BA6932366A755776 > /usr/share/keyrings/deadsnakes.pgp \
     && echo "deb [signed-by=/usr/share/keyrings/deadsnakes.pgp] https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu noble main" | tee /etc/apt/sources.list.d/deadsnakes.list \
     && apt-get -qqy update \
     && apt-get upgrade -yq \
     && apt-get -qqy --no-install-recommends install python${PYTHON_VERSION} python${PYTHON_VERSION}-venv \
     && update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 1 \
     && update-alternatives --install /usr/bin/python python /usr/bin/python${PYTHON_VERSION} 1 \
     && python3 -m ensurepip --upgrade \
     && python3 -m pip install --upgrade pip virtualenv --break-system-packages \
     && rm -rf /var/lib/apt/lists/* /var/cache/apt/* \
     && echo "source $VENV_PATH/bin/activate" >> /etc/bash.bashrc
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a security vulnerability (unencrypted key retrieval) and a reliability issue (single point of failure for the keyserver), proposing a robust solution that improves both aspects.

Medium
Learned
best practice
Safely load .env variables

Avoid unquoted, unsafe env expansion; parse and export variables robustly to
handle spaces and special characters.

tests/build-backward-compatible/bootstrap.sh [37]

-export $(cat .env | xargs)
+set -a
+# shellcheck disable=SC2046
+. <(grep -v '^\s*#' .env | sed -e 's/\r$//')
+set +a
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Harden shell scripts with strict modes and correct quoting/interpolation to handle special characters safely.

Low
  • More

@VietND96 VietND96 merged commit dc257df into trunk Nov 1, 2025
87 of 88 checks passed
@VietND96 VietND96 deleted the update-python branch November 1, 2025 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants