Skip to content

Fail fast when HTTPS is requested but certificates are missing#1640

Open
kmcginnes wants to merge 1 commit intoaws:mainfrom
kmcginnes:fix-silent-https-fallback
Open

Fail fast when HTTPS is requested but certificates are missing#1640
kmcginnes wants to merge 1 commit intoaws:mainfrom
kmcginnes:fix-silent-https-fallback

Conversation

@kmcginnes
Copy link
Copy Markdown
Collaborator

@kmcginnes kmcginnes commented Apr 6, 2026

Description

When PROXY_SERVER_HTTPS_CONNECTION is true but the certificate files are missing, the server silently falls back to HTTP. This is a security concern — the user explicitly requested HTTPS but gets unencrypted traffic with no warning.

This PR makes the server fail fast with a clear error message instead of silently downgrading.

Server config

  • Add ServerConfigError that is thrown when HTTPS is enabled but cert files are missing, with a message listing exactly which files are absent
  • Catch ServerConfigError in node-server.ts with a logger.fatal and process.exit(1)

Shell scripts

  • Fix setup-ssl.sh: add set -e, use POSIX [ -n "$HOST" ], fix the duplicate rootCA.crt check bug (was missing server.key), and report each missing file individually to stderr
  • Fix docker-entrypoint.sh: add set -e, validate .env file exists, anchor the grep pattern with ^ to ignore comments and similarly-named vars, and use POSIX = instead of ==

Tests

  • New docker-entrypoint.test.ts — tests the entrypoint shell script in isolation with stubbed dependencies
  • New config-pipeline.test.ts — end-to-end tests from shell script through Zod parsing to server config
  • New tests in process-environment.test.ts verifying grep-safe .env output
  • Updated server-config.test.ts to assert the new throwing behavior including error messages with specific missing file paths
  • Updated setup-ssl.test.ts to read real config files instead of duplicating them, and test individual missing file reporting

Validation

  • pnpm run checks passes with no errors
  • pnpm test — all 70 tests across the 5 affected test files pass
  • Manual validation using docker locally

Related Issues

Check List

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I have verified pnpm checks passes with no errors.
  • I have verified pnpm test passes with no failures.
  • I have covered new added functionality with unit tests if necessary.
  • I have updated documentation if necessary.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.58%. Comparing base (c5affc5) to head (4a7023d).
⚠️ Report is 123 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1640       +/-   ##
===========================================
+ Coverage   47.81%   72.58%   +24.77%     
===========================================
  Files         382      408       +26     
  Lines        8525    12271     +3746     
  Branches     3159     3517      +358     
===========================================
+ Hits         4076     8907     +4831     
+ Misses       3070     2344      -726     
+ Partials     1379     1020      -359     
Flag Coverage Δ
unittests 72.58% <100.00%> (+24.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@kmcginnes kmcginnes marked this pull request as ready for review April 6, 2026 23:15
@kmcginnes kmcginnes force-pushed the fix-silent-https-fallback branch 3 times, most recently from 2914b13 to 96e6ff7 Compare April 9, 2026 17:00
- Add ServerConfigError and throw when cert files are missing
- Catch ServerConfigError in node-server.ts with a clean fatal log
- Fix docker-entrypoint.sh: add set -e, fix bracket syntax, fix duplicate check
- Fix csr.conf trailing space so sed substitution matches
- Add docker-entrypoint.sh integration tests
- Add config-pipeline integration tests (shell → dotenv → Zod → server config)
- Add ServerConfigError message content tests
- Add grep-safe .env output tests for process-environment.sh
- Add test for openssl failure propagation in setup-ssl
- Read real cert/csr config files in setup-ssl tests

Closes aws#1634
@kmcginnes kmcginnes force-pushed the fix-silent-https-fallback branch from 96e6ff7 to 4a7023d Compare April 9, 2026 17:03
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.

Server silently falls back to HTTP when HTTPS is requested but certificates are missing

2 participants