Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

Fix empty-string date fields silently becoming "now" + document property-level RBAC#1294

Merged
rjzondervan merged 8 commits into
developmentfrom
feature/1292/fix-empty-string-date-conversion
Apr 21, 2026
Merged

Fix empty-string date fields silently becoming "now" + document property-level RBAC#1294
rjzondervan merged 8 commits into
developmentfrom
feature/1292/fix-empty-string-date-conversion

Conversation

@rjzondervan

@rjzondervan rjzondervan commented Apr 21, 2026

Copy link
Copy Markdown
Member

Summary

Two related changes, both driven by work on issue #1292:

  1. Bug fix — Empty-string date / date-time fields no longer silently round-trip as the current date/time. On every user-supplied datetime path (object write, read/render, bulk import, metadata, search, ObjectService::normalizeDateValues) the raw string is now routed through a new central DateTimeNormalizer. null, "", and whitespace-only values normalise to null instead of hitting PHP's new DateTime('') footgun (which returns "now").
  2. Documentation — Property-level RBAC, including the conditional-rule / operator / dynamic-variable system ($organisation, $userId, $now, $lte, etc.) is now documented as a first-class feature. Adds a new feature page, updates the existing access-control.md, and removes "Field-Level Authorization" and "Time-Based Permissions" from the Future Enhancements list since both are implemented.

Closes #1292.

The bug in one sentence

(new DateTime(''))->format('Y-m-d') returns today's date string rather than failing or returning null. Multiple code paths fed user-supplied datetime strings into this pattern unguarded, so a form that cleared a date field and POSTed "publishedAt": \"\" silently populated the object with the current timestamp — the exact opposite of what the user intended, and indistinguishable on read from a legitimate value.

What changed

New: DateTimeNormalizer service

lib/Service/DateTimeNormalizer.php — single canonical entry point for user-datetime conversion. All new code MUST go through this class (documented in the class docblock and referenced in the OpenSpec change).

Contract:

Method Returns
normalize(mixed $value): ?DateTimeImmutable DateTimeImmutable for valid input; null for null / empty / whitespace / unparseable / wrong type. Parse failures log at debug level.
formatForDatabase(mixed $value): ?string Y-m-d H:i:s or null.
formatForIso8601(mixed $value): ?string ISO 8601 with offset or null.

Injected via constructor (no service-locator / \OC::\$server lookups).

Full unit test coverage in tests/Unit/Service/DateTimeNormalizerTest.php: null, empty, whitespace (" ", "\t", "\n"), ISO 8601 with offset, Zulu, DB format, date-only, passthrough of existing DateTime/DateTimeImmutable, garbled strings, numeric/array/object input.

Migrated call sites

All five primary sites where user-supplied datetime strings used to hit new DateTime(\$value) directly:

Site What it does Fix
lib/Service/ObjectService.php::normalizeDateValues Validates/coerces dates on object write Delegates to DateTimeNormalizer; trim → null short-circuit; accepts already-valid Y-m-d dates as-is
lib/Db/MagicMapper/MagicStatisticsHandler.php (date and date-time branches) Formats values on object read / render Delegates to normalizer, formats to Y-m-d or ISO 8601
lib/Db/MagicMapper/MagicBulkHandler.php::formatDateTimeForDatabase Bulk import Delegates to formatForDatabase
lib/Db/MagicMapper.php metadata handling Persists created/updated/expires metadata Routes through normalizer; preserves existing "default to now when key is absent" behaviour for created/updated
lib/Db/ObjectHandlers/MariaDbSearchHandler.php::normalizeDateValue Search predicates on date columns Returns null on empty/invalid input (skips the predicate) rather than matching "today"

Remaining new DateTime(\$var) call sites were audited and classified as either internal/trusted values (lock expiry, cached schema/org metadata, already-validated DB columns) or out-of-scope controller params that 400/404 on garbage rather than silently coercing. Full inventory in openspec/changes/fix-empty-string-date-conversion/tasks.md §5.4.

Tests

  • tests/Unit/Service/DateTimeNormalizerTest.php — full unit coverage of the new service.
  • tests/Unit/Db/MagicStatisticsHandlerTest.php — regression test: stored empty-string date/date-time renders as null, not today/now.
  • Verified end-to-end in the running dev instance: POSTing "publishedAt": \"\" now persists NULL in the DB; prior to the fix it persisted the current datetime.

Documentation

  • New: docs/Features/property-authorization.md — concept, schema structure, rule shape (group + optional match), full operator catalogue ($eq, $ne, $gt, $gte, $lt, $lte, $in, $nin, $exists), dynamic variables ($organisation / $activeOrganisation, $userId / $user, $now), worked examples (scheduled publication via publishedAt: { \"\$lte\": \"\$now\" }, organisation-scoped internal notes, role-restricted updates), evaluation semantics (reads strip the field, writes reject with validation error, admin bypass, null handling), and performance notes (hasPropertyAuthorization() short-circuit).
  • Updated: docs/Features/access-control.md — added cross-link in the Permission Levels bullet; new "Property-Level Authorization" section with canonical examples pointing readers at the full doc; removed "Field-Level Authorization" and "Time-Based Permissions" from Future Enhancements (both now shipped).
  • Updated: CHANGELOG.md — user-facing Unreleased entry describing the empty-string → null behaviour correction across write / read / bulk / metadata / search.

OpenSpec artifacts

Change tracked at openspec/changes/fix-empty-string-date-conversion/:

  • proposal.md — problem statement and scope
  • design.md — call-site inventory, decisions (D1–D5), seed data, migration plan
  • tasks.md — 30+ tasks, ~24 complete (implementation + unit tests + docs); integration tests and cross-app verification tracked as remaining
  • specs/datetime-input-handling/spec.md — new capability spec

Test plan

  • Unit tests pass (DateTimeNormalizerTest, MagicStatisticsHandlerTest)
  • Manual verification: POST { \"publishedAt\": \"\" } → stored as NULL, rendered as null
  • Manual verification: POST { \"publishedAt\": \"2026-04-20T14:00:00Z\" } → round-trips correctly
  • Manual verification: POST without publishedAt → stays null (unchanged behaviour)
  • Integration tests for each migrated site (tasks 6.1–6.7, tracked in OpenSpec)
  • Run opencatalogi / softwarecatalog test suites against the patched backend (tasks 7.1–7.3)
  • composer check:strict green on CI (PHPCS / PHPMD / Psalm / PHPStan)

Migration notes

Objects already persisted with an empty-string date will continue to render as today until they are next written. A one-shot UPDATE ... SET col = NULL WHERE col = '' maintenance command is flagged as optional follow-up (openspec/changes/fix-empty-string-date-conversion/design.md §Migration Plan); on next read/save the normaliser will correct the value in place.

Backwards compatibility

No breaking changes to the API contract. Previously, clients that sent \"\" for a date field received back the current datetime on the next read — this was never documented behaviour and is the bug being fixed. Clients that correctly send null (or omit the key) see no change. Clients that correctly send a valid ISO 8601 value see no change.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ edbbb52

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-04-21 09:12 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 1703178

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit
Newman
Playwright ⏭️

Quality workflow — 2026-04-21 09:19 UTC

Download the full PDF report from the workflow artifacts.

bbrands02
bbrands02 previously approved these changes Apr 21, 2026
@WilcoLouwerse

Copy link
Copy Markdown
Contributor

Nice fix on the datetime normalization — DateTimeNormalizer + the call-site migration is well-scoped and well-tested. Two unrelated hunks in the first commit look like debugging leftovers and should probably be reverted before merging:

1. lib/Service/Object/QueryHandler.php:379 — copy-paste typo:

unset($countQuery['_limit'], $countQuery['_offset'], $countQuery['_page'],
      $countQuery['_facetable'], $paginatedQuery['_extend']);
//                                ^^^^^^^^^^^^^^^ should be $countQuery

The line above correctly unsets $paginatedQuery['_extend']; this line was clearly meant to do the same for $countQuery. Net effect: _extend is never stripped from the count query.

2. lib/Controller/RegistersController.php:287 — the if (in_array('schemas', $extend, true) === true) { guard and its closing } are both commented out, so schemas are now expanded on every index() call regardless of _extend. The surrounding comment ("If 'schemas' is requested in _extend…") is untouched, which suggests this wasn't an intentional behavior change.

Neither is mentioned in the description, CHANGELOG, or OpenSpec artifacts. If they're intentional, happy to be corrected — otherwise recommend dropping both hunks to keep this PR scoped to the datetime fix + RBAC docs.

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See previous comment

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ f397e5b

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-04-21 11:33 UTC

Download the full PDF report from the workflow artifacts.

@WilcoLouwerse

Copy link
Copy Markdown
Contributor

Thanks for the quick turnaround — both issues are resolved. LGTM on the datetime fix.

Small nit (non-blocking): the restored if block in RegistersController.php uses tabs while the rest of the file is 4-space indented, and the closing }//end if has 9 leading spaces. PHPCS (task 8.1) should flag it on the next run.

Approving. 👍

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 203c4d6

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit
Newman
Playwright ⏭️

Quality workflow — 2026-04-21 11:37 UTC

Download the full PDF report from the workflow artifacts.

@rjzondervan rjzondervan merged commit 3ceb932 into development Apr 21, 2026
33 of 41 checks passed
@rjzondervan rjzondervan deleted the feature/1292/fix-empty-string-date-conversion branch April 21, 2026 11:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants