Phase 2: Registry Seeding and Import-Step Ordering

.coderabbit.yaml

@@ -0,0 +1,2 @@
+reviews:
+  path_filters: ["!.planning/**"]

.planning/PROJECT.md

@@ -53,25 +53,40 @@ A second factor that actually holds for in-site users, and that can be deployed
       `X-Forwarded-For`, blank line in the IP whitelist) were found by code review and fixed
       with regression tests, so the plugin is fail-closed rather than fail-crashed
 
+**Registry seeding** — *Validated in Phase 2: Registry Seeding and Import-Step Ordering (2026-07-29)*
+
+- ✓ `Interface ... IGoogleAuthenticatorSettings defines a field ska_secret_key, for which there
+      is no record` is fixed on new Plone site creation. `<depends name="plone.app.registry"/>`
+      declared at `configure.zcml:50`, and the nested `runImportStepFromProfile` re-entry is gone
+      from `src/`. Confirmed against a real site-creation log (UAT 2026-07-29): zero `no record`
+      lines — REG-01, REG-02, REG-04
+- ✓ The ordering is asserted rather than accidental, and the assertion is a *genuine* control:
+      `test_import_step_declares_registry_dependency` asserts the pre-sort
+      `getImportStepMetadata(...)['dependencies']` and was reproduced failing when the `<depends>`
+      line is deleted. The older `test_import_step_ordering` is kept as the outcome check with a
+      docstring that admits it proves nothing alone — REG-03
+- ✓ `ska_secret_key` is seeded once at install time by `setuphandlers._setup_secret_key()`;
+      `get_ska_secret_key()` is a pure read that raises `ValueError` on an empty key (fail-closed,
+      no plaintext-equivalent fallback, no lazy mint on a `transaction.abort()` path). Re-applying
+      the profile leaves an existing key unchanged — REG-04, REG-05
+- ✓ Derived `ska` key components are separated by a length-prefixed netstring join, so two
+      component tuples that collide under bare concatenation now derive to different keys.
+      Asserted with an exact-string check on a provably-colliding fixture — BUG-04
+
 ### Active
 
 **Correctness**
 
-- [ ] Fix `Interface ... IGoogleAuthenticatorSettings defines a field ska_secret_key, for which
-      there is no record` on new Plone site creation — via `<depends name="plone.app.registry"/>`
-      and removing the nested `runImportStepFromProfile`, **not** via the rename. The root cause
-      is Python 2 `set` iteration order over import-step ids, so the rename changes a hash and
-      may make the error vanish without fixing it. An ordering assertion in the test suite is the
-      actual control
-- [ ] `bin/code-analysis` exits 0 (~40 pre-existing findings; the buildout installs a
-      pre-commit hook that fails every commit until this is clean)
+- [ ] `bin/code-analysis` exits 0. The corrected baseline is **318 pre-existing findings**, not
+      the ~40 previously recorded here — measured in plan 01-03 (RESEARCH C-6). 184 of the 318
+      (58%) are `isort` findings, and the rename actively perturbs first-party import ordering,
+      so QUAL-06 must be planned against 318. The buildout installs a pre-commit hook that fails
+      every commit until this is clean (`--no-verify` in the meantime)
 - [ ] Fix open redirect: `next_url` accepted unvalidated at `token.py:112-113`
 - [ ] Fix `UnboundLocalError` on `redirect_url` at `user_setup.py:96`
 - [ ] Use a constant-time comparison for the reset token at `reset_bar_code.py:104` — encoding
       both sides first, because `hmac.compare_digest` raises `TypeError` across `str`/`unicode`
       and the stored and submitted values differ in type
-- [ ] Separate the components of the derived `ska` key at `helpers.py:259` (currently bare
-      concatenation, collidable)
 - [ ] Swap `py2-ipaddress` for `ipaddress == 1.0.23` with `unicode` coercion at `helpers.py:459`
       and `:496`. Forced by adding `cryptography`, which pulls the `ipaddress` backport — both
       distributions install a top-level module of the same name, and the backport raises
@@ -266,6 +281,9 @@ enumerates the bugs, security gaps, and test-coverage holes referenced above.
 | Recovery codes hashed with one salt per user, not per code | A per-code salt forces N hash runs per attempt (~1.1s for 10 codes) on a login-adjacent endpoint — a DoS lever. Per-user still defeats cross-user rainbow tables, which is all a salt does here | — Pending |
 | No upgrade steps for the rename; existing dev ZODBs discarded | No enrolled users to migrate, and pickled module paths make in-place migration far more work than recreating a dev database | — Pending |
 | Don't rename `PAS_ID` (`google_auth`) | Already namespace-neutral; renaming it would create a second plugin on any existing ZODB | — Pending |
+| Seed `ska_secret_key` at install time in `setuphandlers._setup_secret_key()`, **not** lazily on first read | Reverses the 02-01 plan's D-04/D-05 lazy-mint design (CR-02). A mint inside `get_ska_secret_key()` is reachable from `authenticateCredentials()`, a path that ends in `transaction.abort()` on `Unauthorized` — it would discard the key *after* a signed URL using it was already handed to the browser. `get_ska_secret_key()` is now a pure read that raises `ValueError` on an empty key | ✓ Shipped Phase 2 |
+| Derive the `ska` key with a length-prefixed netstring join, not bare concatenation | Bare concatenation of `(user_secret, browser_hash, ska_secret_key)` is collidable: a component-boundary shift yields the same key, so a signature minted in one context validates in another. Asserted with an exact-string check on a fixture that provably collides under the old scheme, so it cannot regress into a cosmetic reformat | ✓ Shipped Phase 2 |
+| Assert the `<depends>` *declaration*, not just the resulting sorted order | The first ordering test was tautological — it stayed green with `<depends name="plone.app.registry"/>` deleted, purely by CPython 2.7 string-hash coincidence. `test_import_step_declares_registry_dependency` asserts the pre-sort `getImportStepMetadata(...)['dependencies']` instead, and was reproduced failing on deletion. The outcome test is kept, with a docstring admitting it proves nothing alone | ✓ Shipped Phase 2 |
 
 ## Evolution
 
@@ -285,4 +303,4 @@ This document evolves at phase transitions and milestone boundaries.
 4. Update Context with current state
 
 ---
-*Last updated: 2026-07-29 — Phase 1 complete (rename + fail-closed); rename requirements moved to Validated*
+*Last updated: 2026-07-29 — Phase 2 complete (registry seeding + import-step ordering); registry-seeding and BUG-04 requirements moved to Validated, three Phase 2 decisions logged, the stale `~40 code-analysis findings` figure corrected to 318*

.planning/REQUIREMENTS.md

@@ -26,11 +26,11 @@ ASVS V2, and to APIs executed against this repo's own Python 2.7.18 interpreter.
 
 ### Site creation and registry (REG)
 
-- [ ] **REG-01**: Creating a new Plone site with the add-on selected completes without the `ska_secret_key ... no record` error
-- [ ] **REG-02**: The `<depends name="plone.app.registry"/>` declaration makes the import-step ordering explicit rather than dependent on Python 2 `set` iteration order
-- [ ] **REG-03**: A test asserts `getSortedImportSteps()` places this package's step after `plone.app.registry` — the ordering assertion, not the rename, is the control
-- [ ] **REG-04**: The nested `runImportStepFromProfile` call is gone; `ska_secret_key` is minted by a lazy accessor on first use
-- [ ] **REG-05**: Re-applying the default profile leaves an existing `ska_secret_key` unchanged, so signed URLs in flight are not invalidated
+- [x] **REG-01**: Creating a new Plone site with the add-on selected completes without the `ska_secret_key ... no record` error
+- [x] **REG-02**: The `<depends name="plone.app.registry"/>` declaration makes the import-step ordering explicit rather than dependent on Python 2 `set` iteration order
+- [x] **REG-03**: A test asserts `getSortedImportSteps()` places this package's step after `plone.app.registry` — the ordering assertion, not the rename, is the control
+- [x] **REG-04**: The nested `runImportStepFromProfile` call is gone; `ska_secret_key` is seeded reliably at install time (revised after CR-02 code review: a lazy-accessor mint on first use was tried first, but it wrote registry state from a request path that `transaction.abort()`s on `Unauthorized`, discarding the mint — see `02-01-SUMMARY.md` "Post-review revision")
+- [x] **REG-05**: Re-applying the default profile leaves an existing `ska_secret_key` unchanged, so signed URLs in flight are not invalidated
 
 ### Secret handling (SEC)
 
@@ -86,7 +86,7 @@ ASVS V2, and to APIs executed against this repo's own Python 2.7.18 interpreter.
 - [ ] **BUG-01**: `next_url` is validated against the portal URL before redirect; an off-site value is refused (`token.py:112-113`)
 - [ ] **BUG-02**: `redirect_url` is always bound on every code path through `user_setup.py`
 - [ ] **BUG-03**: The bar-code reset token comparison is constant-time, with both operands encoded first to avoid `TypeError` across `str`/`unicode`
-- [ ] **BUG-04**: The derived `ska` key separates its components rather than concatenating them bare
+- [x] **BUG-04**: The derived `ska` key separates its components rather than concatenating them bare
 - [ ] **BUG-05**: `py2-ipaddress` is replaced by `ipaddress == 1.0.23`, with `unicode` coercion at the two call sites, so adding `cryptography` cannot break every login through module shadowing
 - [ ] **BUG-06**: Query-string values are URL-encoded on the way in, resolving the `+`-escaping FIXME
 
@@ -171,11 +171,11 @@ lists above is mechanical. Phase names are in `.planning/ROADMAP.md`.
 | RENAME-10 | Phase 1 | Complete |
 | RENAME-11 | Phase 1 | Complete |
 | RENAME-12 | Phase 1 | Complete |
-| REG-01 | Phase 2 | Pending |
-| REG-02 | Phase 2 | Pending |
-| REG-03 | Phase 2 | Pending |
-| REG-04 | Phase 2 | Pending |
-| REG-05 | Phase 2 | Pending |
+| REG-01 | Phase 2 | Complete |
+| REG-02 | Phase 2 | Complete |
+| REG-03 | Phase 2 | Complete |
+| REG-04 | Phase 2 | Complete |
+| REG-05 | Phase 2 | Complete |
 | SEC-01 | Phase 3 | Pending |
 | SEC-02 | Phase 3 | Pending |
 | SEC-03 | Phase 3 | Pending |
@@ -216,7 +216,7 @@ lists above is mechanical. Phase names are in `.planning/ROADMAP.md`.
 | BUG-01 | Phase 7 | Pending |
 | BUG-02 | Phase 3 | Pending |
 | BUG-03 | Phase 3 | Pending |
-| BUG-04 | Phase 2 | Pending |
+| BUG-04 | Phase 2 | Complete |
 | BUG-05 | Phase 3 | Pending |
 | BUG-06 | Phase 7 | Pending |
 | QUAL-01 | Phase 8 | Pending |

.planning/ROADMAP.md

@@ -31,7 +31,7 @@ failure mode has no error page and no log line.
 Decimal phases appear between their surrounding integers in numeric order.
 
 - [x] **Phase 1: Rename and Fail-Closed** - `imio.googleauthenticator` everywhere, and a plugin exception becomes a 500 instead of a password-only login (completed 2026-07-29)
-- [ ] **Phase 2: Registry Seeding and Import-Step Ordering** - New Plone sites install cleanly, and the ordering that makes them clean is asserted rather than accidental
+- [x] **Phase 2: Registry Seeding and Import-Step Ordering** - New Plone sites install cleanly, and the ordering that makes them clean is asserted rather than accidental (completed 2026-07-29)
 - [ ] **Phase 3: Encrypted Seeds and Local QR** - Seeds are Fernet-encrypted at rest, never sent to Google, and never fall back to plaintext
 - [ ] **Phase 4: PAS Boundary** - The second factor cannot be bypassed by any credentials extractor, and the refusal leaks nothing
 - [ ] **Phase 5: Drift, Replay and Lockout** - A replayed code fails, brute force stops at N attempts, and the counters actually persist
@@ -90,12 +90,21 @@ Plans:
 **Success Criteria** (what must be TRUE):
 
   1. Creating a new Plone site with the add-on selected completes with no `IGoogleAuthenticatorSettings defines a field ska_secret_key, for which there is no record` in `var/log/instance.log`.
-  2. A test asserts `getSortedImportSteps()` places this package's step after `plone.app.registry`. **The assertion is the control, not the rename** — the rename changes the step id's hash and can make the error vanish without fixing anything, and it would return the first time any other add-on adds or removes an import step.
-  3. `grep -r runImportStepFromProfile src/` returns nothing, and `ska_secret_key` is minted by a lazy accessor on first use rather than by a nested profile import.
+  2. A test asserts the import step **declares** `plone.app.registry` as a dependency (via `getImportStepMetadata()['dependencies']`), and a second test asserts `getSortedImportSteps()` places this package's step after it. **The declaration assertion is the control, not the rename and not the sorted order** — verified in phase-2 verification: with the `<depends>` line deleted, the sorted-order assertion still passes by CPython 2.7 string-hash accident (index 51 vs 36 of 52), so order alone proves nothing and would flip the first time any other add-on adds or removes an import step.
+  3. `grep -r runImportStepFromProfile src/` returns nothing, and `ska_secret_key` is minted without a nested profile import — seeded once at install time by `setuphandlers._setup_secret_key()`, with `get_ska_secret_key()` a pure read. *(Revised after code review CR-02: the original criterion said "minted by a lazy accessor on first use". A mint inside the accessor writes registry state from `authenticateCredentials()`, a path that ends in `transaction.abort()` on `Unauthorized`, discarding the key after a URL signed with it was already redirected to. See `02-REVIEW.md` CR-02 and `02-01-SUMMARY.md`.)*
   4. A test applies the default profile **twice** and asserts `ska_secret_key` is unchanged, so signed URLs in flight are not invalidated by a reinstall. (A retained value that no longer validates is silently replaced by the default `u''`, with only an INFO log line.)
   5. A test asserts the derived `ska` key separates its components: two different component tuples that share the same bare concatenation produce different keys.
 
-**Plans**: TBD
+**Plans**: 2/2 plans executed
+
+Plans:
+**Wave 1**
+
+- [x] 02-01-PLAN.md — Declared `<depends name="plone.app.registry"/>`, the nested profile re-entry deleted, `ska_secret_key` minted lazily in `get_ska_secret_key`, and one test asserting ordering, records, mint and profile-re-apply preservation
+
+**Wave 2** *(blocked on Wave 1 completion)*
+
+- [x] 02-02-PLAN.md — Length-prefixed `ska` key derivation with the collision it prevents asserted, plus the `get_browser_hash` empty-string regression guard and the changelog
 
 **Phase notes:**
 
@@ -250,7 +259,7 @@ Phases execute in numeric order: 1 → 2 → 3 → 4 → 5 → 6 → 7 → 8
 | Phase | Plans Complete | Status | Completed |
 |-------|----------------|--------|-----------|
 | 1. Rename and Fail-Closed | 4/4 | Complete    | 2026-07-29 |
-| 2. Registry Seeding and Import-Step Ordering | 0/TBD | Not started | - |
+| 2. Registry Seeding and Import-Step Ordering | 2/2 | Complete    | 2026-07-29 |
 | 3. Encrypted Seeds and Local QR | 0/TBD | Not started | - |
 | 4. PAS Boundary | 0/TBD | Not started | - |
 | 5. Drift, Replay and Lockout | 0/TBD | Not started | - |