Repository navigation
chore: Repository hygiene, real CI, installers, security cleanup - #46
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Opselon
force-pushed
the
chore/repo-hygiene
branch
from
October 6, 2026 09:20
ee47fac to
3d12592
Compare
Owner
Author
|
Added a large end-user CLI contract matrix directly to this PR. The branch now contains 121 new test files including the shared CLI harness, with ~360 new end-user tests covering command discoverability, invalid input, Secret Vault failures, config redaction, migration/backup/restore boundaries, hostile/Unicode/long input, security fail-closed behavior, and recovery after failed operations. Changes are committed on |
Opselon
force-pushed
the
chore/repo-hygiene
branch
8 times, most recently
from
October 6, 2026 12:43
f6be4f7 to
c3dc7cc
Compare
- Program.cs: fix ASP0013 (builder.Configuration instead of ConfigureAppConfiguration), CS1998 (add await Task.CompletedTask), CS0162 (remove unreachable code after break) - CloudflareRadarService: make all task-result tuple types nullable (CS8602/CS8625) - Cli: apply TypeRegistrar + async command warning fixes Build: 0 Warning(s), 0 Error(s); Tests: 142/142 passed
…date backups - UnknownFlagFinder + CommandRegistry: reject mistyped/unknown flags for every command instead of silently ignoring them (Spectre swallows them by default) - CliConfirm: confirmation prompts work under piped stdin and re-prompt on an unrecognised answer; no answer at all fails closed instead of exit 255 - BackupIntegrity: refuse to restore a truncated/tampered backup over the live vault (SQLite magic + PRAGMA integrity_check + Secrets schema check) - E2E guard on FOREXBOT_VAULT_DIRECTORY so install/start/stop never run the real docker/dotnet stack from the test harness - install: validate mode first, so a bad mode still reports 'docker'/'native' E2E CLI surface: 367/367 passed; build: 0 warnings, 0 errors
- e2e-cli-platform-matrix only built the E2E project, so 'dotnet run --project Cli --no-build' failed with 'An error occurred trying to start process' (no built CLI output). Build the whole solution first. - WebAPI crashed on container startup: SecretKeyStore.LoadOrCreateKey tried to write the key to LocalApplicationData, which resolves to the read-only image content root. DefaultVaultDirectory now probes the primary path and falls back to the writable 'data' directory the Dockerfile already provisions. - Restore .github/workflows/label.yml (labeler needs it on the PR base).
…ws CI - RunAndCapture / RunExecutableAndCapture waited forever on external diagnostics; 'docker info' on a Windows runner with no reachable daemon hung past the 30s E2E timeout. Now bounded to 10s and killed on expiry. - SecretClipboard only tried powershell.exe; CI images ship pwsh.exe and may not alias the legacy binary, so --copy failed closed with exit 1. Try pwsh first, fall back to powershell.
…d errors On Windows the vault key is DPAPI-protected per user, so a raw 32-byte file (as written by the wrong-key E2E test, or by manual tampering) made ProtectedData.Unprotect throw from the DI factory and terminated the process with exit code -1. Program.Main now loads the key up front and reports a clear 'could not decrypt' message with exit 1. BackupCommand let Sqlite/File failures escape the same way; on Windows runners Defender can briefly lock the temp destination. BackupTo now wraps IOException with an actionable message and the command catches IO/SQLite/unauthorized failures and returns 1 instead of crashing.
The interactive E2E backup step failed on Windows with only 'Actual: 1' because Assert.True carried no message. Attach the CLI's own output so the failure names the real cause.
…locks CI on windows-latest failed with 'The process cannot access the file because it is being used by another process' when publishing the staged backup. The SQLite connection is closed first, but Windows (and its antivirus/indexing services) can keep the freshly released file open for a moment; File.Move does not tolerate that. Retry the move briefly so a transient lock is not reported as a permanent backup failure.
File.Move failed deterministically on windows-latest CI with 'The process cannot access the file because it is being used by another process' at FileSystem.MoveFile. The destination is a freshly created file in the test's temp directory, so Windows Defender's real-time scan of the new file is what holds it; retrying a scan lock never resolves it. File.Copy with overwrite reads the same staged bytes and lets the source be deleted by the finally block, so the publish cannot be blocked by a lock on the destination.
…cess The Docker-based end-user suites (EndUserE2ETests, DockerEndUserSmokeE2ETests, CrossDatabaseEndUserE2ETests) each run 'docker compose up --build' against the same Dockerfile, and xUnit runs classes in parallel by default. Concurrent builds race on shared build cache and daemon state, and one test's 'down --remove-orphans' can reap another's containers — which is what produced 'No container found for service app' on the ubuntu runner. Add an assembly-level CollectionBehavior with DisableTestParallelization so the Docker suites run one at a time while the many CLI collections still parallelize. Also assert the compose up exit code in the install test: it was swallowed, so a failed build surfaced later as a misleading missing-container error.
…redirect /api/config/test is Admin-authorized, so the cross-database test's anonymous POST was redirected to the login page and the HTML body blew up the JSON parse. Log in with the app's bootstrap admin password first, then probe. The install test compared the redirect Location to the exact string '/login.html', but ASP.NET Core can emit an absolute URI with a ReturnUrl query; accept any location that lands on /login.html.
The Production appsettings ship a static Admin:Password, so EnsureAdminCredentials returns early and never writes the bootstrap file the tests expect. Set Admin__Password to empty in the e2e compose files so the app exercises the real first-run bootstrap (one-time credential in a 0600 file), and let the crossdb probe fall back to the configured password if no bootstrap file is present.
PUT /api/secrets/\{key\} rejected a category sent as its string name ("Api")
with a 400 validation error: the default System.Text.Json binder only accepts
enum numbers. Add JsonStringEnumConverter so clients can send either form.
…/app/data Cookie auth tickets are self-contained, so SignOutAsync only cleared the caller's own cookie; every cookie issued before a logout stayed valid until it expired. Add a server-side security stamp (AuthSecurityStamp): it is embedded as a claim at login, rotated on logout, and re-checked in OnValidatePrincipal, so a logout ends every prior session. Separately, a host bind mount over /app/data inherits the host's owner and the app's non-root user could not create the nested vault directory, crashing startup with UnauthorizedAccessException. Add entrypoint.sh: it chowns /app/data as root, then execs the app as appuser via runuser, keeping signal handling intact.
…ever invalidated old cookies The first-run admin credential is written to admin-password.txt with Encoding.UTF8, which emits a UTF-8 BOM. Installers and shells read that file raw, so the BOM became part of the password and every login with the displayed credential failed with 401. Write it with a BOM-less UTF-8 encoder and strip a BOM on read. The login also resolved Admin:Password against a configuration source registered after the wizard, whose snapshot of the default came from a stale appsettings value, so the generated credential could be discarded at runtime. appsettings.Production no longer ships a static admin password, and login falls back to the encrypted vault, which is the authoritative store. SignOutAsync only clears the caller's own cookie; tickets are self-contained, so every cookie minted before a logout stayed valid. Add a server-side security stamp (AuthSecurityStamp): embedded as a claim at login, rotated on logout, re-checked in OnValidatePrincipal, which ends all prior sessions. A host bind mount over /app/data inherits the host owner, so the app's non-root user could not create the nested vault directory and startup died with UnauthorizedAccessException. entrypoint.sh chowns /app/data as root and execs the app as appuser. The post-logout assertion used a client that followed redirects, so it saw 200OK instead of the redirect it was checking for; probe with a non-following client that shares the cookie container.
Opselon
force-pushed
the
chore/repo-hygiene
branch
from
October 7, 2026 05:09
d3e86a1 to
6337c4f
Compare
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
2 participants
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.
Summary
Audit-driven cleanup to make the repo production-grade. Base:
master(the real default branch).Security
local_forex_bot.db(283KB user data),easysetup_config.db(x2), DataProtection key (keys/, unencrypted masterKey — treat as compromised, rotate), rootProgram.csduplicate, duplicateReadMe.md, emptypackage.json/lock, orphansrc/file.gitignore:*.db,keys/,.env*, bin/obj, TestResultsCI (real, not fake)
ci.yml: build + all tests + Docker image build (withload: true— the old config never loaded the image, hence the smoke failure) + real smoke: actual Postgres+Redis containers, API connected to them,/healthzmust return 200; plus SQLite variantlabelworkflow by adding the missing.github/labeler.ymlReal bug fixed
Tests.Applicationtargetednet10.0while the solution/global.json isnet9.0— it could never build. Now net9.0.Tests
DbProviderServiceprovider-resolution tests (aliases, case-insensitivity, unsupported fallback) — 14 passingInstallers
install.sh/install.ps1one-command installers, adapted to master (compose service names,/healthz, Easy Setup wizard wording)GitHub structure
.env.exampleNotes
keys/should also be considered compromised (it encrypted settings); a new one is generated on deploy.