Phase 3: Encrypted Seeds and Local QR - #3
Merged
Conversation
Fernet encryption at rest, in-process QR rendering, and the forced ipaddress swap for Phase 3. Reuses STACK.md/PITFALLS.md's prior execution-verified findings and adds one new executed finding: rebus.b32encode(os.urandom(N)) raises UnicodeDecodeError on almost every call, so SEC-06's 160-bit seed must use stdlib base64.b32encode instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three plans, 12 requirements, waves 1-3. 03-01 carries the ROADMAP's own same-commit group (Fernet + fail-closed + local QR + the ipaddress swap) as one plan, because the roadmap forbids splitting it across plans within a phase. Two corrections to 03-RESEARCH.md/03-PATTERNS.md are baked into the plans: - there are THREE ipaddress.*() call sites in helpers.py, not two; the third (ip_address(proxies[0]) in the private-hop strip loop) would silently disable private-hop stripping under ipaddress==1.0.23, since AddressValueError subclasses the ValueError the loop already catches. ROADMAP success criterion 5 corrected accordingly. - BUG-03 has two comparison sites, not one: reset_bar_code.py handleSubmit AND updateFields. Both route through one shared helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Single prompt-fed lane (headless claude, same model family as planner). gemini auth-blocked, coderabbit over free-plan file limit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Twelve findings from 03-REVIEWS.md folded into the three plans. The three HIGH findings, all incorporated: - 03-01 Task 5 (new): unswallow the ValueError in helpers.enable_two_factor_authentication_for_users, and make both callers report failure instead of "Changes saved." / an unconditional success message. A broken key previously enrolled nobody while the control panel said the change was saved. - 03-01 Task 3(b2) (new): base.cfg [testenv] IMIO_GA_SEED_KEY moves into Wave 1, so plan 03-01's own `bin/test -t '!robot'` gate is reachable. test_generic.py's test_user_setup_view reaches encrypt_seed and had no key until Wave 2. - 03-01 Task 4: setRequest(request)/setRequest(None) in the PAS fail-closed test, read_first repointed at test_unmatched_username_does_not_crash. The assertion stays narrowed to ValueError. Also: all five get_or_create_secret callers enumerated with a per-caller fail-closed decision; api.user.create fail-closed assertion and the account-creation failure mode added to DOC-03; base.cfg [instance] left deliberately undeclared with a prohibition against a placeholder key; a real-authenticator-app <human-check> for ROADMAP criterion 4; STATE.md's ska_secret_key form-field item re-deferred in writing; a behavioural per-call SEC-02 test; brittle greps replaced with AST parses and minimums; env -u for the order-dependent bin/instance check; u'\xe9' for the py2 -c literal; the Django drop changed from assertion to observation; Pillow declared in install_requires. Requirement coverage unchanged: all 12 IDs, no duplicates. Probe accounting still sums to 24 with 6 flagged and 3 backstop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…x ipaddress swap - Fernet-encrypt every TOTP seed (v1$<token> envelope), fail-closed on a missing/malformed IMIO_GOOGLEAUTHENTICATOR_SEED_KEY: no local except returns None, a default, or the plaintext input unchanged (SEC-01..04) - 160-bit os.urandom seed via stdlib base64.b32encode, replacing the third-party encoder that ASCII-decodes raw entropy (SEC-06) - get_barcode_image renders the QR in-process to a data:image/png;base64, URI via qrcode/Pillow -- no outbound request to chart.googleapis.com and no subprocess (SEC-05) - swap the ipaddress distribution (py2-ipaddress -> ipaddress==1.0.23) and coerce all three ipaddress.ip_address()/ip_network() call sites in helpers.py to unicode via a new _to_unicode_ip() helper, so the distribution swap does not inertly disable the IP whitelist (BUG-05) - setup.py/test-4.3.cfg: install_requires and [versions] carry the four new/changed pins (cryptography==3.3.2, ipaddress==1.0.23, qrcode==6.1, cffi==1.15.1, Pillow unpinned); base.cfg [testenv] carries a throwaway Fernet key so bin/test's own suite has a usable key - TestSeedEncryption.test_seed_encryption_round_trip: end-to-end tracer -- generate, encrypt, store, decrypt, and validate through a real onetimepass TOTP - fix TestIPWhitelisting's own direct IPv4Network/IPv4Address(str) instantiations to unicode literals: ipaddress==1.0.23 requires unicode for direct construction too, a call shape RESEARCH.md/PATTERNS.md did not enumerate (Rule 1 auto-fix, scoped to this plan's own dependency swap) git commit --no-verify: bin/code-analysis fails on 318 pre-existing findings until Phase 8 (QUAL-06); not introduced by this change. Env-var name locked via Task 1's checkpoint:decision: IMIO_GOOGLEAUTHENTICATOR_SEED_KEY (human overrode the plan's IMIO_GA_SEED_KEY default).
…ad and ska component - TestSeedEncryption.test_seed_encryption_fails_closed: encrypt_seed/ generate_secret refuse with the key unset, garbage (non-base64), and valid-base64-wrong-length -- storing no plaintext on any path; the exception names IMIO_GOOGLEAUTHENTICATOR_SEED_KEY and never the key value; decrypt_seed refuses an unknown/missing envelope version (SEC-03 enrollment half, SEC-02 no-leak, SEC-04) - TestSeedEncryption.test_encryption_key_is_read_per_call: proves the key is read fresh from os.environ on every call by mutating os.environ between an encrypt and a decrypt (key A then key B) -- deliberately does not rebind the reader, which a module-scope-frozen implementation would also pass (SEC-02 per-call, behavioural) - TestSeedEncryption.test_ciphertext_is_a_safe_ska_key_component: closes 02-SECURITY.md R-02-02 -- get_ska_secret_key() survives a real v1$<fernet-token> ciphertext as its netstring component - TestPas.test_login_is_refused_when_seed_key_is_broken: a 2FA-enabled user's login raises ValueError out of _extractUserIds rather than falling through to a password-only session, with a bound request (so the assertion reaches the crypto path instead of dying on is_whitelisted_client's unbound-getRequest AttributeError) and a non-vacuity control proving the test is not vacuous (SEC-03 validation half) - both new get_or_create_secret(user) calls use overwrite=True: a memberdata property set by an earlier test method can otherwise leak forward under a different test's freshly-generated setUp key, because BaseTest._install()'s testbrowser calls commit inside IntegrationTesting (documented hazard, CLAUDE.md/test-4.3.cfg comments) - Open Question 1 (enrollment-side propagation) confirmed by observation: no raise added to user_setup.py's handleSubmit; Open Question 2 (a richer operator-facing error view) remains declined for this phase Test-only; no production file changed in this commit. git commit --no-verify: bin/code-analysis fails on 318 pre-existing findings until Phase 8 (QUAL-06); not introduced by this change.
…roken - helpers.enable_two_factor_authentication_for_users: a ValueError handler above the existing per-user except Exception re-raises instead of being absorbed at DEBUG -- a key failure is not per-user, it is total, so skipping every user and returning normally reported a success that never happened (T-03-21) - controlpanel.py's Save handler and the @@google-authenticator-enable-for-all-users view both catch that ValueError, show an 'error' status message naming IMIO_GOOGLEAUTHENTICATOR_SEED_KEY, and suppress the success message on that path -- while still applying/redirecting unconditionally, so the operator's other registry edits are not silently discarded alongside the enrollment failure - TestSeedEncryption.test_bulk_enable_reports_failure_when_seed_key_is_broken: asserts all three surfaces (the helper itself, the enable-for-all view, and the control panel Save) by message TYPE, with a real z3c.form Save cycle through GoogleAuthenticatorSettingsEditForm - TestSeedEncryption.test_user_creation_fails_closed_when_seed_key_is_broken: api.user.create raises with the key broken (good-key control run first); observed and asserted, rather than assumed, that within this synchronous test call (no enclosing HTTP transaction to abort) the MemberData object itself can still exist afterward, but enable_two_factor_authentication and two_factor_authentication_secret are never written -- see SUMMARY Re-defers the ska_secret_key control-panel TextLine field (02-SECURITY.md R-02-01): Plone 4.3's z3c.form PasswordWidget extracts empty for an untouched field, so swapping the field type would blank the site signing key on the next Save. Needs its own tested change, not a drive-by here. No change to the schema field in this commit. git commit --no-verify: bin/code-analysis fails on 318 pre-existing findings until Phase 8 (QUAL-06); not introduced by this change.
Records execution outcome, deviations, and decisions for plan 03-01 (Fernet seed encryption, in-process QR, ipaddress swap). Updates STATE.md position/decisions, ROADMAP.md plan-progress, and REQUIREMENTS.md (SEC-01..06, BUG-05 marked complete). git commit --no-verify: bin/code-analysis fails on 318 pre-existing findings in pas_plugin.py (untouched by this plan) until Phase 8 (QUAL-06).
- New subscribers.on_process_starting(event), wired for zope.processlifetime.IProcessStarting in configure.zcml: logs one CRITICAL line naming IMIO_GOOGLEAUTHENTICATOR_SEED_KEY when get_encryption_key() is falsy (absent or empty string), never raises. - New TestOnProcessStarting: direct-call coverage of absent/present/empty key, the never-raises guarantee, and a minidom parse of configure.zcml proving the subscriber registration exists and the file still parses. - Verified end-to-end: env -u IMIO_GOOGLEAUTHENTICATOR_SEED_KEY bin/instance start reaches "Ready to handle requests" and logs the CRITICAL line; bin/instance stop confirms a clean shutdown, not a crash. - bin/code-analysis fails on 318 pre-existing findings (CLAUDE.md); --no-verify authorized until Phase 8/QUAL-06.
- README.rst: new "Seed encryption key (required)" subsection between Buildout and ZMI. Documents the variable, how to generate it, all three consequences of its absence (enrollment, login, and new account creation via userCreatedHandler), that it is per-ZEO-client not per-database, the intermittent InvalidToken-with-no-ZODB-evidence failure mode, that base.cfg [instance] deliberately carries no entry and why, that the deployment buildout supplies [instance]'s copy the way SSO_APPS_CLIENT_SECRET already does, how a local developer sets their own, and the industrialisation concat::fragment as an open dependency that leaves the feature code-complete but not deployable. - tests/test_subscribers.py: added test_seed_key_is_present_in_the_test_environment, which reads (never sets) os.environ to prove base.cfg [testenv]'s key is non-empty and a valid Fernet key -- the same assertion that proves CI inherits a usable key via [test] environment = testenv -- and that a ciphertext from a foreign key raises ValueError under the [testenv] key (SEC-07 adjacency, the mechanised ZEO-skew failure mode). - No base.cfg edit: [testenv]'s line is plan 03-01's, and [instance] stays deliberately absent (a valid placeholder there would silently suppress Task 1's CRITICAL log). No CI workflow file touched -- inheritance is proven by the new test, not a fourth edit. - bin/code-analysis fails on 318 pre-existing findings (CLAUDE.md); --no-verify authorized until Phase 8/QUAL-06.
BUG-03: reset_bar_code.py compared the stored bar_code_reset_token (py2 str) against the request's signature (unicode) with a bare ==/!=, both in handleSubmit and in updateFields -- the same timing oracle on the same secret at two call sites, not the one the requirement named. A naive hmac.compare_digest swap raises TypeError across those two Python 2 string types, so both operands are coerced to str bytes in one shared helper, validate_bar_code_reset_token, used at both sites. An absent or empty stored token now refuses to match anything, including an empty submitted value -- previously the updateFields == comparison returned True for two empty strings. A non-ASCII submitted value returns False instead of raising UnicodeEncodeError. Test-analysis-only (skill R6) violation on line 73 of test_helpers.py (a local `from imio.googleauthenticator import helpers` inside test_get_ip_addresses_whitelist_drops_blank_lines) is pre-existing from plan 03-01 and not introduced by this commit.
BUG-02: research traced all three reachable branches of SetupForm.handleSubmit and found redirect_url bound on every one of them -- the UnboundLocalError the requirement describes does not reproduce on the current source. No production code changes: this closes the requirement with a regression test covering all three branches plus the empty-token short circuit, and says explicitly in its docstring that it is a guard, not a fix. Test mechanics discovered along the way (documented inline): the token widget's TextLine converter requires a unicode submitted value, not str; and ZPublisher's HTTPRequest.get() caches whatever it resolves into request.other, so driving four scenarios against the same shared request object needs request.other cleared alongside request.form between them, or a later scenario reads back an earlier one's stale token value. Also carries forward the cross-test secret-leakage hazard documented in 03-01-SUMMARY.md (BaseTest._install() commits inside a real testbrowser): setUp forces a fresh secret under its own key before updateFields() can read a stale ciphertext left by an earlier test method. CHANGES.rst gains entries covering the whole phase (encryption, fail-closed behaviour, the new required IMIO_GOOGLEAUTHENTICATOR_SEED_KEY environment variable, the local QR render, the dependency swap, the constant-time comparison, and this regression guard), each written for a reader upgrading the package.
BUG-02 and BUG-03 closed; Phase 3 code-complete at 41/41 tests passing. bin/code-analysis fails on 318 pre-existing findings unrelated to this plan's changes (CLAUDE.md-authorized --no-verify, same as every commit in this phase).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Test 1's enrolment half passed (local QR + Fernet round trip verified with a real authenticator app). The login half failed, yielding two gaps: G-03-1 (blocker) 2FA bypassed. Reporter used the Zope root admin, which lives in the root acl_users. pas_plugin.py:139-141 validates the password by delegating to the *site's* other auth plugins; none can resolve a root account, so it returns early before the credential wipe and redirect, and the root user folder then logs the user in on one factor. Pre-existing structural boundary, not a phase-3 regression. The in-scope defect is the false assurance: the setup form enrols such an account and reports success. G-03-2 (major) TypeError: Incorrect secret -> 500. validate_token passes get_secret()'s implicit None straight to onetimepass.valid_totp when the resolved user has no seed. Reproduced under test. Phase 3 success criterion 4 remains UNVERIFIED end to end - the login half must be re-run with a real Plone member account. Also trims COVERAGE.md's no-integration declaration reason under the 200-char limit so the verify:pre api-coverage gate passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Re-ran the login half with a real Plone member. Login IS intercepted and redirected to a correctly signed @@google-authenticator-token URL, closing that half of criterion 4 and confirming G-03-1 is specific to Zope-root accounts rather than a plugin defect. New gap G-03-3 (major): the bar-code reset email fails with UnicodeEncodeError. request_bar_code_reset.py:98-102 calls host.send() without charset, so MailHost._mungeHeaders ASCII-encodes a unicode body via _try_encode's bare text.encode() fallback. The charset='utf-8' on line 94 is an argument to the page template, not to MailHost. The accented character comes from an interpolated value (email_from_name and/or the translated subject) - the template itself is pure ASCII. Only one send site exists. Criterion 4's last step - a valid OTP accepted at the token form - remains unverified: the member had 2FA enabled with a generated secret but was never shown a QR, so no OTP existed, and the recovery path is G-03-3. Also records three non-blocking observations: reset form re-asks for a username, a username-enumeration oracle at line 116, and globally_enabled enrolling users who are never shown a QR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… to end
Test 1 passes on the third run. A real authenticator app enrolled against a
locally-rendered QR, and its code logged member cadam in through the token
form - so both phase-3 deliverables under test hold against real hardware:
local qrcode rendering with no outbound request (SEC-05) and the Fernet seed
round trip (SEC-06 storage half).
Not separately confirmed: the otpauth:// label rendering literally as
<username>@<domain>. Inferred from the app accepting the QR and emitting
codes that validated, not read back from the payload.
Three defects were found along the way and stay recorded in ## Gaps. None is
a phase-3 regression and none is a phase-3 deliverable:
G-03-1 deferred - 2FA silently bypassed for Zope-root accounts. Needs a
scope decision, not a fix, so it is deliberately not status:failed.
G-03-2 open - unguarded null seed -> HTTP 500 at the token form.
G-03-3 open - bar-code reset email dies on non-ASCII. Breaks the only
documented recovery path for a locked-out user.
Canonicalizes 03-VERIFICATION.md from human_needed to passed: it was waiting
only on this human UAT. phase uat-passed --require-verification now returns
passed with no blockers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
G-03-3. request_bar_code_reset.py called host.send() with no charset, so MailHost._mungeHeaders ASCII-encoded the unicode body via _try_encode's bare text.encode() fallback and died on the first accented byte. The accent comes from an interpolated value - the site's email_from_name, or the Subject line resolving through the fr catalogue as "Demande de reinitialisation ..." - not from the template, which is pure ASCII. The charset='utf-8' already present on line 94 is a different argument: it goes to the page template and only sets the Content-Type the message declares, never reaching MailHost. That mismatch is why the message correctly announced utf-8 while being encoded as ASCII. UnicodeEncodeError subclasses ValueError, so the handler's except ValueError swallowed it and showed only "An unexpected error occurred." - leaving a locked-out user with no working recovery path, which is the only reason this ranked above cosmetic. Verified failing first: the new test handed 0 messages to MailHost before the fix, with a control asserting the handler reached the send step. The control initially passed vacuously on leftover memberdata from its sibling test, so setUp now clears bar_code_reset_token. MailBase._send is patched rather than substituting a mock MailHost, so the real _mungeHeaders/_try_encode path - where the encoding decision is actually made - still runs. Suite green at 43 tests, 0 failures, 0 errors. Delivery against a real SMTP server is still unproven; the test stops at MailHost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
G-03-2. helpers.validate_token passed get_secret()'s return value straight to
onetimepass.valid_totp. get_secret returns None *implicitly* for a user whose
two_factor_authentication_secret is empty, and onetimepass base32-decodes
whatever it is handed, so this raised TypeError('Incorrect secret') as an
unhandled 500 on a form whose whole job is to reject bad input. Reported from
a real instance: any code submitted at @@google-authenticator-token after
arriving without a signed auth_user parameter 500'd whether or not the code
was correct.
Guarded in validate_token rather than at the call sites. All three callers
route through it - token.py:96, reset_bar_code.py:94 and user_setup.py:68 -
and reset_bar_code.py carried the same exposure without anyone reporting it,
so a per-caller patch would have left it broken.
The guard is deliberately narrow. A *decryption* failure inside get_secret
raises ValueError and keeps propagating: answering "invalid token" to a
broken-key condition would turn a fail-closed refusal into a silent security
downgrade. The new test pins both halves, since that is the part a later
refactor is most likely to flatten into one try/except.
Verified failing first by stashing only the guard: the test errors with the
reported TypeError, and passes with it restored. Suite green at 44 tests,
0 failures, 0 errors.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
T-03-23, raised as a blocking high during /gsd-secure-phase 03 and dispositioned "fix now" rather than "accept". This plugin is registered in the Plone site's acl_users, so a Zope-root account - typically the buildout inituser admin - is authenticated above the site and its login cannot be intercepted: authenticateCredentials delegates the password pre-check to the site's other IAuthenticationPlugins, none of which resolve a root account, so it declines to veto and the root user folder logs the user in on one factor. Root logins stay ungated by design; the package targets in-site users and gating them would mean installing the plugin in the root acl_users, which is roadmap-level. What is fixed is the false assurance: enrolment previously wrote the flag, stored a seed and reported "successfully enabled" for a second factor that would never be demanded - the same class as T-03-21's zero-user "Changes saved.". helpers.is_site_local_user is the discriminator. plone.api.user.get is NOT usable for this: it returns a MemberData for a root account too, and portal_memberdata stores properties against it, so every obvious check reports the account as ordinary. Only the site PAS lookup separates them. Guarded at both self-service sites that make the claim - user_setup.py and reset_bar_code.py - not just the reported one. api.user.get resolves root accounts, so a root user could have obtained a reset token and hit the same false success. updateFields is guarded too, since rendering the QR mints and stores a seed as a side effect. Guard scope was determined empirically rather than assumed: api.user.get_users() returns only site members, so the bulk-enrolment path cannot reach a root account and needs no guard. The test asserts that, so the day it changes this decision fails loudly. Verified failing without the guard, with assertions ordered so the security property (no flag written) breaks first rather than the return value - the first draft short-circuited on the return value and left the two security-relevant assertions unproven. Also adds 03-SECURITY.md: 29 threats, 28 closed, 1 open at low (T-03-26, username enumeration on the reset form) with an accepted-risk entry, so threats_open is 0 against a high threshold. The register records the four UAT-found threats the plan-time model missed, and a Residual Risks section naming two evidence gaps: reset-email delivery is unproven past MailHost, and the otpauth:// label was inferred rather than read back. Suite green at 46 tests, 0 failures, 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reporter confirmed the two closures that rested on inference rather than observation: - T-03-25: ran the reset flow in the browser and received the email, so delivery is proven end to end - not merely past the encoding step where the original traceback died. The automated test still stops at MailHost by design (it patches MailBase._send), so delivery is covered by observation rather than by CI, and that is now stated as such. - Criterion 4: the otpauth:// label renders as <username>@<domain>, read back directly. This was the last inferred sub-assertion in the UAT test. No closure in this phase now rests on inference. The Residual Risks section is kept rather than deleted - the point is that the gaps were named while they were open, not that the file ends up clean. Also refreshes the UAT Outcome table, which still described all three gaps as open work with the pre-fix verdicts. threats_open stays 0; no code change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both requirements were verified once at execution time by a grep acceptance
criterion, with no assertion surviving into CI.
SEC-07 / T-03-21b (high): nothing asserted that base.cfg's [instance] carries
no seed key. The threat is the tempting edit - a syntactically valid
placeholder added so the buildout parses - which would encrypt every
production seed under a value any reader of this repository has AND suppress
the missing-key CRITICAL log, since the key would no longer be absent. Loud
failure becomes silent compromise. Held until now only by prohibition P6.
The new test asserts the [testenv] positive control FIRST: if the section
reader silently returned nothing, the [instance] assertion would pass for the
wrong reason. Read as text rather than via ConfigParser, whose interpolation
raises on buildout's += keys and ${...} references.
DOC-03: the phase's only requirement with no automated verification. The risk
is not that someone deletes README.rst but that a routine rewrite quietly
drops the operator-facing paragraphs, leaving the requirement marked Complete.
Asserts on load-bearing facts (the out-of-repo Puppet dependency, the ZEO
stale-key failure mode) rather than prose, so rewording stays free and
removing information does not.
Both were proven to fail before being kept: injecting the exact placeholder
T-03-21b describes into [instance] failed the first test, and redacting
concat::fragment failed the second. A test that passes either way would have
left the gap open while appearing to close it.
Suite 46 -> 48 tests, 0 failures, 0 errors.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
03-VALIDATION.md was still the unfilled plan-phase stub - every row
placeholder text (REQ-{XX}, {pytest 7.x}, T-3-01) with status: draft - so this
audit was effectively State B despite a file existing. Rebuilt from the three
PLAN/SUMMARY pairs and cross-referenced against the real suite.
Keyed by requirement rather than task id: the phase's 9 tasks include two
checkpoints with no code, and the rest each satisfy several requirements, so a
task-keyed table would duplicate every row.
Records a requirement-text divergence rather than silently ticking it: SEC-07
says the variable is "present in all four places it must exist - [instance],
[testenv], the CI workflow, and the Puppet fragment", but two are deliberately
empty. [instance] is empty because T-03-21b rates a placeholder there worse
than an absence; the CI workflow is empty because [test]'s environment =
testenv already bakes the key into bin/test, so adding it would create a
second source of truth. The behaviour is right and the sentence describing it
is not; SEC-07's wording should be corrected when REQUIREMENTS.md is revised.
Three manual-only entries kept with their performed/not-done state, including
the one that is genuinely outstanding: the Puppet concat::fragment lives in the
industrialisation repo and has not shipped, so the feature is not deployable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Status gaps_found, but the gap is scope: the milestone is 3 of 8 phases
complete, so a definition-of-done audit cannot pass yet. Nothing built is
broken.
The 3-source cross-reference (traceability table x phase VERIFICATION x
SUMMARY frontmatter) agreed exactly on all 31 satisfied requirements - 13+6+12,
matching the 31 [x] checkboxes with zero discrepancies and zero orphans. All
three phases carry passing VERIFICATION, complete UAT and threats_open: 0. All
five cross-phase seams traced sound; both E2E flows intact.
Two findings worth more than the score:
MFA-01 is open - a 2FA-enabled user can still authenticate via Authorization:
Basic without the second factor. The Core Value ("a second factor that
actually holds") is therefore not yet delivered; phases 1-3 were foundation
work and Phase 4 owns the bypass.
MFA-03 is open and load-bearing - the plugin reaches position 0 among
IAuthenticationPlugin only incidentally, via _add_plugin's
movePluginsDown(iface, listPlugins(iface)[:-1]). ROADMAP Phase 4 criterion 3
states the entire second factor rests on that ordering, and no test asserts
it. A future plugin registration could silently disable MFA with a green
suite.
Dismissed the integration checker's one finding after verifying it: the broad
except Exception in user_setup.py / reset_bar_code.py cannot mask a crypto
misconfiguration, because validate_token runs before the try is entered. Only
memberdata writes, status messages and a stdlib hmac compare are inside.
Records a positive cross-phase consistency result: today's T-03-23 disposition
matches phase 1's R-01, locked 2026-07-29, which already named the Zope root
administrator as the break-glass path "which an in-site PAS plugin never runs
for by construction" and assigned the documentation to Phase 4 DOC-01. The
boundary held across three phases; only the UI's claim had drifted.
Nyquist overall partial: phase 3 COMPLIANT, phase 1 NOT-VALIDATED (draft stub,
not a compliance failure), phase 2 MISSING.
Also flags the cross-cutting deployment blocker: the encryption-key
concat::fragment in the industrialisation repo has not shipped, so phase 3 is
complete and the feature is still not deployable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both requirements were proved once at execution time by a shell/build command, with nothing surviving into CI. RENAME-06: nothing guards what lands in the sdist. A profile XML or locale catalogue missing from it yields a package that installs and then misbehaves - no registry records, or an untranslated UI - with nothing failing at build time. Verified by hand once at 01-UAT test 2; bin/check-manifest is deliberately not wired into bin/code-analysis, so nothing re-checked it. Asserts MANIFEST.in's directives rather than building an sdist: the regression is an edit dropping an include, and a setup.py sdist subprocess would cost seconds per run for the same verdict. The global-exclude assertions matter as much as the includes - shipping .pyc or compiled .mo was the specific pollution phase 1 cleaned up. DOC-04: setup.py:6-15 wraps BOTH file reads in a bare except: that substitutes ''. A rename, move or encoding error in README.rst or CHANGES.rst therefore ships metadata missing that half with no build failure. This is the exact hazard 01-VALIDATION.md's own Manual-Only row named as "the automatable half" and then left manual. Runs the real setup.py --long-description rather than re-reading the two files, because the failure being guarded is precisely that setup.py stopped incorporating one of them. Asserts a marker from EACH file, so losing either half fails - the plan's original length-only criterion passes on README alone. Both proven to fail before being kept: deleting the profiles include failed the first, hiding CHANGES.rst failed the second. Suite 48 -> 50 tests, 0 failures, 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
01-VALIDATION.md was well-filled by plan-phase from RESEARCH but written pre-execution: status draft, all 13 map rows pending, wave_0_complete false with the note "the tree still has src/collective/". Reconciled against the executed tree. All 13 requirement rows re-run green. All six Wave 0 items verified landed individually, not assumed - the renamed layer constant, the installProduct string at testing.py:26, all five named test methods, and the path literal in test_product_is_installed. Two honest records rather than silent ticks: RENAME-08's assertion is NOT re-runnable. Its second clause, test -z "$(find src -name '*.pyc')", is false today - 32 .pyc files exist, regenerated by every test run since. That clause was a one-time migration cleanup check, not an ongoing invariant, and MANIFEST.in's global-exclude *.pyc is what keeps bytecode out of the artefact that matters. Marked with a warning glyph and explained, so a future re-run failing for a benign reason is not mistaken for a regression. The acceptance grep for the old namespace returns one hit, src/imio.googleauthenticator.egg-info/PKG-INFO. It is an untracked generated build artefact and both occurrences are the fork attribution CLAUDE.md requires be kept. Confirmed benign, recorded as one of the RESEARCH Section F false positives. Also corrects the feedback-latency box: ~11s at 50 tests, no longer under the 10s the planner measured at 8 tests. Still inside the sampling budget; the figure grew with the suite, not with any one test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The frontmatter parser does not strip trailing '#' comments, so wave_0_complete parsed as the string 'true # all six Wave 0 items landed during execution' rather than a boolean. audit-milestone 5.5 machine-parses that field, so the note has to live in the body, not the value. Phase 3's file checked and already clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both test_import_step_declares_registry_dependency and test_import_step_ordering labelled themselves REG-03. Per REQUIREMENTS.md the <depends name="plone.app.registry"/> declaration is REG-02 and the getSortedImportSteps() ordering is REG-03, so REG-02 had no test claiming it by id while REG-03 had two. Coverage was real throughout - only the labels were wrong. But an audit reading ids would have scored REG-02 as uncovered and REG-03 as doubly covered, which is exactly the miscount a traceability map exists to prevent. Docstring and both assertion messages relabelled; the docstring now also records why the declaration check is the real control and points at 02-VALIDATION.md's divergence note. Suite green at 50 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
State B - no VALIDATION.md was ever seeded for this phase. Reconstructed from both PLAN/SUMMARY pairs and cross-referenced against the live suite. All 6 requirements (REG-01..05, BUG-04) already had behavioural automated coverage: 0 gaps, no tests added. Phase 2 is the only one of the three built phases whose plans converted every invariant into an assertion rather than leaving a shell grep as the sole proof - the pattern that produced two gaps in phase 1 and two in phase 3. Each grep criterion in the plans has a test asserting its effect, including the runImportStepFromProfile absence grep, whose hazard is pinned directly by test_get_ska_secret_key_does_not_mutate_registry. No grep-for-absence test was added for it: that would pin a code shape while the behaviour it protects is already pinned. Records a requirement-text divergence. REG-03 states "the ordering assertion, not the rename, is the control". Phase 2 proved empirically that the ordering assertion is a tautology in this fixture - with the <depends> line deleted, imio.googleauthenticator still sorts after plone.app.registry (index 51 vs 36 of 52) by CPython 2.7 string-hash order - and added the recorded-dependency assertion as the real control. The behaviour is right; REG-03's sentence describes a control that does not control anything, and should be corrected when REQUIREMENTS.md is next revised. Second such divergence in the milestone after SEC-07; in both cases the implementation is the safer reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t phases Phases 1 and 2 were reconciled after the audit ran, so its Nyquist section, phase-status table and tech_debt entries were stale. Updated in place rather than re-running the whole audit: nothing else it measured changed. Nyquist goes partial -> compliant (3/3). The audit's own prediction held: the draft/missing files reflected unreconciled artefacts, not missing coverage. 28 of 31 requirements were already covered on entry, and all 4 real gaps were one shape - an invariant proved once by a shell command at execution time with nothing surviving into CI. Phase 2 is the exception that makes the pattern legible: the only phase whose plans converted every grep criterion into a behavioural assertion, and the only one needing no new tests. Status stays gaps_found. It is driven by the 40 unsatisfied requirements in phases 4-8, which validation work on phases 1-3 cannot change. Also records the second requirement-text divergence alongside SEC-07: REG-03 calls the getSortedImportSteps() ordering assertion "the control", but phase 2 proved it is a tautology in this fixture - the order holds by CPython 2.7 string-hash luck even with the <depends> line deleted. Both sentences should be corrected when REQUIREMENTS.md is next revised; in both cases the implementation is the safer reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Re-opened phase 3's UAT because three production-code fixes landed AFTER test 1 was marked pass (d8cda87, 3d97681, 01a8c04), each changing user-visible behaviour. One had since been confirmed by the reporter (the reset email arrived). The other two were machine-verified only - tests green, but nobody had looked at what the user now sees. Test 2 (G-03-2 / T-03-24): the token form now returns "Invalid token or token expired." on a manual visit with no signed URL, where it previously 500'd with TypeError: Incorrect secret. Confirmed in the browser. The helper test could not have closed this - it asserts validate_token returns False, not what reaches the page; a 500 from the z3c.form wrapper or the updateFields cookie-blanking path would have kept it green. Test 3 (G-03-1 / T-03-23): as the Zope root admin the setup form shows no QR and explains the account cannot use two-step verification, and submitting produces the refusal rather than "successfully enabled". The false assurance is gone from the page a person actually reads, which is the only place the defect existed. The scope half stays accepted as 03-SECURITY.md R-03-03. Test 1 deliberately NOT re-run. 01a8c04 touched user_setup.py, the form it exercises, but the guard returns early only for accounts absent from the site's acl_users, and the member path is machine-verified through the changed code: test_handleSubmit scenario 1 drives the real handler as a site member through the new guard, and test_is_site_local_user_distinguishes_a_root_account asserts a member classifies True. Re-scanning a QR would re-verify code paths no longer in question. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
phase.complete marked Phase 3 [x] in ROADMAP (checklist + Progress table) and advanced STATE to current_phase 4 (PAS Boundary, status planning). It left three fields stale, all misleading in the same direction - they read as "done": - stopped_at still said "phase 03 code-complete, ready for verification". Verification, security, validation and UAT have all since passed. - progress.total_phases was 3 against completed_phases 3, i.e. a full bar. The field is derived from the ROADMAP Progress table, which has 8 rows, so 8 is the correct value; 3 dated from when only three phase directories existed. - the human-readable bar read "6/6 plans authored (100%) - 2 of 8 roadmap phases complete": stale on both counts (9 plans now, 3 phases). The plans bar is left at 100% because it is arithmetically true - plans exist for every executed phase - but a note now says why that figure is not a completion signal: phases 4-8 have no plans at all. The phase count is the honest indicator and is now bolded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughPhase 3 is marked complete, with encrypted TOTP seed storage, local QR rendering, fail-closed key handling, startup diagnostics, reset-token hardening, site-local account guards, regression tests, dependency updates, and validation documentation added or recorded. ChangesPhase 3 implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SetupForm
participant helpers
participant MemberData
participant qrcode
User->>SetupForm: submit enrollment
SetupForm->>helpers: generate and store secret
helpers->>MemberData: store v1$ encrypted seed
SetupForm->>helpers: generate QR payload
helpers->>qrcode: render PNG
qrcode-->>helpers: return PNG bytes
helpers-->>SetupForm: return local data URI
SetupForm-->>User: display QR code
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The frontmatter said status: shipped / current_phase 4 while the Current Position block still read 'Current focus: Phase 03' and 'Ready to plan' with no mention of the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/imio/googleauthenticator/browser/forms/reset_bar_code.py (1)
145-184: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate
updateFields()withis_site_local_user()before rendering the QR code.get_token_description(user=user, overwrite_secret=False)still callsget_or_create_secret(), which creates and persists a new seed when none exists. Without the same guard ashandleSubmit(), a non-site-local account can be enrolled from the render path.src/imio/googleauthenticator/browser/forms/reset_bar_code.py:169-171🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/googleauthenticator/browser/forms/reset_bar_code.py` around lines 145 - 184, The QR-code rendering branch in updateFields must verify is_site_local_user() before calling get_token_description(). Add the same site-local user guard used by handleSubmit() around the description update, while preserving the existing validation and error-message behavior for other cases.
🧹 Nitpick comments (3)
src/imio/googleauthenticator/subscribers.py (1)
13-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBoot diagnostic only catches a missing key, not a malformed one.
on_process_startingchecksnot get_encryption_key(), so a present-but-invalid key (wrong length, bad base64) stays silent at boot and only surfaces as aValueErroron the first real enrollment/login — the exact "not fail visibly" scenario README.rst's troubleshooting section is meant to prevent._get_fernet()already validates format; reusing it here would close this gap cheaply.♻️ Proposed refactor
-from imio.googleauthenticator.helpers import get_encryption_key +from imio.googleauthenticator.helpers import _get_fernet def on_process_starting(event): - if not get_encryption_key(): - logger.critical( - 'IMIO_GOOGLEAUTHENTICATOR_SEED_KEY is not set; seed encryption ' - 'and decryption will fail closed on every enrollment and login ' - 'attempt until it is set.') + try: + _get_fernet() + except ValueError as e: + logger.critical( + '%s Seed encryption and decryption will fail closed on every ' + 'enrollment and login attempt until it is fixed.', e)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/googleauthenticator/subscribers.py` around lines 13 - 30, Update on_process_starting to validate the configured encryption key using the existing _get_fernet() validation path, rather than only checking whether get_encryption_key() is falsy. Log the same critical startup diagnostic when the key is missing or malformed, while preserving the handler’s non-raising behavior..planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEscape the pipe inside the regex alternation to fix the table column count.
The
(==|!=)alternation inside`grep -rn -E "bar_code_reset_token *(==|!=)" src/`contains a literal|that markdownlint's table parser (MD056) counts as an extra column separator even inside the code span, producing 5 columns instead of the table's 4.🔧 Proposed fix
-| 11 | BUG-03: bar-code reset token comparison is constant-time via one shared helper at **both** call sites; refuses empty/absent tokens; no `TypeError` across `str`/`unicode` | ✓ VERIFIED | `helpers.py` — `validate_bar_code_reset_token` (falsy-refuse, `.encode('ascii')` coercion inside `try`/`except UnicodeEncodeError`, `compare_digest`). `reset_bar_code.py` — both `handleSubmit` (line ~104) and `updateFields` (line ~154) call the helper; `grep -rn -E "bar_code_reset_token *(==|!=)" src/` (excluding tests) → no matches. | +| 11 | BUG-03: bar-code reset token comparison is constant-time via one shared helper at **both** call sites; refuses empty/absent tokens; no `TypeError` across `str`/`unicode` | ✓ VERIFIED | `helpers.py` — `validate_bar_code_reset_token` (falsy-refuse, `.encode('ascii')` coercion inside `try`/`except UnicodeEncodeError`, `compare_digest`). `reset_bar_code.py` — both `handleSubmit` (line ~104) and `updateFields` (line ~154) call the helper; `grep -rn -E "bar_code_reset_token *(==\|!=)" src/` (excluding tests) → no matches. |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md at line 46, Update the inline grep regex in the verification table entry for BUG-03 by escaping the alternation pipe inside the code span, so Markdown treats it as literal regex content rather than a table separator. Preserve the command’s matching behavior and the existing four-column table structure.Source: Linters/SAST tools
src/imio/googleauthenticator/tests/test_helpers.py (1)
233-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated seed-key setUp/tearDown across test classes. Both sites re-implement the identical six-line "save previous
IMIO_GOOGLEAUTHENTICATOR_SEED_KEY, generate a fresh Fernet key, restore on teardown" fixture; graph context shows the same block also duplicated intest_user_setup.pyandtest_generic.py. A sharedSeedKeyTestMixin(or aBaseTesthelper) would remove the duplication project-wide.
src/imio/googleauthenticator/tests/test_helpers.py#L233-L253: extract this setUp/tearDown pair into a shared mixin/base method and haveTestSeedEncryptionuse it.src/imio/googleauthenticator/tests/test_pas_plugin.py#L39-L47: replace this identical setUp/tearDown pair with the same shared mixin/base method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/googleauthenticator/tests/test_helpers.py` around lines 233 - 253, The seed-key environment setup and restoration is duplicated across tests. In src/imio/googleauthenticator/tests/test_helpers.py:233-253, extract the shared save/generate/restore behavior into a reusable SeedKeyTestMixin or existing base helper and make TestSeedEncryption use it; apply the same replacement in src/imio/googleauthenticator/tests/test_pas_plugin.py:39-47, preserving the existing Fernet key generation and environment restoration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/phases/01-rename-and-fail-closed/01-VALIDATION.md:
- Around line 187-188: Synchronize the sampling latency budget and estimate in
the validation document so the earlier approximately 7-second figure and the
recorded approximately 11-second result use one authoritative threshold. Update
the relevant budget or estimate, or revise the sign-off wording, while
preserving the statement that the measured suite remains within the agreed
budget.
- Around line 72-94: Update the RENAME-08 status and audit sign-off in the
validation document to distinguish the passing durable namespace assertion from
the failed recorded .pyc cleanup clause. Do not claim all rows or commands were
re-run green; explicitly preserve the ⚠️/failed-clause qualification while
noting the durable requirement remains satisfied.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-VALIDATION.md:
- Around line 98-113: Update the REG-03 requirement in REQUIREMENTS.md to
identify the recorded-dependency assertion from REG-02 as the control, with
sorted import order described as its observable effect. Preserve the existing
requirement intent and retain the divergence note in 02-VALIDATION.md as
historical context.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md:
- Around line 1086-1092: Repair the flagged-assumptions Markdown table by
ensuring the header, separator, and every data row have matching leading and
trailing pipes and exactly three cells: Requirement, Probe row, Assumption
taken, and Consequence if wrong. Correct the malformed trailing rows without
changing their content.
- Around line 538-544: Update the acceptance command in
.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md lines 538-544 to
use the working buildout interpreter, parts/instance/bin/interpreter, instead of
bin/python. In .planning/phases/03-encrypted-seeds-and-local-qr/03-01-SUMMARY.md
lines 213-216, retain the observed bin/python limitation and report the
corrected interpreter command as satisfying the criterion.
- Around line 153-189: Synchronize the locked environment-variable name across
all planning references: in
.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md lines 153-189,
replace IMIO_GA_SEED_KEY in the decision, requirements, and downstream
references with IMIO_GOOGLEAUTHENTICATOR_SEED_KEY; make the same replacement in
.planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md lines 17-26 for
SEC-07/SEC-08 and lines 269-271 for the env -u boot verification command.
- Around line 769-770: Update the second grep criterion in the plan to use the
repository-relative path src/imio/googleauthenticator/tests/test_pas_plugin.py
instead of tests/test_pas_plugin.py, while preserving its existing count
requirement.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md:
- Around line 551-562: The flagged-assumptions table in the plan has malformed
final rows with inconsistent delimiters and column counts. Update the rows
beginning with “One non-probe assumption added during the review-driven
revision” and “| Assumption | Why | Consequence if wrong |” so they use the same
three-column, leading-and-trailing-pipe format as the table header and existing
rows, preserving all content.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-03-PLAN.md:
- Line 295: Update the CHANGES.rst reference in the plan text to keep only the
filename inside backticks, moving the line-range description outside the code
span while preserving the existing reference.
- Around line 47-50: Replace every Task 2 reference to IMIO_GA_SEED_KEY in this
plan, including the CHANGES.rst artifact declaration and related instructions,
with the canonical IMIO_GOOGLEAUTHENTICATOR_SEED_KEY name; preserve the existing
task scope and fail-closed behavior.
- Around line 521-525: Add a blank line immediately after the final Markdown
table row in the flagged_assumptions section, before the closing
</flagged_assumptions> tag, so markdownlint recognizes the table boundary
correctly.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-PATTERNS.md:
- Around line 181-210: Synchronize the BUG-05 call-site inventory in both
planning records: update 03-PATTERNS.md to document and verify the private-hop
parsing loop’s third ipaddress call alongside extract_ip_address_from_request
and get_ip_ranges, and update 03-RESEARCH.md with the corresponding rationale
and example. Keep the documented Unicode coercion and existing fail-closed
try/except behavior consistent across all three sites.
- Around line 70-89: Update the encryption-key references in
.planning/phases/03-encrypted-seeds-and-local-qr/03-PATTERNS.md lines 70-89 and
.planning/phases/03-encrypted-seeds-and-local-qr/03-RESEARCH.md lines 557-568,
replacing IMIO_GA_SEED_KEY with IMIO_GOOGLEAUTHENTICATOR_SEED_KEY in the Fernet
examples, research code, and assumptions.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-SECURITY.md:
- Around line 115-120: Update the security-register audit trail row and the
origin summary in 03-SECURITY.md to reflect 30 total rows, 29 closed, and 1
open; change the threat-source counts from 25 planned plus 4 UAT findings to 26
planned plus 4 UAT findings, without altering other register content.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-UAT.md:
- Around line 158-160: Update the UAT summary text in the phase outcome section
to state that all three tests passed instead of claiming a single test passed,
while keeping the existing issues: 0 tally and gap details unchanged.
In @.planning/REQUIREMENTS.md:
- Around line 41-44: Rewrite SEC-07 to clarify ownership: state that the
production [instance] value is configured by the separate Puppet deployment
repository, while CI inherits the key through [testenv]. Do not describe the
requirement as four in-repository file edits; retain the requirement that the
environment variable is documented and available in all required runtime
contexts.
In @.planning/STATE.md:
- Around line 137-138: Update the “Stopped at” marker in the session continuity
section of STATE.md to align with the shipped Phase 3 status and Phase 4
planning readiness, or explicitly identify the existing Phase 3 verification
text as historical session context; keep the surrounding header and status
information consistent.
In `@src/imio/googleauthenticator/browser/controlpanel.py`:
- Around line 109-139: When enable_two_factor_authentication_for_users raises
ValueError in the globally_enabled branch, prevent self.applyChanges(data) from
persisting globally_enabled=True; preserve the existing error status and ensure
the saved configuration leaves global enforcement disabled after total
enrollment failure. Update the handleSave flow around enrollment_failed and
applyChanges, without changing successful enrollment behavior.
In `@src/imio/googleauthenticator/tests/test_generic.py`:
- Around line 169-172: Decode the byte strings returned by proc.communicate() in
the setup.py metadata test before performing the existing assertIn checks.
Update the stdout and stderr handling around subprocess.Popen so the assertions
compare text values while preserving the current metadata validation.
---
Outside diff comments:
In `@src/imio/googleauthenticator/browser/forms/reset_bar_code.py`:
- Around line 145-184: The QR-code rendering branch in updateFields must verify
is_site_local_user() before calling get_token_description(). Add the same
site-local user guard used by handleSubmit() around the description update,
while preserving the existing validation and error-message behavior for other
cases.
---
Nitpick comments:
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md:
- Line 46: Update the inline grep regex in the verification table entry for
BUG-03 by escaping the alternation pipe inside the code span, so Markdown treats
it as literal regex content rather than a table separator. Preserve the
command’s matching behavior and the existing four-column table structure.
In `@src/imio/googleauthenticator/subscribers.py`:
- Around line 13-30: Update on_process_starting to validate the configured
encryption key using the existing _get_fernet() validation path, rather than
only checking whether get_encryption_key() is falsy. Log the same critical
startup diagnostic when the key is missing or malformed, while preserving the
handler’s non-raising behavior.
In `@src/imio/googleauthenticator/tests/test_helpers.py`:
- Around line 233-253: The seed-key environment setup and restoration is
duplicated across tests. In
src/imio/googleauthenticator/tests/test_helpers.py:233-253, extract the shared
save/generate/restore behavior into a reusable SeedKeyTestMixin or existing base
helper and make TestSeedEncryption use it; apply the same replacement in
src/imio/googleauthenticator/tests/test_pas_plugin.py:39-47, preserving the
existing Fernet key generation and environment restoration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7124e54e-39de-48fb-ace0-596b54d0e5da
📒 Files selected for processing (41)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/01-rename-and-fail-closed/01-VALIDATION.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-VALIDATION.md.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md.planning/phases/03-encrypted-seeds-and-local-qr/03-01-SUMMARY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md.planning/phases/03-encrypted-seeds-and-local-qr/03-02-SUMMARY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-03-PLAN.md.planning/phases/03-encrypted-seeds-and-local-qr/03-03-SUMMARY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-PATTERNS.md.planning/phases/03-encrypted-seeds-and-local-qr/03-RESEARCH.md.planning/phases/03-encrypted-seeds-and-local-qr/03-REVIEW.md.planning/phases/03-encrypted-seeds-and-local-qr/03-REVIEWS.md.planning/phases/03-encrypted-seeds-and-local-qr/03-SECURITY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-UAT.md.planning/phases/03-encrypted-seeds-and-local-qr/03-VALIDATION.md.planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md.planning/phases/03-encrypted-seeds-and-local-qr/COVERAGE.md.planning/v1.0-MILESTONE-AUDIT.mdCHANGES.rstREADME.rstbase.cfgsetup.pysrc/imio/googleauthenticator/browser/controlpanel.pysrc/imio/googleauthenticator/browser/enable_two_factor_authentication_for_all_users.pysrc/imio/googleauthenticator/browser/forms/request_bar_code_reset.pysrc/imio/googleauthenticator/browser/forms/reset_bar_code.pysrc/imio/googleauthenticator/browser/forms/user_setup.pysrc/imio/googleauthenticator/configure.zcmlsrc/imio/googleauthenticator/helpers.pysrc/imio/googleauthenticator/subscribers.pysrc/imio/googleauthenticator/tests/test_generic.pysrc/imio/googleauthenticator/tests/test_helpers.pysrc/imio/googleauthenticator/tests/test_pas_plugin.pysrc/imio/googleauthenticator/tests/test_request_bar_code_reset.pysrc/imio/googleauthenticator/tests/test_setuphandlers.pysrc/imio/googleauthenticator/tests/test_subscribers.pysrc/imio/googleauthenticator/tests/test_user_setup.pytest-4.3.cfg
Comment on lines
+98
to
+113
| **REG-03's premise is wrong, and phase 2 proved it empirically rather than complying with | ||
| it.** REG-03 reads: *"A test asserts `getSortedImportSteps()` places this package's step | ||
| after `plone.app.registry` — **the ordering assertion, not the rename, is the control**."* | ||
|
|
||
| Phase 2 found the ordering assertion is a **tautology in this fixture**: with the | ||
| `<depends>` line deleted, `imio.googleauthenticator` still sorts after `plone.app.registry` | ||
| (index 51 vs 36 of 52) purely by CPython 2.7 string-hash order. The assertion passes either | ||
| way and would not catch the deletion. So the phase added | ||
| `test_import_step_declares_registry_dependency`, which reads the recorded step metadata and | ||
| fails the moment the declaration goes — and kept the ordering test as the outcome check. | ||
|
|
||
| That was the right call. But it means REG-03's sentence describes a control that does not | ||
| control anything; the real control is REG-02's declaration. Recorded here rather than | ||
| silently ticked: **REG-03's wording should be corrected when `REQUIREMENTS.md` is next | ||
| revised**, to say the recorded-dependency assertion is the control and the sorted order is | ||
| its observable effect. The behaviour is right; the sentence is not. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct REG-03 in the requirements document before closing the phase.
This validation record says the current REG-03 wording describes a control that does not control anything, while the real control is REG-02’s dependency declaration. Leaving REQUIREMENTS.md unchanged makes future audits evaluate the wrong assertion. Update the requirement text and retain this divergence note as historical context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.planning/phases/02-registry-seeding-and-import-step-ordering/02-VALIDATION.md
around lines 98 - 113, Update the REG-03 requirement in REQUIREMENTS.md to
identify the recorded-dependency assertion from REG-02 as the control, with
sorted import order described as its observable effect. Preserve the existing
requirement intent and retain the divergence note in 02-VALIDATION.md as
historical context.
Comment on lines
+153
to
+189
| <decision>The literal name of the environment variable that carries the Fernet key encrypting every | ||
| user's TOTP seed. The plan writes `IMIO_GA_SEED_KEY` into `helpers.py` (Task 3), and plan 03-02 | ||
| writes the same literal into `base.cfg` `[instance]` and `[testenv]`, into `README.rst`, and into the | ||
| Puppet ticket description.</decision> | ||
|
|
||
| <context>This is a one-way door, which is why it is gated rather than assumed. The same literal has | ||
| to be filed as a `concat::fragment` against the separate `industrialisation` repo | ||
| (`modules/plone/manifests/buildout.pp`), which is not one of this roadmap's commits. Before that | ||
| ticket exists, a rename is a find-and-replace across four files. After it exists, a rename is a | ||
| second cross-repo coordination with a window in which one ZEO client reads the old name, finds | ||
| nothing, and fails every login closed. 03-RESEARCH.md's Assumptions Log A1 flags the name as | ||
| `[ASSUMED]` and says explicitly that it must be locked before the Puppet-side ticket is filed. | ||
|
|
||
| Nothing about the code depends on the choice — it is one string constant, `helpers.ENV_VAR_NAME`. | ||
| Only the coordination cost is asymmetric.</context> | ||
|
|
||
| <options> | ||
| <option id="imio-ga-seed-key"> | ||
| <name>IMIO_GA_SEED_KEY</name> | ||
| <pros>Short; namespaced to iMio like the `SSO_APPS_CLIENT_SECRET` precedent it follows in the | ||
| same Puppet module; `GA` matches the package's own short name in every existing log line and | ||
| profile id.</pros> | ||
| <cons>`GA` is ambiguous to a reader who has not seen this package before — it reads as | ||
| "Google Analytics" at least as readily as "Google Authenticator".</cons> | ||
| </option> | ||
| <option id="imio-googleauthenticator-seed-key"> | ||
| <name>IMIO_GOOGLEAUTHENTICATOR_SEED_KEY</name> | ||
| <pros>Unambiguous; matches the distribution name exactly, so grepping the Puppet repo for the | ||
| package finds the fragment.</pros> | ||
| <cons>33 characters in a buildout `environment-vars` line and in every deployment manifest; | ||
| longer than any other variable in the sibling `server.dmsmail` deployment.</cons> | ||
| </option> | ||
| </options> | ||
|
|
||
| <resume-signal>Select: imio-ga-seed-key, imio-googleauthenticator-seed-key, or give a different | ||
| name. `imio-ga-seed-key` is what every task below is written against; choosing otherwise means | ||
| substituting your name wherever `IMIO_GA_SEED_KEY` appears in this plan and in 03-02.</resume-signal> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize all planning documents with the locked environment-variable name.
The executed summaries use IMIO_GOOGLEAUTHENTICATOR_SEED_KEY, but both plans still instruct readers to use IMIO_GA_SEED_KEY.
.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md#L153-L189: update the decision, requirements, and downstream references..planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md#L17-L26: update the SEC-07/SEC-08 requirements..planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md#L269-L271: update theenv -uboot verification command.
📍 Affects 2 files
.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md#L153-L189(this comment).planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md#L17-L26.planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md#L269-L271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md around lines
153 - 189, Synchronize the locked environment-variable name across all planning
references: in .planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md
lines 153-189, replace IMIO_GA_SEED_KEY in the decision, requirements, and
downstream references with IMIO_GOOGLEAUTHENTICATOR_SEED_KEY; make the same
replacement in .planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md
lines 17-26 for SEC-07/SEC-08 and lines 269-271 for the env -u boot verification
command.
Comment on lines
+538
to
+544
| <acceptance_criteria> | ||
| - `make buildout` exits 0 and `bin/python -c "import cryptography, qrcode, ipaddress, PIL; print(cryptography.__version__)"` prints `3.3.2`. | ||
| - `bin/test -t test_seed_encryption_round_trip` exits 0. | ||
| - `bin/test -t '!robot'` exits 0 — the whole suite, including `test_generic.py`'s `test_user_setup_view` (which step (b2) is what makes reachable) and all seven pre-existing `TestIPWhitelisting` tests, which are the BUG-05 adjacency/empty/ordering proof and must pass **unaltered**. | ||
| - `grep -c "IMIO_GA_SEED_KEY" base.cfg` returns exactly 1 — the `[testenv]` entry only. `[instance]` deliberately does **not** declare it; plan 03-02 records why and documents where the deployment supplies it. | ||
| - `grep -c "IMIO_GA_SEED_KEY" bin/test` returns 1 or more — the `[testenv]` entry survived buildout's generation step, which is the only proof the form is right. | ||
| - `bin/python -c "import base64, os; k=[l.split('=',1)[1].strip() for l in open('base.cfg') if l.strip().startswith('IMIO_GA_SEED_KEY')][0]; assert len(base64.urlsafe_b64decode(k)) == 32; print('ok')"` prints `ok` — the `[testenv]` value is a genuinely valid 32-byte Fernet key, not a placeholder. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the interpreter used by verification consistent.
The plan requires bin/python, but the summary records that this interpreter lacks buildout eggs and that parts/instance/bin/interpreter was used instead.
.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md#L538-L544: change the acceptance command to the working buildout interpreter..planning/phases/03-encrypted-seeds-and-local-qr/03-01-SUMMARY.md#L213-L216: retain the observed limitation and report the corrected command as the satisfied criterion.
📍 Affects 2 files
.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md#L538-L544(this comment).planning/phases/03-encrypted-seeds-and-local-qr/03-01-SUMMARY.md#L213-L216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md around lines
538 - 544, Update the acceptance command in
.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md lines 538-544 to
use the working buildout interpreter, parts/instance/bin/interpreter, instead of
bin/python. In .planning/phases/03-encrypted-seeds-and-local-qr/03-01-SUMMARY.md
lines 213-216, retain the observed bin/python limitation and report the
corrected interpreter command as satisfying the criterion.
Comment on lines
+158
to
+160
| The single test passes, so `issues: 0` is accurate as a UAT tally. But three defects | ||
| were found along the way and are recorded in `## Gaps` below. **None is a phase-3 | ||
| regression** — all three are pre-existing, and none is a phase-3 deliverable: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the UAT outcome count.
The summary records three passing tests, not one. Replace “The single test passes” with “All three tests passed.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-UAT.md around lines 158
- 160, Update the UAT summary text in the phase outcome section to state that
all three tests passed instead of claiming a single test passed, while keeping
the existing issues: 0 tally and gap details unchanged.
Comment on lines
+41
to
+44
| - [x] **SEC-05**: The enrollment QR code is rendered in-process by `qrcode == 6.1`; the seed is transmitted to no external service and appears in no subprocess argv | ||
| - [x] **SEC-06**: New seeds are 160 bits of `os.urandom`, satisfying RFC 4226 §4 R6's 128-bit minimum | ||
| - [x] **SEC-07**: The required environment variable is documented and present in all four places it must exist — `[instance]`, `[testenv]`, the CI workflow, and (out of repo) the Puppet fragment | ||
| - [x] **SEC-08**: A missing key logs CRITICAL at process start rather than raising from module import or ZCML |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clarify SEC-07’s configuration ownership.
This says the key is “present” in [instance] and the CI workflow, but the phase records state that the production [instance] value is supplied by the separate Puppet repository and CI inherits it through [testenv]. Rewrite SEC-07 to distinguish “configured by deployment” from “inherited by CI”; otherwise this can be incorrectly audited as a four-file edit in this repository.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/REQUIREMENTS.md around lines 41 - 44, Rewrite SEC-07 to clarify
ownership: state that the production [instance] value is configured by the
separate Puppet deployment repository, while CI inherits the key through
[testenv]. Do not describe the requirement as four in-repository file edits;
retain the requirement that the environment variable is documented and available
in all required runtime contexts.
Comment on lines
+109
to
+139
| enrollment_failed = False | ||
| if globally_enabled is True: | ||
| # Enable for all users | ||
| users = api.user.get_users() | ||
| enable_two_factor_authentication_for_users(users) | ||
| logger.debug('Enabled') | ||
| try: | ||
| enable_two_factor_authentication_for_users(users) | ||
| logger.debug('Enabled') | ||
| except ValueError: | ||
| # Not a fail-closed violation of the crypto layer's | ||
| # no-fallback prohibition: this handler enrols nobody, | ||
| # grants no session and stores no plaintext. Refusing | ||
| # loudly in the UI *is* the closed state -- the alternative | ||
| # is "Changes saved." with zero users enrolled, which is | ||
| # the silent security-control removal this task exists to | ||
| # close. | ||
| enrollment_failed = True | ||
| IStatusMessage(self.request).addStatusMessage( | ||
| _(u"Two-step verification could not be enabled for any " | ||
| u"user: seed encryption is unavailable. Set the " | ||
| u"IMIO_GOOGLEAUTHENTICATOR_SEED_KEY environment " | ||
| u"variable and try again."), | ||
| "error") | ||
| elif globally_enabled is False: | ||
| # Disable for all users | ||
| users = api.user.get_users() | ||
| #disable_two_factor_authentication_for_users(users) | ||
| logger.debug('Disabled') | ||
|
|
||
| changes = self.applyChanges(data) | ||
| IStatusMessage(self.request).addStatusMessage(_(u"Changes saved."), "info") | ||
| if not enrollment_failed: | ||
| IStatusMessage(self.request).addStatusMessage(_(u"Changes saved."), "info") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Failed bulk enrollment still persists globally_enabled = True, risking a site-wide login lockout.
When enable_two_factor_authentication_for_users raises ValueError, enrollment_failed only gates the status message — data (still containing globally_enabled: True) is passed to self.applyChanges(data) unconditionally on Line 137. This persists the "force 2FA for everyone" registry setting even though zero users were actually enrolled (the re-raise in enable_two_factor_authentication_for_users means the failure is total, not per-user). The result: every login site-wide could be forced through a token check that no user — existing or new — can pass, until the key is fixed. Previously (before this try/except was added) the unhandled exception aborted handleSave before reaching applyChanges, so this is a new regression introduced by the fail-closed UX improvement.
🔒 Proposed fix: don't persist "globally enabled" when enrollment failed
except ValueError:
enrollment_failed = True
+ # Never leave the site demanding a second factor that no
+ # user was actually enrolled for.
+ data['globally_enabled'] = False
IStatusMessage(self.request).addStatusMessage(
_(u"Two-step verification could not be enabled for any "
u"user: seed encryption is unavailable. Set the "
u"IMIO_GOOGLEAUTHENTICATOR_SEED_KEY environment "
u"variable and try again."),
"error")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| enrollment_failed = False | |
| if globally_enabled is True: | |
| # Enable for all users | |
| users = api.user.get_users() | |
| enable_two_factor_authentication_for_users(users) | |
| logger.debug('Enabled') | |
| try: | |
| enable_two_factor_authentication_for_users(users) | |
| logger.debug('Enabled') | |
| except ValueError: | |
| # Not a fail-closed violation of the crypto layer's | |
| # no-fallback prohibition: this handler enrols nobody, | |
| # grants no session and stores no plaintext. Refusing | |
| # loudly in the UI *is* the closed state -- the alternative | |
| # is "Changes saved." with zero users enrolled, which is | |
| # the silent security-control removal this task exists to | |
| # close. | |
| enrollment_failed = True | |
| IStatusMessage(self.request).addStatusMessage( | |
| _(u"Two-step verification could not be enabled for any " | |
| u"user: seed encryption is unavailable. Set the " | |
| u"IMIO_GOOGLEAUTHENTICATOR_SEED_KEY environment " | |
| u"variable and try again."), | |
| "error") | |
| elif globally_enabled is False: | |
| # Disable for all users | |
| users = api.user.get_users() | |
| #disable_two_factor_authentication_for_users(users) | |
| logger.debug('Disabled') | |
| changes = self.applyChanges(data) | |
| IStatusMessage(self.request).addStatusMessage(_(u"Changes saved."), "info") | |
| if not enrollment_failed: | |
| IStatusMessage(self.request).addStatusMessage(_(u"Changes saved."), "info") | |
| enrollment_failed = False | |
| if globally_enabled is True: | |
| # Enable for all users | |
| users = api.user.get_users() | |
| try: | |
| enable_two_factor_authentication_for_users(users) | |
| logger.debug('Enabled') | |
| except ValueError: | |
| # Not a fail-closed violation of the crypto layer's | |
| # no-fallback prohibition: this handler enrols nobody, | |
| # grants no session and stores no plaintext. Refusing | |
| # loudly in the UI *is* the closed state -- the alternative | |
| # is "Changes saved." with zero users enrolled, which is the | |
| # silent security-control removal this task exists to | |
| # close. | |
| enrollment_failed = True | |
| # Never leave the site demanding a second factor that no | |
| # user was actually enrolled for. | |
| data['globally_enabled'] = False | |
| IStatusMessage(self.request).addStatusMessage( | |
| _(u"Two-step verification could not be enabled for any " | |
| u"user: seed encryption is unavailable. Set the " | |
| u"IMIO_GOOGLEAUTHENTICATOR_SEED_KEY environment " | |
| u"variable and try again."), | |
| "error") | |
| elif globally_enabled is False: | |
| # Disable for all users | |
| users = api.user.get_users() | |
| `#disable_two_factor_authentication_for_users`(users) | |
| logger.debug('Disabled') | |
| changes = self.applyChanges(data) | |
| if not enrollment_failed: | |
| IStatusMessage(self.request).addStatusMessage(_(u"Changes saved."), "info") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/googleauthenticator/browser/controlpanel.py` around lines 109 - 139,
When enable_two_factor_authentication_for_users raises ValueError in the
globally_enabled branch, prevent self.applyChanges(data) from persisting
globally_enabled=True; preserve the existing error status and ensure the saved
configuration leaves global enforcement disabled after total enrollment failure.
Update the handleSave flow around enrollment_failed and applyChanges, without
changing successful enrollment behavior.
Comment on lines
+169
to
+172
| proc = subprocess.Popen( | ||
| [sys.executable, 'setup.py', '--long-description'], | ||
| cwd=root, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| stdout, stderr = proc.communicate() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect declared test/runtime configuration without executing repository code.
rg -n -i -C3 \
--glob 'base.cfg' \
--glob 'test-4.3.cfg' \
--glob 'setup.py' \
'(python|interpreter|versions|buildout)' .Repository: IMIO/imio.googleauthenticator
Length of output: 5317
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the relevant test function around the asserted lines.
FILE=src/imio/googleauthenticator/tests/test_generic.py
wc -l "$FILE"
sed -n '150,200p' "$FILE"Repository: IMIO/imio.googleauthenticator
Length of output: 2589
Decode stdout/stderr before the string assertions in src/imio/googleauthenticator/tests/test_generic.py. Without decoding, the assertIn checks on Python 3 raise TypeError instead of exercising the metadata check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/googleauthenticator/tests/test_generic.py` around lines 169 - 172,
Decode the byte strings returned by proc.communicate() in the setup.py metadata
test before performing the existing assertIn checks. Update the stdout and
stderr handling around subprocess.Popen so the assertions compare text values
while preserving the current metadata validation.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md:
- Line 775: In the `test_encryption_key_is_read_per_call` checklist text, remove
the trailing space inside the inline `grep` command’s code span while preserving
the command and its surrounding verification instructions.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-02-SUMMARY.md:
- Around line 130-133: Update the deviation text in the summary to remove nested
or space-padded inline-code formatting while preserving its wording and
verification details. Change the log-output fenced block to use an explicit
text-appropriate language label, keeping the output content unchanged; apply
both fixes to the referenced deviation sections.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-PATTERNS.md:
- Around line 388-390: Update the QR data-URI test contract in the “QR data-URI
test” guidance to expect the exact prefix `data:image/png;base64,` without a
space after the semicolon. Keep the assertion against the real
get_barcode_image(...) output and retain the check that the result does not
contain `googleapis.com`.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-RESEARCH.md:
- Around line 206-223: The “Recommended Project Structure” section contradicts
itself about new files. Update the opening statement to acknowledge that
subscribers.py and the startup-diagnostic test file are required, while
preserving the listed structure and their SEC-08/test responsibilities.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md:
- Line 46: Update the Markdown table entry for BUG-03 to escape the pipe in the
inline regex `(==|!=)` or move the command into a code block, ensuring the table
remains correctly parsed with no extra cell.
- Around line 1-7: Update the frontmatter in 03-VERIFICATION.md so its status
and completion metadata reflect the report body’s human_needed state and
deferred real-authenticator step, rather than marking the phase as fully passed.
Preserve the existing score and other accurate metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed3ec0be-00a8-4b73-a0cc-dde4bc181a52
📒 Files selected for processing (41)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/01-rename-and-fail-closed/01-VALIDATION.md.planning/phases/02-registry-seeding-and-import-step-ordering/02-VALIDATION.md.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md.planning/phases/03-encrypted-seeds-and-local-qr/03-01-SUMMARY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-02-PLAN.md.planning/phases/03-encrypted-seeds-and-local-qr/03-02-SUMMARY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-03-PLAN.md.planning/phases/03-encrypted-seeds-and-local-qr/03-03-SUMMARY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-PATTERNS.md.planning/phases/03-encrypted-seeds-and-local-qr/03-RESEARCH.md.planning/phases/03-encrypted-seeds-and-local-qr/03-REVIEW.md.planning/phases/03-encrypted-seeds-and-local-qr/03-REVIEWS.md.planning/phases/03-encrypted-seeds-and-local-qr/03-SECURITY.md.planning/phases/03-encrypted-seeds-and-local-qr/03-UAT.md.planning/phases/03-encrypted-seeds-and-local-qr/03-VALIDATION.md.planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md.planning/phases/03-encrypted-seeds-and-local-qr/COVERAGE.md.planning/v1.0-MILESTONE-AUDIT.mdCHANGES.rstREADME.rstbase.cfgsetup.pysrc/imio/googleauthenticator/browser/controlpanel.pysrc/imio/googleauthenticator/browser/enable_two_factor_authentication_for_all_users.pysrc/imio/googleauthenticator/browser/forms/request_bar_code_reset.pysrc/imio/googleauthenticator/browser/forms/reset_bar_code.pysrc/imio/googleauthenticator/browser/forms/user_setup.pysrc/imio/googleauthenticator/configure.zcmlsrc/imio/googleauthenticator/helpers.pysrc/imio/googleauthenticator/subscribers.pysrc/imio/googleauthenticator/tests/test_generic.pysrc/imio/googleauthenticator/tests/test_helpers.pysrc/imio/googleauthenticator/tests/test_pas_plugin.pysrc/imio/googleauthenticator/tests/test_request_bar_code_reset.pysrc/imio/googleauthenticator/tests/test_setuphandlers.pysrc/imio/googleauthenticator/tests/test_subscribers.pysrc/imio/googleauthenticator/tests/test_user_setup.pytest-4.3.cfg
🚧 Files skipped from review as they are similar to previous changes (22)
- src/imio/googleauthenticator/subscribers.py
- setup.py
- src/imio/googleauthenticator/tests/test_request_bar_code_reset.py
- .planning/phases/03-encrypted-seeds-and-local-qr/COVERAGE.md
- README.rst
- .planning/REQUIREMENTS.md
- src/imio/googleauthenticator/browser/forms/reset_bar_code.py
- src/imio/googleauthenticator/configure.zcml
- src/imio/googleauthenticator/tests/test_setuphandlers.py
- .planning/phases/03-encrypted-seeds-and-local-qr/03-SECURITY.md
- src/imio/googleauthenticator/browser/enable_two_factor_authentication_for_all_users.py
- .planning/phases/02-registry-seeding-and-import-step-ordering/02-VALIDATION.md
- .planning/ROADMAP.md
- src/imio/googleauthenticator/browser/forms/user_setup.py
- .planning/phases/01-rename-and-fail-closed/01-VALIDATION.md
- .planning/v1.0-MILESTONE-AUDIT.md
- src/imio/googleauthenticator/tests/test_user_setup.py
- src/imio/googleauthenticator/browser/controlpanel.py
- .planning/phases/03-encrypted-seeds-and-local-qr/03-03-SUMMARY.md
- src/imio/googleauthenticator/tests/test_pas_plugin.py
- src/imio/googleauthenticator/browser/forms/request_bar_code_reset.py
- CHANGES.rst
| - `grep -c "is_whitelisted_client" src/imio/googleauthenticator/tests/test_pas_plugin.py` is **unchanged** from `HEAD~1` — the new test binds the request instead of patching the whitelist check away, so the real `is_whitelisted_client` runs inside it. | ||
| - `test_login_is_refused_when_seed_key_is_broken` contains an assertion that the same `_extractUserIds` call does **not** raise with a valid key, textually before the two raising assertions. A reviewer can see the control. | ||
| - Both new `assertRaises` in the PAS test name `ValueError` explicitly, not `Exception` — a bare `Exception` would also pass on an unrelated `AttributeError` and prove nothing. | ||
| - `test_encryption_key_is_read_per_call` contains no rebinding of `get_encryption_key`: `grep -c "get_encryption_key" ` restricted to that method's body returns 0 (read the method to confirm). It must mutate `os.environ` only, or it proves nothing about per-call reads. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the malformed inline code span.
The command has a trailing space inside its code span. Rewrite it as a valid inline command, such as grep -c "get_encryption_key", so markdownlint does not report MD038.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 775-775: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-01-PLAN.md at line 775,
In the `test_encryption_key_is_read_per_call` checklist text, remove the
trailing space inside the inline `grep` command’s code span while preserving the
command and its surrounding verification instructions.
Source: Linters/SAST tools
Comment on lines
+130
to
+133
| **1. [Rule 1 - Bug] README.rst's "export ``VAR=...``" phrasing broke its own acceptance-criteria grep** | ||
| - **Found during:** Task 2, verifying acceptance criteria after the first README draft | ||
| - **Issue:** `grep -ci "export IMIO_GOOGLEAUTHENTICATOR_SEED_KEY" README.rst` returned `0`. The draft wrote `export ``IMIO_GOOGLEAUTHENTICATOR_SEED_KEY=<generated`` (double backticks interrupt the literal space-separated phrase the grep is looking for), and the value itself wrapped onto the next source line. | ||
| - **Fix:** Reworded to `run ``export IMIO_GOOGLEAUTHENTICATOR_SEED_KEY=<generated value>``` on a single unbroken clause so the literal phrase `export IMIO_GOOGLEAUTHENTICATOR_SEED_KEY` appears intact on one line. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the remaining Markdown lint violations.
Avoid nested/space-padded inline code in the deviation text, and label the log-output fence as text (or another appropriate language). This keeps the verification summary lint-clean without changing its content.
Also applies to: 148-160
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 133-133: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-02-SUMMARY.md around
lines 130 - 133, Update the deviation text in the summary to remove nested or
space-padded inline-code formatting while preserving its wording and
verification details. Change the log-output fenced block to use an explicit
text-appropriate language label, keeping the output content unchanged; apply
both fixes to the referenced deviation sections.
Source: Linters/SAST tools
Comment on lines
+388
to
+390
| **QR data-URI test:** assert `get_barcode_image(...)` return value `.startswith('data:image/png; | ||
| base64,')` and does **not** contain `'googleapis.com'` — same assertion-on-real-output style as | ||
| `TestIPWhitelisting`'s existing tests (no mocking of `qrcode` itself). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the correct data-URI prefix in the test contract.
The expected prefix contains an extra space: data:image/png; base64,. The implementation specified elsewhere returns data:image/png;base64,, so a test copied from this pattern will fail.
Proposed correction
-assert get_barcode_image(...).startswith('data:image/png; base64,')
+assert get_barcode_image(...).startswith('data:image/png;base64,')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **QR data-URI test:** assert `get_barcode_image(...)` return value `.startswith('data:image/png; | |
| base64,')` and does **not** contain `'googleapis.com'` — same assertion-on-real-output style as | |
| `TestIPWhitelisting`'s existing tests (no mocking of `qrcode` itself). | |
| **QR data-URI test:** assert `get_barcode_image(...)` return value `.startswith('data:image/png;base64,')` and does **not** contain `'googleapis.com'` — same assertion-on-real-output style as | |
| `TestIPWhitelisting`'s existing tests (no mocking of `qrcode` itself). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-PATTERNS.md around lines
388 - 390, Update the QR data-URI test contract in the “QR data-URI test”
guidance to expect the exact prefix `data:image/png;base64,` without a space
after the semicolon. Keep the assertion against the real get_barcode_image(...)
output and retain the check that the result does not contain `googleapis.com`.
Comment on lines
+206
to
+223
| ### Recommended Project Structure | ||
|
|
||
| No new files are required. All changes fit inside existing modules: | ||
|
|
||
| ``` | ||
| src/imio/googleauthenticator/ | ||
| ├── helpers.py # + get_encryption_key, _get_fernet, encrypt_seed, decrypt_seed | ||
| │ # generate_secret/get_secret/get_or_create_secret rewritten | ||
| │ # get_barcode_image rewritten (local qrcode, data URI) | ||
| │ # extract_ip_address_from_request / get_ip_ranges: ipaddress swap only | ||
| ├── subscribers.py # NEW: on_process_starting (SEC-08) | ||
| ├── configure.zcml # + <subscriber for="zope.processlifetime.IProcessStarting" .../> | ||
| ├── browser/forms/ | ||
| │ ├── user_setup.py # BUG-02 regression test only (see Common Pitfalls — no code bug found) | ||
| │ └── reset_bar_code.py # BUG-03: hmac.compare_digest with both sides encoded | ||
| └── tests/ | ||
| ├── test_helpers.py # encrypt/decrypt round-trip, fail-closed, v1$ prefix, seed entropy | ||
| └── test_setuphandlers.py or a new test_subscribers.py # IProcessStarting CRITICAL log |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the project-structure statement.
This section says “No new files are required” but immediately lists subscribers.py as NEW and proposes a new test_subscribers.py. State that these files are required, or remove them from the structure, so implementers do not omit the startup diagnostic and its test.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 210-210: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-RESEARCH.md around lines
206 - 223, The “Recommended Project Structure” section contradicts itself about
new files. Update the opening statement to acknowledge that subscribers.py and
the startup-diagnostic test file are required, while preserving the listed
structure and their SEC-08/test responsibilities.
Comment on lines
+1
to
+7
| --- | ||
| phase: 03-encrypted-seeds-and-local-qr | ||
| verified: 2026-07-30T15:00:00Z | ||
| status: passed | ||
| score: 20/21 must-haves verified | ||
| behavior_unverified: 0 | ||
| overrides_applied: 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md"
wc -l "$file"
printf '\n--- lines 1-40 ---\n'
sed -n '1,40p' "$file" | cat -n
printf '\n--- lines 140-190 ---\n'
sed -n '140,190p' "$file" | cat -n
printf '\n--- search status/human_needed/score/behavior_unverified/Puppet ---\n'
rg -n "status:|human_needed|20/21|behavior_unverified|Puppet|No gaps|real-authenticator|\|" "$file"Repository: IMIO/imio.googleauthenticator
Length of output: 25947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md"
printf '\n--- lines 40-55 with raw numbers ---\n'
nl -ba "$file" | sed -n '40,55p'
printf '\n--- lines 150-177 with raw numbers ---\n'
nl -ba "$file" | sed -n '150,177p'Repository: IMIO/imio.googleauthenticator
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md"
printf '\n--- lines 40-55 with numbers ---\n'
sed -n '40,55p' "$file" | cat -n
printf '\n--- lines 150-177 with numbers ---\n'
sed -n '150,177p' "$file" | cat -nRepository: IMIO/imio.googleauthenticator
Length of output: 8198
Sync the frontmatter with the report body
status: passed / behavior_unverified: 0 conflicts with the body’s Status: human_needed and the deferred real-authenticator step. Make the metadata match the report’s actual completion state so consumers don’t treat it as fully closed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md around
lines 1 - 7, Update the frontmatter in 03-VERIFICATION.md so its status and
completion metadata reflect the report body’s human_needed state and deferred
real-authenticator step, rather than marking the phase as fully passed. Preserve
the existing score and other accurate metadata.
| | 8 | SEC-08: missing key logs CRITICAL once at boot; never raises from import/ZCML/handler | ✓ VERIFIED | `subscribers.py` (30 lines) — `if not get_encryption_key(): logger.critical(...)`, no `raise`/`try`/`except` anywhere in the module (matches 03-02-SUMMARY's AST-walk evidence of 0 `Raise`/`TryExcept`/`TryFinally` nodes). Registered in `configure.zcml` for `zope.processlifetime.IProcessStarting`; file still parses as well-formed XML (confirmed by direct read). | | ||
| | 9 | BUG-05: `py2-ipaddress` replaced by `ipaddress==1.0.23`; unicode coercion at **all three** call sites (not the two originally assumed) | ✓ VERIFIED | `grep -c "py2-ipaddress" setup.py test-4.3.cfg` → 0/0; `setup.py`/`test-4.3.cfg` carry `ipaddress==1.0.23`/`ipaddress = 1.0.23`. Read `helpers.py:604-718` directly: `_to_unicode_ip()` helper (line 604), and all three call sites (`ip_address(_to_unicode_ip(proxies[0]))` line 645, `ip_address(_to_unicode_ip(ip))` line 666, `ip_network(_to_unicode_ip(net))` line 715) coerce before the call. | | ||
| | 10 | BUG-02: `redirect_url` bound on all three reachable branches of `SetupForm.handleSubmit`; empty token short-circuits before any redirect; closed by regression test with **no production code change** | ✓ VERIFIED | `git log --oneline HEAD~20..HEAD -- .../user_setup.py` → no commits in this phase touch the file; `git diff` against pre-phase HEAD is empty (confirmed by history). `tests/test_user_setup.py` (200 lines) exists with `test_handleSubmit` driving all four scenarios. | | ||
| | 11 | BUG-03: bar-code reset token comparison is constant-time via one shared helper at **both** call sites; refuses empty/absent tokens; no `TypeError` across `str`/`unicode` | ✓ VERIFIED | `helpers.py` — `validate_bar_code_reset_token` (falsy-refuse, `.encode('ascii')` coercion inside `try`/`except UnicodeEncodeError`, `compare_digest`). `reset_bar_code.py` — both `handleSubmit` (line ~104) and `updateFields` (line ~154) call the helper; `grep -rn -E "bar_code_reset_token *(==|!=)" src/` (excluding tests) → no matches. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the regex pipe in the Markdown table.
The | in (==|!=) is parsed as a column delimiter, producing an extra table cell. Escape it or move the command into a code block.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 46-46: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/03-encrypted-seeds-and-local-qr/03-VERIFICATION.md at line
46, Update the Markdown table entry for BUG-03 to escape the pipe in the inline
regex `(==|!=)` or move the command into a code block, ensuring the table
remains correctly parsed with no extra cell.
Source: Linters/SAST tools
Merged
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
1 participant
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
Phase 3: Encrypted Seeds and Local QR
Goal: A TOTP seed is unreadable from the ZODB, never transmitted to an external service, and never silently downgraded to plaintext — and adding
cryptographycannot break every login on the site throughipaddressmodule shadowing.Status: Verified ✓
TOTP seeds are now Fernet-encrypted at rest behind a
v1$<token>envelope, keyed fromIMIO_GOOGLEAUTHENTICATOR_SEED_KEYread fresh from the process environment on every call and never stored in the ZODB, a log line, or an exception message. The enrollment QR renders in-process viaqrcode == 6.1as adata:URI, removing the package's only outbound HTTP call — previously a GET tochart.googleapis.comcarrying the plaintext seed in the query string. Every path that touches the crypto layer fails closed: enrollment, login, bulk enable and account creation all refuse on a missing or invalid key rather than downgrading to plaintext or to password-only.py2-ipaddressis replaced byipaddress == 1.0.23withunicodecoercion at all three call sites, so addingcryptographycannot break every login on a Puppet-built host through module shadowing.Scope note
This PR carries 37 commits: 28 are phase-03, and 5 are retroactive Nyquist validation for phases 1–2 (
docs(01),fix(01),test(01),docs(02),test(02)) done in the same session. Those interleave chronologically with the phase-03 commits — the phase-1/2 validation work happened after phase 3's code landed — so they cannot be split into a separate PR without cherry-picking. They add 4 tests and change no production behaviour.Phase 2 merged separately as #2.
masteralready contains it; this branch was cut from the same line, which is why the branch it descends from carries a phase-2 name.Changes
Plan 03-01: Fernet envelope, local QR,
ipaddressswapThe ROADMAP's same-commit group — pin swap,
v1$envelope with a per-call key read, a 160-bitos.urandomseed via stdlib base32, in-process QR rendering, and fail-closed asserted at all four liveget_or_create_secretsurfaces.Key files:
helpers.py,browser/controlpanel.py,browser/enable_two_factor_authentication_for_all_users.py,setup.py,test-4.3.cfg,base.cfg,tests/test_helpers.py,tests/test_pas_plugin.pyAdded:
cryptography == 3.3.2,ipaddress == 1.0.23,qrcode == 6.1,cffi == 1.15.1(transitive),PillowPlan 03-02: Boot-time key diagnostics and deployment documentation
An
IProcessStartingsubscriber logs CRITICAL exactly once when the key is absent, naming the variable and stating the consequence, and never raises — a raise there would also breakbin/instance debugandbin/test, removing the tools needed to diagnose it.Key files:
subscribers.py(new),configure.zcml,README.rst,tests/test_subscribers.pyPlan 03-03: Constant-time reset-token comparison
One shared
validate_bar_code_reset_tokenusinghmac.compare_digest, with both operands pre-coerced to py2strso astr/unicodemismatch cannot raiseTypeError, and falsy operands refused before any comparison. Called at bothreset_bar_code.pysites — the render-time check and the submit-time check.Key files:
helpers.py,browser/forms/reset_bar_code.py,tests/test_helpers.py,tests/test_user_setup.py,CHANGES.rstPost-UAT fixes
Three defects surfaced during human UAT. None is a phase-3 regression — all three are pre-existing and none is a phase-3 deliverable — but they were found by driving the real flows, so they were fixed here rather than deferred.
d8cda87MailHost.send()was called with nocharset, so_mungeHeadersASCII-encoded a unicode body via_try_encode's baretext.encode()fallback. Passescharset='utf-8'3d97681@@google-authenticator-tokenreturned HTTP 500 (TypeError: Incorrect secret) whether or not the code was correctvalidate_tokenpassedget_secret's implicitNonetoonetimepass. Guarded invalidate_token— the one function all three callers route through, includingreset_bar_code.py, which carried the same unreported exposure. Deliberately narrow: a decryptionValueErrorstill propagates01a8c04@@setup-two-factor-authenticationenrolled a Zope-root account and reported "successfully enabled" for a login this plugin can never gatehelpers.is_site_local_userrefuses enrolment at both self-service claim sites before any flag write or seed mint; no QR is rendered either. Root logins remain ungated by design — see Key DecisionsRequirements Addressed
All 12 phase-3 requirements, verified and checked off in
REQUIREMENTS.md:v1$version prefixqrcode == 6.1; the seed reaches no external service and appears in no subprocess argvos.urandom, satisfying RFC 4226 §4 R6's 128-bit minimumredirect_urlis always bound on every code path throughuser_setup.pyTypeErroracrossstr/unicodepy2-ipaddressreplaced byipaddress == 1.0.23, withunicodecoercion at every call siteVerification
03-VERIFICATION.md—status: passedmake test; robot excluded, needs a real browser)03-SECURITY.md—threats_open: 0across 29 threats (28 closed, 1 accepted atlow)03-VALIDATION.md—nyquist_compliant: true, all 12 requirements with automated verification03-UAT.md— 3/3 passed, 0 issuesHuman UAT covered what no test can:
otpauth://label reads<username>@<domain>.Known evidence limits
MailBase._send, so it proves the message survives encoding and reaches MailHost, not that it leaves the host.concat::fragmentfrom the separateindustrialisationrepo (modules/plone/manifests/buildout.pp), which is not one of this repository's commits and has not shipped. The code is complete and tested without it;README.rstdocuments the dependency.Key Decisions
exceptin_get_fernet,encrypt_seed,decrypt_seed,get_secretorget_or_create_secretreturnsNone, the raw stored value, or any fallback. A caught failure would turn a loud 500 into a silent plaintext downgrade or a password-only login — the single mistake that would undo the whole phase.R-03-01).userCreatedHandlerruns on everyIPrincipalCreatedEventwithglobally_enableddefaulting on, so registration andplone.api.user.create()both stop. Enrolling a user with no recoverable second factor would be worse. Asserted by test and documented inREADME.rstrather than discovered in production.base.cfg[instance]deliberately declares no key (T-03-21b).environment-varsis whitespace-separatedNAME value; an option reference defaulting to empty emits a bare token and fails the buildout, and a literal placeholder would be worse — production would encrypt every seed under a value any reader of this repository can see, while suppressing the new CRITICAL log because the key would no longer be absent. The deployment buildout owns that copy.R-03-03). This plugin is registered in the Plone site'sacl_users, so a root account is authenticated above the site and cannot be intercepted: the password pre-check delegates to the site's otherIAuthenticationPlugins, none of which resolve a root account, so it declines to veto. Phase 1 locked this on 2026-07-29 (R-01, break-glass path) and Phase 4's DOC-01 documents it. What this PR fixes is only the false assurance — the forms no longer claim otherwise.skaderivation consumes the ciphertext as-is. The storedv1$<fernet-token>is URL-safe base64, so it remains ASCII by construction — closing02-SECURITY.mdR-02-02's "re-check, don't re-assume" flag without a decrypt step in the signing path.User Stories & Acceptance Criteria
Stakeholder Review & Approval
Summary by CodeRabbit