Skip to content

fix(auth): validate the whole whitelist entry, including its CIDR prefix - #59

Merged
mrrobot47 merged 5 commits into
EasyEngine:developfrom
mrrobot47:fix/validate-whitelist-cidr
Sep 29, 2026
Merged

mrrobot47 merged 5 commits into
EasyEngine:developfrom
mrrobot47:fix/validate-whitelist-cidr

Conversation

@mrrobot47

@mrrobot47 mrrobot47 commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Problem

  • Only the part of a whitelist entry before / was validated, so entries like 192.0.2.1/abc, 192.0.2.1/99 or 2001:db8::/200 were stored and written to the _acl file. nginx rejects them, so nginx -t fails and every later proxy reload is skipped, for all sites, until the entry is removed. With ee auth create global --ip=... this breaks default_acl for every site.
  • A repeated IP in one ee auth create --ip list (e.g. --ip=192.0.2.7,192.0.2.7) crashed with an uncaught database error on the unique (site_url, ip) constraint, leaving the first row stored and the _acl file never written.

Fix

  • Validate the full entry: an IPv4 or IPv6 address with an optional CIDR prefix of 0-32 or 0-128 (digits only, no sign or leading zero, at most one /). 255.255.255.255 and an IPv6 :: that stands for no group (e.g. 1:2:3:4:5:6:7::) are rejected too, since nginx refuses them. The error now names the bad entry.
  • De-duplicate the --ip list, so a repeated IP is stored once.
  • Invalid entries already stored by older versions are skipped when the _acl files are generated, with a warning that names the entry and the command that removes it. Stored entries that nginx accepts but that are no longer valid input (e.g. 10.0.0.0/08, or a trailing CR) are written in their normalized form. ee auth delete <site> --ip=<entry> still accepts a stored entry, so it can be cleaned up.
  • A new migration removes invalid allow lines from existing _acl files, and normalizes the ones nginx accepts, in place (keeping the rest of the file and its mode), then reloads the proxy once. It runs before the image migration, so the proxy isn't recreated on a file nginx rejects, and a file that can't be read or rewritten stops the upgrade and leaves the migration pending for the next run. Other allow arguments nginx accepts (all, unix:) are kept. It writes no new _acl file, so it leaves the _wildcard.* staging from fix(migration): don't apply wildcard auth files on the old nginx-proxy template #58 alone, and it keeps the DB rows.

Behaviour change

  • The --ip list is now trimmed and split on any whitespace or comma, so tab-, CRLF- or space-separated lists are accepted. Before, only spaces, newlines and commas separated entries, and leading or trailing whitespace produced an empty entry that failed validation. Empty entries between commas (192.0.2.1,) are still rejected.
  • Because --ip now splits on all whitespace, an old stored entry that contains whitespace can't be targeted with --ip=<entry> anymore; the warning suggests ee auth delete <site> --ip and re-adding the valid entries instead.

Dependencies

None. Based on the current develop, which includes #58.

Only the address before `/` was checked, so entries like `192.0.2.1/99`, `/99` or `2001:db8::/200` were stored and written to the `_acl` files. nginx rejects such a file, so every later proxy reload was skipped, and a proxy restart failed. An entry must now be an IPv4 or IPv6 address nginx accepts, optionally with a prefix of 0-32 or 0-128 (digits only), and the error names the rejected entry.

Invalid entries stored by older versions are skipped with a warning when the `_acl` files are written, and `ee auth delete --ip` still accepts them so they can be removed.

The list is split on any whitespace and deduplicated, so a repeated IP no longer hits the UNIQUE(site_url, ip) constraint on create (AUTH-14).
Hosts upgraded from a version that stored unchecked entries may have an `_acl` file nginx rejects, which makes the proxy fail when the upgrade recreates it. The migration removes only those `allow` lines from the existing files, in place, so no new file is written while an older nginx-proxy runs, and warns with the removed entries. The rows stay in the database.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The migration loses file modes and suppresses rewrite failures, allowing invalid ACLs to remain permanently.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Validates complete whitelist CIDR entries, deduplicates inputs, and cleans invalid legacy ACL entries.

Changes:

  • Adds strict IPv4/IPv6 CIDR validation.
  • Skips invalid stored entries with cleanup guidance.
  • Adds an ACL cleanup migration.
File Description
src/​auth-utils.php Adds validation and safe ACL generation.
src/​Auth_Command.php Validates and deduplicates CLI input.
migrations/​container/​20260927120000_auth-command_drop_invalid_whitelist_entries_from_acl_files.php Removes invalid legacy ACL lines.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…entries

- An IPv6 address of seven groups and a trailing `::` (e.g. `1:2:3:4:5:6:7::`) passed `filter_var`, but nginx's `allow` rejects it, so it still broke `nginx -t` for every site. It is now refused on input, skipped when stored, and removed by the migration.
- Stored entries that nginx accepts, with a prefix leading zero (`10.0.0.0/08`) or surrounding whitespace such as a CR, are normalized instead of dropped, so the upgrade no longer revokes a working whitelist.
- The migration stops the upgrade when an `_acl` file can't be rewritten, instead of recording itself and letting the image migration recreate the proxy on the invalid file.
- The migration reads each file once, warns once per set of removed entries, and names the delete command of each stored entry, global ones included, with the same warning as the `_acl` generation.
…t removals after a failed write

- Stored entries are trimmed of spaces, tabs, CR and LF only: nginx doesn't separate on a vertical tab or NUL, so an entry with one never took effect and must stay invalid.

- The migration's removal warnings are printed even when a later file can't be rewritten, since a retry no longer sees the files already fixed.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The migration can silently skip unreadable ACL files and remove nginx-supported non-IP allow directives.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Fix grammar and terminology in validation message

src/​Auth_Command.php:132

The user-facing sentence is ungrammatical; use “check that … does not contain” and “invalid” rather than “wrong” for a clear validation error.

…eadable ACL file

`allow all;` and `allow unix:;` are valid nginx, so a hand-edited file keeps them. A file that can't be read may still hold an invalid entry, so the migration now fails after fixing the others and stays pending, like a failed write.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

The migration misses invalid allow directives using valid nginx indentation or trailing comments.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Handle formatted nginx allow directives during cleanup

migrations/​container/​20260927120000_auth-command_drop_invalid_whitelist_entries_from_acl_files.php:45

This only inspects lines beginning with exactly allow and ending immediately after ;, so valid nginx formatting such as an indented allow 192.0.2.1/99; or allow 192.0.2.1/99; # stale bypasses cleanup. The invalid directive then remains for the reload while the migration can still be recorded as complete. Parse optional indentation/spacing and trailing comments, validate the captured argument, and preserve the surrounding formatting when normalizing it.

@mrrobot47

Copy link
Copy Markdown
Member Author

On the two overview-only items (no threads):

  • Indented or commented allow lines: not changed. EE writes these files itself, one allow <entry>; per line, and that format (plus editor CRLF) is what the migration fixes. An indented or commented invalid line can only come from a hand edit, which nginx already rejected before the upgrade, and parsing free-form directives would mean rewriting admin-written config.
  • The validation message wording: kept on purpose. Please check your list do not have any empty or wrong IP addresses. is the existing prefix that current scripts and tests match; the new part after it names the entry.

@mrrobot47
mrrobot47 merged commit af05c40 into EasyEngine:develop Sep 29, 2026
@mrrobot47
mrrobot47 deleted the fix/validate-whitelist-cidr branch September 30, 2026 07:57
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.

2 participants