Feat security captcha contract#111
Conversation
dominikletica
added
enhancement
dominikletica
left a comment
dominikletica
left a comment
There was a problem hiding this comment.
Progress
- Continue after branch split
- Feature implementation branch.
- Checks passed:
bin/phpunit,bin/jstest,bin/lint. - Ready for local review.
dominikletica
left a comment
dominikletica
left a comment
There was a problem hiding this comment.
Progress
- Addressed local review findings
- Local review reported 12 issues: 11 resolved, 1 rejected
- Checks passed:
bin/phpunit,bin/jstest,bin/lint. - Ready for review.
- Review note: Rejected the read-only extension loader finding because runtime faulting is intentional health enforcement: any main request may move broken active extension code out of runtime through the central loader; Admin read paths must only use that shared boundary and add no separate read-only mutations.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a309e419bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dominikletica
left a comment
dominikletica
left a comment
There was a problem hiding this comment.
Progress
- Addressed review findings
- Local review reported 12 issues: 11 resolved, 1 rejected
- Codex Cloud Review reported 5 issues: 3 resolved, 1 rejected, 1 handled by explicit product/policy decision
- Checks passed:
bin/phpunit,bin/jstest,bin/lint. - Ready for review.
- Review note: Rejected verified-only captcha for ban recovery because captcha providers are optional and owners must not be locked out when no provider is active. Extension-local Composer/vendor support was removed by policy for early releases because package-version conflicts cannot be safely isolated across active extensions in one PHP runtime.
dominikletica
left a comment
dominikletica
left a comment
There was a problem hiding this comment.
Progress
- Addressed review findings
- Local review reported 16 issues: 13 resolved, 1 rejected, 2 handled by explicit product/policy decision.
- Codex Cloud Review reported 5 issues: 3 resolved, 1 rejected, 1 handled by explicit product/policy decision.
- Checks passed:
bin/phpunit,bin/jstest,bin/lint. - Review notes:
- Extension runtime helpers attribute the currently executing extension-owned code, not the upstream caller. Delegating to another extension's public callable is an intentional dependency/integration feature; the callee remains responsible for validating its public API inputs.
- Captcha providers are optional. In the no-provider state, recovery must remain possible and must not lock owners out. Captcha recovery requires the active provider when one exists, but accepts the documented skipped fallback when no provider is available.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62a6d10c21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return $this->reference($this->entityManager->getRepository(ContentItem::class)->findOneBy([ | ||
| 'slug' => $identifier, | ||
| 'status' => ContentStatus::Published, | ||
| ]), $referenceOptions); |
There was a problem hiding this comment.
When a site has two published content items with the same slug under different parents, which is allowed by the parent_uid, slug uniqueness model, this plain findOneBy() can return an arbitrary matching row. In that case extension_content_get('child') may expose fields for the wrong nested page or miss the intended root page; use the rooted content repository lookup or require callers to pass a path/parent for non-root content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Decision: support a nested parent/child hierarchic notation and return a contextual failure (distinguishable from "no result") when the string matches more than one entities.
Lookup-functions that list entities must also return the complete hierarchy for every content entity instead of just the sole slug.
| return $this->lookupContentRoute($identifier, $options); | ||
| } | ||
|
|
||
| return $this->contentReference($this->findOneBy(ContentItem::class, ['slug' => $identifier]), $options); |
There was a problem hiding this comment.
For extension_lookup('content', $slug), this fetches the first row with that slug even though slugs are only unique per parent. If a draft/private or nested item shares the slug with a visible routed page, the reference lookup can return null or the wrong content reference depending on database order; resolve root slugs through the content repository/path lookup or require an unambiguous path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Decision: Same as the previous issue - support nested parent/child notation and return a distinguishable failure when the string matches more than one entities.
| if (!$lock->acquire()) { | ||
| return null; |
There was a problem hiding this comment.
When two submissions reuse the same captcha instance, or the lock backend otherwise refuses this nonblocking acquire, this branch returns from inside the try while the finally still calls release() on a lock this request never acquired. Symfony locks can fail release when another owner still holds the resource, so the request guard can turn an expected failed captcha into a 500; track whether acquire() succeeded before releasing.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR is the second part of the security/extension branch split. The first part moved the large Packages -> Extensions rename and related groundwork out of the way; this branch focuses on the extension runtime/captcha contract layer.
It adds the generic extension runtime foundation needed for self-contained providers and future community extensions:
operations/scheduler-tasksscopesTesting
bin/phpunit: Passed locally.OK, Tests: 1993, Assertions: 12334.bin/jstest: Passed locally.37Node tests passed.bin/lint: Passed locally. Includes PHP syntax, container lint, Twig syntax, UX icon refs, YAML/translations, JS syntax, JSON syntax, Markdown parse, Tailwind build, translation source key checks, and Git diff whitespace.php bin/console lint:containerpassed./user/logininAPP_ENV=testpassed and confirmed login stays captcha-free. Dev render attempts depend on local DB state;/user/registerrender proof was limited by local SQLite/dev DB state, while Twig syntax is covered by full lint.Documentation
README.md) - not applicable for this slice; extension-facing behavior is documented in drafts/manuals instead.dev/draft/*.md)dev/CLASSMAP.md)dev/WORKLOG.md)dev/manual/*.md/docs/*.md) - dev manuals updated; user docs not applicable for this developer/runtime contract slice.Additional Checks
Linked Issues / Discussions
Part of the
feat-securitybranch tree (#102).Review Notes
extension_mail()is intentionally a development-stub facade until the real configurable mail workflow contract lands.brandingextensions are documented as a future isolated slice, not implemented here.