Extract admin page helpers from app.main#372
Conversation
|
Warning Review limit reached
More reviews will be available in 4 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors admin-page logic into reusable helper functions and expands tests around admin context building and bounty creation, while hardening redirect safety checks.
Changes:
- Centralized admin template context generation in
admin_page_context. - Added
create_admin_bounty_from_formhelper and updated admin POST handler to use it. - Hardened
_safe_next_pathby validating decoded redirect targets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_admin_helpers.py | Adds coverage for new admin helpers (admin_page_context, create_admin_bounty_from_form). |
| app/main.py | Uses admin_page_context/create_admin_bounty_from_form and strengthens _safe_next_path redirect validation. |
| app/admin.py | Introduces shared admin helpers and a shared webhook-limit options constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _safe_next_path(next_path: str | None) -> str: | ||
| decoded_next_path = unquote(next_path) if next_path else "" | ||
| if ( | ||
| not next_path | ||
| or not next_path.startswith("/") | ||
| or next_path.startswith("//") | ||
| or len(next_path) > 2048 | ||
| or "\\" in next_path | ||
| or decoded_next_path.startswith("//") | ||
| or "\\" in decoded_next_path | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in next_path) | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in decoded_next_path) | ||
| ): | ||
| return "/me" | ||
| return next_path |
| from app.ledger.service import create_bounty | ||
| from app.models import WebhookEvent | ||
|
|
||
| ADMIN_WEBHOOK_LIMIT_OPTIONS = [10, 25, 50, 100] |
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers from my review. I inspected app/admin.py, app/main.py, and tests/test_admin_helpers.py on head 9dea4845.
What I checked:
admin_page_context()preserves the existing/admintemplate context keys, normalizes the webhook status filter before rendering, keeps the same event ordering/summary helpers, and keeps the CSRF token supplied by the route instead of creating a new auth path.create_admin_bounty_from_form()is a thin wrapper aroundcreate_bounty(), returns the created id, and leavesLedgerErrorhandling in the route unchanged.- The OAuth
nexthardening rejects decoded protocol-relative paths and decoded backslashes/control characters while still returning the original relative path, which is consistent with validating the submitted redirect target beforeRedirectResponsereceives it.
Validation run locally on this PR head:
./.venv/bin/python -m pytest tests/test_admin_helpers.py tests/test_security.py -q-> 58 passed./.venv/bin/python -m pytest -q-> 337 passed./.venv/bin/python -m ruff check app/admin.py app/main.py tests/test_admin_helpers.py-> passed./.venv/bin/python -m ruff format --check app/admin.py app/main.py tests/test_admin_helpers.py-> passed./.venv/bin/python -m mypy app-> passedgit diff --check origin/main...HEAD-> passed
ayskobtw-lil
left a comment
ayskobtw-lil
left a comment
There was a problem hiding this comment.
No blockers from my review on current head 9dea484.
Evidence checked:
- Inspected
app/admin.py, the admin route wiring inapp/main.py, andtests/test_admin_helpers.py. - Verified
admin_page_context()preserves the/admintemplate context shape: login, CSRF token supplied by the route, normalized webhook status, event list, status summary, selected limit, and the existing limit options. - Verified
create_admin_bounty_from_form()remains a thin wrapper overcreate_bounty(), returns the created id, keeps repo canonicalization/max-awards behavior from the ledger service, and leavesLedgerErrorhandling in the route. - Rechecked the route-level admin page and CSRF-backed admin bounty form paths, not just the new helper unit tests.
- Verified the decoded
nexthardening still rejects encoded redirect ambiguity/backslash cases through existing OAuth regressions. - Hosted Quality/readiness/docs/image check is green.
Validation run locally:
python -m pytest tests/test_admin_helpers.py tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 6 passed.python -m pytest tests/test_security.py::test_admin_page_renders_safe_webhook_events_for_cookie_admin tests/test_security.py::test_admin_bounty_form_requires_csrf_for_cookie_auth tests/test_admin_helpers.py -q-> 7 passed.python -m pytest tests/test_wallet_api.py::test_oauth_next_path_rejects_redirect_ambiguity tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 4 passed.python -m ruff check app/admin.py app/main.py tests/test_admin_helpers.py-> passed.python -m ruff format --check app/admin.py app/main.py tests/test_admin_helpers.py-> 3 files already formatted.python -m mypy app/admin.py app/main.py-> success.git diff --check origin/main...HEAD-> clean.
Residual risk is low and mostly around existing admin/page behavior; this PR moves admin-specific data shaping into a smaller module and keeps the route as auth/CSRF/render wiring.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
ramimbo
added
mrwk:accepted
Refs #320
Summary
app.admin.admin_page_context.app.admin.create_admin_bounty_from_form, leaving the route focused on auth, CSRF, and redirect behavior.Complexity reduced
app.mainno longer owns admin page query composition, webhook summary context, limit options, or admin form bounty construction. Admin route handlers now mostly wire dependencies and render/redirect, while admin-specific data shaping is testable inapp.admin.Tests
python -m pytest tests\test_admin_helpers.py tests\test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -qpython -m pytest -qpython -m ruff check .git diff --check