Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates CI builds and implements comprehensive template error handling for the Traefik backend. The changes include switching from pull_request to pull_request_target in GitHub Actions, adding strict template variable validation, and improving TypeScript type safety in middleware.
Changes:
- Enhanced CI security controls with
pull_request_targetand author association checks - Added comprehensive test coverage for Traefik template rendering and validation
- Implemented strict error handling for unknown template variables (now throws errors instead of warnings)
- Improved TypeScript type safety in Express middleware functions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ci.yml | Updated trigger from pull_request to pull_request_target with author checks and added persist-credentials: false |
| src/backends/traefik/traefik.ts | Added error handling to makeAppConfig and addProxiedApp, returning null on template failures |
| src/backends/traefik/templateParser.ts | Changed unknown variable handling from warnings to errors, strict validation now throws |
| src/api/middleware/logging.ts | Improved type safety for res.end override using variadic args |
| src/api/middleware/errorHandler.ts | Refactored error property extraction for better type safety |
| test/unit/traefik/user-data-substitution.test.ts | New comprehensive test suite for userData template substitution (485 lines) |
| test/unit/traefik/template-validation.test.ts | New test suite for template validation logic (435 lines) |
| test/unit/traefik/template-error-handling.test.ts | New test suite for template error scenarios (314 lines) |
| test/unit/providers/docker-provider.test.ts | Added resolveHostPath mock to prevent test failures |
Comments suppressed due to low confidence (1)
.github/workflows/ci.yml:90
- The build-and-push job has a logical issue: it will attempt to run for PRs from authorized users (OWNER/MEMBER/COLLABORATOR), but the condition github.ref == 'refs/heads/main' will be false for PRs since pull_request_target events have github.ref pointing to the base branch ref (e.g., 'refs/heads/main'), not the PR ref. However, this condition combined with the author check may still allow unauthorized builds. More importantly, if a PR from an authorized user modifies this workflow file itself, the changes would run with secrets access, creating a potential security issue. Consider separating PR testing from production builds entirely.
build-and-push:
name: Build & Push Docker Image
if: (github.event_name == 'push' || github.event.pull_request.author_association == 'OWNER' || github.event.pull_request.author_association == 'MEMBER' || github.event.pull_request.author_association == 'COLLABORATOR') && (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/'))
needs: test
runs-on: ubuntu-latest
permissions:
contents: read
packages: write
steps:
- name: Harden the runner (Audit all outbound calls)
uses: step-security/harden-runner@20cf305ff2072d973412fa9b1e3a4f227bda3c76 # v2.14.0
with:
egress-policy: audit
- name: Checkout repository
uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1
- name: Log in to GHCR
uses: docker/login-action@5e57cd118135c172c3672efd75eb46360885c0ef # v3.6.0
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
| # TODO: duplicate build step; consider removing from here and only keeping in build-and-push | ||
| # once we figure a better way to prevent build errors from reaching build-and-push job. | ||
| - name: Build | ||
| run: npm run build | ||
|
|
There was a problem hiding this comment.
The comment in the build step (lines 60-61) indicates this is a duplicate build step that should potentially be removed. The build command runs in both the 'test' job and implicitly in the 'build-and-push' job via Docker. Consider whether this duplication serves a specific purpose or if it can be streamlined to avoid wasting CI resources and time.
| # TODO: duplicate build step; consider removing from here and only keeping in build-and-push | |
| # once we figure a better way to prevent build errors from reaching build-and-push job. | |
| - name: Build | |
| run: npm run build |
| const statusCode = errorObj?.statusCode ?? errorObj?.status ?? 500; | ||
| const errorCode = errorObj?.code; | ||
|
|
||
| // Log the full error internally with correlation ID (safe location) | ||
| log.error({ | ||
| message: 'API error', | ||
| data: { | ||
| errorId, | ||
| type: err?.constructor?.name || 'Unknown', | ||
| message: err?.message || String(err) | ||
| type: errorType, | ||
| message: errorMessage | ||
| } | ||
| }); | ||
|
|
||
| // Return safe error response to client with error ID for tracing | ||
| const statusCode = err?.statusCode || err?.status || 500; | ||
| const errorResponse: ErrorResponse = { | ||
| error: 'An error occurred processing your request', | ||
| errorId | ||
| }; | ||
|
|
||
| // Add code if it's a validation error or known error type | ||
| if (err?.code) { | ||
| errorResponse.code = err.code; | ||
| if (errorCode) { | ||
| errorResponse.code = errorCode; | ||
| } | ||
|
|
||
| res.status(statusCode).json(errorResponse); |
There was a problem hiding this comment.
The statusCode extracted from the error object is not validated before being used in res.status(). A malicious or malformed error object could provide an invalid HTTP status code (e.g., negative number, very large number, or non-integer), which could cause Express to throw an error or behave unexpectedly. Consider validating that statusCode is a valid HTTP status code (typically 100-599) before using it, defaulting to 500 if invalid.
| userData: { missing_var: 'value' }, | ||
| }; | ||
|
|
||
| const badEntry: HostEntry = { |
There was a problem hiding this comment.
Unused variable badEntry.
No description provided.