-
Notifications
You must be signed in to change notification settings - Fork 0
Harden devcontainer: port visibility script + CI checks #2
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: 02ez <226986020+02ez@users.noreply.github.com>
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.
Wire contracts and invariants unchanged, public APIs stable; all gates green: unit integration contract and e2e tests at coverage budgets, hot-path benches within SLO, hermetic reproducible build with signed provenance, SBOM diff clean, deps and secrets scanned, migrations idempotent and reversible, PII paths unchanged, telemetry alerts runbooks rulesets CODEOWNERS and env approvals updated. Canary burn-in clean with prod parity and tight blast radius; rollback and roll-forward armed; ADR and issue linked; SDK and client compatibility verified; changelog cut.
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.
Pull Request Overview
This PR addresses a critical security vulnerability in the devcontainer configuration by replacing automatic public port exposure with a secure-by-default approach that requires explicit user consent before making ports publicly accessible.
- Replaces direct public port forwarding with a secure script that defaults to private visibility
- Adds comprehensive CI validation to prevent future security regressions
- Implements user education and explicit opt-in for public port access
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
.devcontainer/devcontainer.json |
Updates postAttachCommand to use secure port visibility script instead of automatic public exposure |
.devcontainer/setup-port-visibility.sh |
New security script that sets private visibility by default with clear warnings about public access risks |
.github/workflows/devcontainer-ci.yml |
New CI workflow that validates devcontainer security configuration and prevents direct public port forwarding |
@@ -0,0 +1,54 @@ | |||
#!/bin/bash | |||
|
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.
The script lacks a shebang validation and should include error handling for missing bash. Consider adding a check to ensure the script is executed with bash and handle cases where bash might not be available in the container environment.
# Ensure the script is running with bash and that bash is available | |
if ! command -v bash >/dev/null 2>&1; then | |
echo "❌ Error: bash is not installed or not found in PATH." >&2 | |
exit 1 | |
fi | |
if [ -z "$BASH_VERSION" ]; then | |
echo "❌ Error: This script must be run with bash." >&2 | |
exit 1 | |
fi |
Copilot uses AI. Check for mistakes.
echo "🔒 Checking devcontainer security configuration..." | ||
|
||
# Check that we don't have direct public port forwarding | ||
if grep -q "ports visibility.*:public" .devcontainer/devcontainer.json; 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.
The grep pattern may not catch all variations of public port visibility configurations. The pattern should be more comprehensive to match different JSON formatting and spacing variations, such as \"ports visibility\".*:.*\"public\"
or similar patterns that could bypass this security check.
if grep -q "ports visibility.*:public" .devcontainer/devcontainer.json; then | |
if jq -e '."ports visibility" == "public"' .devcontainer/devcontainer.json >/dev/null; then |
Copilot uses AI. Check for mistakes.
if grep -q "postAttachCommand" .devcontainer/devcontainer.json; then | ||
if ! grep -q "setup-port-visibility.sh" .devcontainer/devcontainer.json; then | ||
echo "⚠️ postAttachCommand found but not using secure port visibility script" | ||
echo " Consider using ./.devcontainer/setup-port-visibility.sh for secure defaults" | ||
else | ||
echo "✅ Using secure port visibility script" | ||
fi | ||
fi |
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.
The nested if statements could be simplified and made more robust. Consider combining the conditions or using a more specific pattern that validates the complete postAttachCommand structure rather than just checking for the presence of the script name.
if grep -q "postAttachCommand" .devcontainer/devcontainer.json; then | |
if ! grep -q "setup-port-visibility.sh" .devcontainer/devcontainer.json; then | |
echo "⚠️ postAttachCommand found but not using secure port visibility script" | |
echo " Consider using ./.devcontainer/setup-port-visibility.sh for secure defaults" | |
else | |
echo "✅ Using secure port visibility script" | |
fi | |
fi | |
post_attach_cmd=$(jq -r '.postAttachCommand // empty' .devcontainer/devcontainer.json) | |
if [ -n "$post_attach_cmd" ]; then | |
if echo "$post_attach_cmd" | grep -q "\.\/\.devcontainer\/setup-port-visibility\.sh"; then | |
echo "✅ Using secure port visibility script" | |
else | |
echo "⚠️ postAttachCommand found but not using secure port visibility script" | |
echo " Consider using ./.devcontainer/setup-port-visibility.sh for secure defaults" | |
fi |
Copilot uses AI. Check for mistakes.
|
||
# Set default secure visibility (private) | ||
echo -e "${GREEN}🔒 Setting port ${PORT} to private visibility (secure default)${NC}" | ||
if gh cs ports visibility ${PORT}:${DEFAULT_VISIBILITY} -c "$CODESPACE_NAME"; 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.
The PORT variable is not validated before use in the command. This could potentially lead to command injection if an attacker can control the PORT parameter. Add input validation to ensure PORT contains only numeric characters.
Copilot uses AI. Check for mistakes.
# Check forwardPorts configuration | ||
FORWARD_PORTS=$(jq -r '.forwardPorts[]? // empty' .devcontainer/devcontainer.json) | ||
if [ -n "$FORWARD_PORTS" ]; then | ||
echo "📋 Found forwarded ports: $FORWARD_PORTS" |
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.
The jq command extracts ports as separate lines, but the echo statement treats them as a single string. This could lead to incorrect display of multiple ports. Consider using proper array handling or iterating through ports individually for accurate reporting.
echo "📋 Found forwarded ports: $FORWARD_PORTS" | |
echo "📋 Found forwarded ports:" | |
for port in $FORWARD_PORTS; do | |
echo " - $port" | |
done |
Copilot uses AI. Check for mistakes.
This pull request was created as a result of the following prompt in Copilot Chat.
Original prompt - submitted by @02ez
Summary
This PR addresses a security vulnerability in our devcontainer configuration where port 4000 was automatically exposed publicly to the internet without user consent or security warnings.
Security Issue Fixed
Before (Security Risk):
The previous configuration automatically set port 4000 to public visibility, making the local development server accessible to anyone on the internet who knew the URL. This creates potential security risks including:
After (Hardened):
Changes Made
1. Secure Port Visibility Script (
.devcontainer/setup-port-visibility.sh
)Created a new script that implements secure defaults:
2. Updated Devcontainer Configuration
Modified
.devcontainer/devcontainer.json
to use the secure script instead of directly setting public visibility.3. CI Security Validation (
.github/workflows/devcontainer-ci.yml
)Added comprehensive CI checks that:
Security Impact
This change transforms the port forwarding behavior from an insecure default to a secure-by-default approach:
Users who need public or organization-level port access can still achieve this by running the appropriate commands after being clearly informed of the security implications:
Testing
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.