Skip to content

Fix authorization, session races and deployment consistency - #154

Closed
shedowe19 wants to merge 5 commits into
developfrom
codex/fix-review-october-15
Closed

shedowe19 wants to merge 5 commits into
developfrom
codex/fix-review-october-15

Conversation

@shedowe19

@shedowe19 shedowe19 commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

What changes

The October 15 review found 18 defect groups on the merged develop baseline. This PR fixes all 18 and adds regressions for the affected authorization, session, configuration, database, shutdown and deployment paths.

  • Restrict shared-CA client-certificate issuance to current database administrators and require an explicitly verified SHA256 host pin before SSH authentication. Normalize legacy ACL metadata before redacting host responses.
  • Serialize cookie-changing authentication requests through complete HTTP responses, coordinate supported tabs with Web Locks, verify restored identities through /users/me, and reject stale synchronization events. Two real Chromium cases reproduce delayed HttpOnly refresh cookies before impersonation/logout and verify the next request's identity.
  • Coordinate ACL detach/activation/cleanup, Docker reloads and GitOps pruning through the Nginx queue with rollback. Persist failed Git-root activations for retry; reconcile successful dialog syncs without overwriting edited fields or allowing a pending save.
  • Drain analytics and relay work correctly, stop terminal connections before HTTP shutdown, prioritize never-checked monitors, preserve large analytics downloads with BIGINT, and reserve active domains atomically across host types on SQLite, PostgreSQL and MariaDB. GitOps graph writes now roll back transactionally on every supported engine.
  • Share a literal-only, atomic env parser between native installation and updates. Test image candidates before pushing and promote only validated architecture digests to public tags. Exclude backend tests from runtime payloads, make high/critical dependency audits blocking and reduce job permissions.
  • Align analytics host selection with analytics permissions, preserve locale/theme preferences when storage is blocked, refresh settings opening snapshots and correct bounded integration-test/fixture contracts.

No dependency or version bump. No merge, release, image publication or deployment is performed by this PR.

Validation

Final source commit: 6ceba0dfe3706ae72bc8fd90b6274feb324a9e0e. Results below must refer to that head.

  • Quality, browser and MariaDB: 2,232 backend tests passed (6 intentional skips), 974 frontend tests passed across 218 suites, no unhandled errors; backend/frontend types, locales and production build passed. Nine real Chromium tests and four real MariaDB migration/concurrency tests passed.
  • Compressed bundle budgets passed: JavaScript total 1,117,955 / 1,290,000 bytes, largest chunk 106,548 / 299,000 bytes, CSS 16,325 / 16,600 bytes.
  • Blocking dependency audits: 0 vulnerabilities across 522 backend and 628 frontend packages.
  • TScanner: 73 integration tests passed, 48 changed product files checked, no new findings/advisories/scanner failures. Infrastructure: 172 tests completed with 2 explicit CI environment skips. CodeQL, Dockerlint, spelling and JSON checks passed.
  • Native Docker validation: x86_64 and ARM64 builds, GeoIP double-boot checks, root/UID-1000 runtime smokes, native packages and LXC templates all passed. Publication/release jobs were correctly skipped for the PR.

Local backend types, full backend/frontend Biome checks, 55 schema regressions, focused runtime/data/authorization suites and 172 infrastructure tests (21 explicit local environment skips) also passed. All 292 relative links in the changed documentation resolve. Earlier failed intermediate-head runs are retained in Actions and are not counted as final success.

Upgrade notes and limits

  • Existing terminal targets require a manually verified OpenSSH SHA256 fingerprint before connecting. Missing or mismatched pins fail closed; there is no automatic trust-on-first-use.
  • Existing client certificates and the shared CA remain valid. Restricting future issuance does not revoke earlier certificates.
  • The domain migration reports existing active conflicts with domain and host identifiers before DDL; it never deletes hosts. MySQL/MariaDB requires TRIGGER privileges and transactional InnoDB tables. Equivalent raw Unicode/regex normalization across SQL engines is not claimed.
  • Web Locks provides cross-tab request serialization where supported. Without it, the guaranteed ordering covers one page; storage/BroadcastChannel synchronization remains best effort. Synchronization metadata contains no tokens or CSRF secrets.
  • Analytics BIGINT is deliberately retained on down migration to avoid truncating valid large-download records. Custom database providers present only in additional systemd drop-ins remain outside automatic recovery discovery.

Documentation

Updated the review and A01–A18 implementation matrix, affected module/build/deployment/migration pages, test guidance, PKI user guide, OpenAPI host-selection contract, index and open questions. Historical scan evidence and unrelated operational TODOs remain clearly identified.

@shedowe19
shedowe19 marked this pull request as ready for review October 15, 2026 08:32
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T08:46:42.492101Z 6ceba0d Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ceba0dfe3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

table.charset("utf8mb4");
table.collate("utf8mb4_bin");
}
table.string("domain_name", 255).notNullable();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve permitted long server-name patterns

When an existing redirection or dead host contains a regex server_name longer than 255 characters, this migration copies the value from its unrestricted JSON domain_names field into varchar(255). PostgreSQL rejects that insert, and MySQL either rejects or truncates it depending on SQL mode, so a valid existing configuration can block a production upgrade; subsequent writes encounter the same trigger failure. Use a length-independent reservation key or enforce and migrate a matching limit before creating this table.

AGENTS.md reference: AGENTS.md:L120-L122

Useful? React with 👍 / 👎.

Comment on lines +478 to +480
await proxyHostModel.query(trx).where("id", host.id).where("access_list_id", 0).patch({
access_list_id: row.id,
upload_relay_enabled: host.upload_relay_enabled,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve explicit concurrent ACL detaches during rollback

When Nginx activation fails while another proxy-host update has already committed access_list_id = 0, this rollback mistakes that valid new value for the deletion's own detach and restores both the old ACL and the old upload-relay flag. Proxy-host updates commit their database transaction before waiting for the serialized Nginx configuration queue, so the concurrent update can subsequently return success even though its database changes were overwritten here. Track which row version was detached, or use a deletion-specific marker, rather than treating every zero assignment as rollback-owned.

AGENTS.md reference: AGENTS.md:L128-L128

Useful? React with 👍 / 👎.

@shedowe19 shedowe19 closed this Oct 17, 2026
@shedowe19
shedowe19 deleted the codex/fix-review-october-15 branch October 17, 2026 20:41
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.

1 participant