Repository navigation
Ccr 463348c6 ulw3ws - #159
Closed
sabrydawood wants to merge 22 commits into
Closed
sabrydawood wants to merge 22 commits into
sabrydawood wants to merge 22 commits into
Conversation
bullmq pins ioredis 5.10.1 while the root depends on 5.11.1, which made tsc fail on the Queue/Worker connection types and broke build, CI and start. An npm override forces a single ioredis copy.
… (SEC-C1) - ALLOW_REGISTRATION defaults to false. While the Users table is empty the first registration is accepted and becomes Admin; afterwards /register returns 403 unless registration is explicitly enabled. - Self-registered accounts are always Viewer; Role is never read from the request. The count check and insert share a SERIALIZABLE transaction so concurrent first registrations cannot create two admins. - Every /api/auth body is validated with a Joi schema (stripUnknown). - New GET /api/auth/registration-status for the login page. - Boot no longer fails on a fresh install without DEFAULT_ADMIN_*; it logs that the first registration creates the administrator. - Replace the test that asserted self-assigned roles with negative tests.
- io.use() verifies the access_token cookie (or auth.token) and reloads the user, rejecting anonymous, forged, disabled and suspended sessions. - join:project / join:deployment are authorized through the new AccessControlService, the same rules the HTTP layer uses; join:user only accepts the caller's own id. Rooms are decided by the server: the user room, a staff room for Admin/Manager and the accessible project rooms for Developer/Viewer. - Remove global IO.emit broadcasts. Deployment events go to the staff, project and deployment rooms only (once per socket) and carry an allow-listed DTO, never a Sequelize instance or project secrets. - Queue health failure reasons are sent to staff only. - Integration tests cover anonymous/forged/disabled rejection, cross-project joins, foreign user rooms and DTO contents.
…C-C3) - A correct password for a 2FA account now issues a random single-use token (5 minutes, stored hashed in MfaChallenges) in an httpOnly mfa_pending cookie scoped to /api/auth. The login response no longer contains the user id. - /verify-2fa derives the user from that challenge only; a body UserId is stripped. The challenge is claimed atomically before the code is checked, so it cannot be replayed or raced, and it is burned after 5 wrong codes. - Per-account lockout: 5 consecutive failures lock the second factor for 15 minutes regardless of IP or challenge. - IsActive, AccountStatus and TwoFactorEnabled are re-checked at the second step. - Backup codes are 80-bit (XXXX-XXXX-XXXX-XXXX), stored as salted SHA-256 hashes. Pre-3.0.1 codes (about 20 bits) are no longer accepted; the 2FA status reports legacyBackupCodesInvalidated so users regenerate. - Migration 022 adds MfaChallenges and TwoFactorAuth.FailedAttempts / LockedUntil (idempotent, with down()).
…ion (ENG-C3)
Context values such as the commit message were substituted unescaped
into a string that was then executed as JavaScript inside the server
process. RunIf is now evaluated by ConditionEvaluator, a tokenizer plus
recursive-descent parser that supports the documented syntax only:
==, ===, !=, !==, contains, &&, ||, !, parentheses, string / number /
boolean literals, hasVar('X') and variables written as Name, $NAME or
{{Name}}. Values are always data and no global is reachable.
Anything that cannot be parsed or references an unknown variable fails
closed: the step is skipped and a warning is written to the deployment
log. Tests cover the syntax, fail-closed inputs and injection payloads
(quotes, template strings, newlines).
…G-C2)
- New Utils/SafeExec: RunFile wraps execFile with argument arrays and
shell:false. Every git / rsync / chown / ssh-keygen call in
DeploymentService, SshKeyManager, SshKeyGenerator and AutoRecovery now
uses it, with -- before repository URLs, refs and paths.
- Strict validation, enforced in CreateDeployment, at clone time and when
a project is created or updated (400 on failure):
- commit: ^[0-9a-f]{7,40}$
- branch: git check-ref-format --branch, plus a conservative charset
because the branch is also substituted into pipeline commands
- RepoUrl: URL parser with an https / ssh / git@host:path allowlist
- RsyncOptions: allowlisted flags only (no -e, --rsh, --rsync-path)
- filesystem paths may not start with - or contain control characters
- Repository credentials are stripped from clone commands written to
deployment logs.
- SecurityMiddleware's regex filter stays as defence in depth; the new
tests run without it and cover shell metacharacters, option injection
and newline payloads.
…nly (SEC-H4, SEC-H3)
Access (SEC-H4):
- ProjectAccess and DeploymentAccess middleware now delegate to
AccessControlService (membership, or legacy CreatedBy), the same rules
as the Socket.IO rooms. Viewers only see projects they are members of,
as documented in the RBAC matrix; Developer deployment access is based
on membership instead of CreatedBy alone.
- Added checks to: deployment log download, manual deploy, project
deployment listing, project queue status, per-project statistics,
GET /projects/name/:name, the public SSH key, and SSH key
generate/regenerate/delete/toggle (previously role-only).
- The test-webhook endpoints require authentication and project access.
- Managers are no longer refused on SSH key management, project deploy
and cancel, matching the RBAC matrix.
Secrets (SEC-H3):
- Project responses go through ToProjectDto: WebhookSecret and the
encrypted SSH key are removed, RepoUrl credentials are stripped, and
Config secrets (legacy variables except the platform build keys,
notification webhook URLs, SMTP passwords, bot tokens) are replaced
with a mask. A Secrets map reports { HasValue } per write-only field.
- Deployment responses embed the same project DTO.
- Saving a project with the mask (or the credential-free RepoUrl) keeps
the stored value, so clients never need the secret to edit a project.
- Env var listings add HasValue; user management responses drop
PasswordHash.
- Route-table tests for Developer and Viewer outsiders, member and
Manager cases, and payload scans for leaked secrets.
- New ConfigValidator. With NODE_ENV=production the server exits at boot when JWT_SECRET, JWT_REFRESH_SECRET or ENCRYPTION_KEY is missing, shorter than 32 characters or a known default / placeholder, when the two JWT secrets are equal, or when DB_PASSWORD / DEFAULT_ADMIN_PASSWORD is a shipped placeholder. Outside production the same findings are logged as warnings. Messages never include secret values. - Real environment variables now take precedence over the .env file (including DB_AUTO_MIGRATE), so NODE_ENV=production from systemd, Docker or PM2 is honoured. - Unit tests for every rule and for the precedence order.
…re compiling (OPS-C1, OPS-H1) - npm audit fix (no --force), nodemailer 10.2.1, and a qs override for the express copy nested under @bull-board/express. npm audit --omit=dev now reports 0 vulnerabilities; only dev-only deepmerge (via jest) remains, with no non-breaking fix. - engines.node >=20 (nodemailer 10 requirement; Node 18 is EOL). CI tests Node 20 and 22, and the security workflow fails on high or critical production advisories instead of continue-on-error. - npm run build compiles into a staging directory and swaps it into dist/ only after tsc succeeds; the prebuild rimraf is gone, so a failed build leaves the running code intact. - test:unit / test:integration use Jest 30's --testPathPatterns.
The script was placed under scripts/build/, which the build/ ignore rule excluded, so the previous commit referenced a file that was not in the repository. Move it to scripts/safe-build.js.
…, OPS-H2)
OPS-C2:
- Migration 000_initial_schema creates the full model schema when none of
Users / Projects / Deployments exist, so migration 001 no longer fails
with 'No description found for Projects' on a new install. On any
existing database it is a no-op; every later migration was already
idempotent and skips on the baseline schema.
- InitializeAssociations is idempotent so the baseline can run from the
boot path and the migrate CLI.
- New CI job boots the real server in production mode on an empty
MariaDB, checks /health and the first-admin setup state, then restarts
it to prove the migrations are no-ops the second time.
OPS-H2:
- CSRF exemption is now route-based and POST-only for
/api/webhooks/{github,gitlab,bitbucket}[/:name] and the v2.1 legacy
/webhook/... mount, which used to return 403. Prefixes such as
/api/webhooks/github-evil or deeper paths are no longer exempt.
The v3.0 UI served from public/ refuses to submit a 2FA code unless the login challenge contains a user id, which broke 2FA logins through it. The id is returned again for compatibility only: /verify-2fa still derives the user from the mfa_pending cookie and ignores any body UserId, and the tests keep asserting that.
npm run rotate:webhook-secrets -- --all | --only <id|name> [--out FILE] [--dry-run]. New secrets are written only to a 0600 file that is never overwritten; the terminal and logs list project names, not secrets. Each rotation is recorded in the audit log as webhook_secret_rotated. Also runnable from dist/ on hosts without dev dependencies.
- docs/guides/security-hotfix-3.0.1.md: what changed, breaking changes, upgrade steps, mandatory rotation of webhook secrets, JWT secrets and SSH deploy keys, and known gaps (UI-H0). - CHANGELOG entry for 3.0.1 by finding category. - Version 3.0.1.
The job sets NODE_ENV=production for the server, which made npm ci skip devDependencies, so the TypeScript build had no compiler.
…e config keys Move the shape check of the mfa_pending cookie into CompleteTwoFactorLogin, next to the hashed lookup that decides it, so the controller holds no security condition of its own. When merging masked secrets, drop __proto__, constructor and prototype keys and only restore values the stored config owns. Exclude __tests__ from CodeQL.
The rotation script changed the database first and wrote the secrets file afterwards, so a failed write left live secrets nobody had. It now plans the new secrets, writes and fsyncs the 0600 file, then applies every project in one transaction, and removes the file if that transaction fails.
Rooms were resolved once at connection time, so a socket kept receiving a project's events after the user lost access. Every 30 seconds, and right after a role change, a deactivation or a removed project membership, each socket now re-verifies its token (expiry and current JWT secret), reloads the user, leaves rooms it may no longer see and joins newly granted ones. A disabled account or an invalid token disconnects the socket. The 2FA service also stops branching on the cookie value: a missing or malformed token simply matches no stored challenge.
…tted A transaction can commit even when its acknowledgement is lost. On an apply error the script now reads back the stored secrets: if all match, it reports success; if none match, it removes the file; otherwise it keeps the file and says how to check.
Builds the merged variable map with Object.fromEntries from the filtered entries, so no property name taken from the request is assigned on an existing object.
Removing a project's creator from its members now removes their access. Migration 023 backfills an owner membership for every existing creator so no one loses access on upgrade. Signed-off-by: Sabry Dawood <kazouya25@gmail.com>
- Project delete requires an owner membership; drop the CreatedBy fallback. - Redact URL credentials from RunFile errors, cmd, stdout and stderr. - Reconcile a deleted user's sockets right away. - GET /api/users returns user DTOs (no TwoFactorSecret). - On an expired token, close only the socket transport so the client reconnects with its refreshed token instead of staying disconnected. Signed-off-by: Sabry Dawood <kazouya25@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
documentation
github-actions
Bot
added
security
|
Caution Review failedThe pull request is closed. Walkthrough
Merge Risk:
 |
This was referenced Oct 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
📋 Description
What does this PR do?
Why is this change necessary?
🔗 Related Issues
Fixes #(issue number)
Closes #(issue number)
Related to #(issue number)
🏷️ Type of Change
🧪 How Has This Been Tested?
Test Environment:
Test Cases:
Test 1:
Test 2:
Manual Testing:
Automated Testing:
📸 Screenshots (if applicable)
Before:
After:
📝 Changes Made
Added
POST /api/...UtilityName()Changed
src/Services/ServiceName.tsRemoved
OldMethod()OldFile.tsFixed
🔧 Configuration Changes
New Environment Variables:
Database Migrations:
-- Add migration script if needed ALTER TABLE...📚 Documentation
Does this PR introduce breaking changes?
If yes, describe:
Example:
🚀 Deployment Notes
Pre-deployment:
Post-deployment:
Rollback Plan:
✅ Pre-submission Checklist
Code Quality
Testing
Documentation
Linting & Formatting
npm run lint)npm run format)npm run build)Security
Dependencies
👀 Reviewers Needed
@Maintainer1
@maintainer2
Specific review focus:
💬 Additional Notes
📊 Code Coverage
Before:
After:
🔄 Migration Guide (for breaking changes)
Update usage: