Skip to content

Conversation

@justindbaur
Copy link
Member

🎟️ Tracking

📔 Objective

A few dev container improvements that I have been running with.

  • Add the rust feature, to fully build the solution we build some rust code, this is only needed for tests and doesn't block the main uses but it's nice to not have failures.
  • Make it easy to include an additional root CA. This is useful when your host machine communicates to the internet through a forward proxy.
  • Make it so that the community version can be better booted up in a GitHub Codespace.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@claude
Copy link
Contributor

claude bot commented Nov 26, 2025

Claude finished @justindbaur's task —— View job


Review Complete

I've reviewed the latest changes to this PR. Here's the status of previous findings and any new issues:

Previous Findings Status

Finding 1 (File corruption) - FIXED: The file now ends properly with one_time_setup on line 87.

Finding 2 (Syntax error with comma) - FIXED: Line 45 now correctly uses spaces instead of commas in the function call.

⚠️ Finding 3 (File permissions) - NOT ADDRESSED: The onCreateCommand.sh still uses sudo cp without setting explicit permissions (.devcontainer/internal_dev/onCreateCommand.sh:8).

⚠️ Finding 4 (Certificate validation) - NOT ADDRESSED: No validation is performed before installing the custom root CA (.devcontainer/internal_dev/onCreateCommand.sh:6-12).

Finding 5 (Secret handling newline) - PARTIALLY FIXED, NEW ISSUE: The newline after silent read was NOT added, but I discovered a more critical issue...

New Critical Finding

Finding 6: Secret value incorrectly echoed to stdout

In .devcontainer/community_dev/postCreateCommand.sh:29-30, when handling secrets with -s flag, the function echoes the secret value to stdout:

if [[ "$is_secret" == 1 ]]; then
    read -r -s -p "$question_text" response
    echo "$response"  # ❌ This defeats the purpose of -s flag!

Why this is critical: The -s flag suppresses echo during input to prevent shoulder-surfing, but then echo "$response" prints the secret in plain text. This secret gets captured by the calling code on line 45 and could appear in logs or terminal history.

The real issue: The original Finding 5 misidentified the problem. The missing newline is cosmetic, but echoing secrets to stdout is a security concern.

Suggested fix:

  • If secrets must be returned to the caller, this is unavoidable in the current architecture
  • Consider adding a comment explaining why the secret is echoed despite using -s
  • OR redesign to set environment variables instead of returning via stdout

The -s flag only protects during interactive input but doesn't prevent the value from being used in the script afterward. The current design may be intentional, but it's worth clarifying the security model.

Summary

Two critical issues were fixed (file corruption and syntax error). The file permission and certificate validation suggestions remain unaddressed (these are improvements, not blockers). A new security concern was identified regarding secret handling that warrants discussion.

The PR successfully adds Rust feature support, custom root CA loading, and non-interactive setup for GitHub Codespaces as intended.


if [ -e "$file" ]; then
echo "Adding custom root CA"
sudo cp "$file" /usr/local/share/ca-certificates/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Finding 3: Recommend more restrictive file permissions

When copying CA certificates to system trust store, consider setting restrictive permissions before copying:

sudo install -m 644 -o root -g root "$file" /usr/local/share/ca-certificates/

This ensures the CA certificate has proper ownership and permissions (644 = rw-r--r--).

Comment on lines +6 to +12
if [ -e "$file" ]; then
echo "Adding custom root CA"
sudo cp "$file" /usr/local/share/ca-certificates/
sudo update-ca-certificates
else
echo "No custom root CA found, skipping..."
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Finding 4: Consider validating certificate format

Before installing the custom root CA, consider validating it's actually a valid PEM-formatted certificate. This prevents installation of malformed files that could cause update-ca-certificates to fail or behave unexpectedly.

Example validation approach
if [ -e "$file" ]; then
  echo "Adding custom root CA"
  if ! openssl x509 -in "$file" -noout 2>/dev/null; then
    echo "Error: $file is not a valid X.509 certificate"
    exit 1
  fi
  sudo cp "$file" /usr/local/share/ca-certificates/
  sudo update-ca-certificates
else
  echo "No custom root CA found, skipping..."
fi

Comment on lines +29 to +30
read -r -s -p "$question_text" response
echo "$response"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Finding 5: Secret handling uses -s flag incorrectly

The -s flag suppresses echo but doesn't add a newline after reading, which can cause the response value to be concatenated with subsequent output. After a silent read, you should echo a newline for proper formatting:

if [[ "$is_secret" == 1 ]]; then
    read -r -s -p "$question_text" response
    echo  # Add newline after silent read
    echo "$response"

justindbaur and others added 2 commits November 26, 2025 13:14
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details67600337-a7d7-4b76-8e31-f57d30e9d251

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.27%. Comparing base (f52a630) to head (16af8ab).
⚠️ Report is 38 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6651      +/-   ##
==========================================
+ Coverage   53.10%   53.27%   +0.16%     
==========================================
  Files        1913     1906       -7     
  Lines       85277    84973     -304     
  Branches     7634     7639       +5     
==========================================
- Hits        45286    45268      -18     
+ Misses      38232    37952     -280     
+ Partials     1759     1753       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants