feat(e2e): implement local https proxy for e2e testing #457
Conversation
|
WalkthroughThis PR adds local HTTPS support for e2e by introducing an nginx-based Docker https-proxy, mkcert-based certificate bootstrap tooling, updated OIDC app and Vite configs to use HTTPS/0.0.0.0, new npm scripts, documentation for setup, and gitignore updates for generated certs. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Scripts as npm / scripts
participant Mkcert as mkcert
participant Docker as Docker Compose
participant Proxy as https-proxy (nginx)
participant Backends as E2E Backends
Dev->>Scripts: pnpm setup:https
Scripts->>Mkcert: bootstrap-https.sh
Mkcert->>Mkcert: ensure root CA (install if needed)
Mkcert-->>Scripts: write proxy-cert.pem / proxy-key.pem to e2e/certs/
Dev->>Scripts: pnpm https-proxy:up
Scripts->>Docker: docker compose -f e2e/docker-compose.yml up --build
Docker->>Proxy: mount certs + ENV upstreams
Proxy->>Proxy: entrypoint renders default.conf from template
Proxy-->>Docker: nginx starts (listening :8443)
Dev->>Proxy: HTTPS request (https://localhost:8443/<path>)
Proxy->>Backends: proxy_pass to appropriate upstream (path-based)
Backends-->>Proxy: response
Proxy-->>Dev: HTTPS response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit b5bf689
☁️ Nx Cloud last updated this comment at |
Implements a local HTTPS development environment using an nginx reverse proxy to support E2E testing of features that require a secure context, such as WebAuthn. Key changes include: - Added an nginx reverse proxy to terminate SSL for local development. - Simplified and corrected the nginx configuration for robustness and proper path-forwarding for multi-page applications. - Updated the OIDC application to use `https` in its redirect URIs and corrected mismatches. - Introduced `pnpm` scripts (`setup:https`, `https-proxy:up`) to streamline setup and usage. - Added documentation for the new local HTTPS setup, including the requirement for `--host=0.0.0.0` when running applications behind the proxy.
ryanbas21
force-pushed
the
nginx-https-webauthn
branch
from
999e67a to
b5bf689
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/oidc-app/src/ping-one/main.ts (1)
17-17: Verify trailing slash consistency (same issue as ping-am).Like
/ping-am, this redirectUri useshttps://localhost:8443/ping-one(no trailing slash), while the nginx configuration defineslocation /ping-one/(with trailing slash). Ensure this doesn't cause OAuth redirect issues.This can be verified alongside the ping-am check using the script provided in the earlier comment.
🧹 Nitpick comments (7)
e2e/https-proxy/default.conf.template (1)
20-22: Consider removing the HTTP to HTTPS redirect.This redirect check is inside an SSL-only server block (line 7:
listen ${LISTEN_PORT} ssl), so$schemewill always behttps. The conditionif ($scheme = http)will never be true, making this redirect dead code.If HTTP to HTTPS redirection is needed, it typically requires a separate server block listening on port 80.
Consider removing the dead code:
- if ($scheme = http) { - return 301 https://$host$request_uri; - } -contributing_docs/README.md (1)
39-42: Good addition of documentation link.The link to the local HTTPS setup documentation is properly formatted and helpful.
Minor suggestion: Consider moving this section under "🔧 Development Guides" instead of after "Release Management," as HTTPS proxy setup is more aligned with development and testing workflows.
e2e/https-proxy/docker-entrypoint.d/10-generate-config.sh (1)
1-17: Add error handling and validation for missing template.The script lacks defensive checks for the template file existence and doesn't validate that all required environment variables are set before attempting substitution. While envsubst will fail if the template is missing, adding explicit checks would provide clearer error messages in container logs.
Consider adding a pre-flight check:
#!/bin/sh set -eu template="/etc/nginx/templates/default.conf.template" output="/etc/nginx/conf.d/default.conf" +if [ ! -f "$template" ]; then + echo "[https-proxy] ERROR: Template file not found: $template" >&2 + exit 1 +fi + echo "[https-proxy] Rendering nginx config..." envsubst '\ ${LISTEN_PORT} \Additionally, consider adding a success message after substitution:
${MOCK_API_UPSTREAM} \ ' < "$template" > "$output" +echo "[https-proxy] nginx config rendered successfully to $output"scripts/bootstrap-https.sh (2)
16-19: Add defensive check for mkcert command execution.Line 16 invokes
mkcert -CAROOTwithin command substitution without verifying success. If mkcert is misconfigured or unavailable at that point, the path construction could fail silently.Consider adding an explicit check:
+caroot=$(mkcert -CAROOT) || { echo "Error: mkcert -CAROOT failed" >&2; exit 1; } -if [ ! -f "$(mkcert -CAROOT)/rootCA.pem" ]; then +if [ ! -f "${caroot}/rootCA.pem" ]; thenThis makes failures explicit and reuses the CAROOT result.
21-22: Consider warning or preserving previous certificates on regeneration.The script silently overwrites existing certificates (line 22). For reproducibility or debugging, consider logging this action or preserving backups of prior certificates when regenerating.
Add a message before regeneration:
echo "Generating proxy certificate..." +if [ -f proxy-cert.pem ]; then + echo " (overwriting existing certificate)" +fi mkcert -cert-file proxy-cert.pem -key-file proxy-key.pem "${DOMAIN_LIST[@]}"contributing_docs/local-https.md (2)
35-38: Clarify Docker proxy restart requirement after certificate regeneration.While the guidance to edit
DOMAIN_LISTand re-runsetup:httpsis clear, users may not realize they need to restart the running Docker proxy to pick up the new certificate.Add clarification:
## Regenerating or customizing - Re-run `pnpm run setup:https` at any time; it overwrites the pem files in place. +- If the proxy is already running, restart it to load the new certificate: `pnpm https-proxy:restart` or `docker compose -f e2e/docker-compose.yml restart https-proxy`. - To add additional hostnames (for example `dev.ping.local`), edit `DOMAIN_LIST` inside `scripts/bootstrap-https.sh` before re-running the script. Make sure the new hostnames resolve to your proxy (via `/etc/hosts`, corporate DNS, etc.).
57-67: Clarify applicability of--host=0.0.0.0to other dev server types.Line 61 specifies "Vite-based applications" but doesn't mention whether other frameworks (Next.js, Node dev servers, etc.) have similar requirements or different approaches.
Consider adding:
For Vite-based applications (like `oidc-app` or `davinci-app`), you need to start the dev server with the `--host=0.0.0.0` flag. This tells the server to listen on all available network interfaces, not just `localhost`. This is essential for the `nginx` proxy container to be able to connect to your application's dev server. +**Note:** If using other dev servers or frameworks (e.g., Next.js, Node), consult their documentation for the equivalent network binding flag. The key requirement is that the dev server must be accessible to the nginx container via `host.docker.internal:<port>`. + Here is an example command:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(2 hunks)contributing_docs/README.md(1 hunks)contributing_docs/local-https.md(1 hunks)e2e/docker-compose.yml(1 hunks)e2e/https-proxy/Dockerfile(1 hunks)e2e/https-proxy/default.conf.template(1 hunks)e2e/https-proxy/docker-entrypoint.d/10-generate-config.sh(1 hunks)e2e/oidc-app/src/ping-am/main.ts(1 hunks)e2e/oidc-app/src/ping-one/main.ts(1 hunks)e2e/oidc-app/vite.config.ts(1 hunks)package.json(1 hunks)scripts/bootstrap-https.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (16)
e2e/oidc-app/vite.config.ts (1)
12-19: LGTM! Configuration correctly supports the HTTPS proxy setup.The changes to bind on
0.0.0.0:5173for both server and preview modes are appropriate for the reverse proxy architecture, where:
- Vite serves plain HTTP on port 5173
- The nginx proxy terminates TLS on port 8443
0.0.0.0binding allows Docker containers to reach the service viahost.docker.internal.gitignore (2)
63-63: LGTM! Correctly ignores generated certificates.Ignoring
e2e/certs/prevents accidentally committing the TLS certificates and private keys generated by the bootstrap script.
85-86: Good cleanup of the GEMINI.md ignore pattern.The change from
-**/GEMINI.mdto**/GEMINI.mdremoves the non-standard leading minus sign. Both patterns ignore GEMINI.md files at all subdirectory levels, but the new pattern is cleaner and more conventional.e2e/https-proxy/default.conf.template (5)
1-4: LGTM! Standard WebSocket upgrade map.The map configuration correctly handles WebSocket upgrade headers, which is essential for apps that may use WebSocket connections.
6-17: LGTM! Secure TLS configuration.The TLS configuration is secure and modern:
- HTTP/2 support for better performance
- TLS 1.2 and 1.3 only (excludes vulnerable older versions)
- Appropriate session cache settings
- Server cipher preference enabled
24-27: Security headers are appropriate for local development.The security headers provide good protection:
- HSTS enforces HTTPS for 1 year
- X-Content-Type-Options prevents MIME sniffing
- X-Frame-Options prevents clickjacking
Note:
X-XSS-Protection(line 27) is deprecated in modern browsers, which now rely on Content Security Policy instead. However, for local e2e testing, this is acceptable.
29-39: LGTM! Proxy headers are correctly configured.The proxy configuration properly handles:
- Standard forwarding headers (Host, X-Real-IP, X-Forwarded-For, X-Forwarded-Proto)
- WebSocket upgrade support via HTTP/1.1 and the upgrade map
- Appropriate body size limit (20m) for file uploads
41-72: LGTM! Location blocks are well-structured.The routing configuration appropriately handles different upstream services:
- Path-stripping rewrites for
/davinci/,/protect/,/device-client/, and/mock-api/(indicating these apps don't expect the prefix)- Direct proxying for
/ping-am/and/ping-one/(indicating these multi-page apps expect to see their paths)- Sensible fallback to the OIDC app for unmatched routes
The pattern is consistent and correctly implemented.
package.json (1)
30-34: LGTM! Scripts properly support the HTTPS proxy workflow.The two new scripts are well-structured:
setup:https: Bootstraps TLS certificates (must be run first)https-proxy:up: Starts the Docker-based nginx proxy with rebuild flagThe use of
docker compose(v2 syntax) is correct and modern.e2e/https-proxy/Dockerfile (4)
1-3: LGTM! Appropriate base image and dependencies.Using
nginx:1.27-alpineprovides a recent, secure nginx version with a small footprint. Thegettextpackage is necessary for environment variable substitution in the configuration template.
14-15: Template and entrypoint setup is correct.The COPY instructions properly integrate with nginx's startup sequence:
- Templates go to
/etc/nginx/templates/(nginx convention)- Custom entrypoint script in
/docker-entrypoint.d/runs during container startup- The custom script renders the template to
/etc/nginx/conf.d/default.confThe setup follows nginx best practices for dynamic configuration generation.
17-17: LGTM! Port exposure matches the SSL configuration.Exposing port 8443 aligns with the
LISTEN_PORTenvironment variable and the HTTPS-only nature of this proxy.
5-12: Environment variables are correctly configured with proper Linux support.The Dockerfile changes are appropriate:
OIDC_UPSTREAM=host.docker.internal:5173correctly matches the Vite configuration- TLS certificate paths are well-defined
- All upstream ports are distinct and properly assigned
The Linux compatibility concern is already addressed: the docker-compose.yml (lines 19-20) includes
extra_hosts: - "host.docker.internal:host-gateway", which resolveshost.docker.internalon Linux without requiring additional configuration.e2e/oidc-app/src/ping-am/main.ts (1)
17-17: The trailing slash inconsistency does not cause routing issues, but consider making it explicit.The redirectUri
https://localhost:8443/ping-am(no trailing slash) doesn't match the nginx location/ping-am/(with trailing slash). Requests without a trailing slash will fall through to the rootlocation /, which also proxies toOIDC_UPSTREAM. Since both paths route to the same upstream, there's no functional routing problem. However, for clarity and maintainability, align the paths: either update the nginx locations to match the redirectUri patterns (remove trailing slashes from/ping-am/and/ping-one/), or update the redirectUri values to include trailing slashes.e2e/docker-compose.yml (1)
1-20: Docker Compose configuration looks solid.The https-proxy service is correctly configured with proper networking (host.docker.internal), read-only volume mount for certificates, and environment variables for dynamic upstream routing. The setup aligns with the E2E proxy requirements.
scripts/bootstrap-https.sh (1)
4-5: Verify IPv6 loopback (::1) support in mkcert across all platforms.Including
::1in DOMAIN_LIST assumes mkcert and the system handle IPv6 correctly. While this generally works on modern systems, some Windows configurations or restricted environments may not support IPv6. Consider documenting this constraint or adding a platform-specific check.Do you want to verify that mkcert handles
::1consistently, or should this be platform-specific (e.g., IPv6 only on macOS/Linux)?
|
Let's wait on this. I believe my solution is an easier and simpler solution that works across one's entire ecosystem without having to alter any of the project's tooling. |
@forgerock/davinci-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed 1681ce4 to https://ForgeRock.github.io/ping-javascript-sdk/pr-457/1681ce445a20c08fcd158681a10e9bfb91a57a0c branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 82.0 KB (+0.0 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/bootstrap-https.sh (2)
16-19: Consider adding explicit error handling for mkcert -install.The
mkcert -installcommand can fail due to insufficient permissions or user cancellation. Whileset -ewill catch the failure, adding an explicit error message would improve the user experience.Apply this diff to add explicit error handling:
if [ ! -f "$(mkcert -CAROOT)/rootCA.pem" ]; then echo "Installing mkcert root CA..." - mkcert -install + if ! mkcert -install; then + echo "Failed to install mkcert root CA. Administrator privileges may be required." >&2 + exit 1 + fi fi
21-26: Consider validating certificate generation success.While
set -ewill catch command failures, explicitly verifying that the certificate files were created would make the script more robust and provide clearer feedback.Apply this diff to add file validation:
echo "Generating proxy certificate..." mkcert -cert-file proxy-cert.pem -key-file proxy-key.pem "${DOMAIN_LIST[@]}" +if [ ! -f proxy-cert.pem ] || [ ! -f proxy-key.pem ]; then + echo "Certificate generation failed - files not found" >&2 + exit 1 +fi + popd >/dev/null echo "Certificate material written to ${CERT_DIR}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(2 hunks)contributing_docs/README.md(1 hunks)contributing_docs/local-https.md(1 hunks)e2e/docker-compose.yml(1 hunks)e2e/https-proxy/Dockerfile(1 hunks)e2e/https-proxy/default.conf.template(1 hunks)e2e/https-proxy/docker-entrypoint.d/10-generate-config.sh(1 hunks)e2e/oidc-app/src/ping-am/main.ts(1 hunks)e2e/oidc-app/src/ping-one/main.ts(1 hunks)e2e/oidc-app/vite.config.ts(1 hunks)package.json(1 hunks)scripts/bootstrap-https.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- contributing_docs/README.md
- e2e/docker-compose.yml
- .gitignore
- e2e/https-proxy/Dockerfile
- package.json
- e2e/https-proxy/docker-entrypoint.d/10-generate-config.sh
- e2e/oidc-app/vite.config.ts
- e2e/https-proxy/default.conf.template
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (2)
e2e/oidc-app/src/ping-one/main.ts (1)
17-17: Verify that the redirect URI is registered in the PingOne identity provider.The redirect URI has been updated from
http://localhost:8443/ping-one/tohttps://localhost:8443/ping-one. Two changes occurred: the protocol changed to HTTPS and the trailing slash was removed. OIDC providers perform exact matching on redirect URIs, so ensure the new URI (without the trailing slash) is registered in your PingOne application configuration. Otherwise, the OAuth flow will fail with a redirect URI mismatch error.e2e/oidc-app/src/ping-am/main.ts (1)
17-17: Verify that the redirect URI is registered in the PingAM OAuth client.The redirect URI has been updated from
http://localhost:8443/ping-am/tohttps://localhost:8443/ping-am. Both the protocol changed to HTTPS and the trailing slash was removed. Ensure the OAuth client configuration in PingAM reflects this exact URI (without the trailing slash) to avoid redirect URI mismatch errors during authentication.
| - Installs the mkcert root CA into the system trust store if it is not present | ||
| - Creates (or refreshes) `e2e/certs/proxy-cert.pem` and `e2e/certs/proxy-key.pem` with SANs for `localhost`, `127.0.0.1`, and `::1` | ||
|
|
||
| > The files can safely be committed to your local clone—they should **not** be committed to git. |
There was a problem hiding this comment.
Clarify the wording about certificate files.
The statement "The files can safely be committed to your local clone—they should not be committed to git" is confusing because a "local clone" is still a git repository. This creates a contradiction.
Apply this diff to clarify:
-> The files can safely be committed to your local clone—they should **not** be committed to git.
+> The files will exist in your local working directory but should **not** be committed to git (they are already in `.gitignore`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > The files can safely be committed to your local clone—they should **not** be committed to git. | |
| > The files will exist in your local working directory but should **not** be committed to git (they are already in `.gitignore`). |
🤖 Prompt for AI Agents
In contributing_docs/local-https.md around line 33, the sentence "The files can
safely be committed to your local clone—they should **not** be committed to git"
is contradictory and unclear; replace it with a clarified sentence that
explicitly states these certificate files should remain in your working
directory but must not be committed to the repository or pushed to remotes
(e.g., "You may keep these files locally in your working copy, but do not commit
or push them to the repository — add them to .gitignore"). Also update guidance
to recommend adding the specific cert filenames to .gitignore and, if present,
mention secure storage alternatives (e.g., system keychain or secret manager).
Sure. I just have concerns about relying on a domain controlled by a single person. This is only 2 commands 1 to setup and 1 to run the proxy. |
JIRA Ticket
N/A
Description
Implements a local HTTPS development environment using an nginx reverse proxy to support E2E testing of features that require a secure context, such as WebAuthn.
Key changes include:
httpsin its redirect URIs and corrected mismatches.pnpmscripts (setup:https,https-proxy:up) to streamline setup and usage.--host=0.0.0.0when running applications behind the proxy.This is my take on how we can setup an https setup for webauthn under localhost. All commands should be aliased under pnpm root scripts, I tested both the ping-am and ping-oidc routes for oidc app and i tested the davinci-app as well. This does require 2 terminals to be open but the documentation should cover everything needed. its 2 commands to setup, and run
Summary by CodeRabbit