feat: add browse frontend auth with is_public filtering #27

Merged

forgejo_admin merged 4 commits from 26-browse-auth into main 2026-02-26 05:08:45 +00:00

Conversation 4 Commits 4 Files changed 17 +629 -35

forgejo_admin commented 2026-02-26 01:18:12 +00:00

Owner

Copy link

Summary

• Add session-based authentication to the browse frontend so private notes (is_public=False) are hidden from unauthenticated visitors
• Unauthenticated users see only public notes; authenticated users see all notes with a visual "private" badge
• API routes remain completely unauthenticated -- only the /browse/ frontend gets auth

Changes

• models.py: Add User model with id, email, hashed_password, is_approved, created_at
• auth.py (new): bcrypt password hashing, authenticate_user(), get_current_user() session cookie reader
• config.py: Add PALDOCS_SECRET_KEY, PALDOCS_SEED_EMAIL, PALDOCS_SEED_PASSWORD settings
• main.py: Add SessionMiddleware, seed user on startup via lifespan
• routes/frontend.py: Add /browse/login (GET/POST), /browse/logout (POST); add is_public filtering to all browse queries; pass user context to all templates
• templates/login.html (new): Email + password login form styled to match existing CSS
• templates/base.html: Add Login/Logout button to nav bar, add .badge-private CSS
• templates/landing.html, project_notes.html, tag_notes.html, note.html: Add private badge on notes
• alembic/versions/b2c3d4e5f6a7_add_users_table.py (new): Migration for users table
• alembic/env.py: Import User model
• pyproject.toml: Add bcrypt>=4.0, itsdangerous>=2.0, python-multipart>=0.0.6
• tests/test_auth.py (new): 14 tests for auth flow and filtering
• tests/conftest.py: Import User model

Test Plan

• Tests pass locally (28/28 pass)
• Login with valid credentials redirects to /browse/
• Login with bad credentials returns 401 with error message
• Logout clears session and redirects
• Unauthenticated landing page shows only public notes
• Unauthenticated user gets 404 for private note direct URL
• Authenticated user sees all notes including private ones
• Private badge visible on private notes when authenticated
• Tag notes and project notes pages filter correctly
• All 14 pre-existing tests still pass (no regressions)
• ruff check and ruff format --check pass

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive

Related Notes

• Forgejo issue #26 -- the issue this PR addresses
• plan-2026-02-25-private-notes-auth -- Phase 1 implementation

## Summary - Add session-based authentication to the browse frontend so private notes (`is_public=False`) are hidden from unauthenticated visitors - Unauthenticated users see only public notes; authenticated users see all notes with a visual "private" badge - API routes remain completely unauthenticated -- only the `/browse/` frontend gets auth ## Changes - `models.py`: Add `User` model with `id`, `email`, `hashed_password`, `is_approved`, `created_at` - `auth.py` (new): bcrypt password hashing, `authenticate_user()`, `get_current_user()` session cookie reader - `config.py`: Add `PALDOCS_SECRET_KEY`, `PALDOCS_SEED_EMAIL`, `PALDOCS_SEED_PASSWORD` settings - `main.py`: Add `SessionMiddleware`, seed user on startup via lifespan - `routes/frontend.py`: Add `/browse/login` (GET/POST), `/browse/logout` (POST); add `is_public` filtering to all browse queries; pass `user` context to all templates - `templates/login.html` (new): Email + password login form styled to match existing CSS - `templates/base.html`: Add Login/Logout button to nav bar, add `.badge-private` CSS - `templates/landing.html`, `project_notes.html`, `tag_notes.html`, `note.html`: Add private badge on notes - `alembic/versions/b2c3d4e5f6a7_add_users_table.py` (new): Migration for users table - `alembic/env.py`: Import `User` model - `pyproject.toml`: Add `bcrypt>=4.0`, `itsdangerous>=2.0`, `python-multipart>=0.0.6` - `tests/test_auth.py` (new): 14 tests for auth flow and filtering - `tests/conftest.py`: Import `User` model ## Test Plan - [x] Tests pass locally (28/28 pass) - [x] Login with valid credentials redirects to /browse/ - [x] Login with bad credentials returns 401 with error message - [x] Logout clears session and redirects - [x] Unauthenticated landing page shows only public notes - [x] Unauthenticated user gets 404 for private note direct URL - [x] Authenticated user sees all notes including private ones - [x] Private badge visible on private notes when authenticated - [x] Tag notes and project notes pages filter correctly - [x] All 14 pre-existing tests still pass (no regressions) - [x] `ruff check` and `ruff format --check` pass ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Forgejo issue #26 -- the issue this PR addresses - `plan-2026-02-25-private-notes-auth` -- Phase 1 implementation

forgejo_admin added 1 commit 2026-02-26 01:18:12 +00:00

feat: add browse frontend auth with is_public filtering (#26) ... fd3b6fb6b8

Add session-based authentication to the browse frontend so private notes
(is_public=False) are hidden from unauthenticated visitors.

- User model with email, hashed_password, is_approved fields + migration
- bcrypt password hashing via auth.py module
- SessionMiddleware with signed cookies for login state
- Login/logout routes at /browse/login and /browse/logout
- is_public filtering on all browse queries (landing, projects, tags, notes)
- Private note returns 404 for unauthenticated visitors
- Visual "private" badge on notes when logged in
- Seed user via PALDOCS_SEED_EMAIL + PALDOCS_SEED_PASSWORD env vars
- 14 new auth tests covering login flow and filtering
- API routes remain unauthenticated

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin added 1 commit 2026-02-26 01:19:24 +00:00

refactor: clean up test_auth.py helpers after self-review ... 57702d40d9

Remove unused `client` parameter from `_create_user()` and extract
`_get_test_db()` helper to reduce boilerplate in authenticated tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented 2026-02-26 01:19:32 +00:00

Author

Owner

Copy link

Review round 1 fix:

• Removed unused client parameter from _create_user() test helper
• Extracted _get_test_db() helper to reduce boilerplate in authenticated test functions (was repeating the same 4-line pattern in 7 tests)

All 28 tests pass, ruff check and format clean.

**Review round 1 fix:** - Removed unused `client` parameter from `_create_user()` test helper - Extracted `_get_test_db()` helper to reduce boilerplate in authenticated test functions (was repeating the same 4-line pattern in 7 tests) All 28 tests pass, ruff check and format clean.

forgejo_admin commented 2026-02-26 01:41:11 +00:00

Author

Owner

Copy link

QA Review — PR #27: Browse Frontend Auth with is_public Filtering

Verdict: PASS (with 1 minor security note and 2 suggestions)

No blockers found. The implementation is solid, well-tested, and follows existing codebase patterns.

---

Test Results

• 28/28 tests pass (14 new auth tests + 14 existing tests, zero regressions)
• Linting: clean (ruff check . — all checks passed)

---

Security Findings

No blockers. Detailed analysis:

Check	Result
Unauthenticated user direct access to /browse/notes/{private-slug}	PASS — returns 404 (line 128-129 of frontend.py)
Private notes in landing page listings	PASS — _apply_public_filter() filters query (line 69)
Private notes in project listings	PASS — filtered (line 108)
Private notes in tag listings	PASS — filtered (line 165)
Password hashing	PASS — bcrypt with gensalt(), no plaintext storage
Session cookie httponly	PASS — Starlette SessionMiddleware always sets httponly
Session cookie samesite	PASS — defaults to lax
Session cookie signed	PASS — itsdangerous.TimestampSigner with configurable secret_key
SQL injection vectors	PASS — all queries use SQLAlchemy ORM, no raw SQL
Auth bypass via API routes	N/A by design — API routes remain unauthenticated (for MCP agents behind Tailscale)
Timing attack on login	PASS — bcrypt's checkpw is constant-time
Seed password logged	PASS — only email is logged, not password

Minor note (non-blocker): The https_only parameter on SessionMiddleware defaults to False, meaning the session cookie lacks the Secure flag. Since this runs behind Tailscale (not public internet) and uses HTTPS via the Tailscale cert, this is acceptable for Phase 1. Consider setting https_only=True in a future phase if the service ever faces the public internet.

Minor note (non-blocker): Linked notes on the /browse/notes/{slug} page (lines 132-145 of frontend.py) do NOT filter by is_public. If a public note links to a private note, an unauthenticated visitor will see the private note's title and slug in the "Linked Notes" section (though clicking it returns 404). This is a minor information leak. Consider filtering linked notes by is_public for unauthenticated users in a follow-up.

---

Bugs Found

None.

---

Code Quality

• Follows existing patterns perfectly — same template style, same SQLAlchemy query patterns, same route structure
• Clean separation: auth.py handles password/session logic, frontend.py handles routes
• _apply_public_filter() helper is DRY and applied consistently to all listing queries
• Migration has proper upgrade() and downgrade() with unique index on email
• Model matches migration (both have server_default=sa.text('1') for is_approved)
• hashed_password column is String(128) — bcrypt hashes are 60 chars, plenty of room
• The _seed_user() function correctly skips if user already exists (idempotent)

---

Suggestions (nice-to-haves, not blockers)

1. Linked notes leak: Filter the outgoing/incoming note link queries by is_public when user is None. Low priority since titles are not sensitive in this context, but worth a follow-up.

2. https_only=True: Consider adding SessionMiddleware(..., https_only=True) since the service runs over HTTPS via Tailscale. This adds the Secure cookie flag as defense-in-depth.

3. TemplateResponse deprecation warnings: 11 test warnings about TemplateResponse(name, {"request": request}) vs the new TemplateResponse(request, name) API. Not introduced by this PR (pre-existing), but worth noting for a cleanup pass.

---

Summary

Clean implementation. Auth flow works correctly (login, logout, session cookies). Private notes are properly hidden from unauthenticated users across all browse pages. API routes remain unaffected. Tests cover all the critical paths. Good to merge.

## QA Review — PR #27: Browse Frontend Auth with is_public Filtering ### Verdict: PASS (with 1 minor security note and 2 suggestions) No blockers found. The implementation is solid, well-tested, and follows existing codebase patterns. --- ### Test Results - **28/28 tests pass** (14 new auth tests + 14 existing tests, zero regressions) - **Linting: clean** (`ruff check .` — all checks passed) --- ### Security Findings **No blockers.** Detailed analysis: | Check | Result | |---|---| | Unauthenticated user direct access to `/browse/notes/{private-slug}` | **PASS** — returns 404 (line 128-129 of `frontend.py`) | | Private notes in landing page listings | **PASS** — `_apply_public_filter()` filters query (line 69) | | Private notes in project listings | **PASS** — filtered (line 108) | | Private notes in tag listings | **PASS** — filtered (line 165) | | Password hashing | **PASS** — bcrypt with `gensalt()`, no plaintext storage | | Session cookie `httponly` | **PASS** — Starlette `SessionMiddleware` always sets `httponly` | | Session cookie `samesite` | **PASS** — defaults to `lax` | | Session cookie signed | **PASS** — `itsdangerous.TimestampSigner` with configurable `secret_key` | | SQL injection vectors | **PASS** — all queries use SQLAlchemy ORM, no raw SQL | | Auth bypass via API routes | **N/A by design** — API routes remain unauthenticated (for MCP agents behind Tailscale) | | Timing attack on login | **PASS** — bcrypt's `checkpw` is constant-time | | Seed password logged | **PASS** — only email is logged, not password | **Minor note (non-blocker):** The `https_only` parameter on `SessionMiddleware` defaults to `False`, meaning the session cookie lacks the `Secure` flag. Since this runs behind Tailscale (not public internet) and uses HTTPS via the Tailscale cert, this is acceptable for Phase 1. Consider setting `https_only=True` in a future phase if the service ever faces the public internet. **Minor note (non-blocker):** Linked notes on the `/browse/notes/{slug}` page (lines 132-145 of `frontend.py`) do NOT filter by `is_public`. If a public note links to a private note, an unauthenticated visitor will see the private note's **title and slug** in the "Linked Notes" section (though clicking it returns 404). This is a minor information leak. Consider filtering linked notes by `is_public` for unauthenticated users in a follow-up. --- ### Bugs Found **None.** --- ### Code Quality - Follows existing patterns perfectly — same template style, same SQLAlchemy query patterns, same route structure - Clean separation: `auth.py` handles password/session logic, `frontend.py` handles routes - `_apply_public_filter()` helper is DRY and applied consistently to all listing queries - Migration has proper `upgrade()` and `downgrade()` with unique index on email - Model matches migration (both have `server_default=sa.text('1')` for `is_approved`) - `hashed_password` column is `String(128)` — bcrypt hashes are 60 chars, plenty of room - The `_seed_user()` function correctly skips if user already exists (idempotent) --- ### Suggestions (nice-to-haves, not blockers) 1. **Linked notes leak**: Filter the outgoing/incoming note link queries by `is_public` when user is `None`. Low priority since titles are not sensitive in this context, but worth a follow-up. 2. **`https_only=True`**: Consider adding `SessionMiddleware(..., https_only=True)` since the service runs over HTTPS via Tailscale. This adds the `Secure` cookie flag as defense-in-depth. 3. **TemplateResponse deprecation warnings**: 11 test warnings about `TemplateResponse(name, {"request": request})` vs the new `TemplateResponse(request, name)` API. Not introduced by this PR (pre-existing), but worth noting for a cleanup pass. --- ### Summary Clean implementation. Auth flow works correctly (login, logout, session cookies). Private notes are properly hidden from unauthenticated users across all browse pages. API routes remain unaffected. Tests cover all the critical paths. Good to merge.

forgejo_admin referenced this pull request from a commit 2026-02-26 02:01:44 +00:00

fix: harden session cookie and private note UX (#27)

forgejo_admin added 1 commit 2026-02-26 02:01:44 +00:00

fix: harden session cookie and private note UX (#27) ... 1ac28f40ea

- Set https_only=True on SessionMiddleware for defense-in-depth
- Redirect unauthenticated users to login (with message) instead of 404
  when they hit a private note URL, with ?next= for post-login redirect
- Filter linked notes queries by is_public for unauthenticated users so
  private note titles/slugs don't leak through link relationships
- Update test client to use HTTPS base_url to match https_only cookie
- Add tests for login redirect, next-param flow, and linked notes filtering

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented 2026-02-26 02:06:06 +00:00

Author

Owner

Copy link

QA Review — PR #27 (Updated)

Verdict: PASS with one non-blocking finding

---

1. Security Findings

FIXED — Previously reported issues confirmed resolved:

• https_only=True on SessionMiddleware — Confirmed on main.py:49. The secure flag is now set on session cookies.
• Private note login redirect — frontend.py:132-137 correctly redirects unauthenticated users to /browse/login with message and next parameters instead of returning a 404.
• Linked notes filtering — frontend.py:150-152 correctly filters both outgoing and incoming links by is_public for unauthenticated users.

Session cookie security — PASS:

• Starlette's SessionMiddleware always sets httponly in security flags (verified in source)
• With https_only=True, the secure flag is also set
• same_site=lax (Starlette default) — appropriate
• Cookies are signed via itsdangerous.TimestampSigner with the configured secret_key
• Session data is base64-encoded JSON (only stores user_email), not raw credentials

Password hashing — PASS:

• auth.py:9 uses bcrypt.hashpw with bcrypt.gensalt() — correct
• auth.py:13 uses bcrypt.checkpw for verification — correct
• No timing attack surface beyond what bcrypt provides

Auth bypass vectors — PASS:

• get_current_user() queries the DB each request (no stale session data)
• authenticate_user() checks is_approved flag — unapproved users can't login
• Private note access is blocked at the view level (browse_note), not just hidden from listings
• All browse routes (landing, projects/{slug}, tags/{name}) consistently apply _apply_public_filter()
• The REST API (/notes, /notes/{slug}) remains unprotected — this is intentional and documented (API auth tracked in issue #2)

Open redirect on next parameter — LOW SEVERITY (non-blocking):

frontend.py:41,54:

next_url = request.query_params.get("next", "/browse/")
...
return RedirectResponse(url=next_url, status_code=302)

An attacker could craft ?next=https://evil.com or ?next=//evil.com to redirect a user to a malicious site after login. This is a classic open redirect.

Mitigation factors that make this non-blocking:

1. The service runs behind Tailscale (no public internet exposure)
2. The only user is the platform owner (no public registration)
3. An attacker would need Tailscale access to even reach the login page

Suggested fix for a future PR: Validate that next_url starts with / and does not start with //:

if not next_url.startswith("/") or next_url.startswith("//"):
    next_url = "/browse/"

---

2. Bugs Found

None.

---

3. Login Redirect UX — PASS

• Private note → redirects to /browse/login?message=Log+in+to+view+private+notes&next=/browse/notes/{slug} ✓
• Login page renders the message query param in a styled info box ✓
• POST /browse/login?next=... reads next from query params and redirects after successful auth ✓
• The login.html form action preserves the query string: action="/browse/login{{ '?' + request.query_string.decode() if request.query_string else '' }}" ✓

---

4. Linked Notes Filtering — PASS

• frontend.py:140-152: Outgoing links (NoteLink.source_id == note.id) filtered by Note.is_public for unauthenticated ✓
• frontend.py:146-152: Incoming links (NoteLink.target_id == note.id) filtered by Note.is_public for unauthenticated ✓
• Authenticated users (user is not None) see all links without filtering ✓
• Three dedicated tests cover these scenarios (test_unauthenticated_linked_notes_hides_private, test_authenticated_linked_notes_shows_all, test_unauthenticated_incoming_links_hides_private) ✓

---

5. Test Results — PASS

All 33 tests passed (19 auth + 11 CRUD + 2 health + 1 revision):

tests/test_auth.py: 19 passed
tests/test_crud.py: 12 passed
tests/test_health.py: 2 passed

Linting: ruff check and ruff format --check both clean.

Test coverage assessment:

The 19 auth tests cover:

• Login flow (valid creds, bad creds, nonexistent user, logout)
• is_public filtering on landing, tag pages, project pages
• Private note redirect to login (with message + next)
• Login with next param redirect
• Linked notes filtering (outgoing + incoming, auth vs unauth)
• Private badge rendering

Minor gap (non-blocking):

• No test for open redirect on next parameter (matches the finding above)
• No test for is_approved=False user being rejected (the code handles it in auth.py:22, but no test exercises it)

---

6. Suggestions (non-blocking)

1. Open redirect validation on next parameter (described above) — track as a separate issue
2. Test is_approved=False rejection — small gap in auth test coverage
3. secret_key default — config.py:8 has secret_key: str = "change-me-in-production". This is fine for dev, but ensure the k8s deployment sets PALDOCS_SECRET_KEY via a secret. (Not in scope for this PR.)
4. Deprecation warnings — 15 warnings about TemplateResponse(name, {"request": request}) being deprecated in favor of TemplateResponse(request, name). Not blocking, but worth a cleanup PR.

---

Overall: APPROVED — The two previously reported findings are correctly fixed. The open redirect is low-severity given Tailscale-only access and can be addressed in a follow-up.

## QA Review — PR #27 (Updated) **Verdict: PASS with one non-blocking finding** --- ### 1. Security Findings #### FIXED — Previously reported issues confirmed resolved: - **`https_only=True` on SessionMiddleware** — Confirmed on `main.py:49`. The `secure` flag is now set on session cookies. - **Private note login redirect** — `frontend.py:132-137` correctly redirects unauthenticated users to `/browse/login` with `message` and `next` parameters instead of returning a 404. - **Linked notes filtering** — `frontend.py:150-152` correctly filters both outgoing and incoming links by `is_public` for unauthenticated users. #### Session cookie security — PASS: - Starlette's `SessionMiddleware` always sets `httponly` in security flags (verified in source) - With `https_only=True`, the `secure` flag is also set - `same_site=lax` (Starlette default) — appropriate - Cookies are signed via `itsdangerous.TimestampSigner` with the configured `secret_key` - Session data is base64-encoded JSON (only stores `user_email`), not raw credentials #### Password hashing — PASS: - `auth.py:9` uses `bcrypt.hashpw` with `bcrypt.gensalt()` — correct - `auth.py:13` uses `bcrypt.checkpw` for verification — correct - No timing attack surface beyond what bcrypt provides #### Auth bypass vectors — PASS: - `get_current_user()` queries the DB each request (no stale session data) - `authenticate_user()` checks `is_approved` flag — unapproved users can't login - Private note access is blocked at the view level (`browse_note`), not just hidden from listings - All browse routes (`landing`, `projects/{slug}`, `tags/{name}`) consistently apply `_apply_public_filter()` - The REST API (`/notes`, `/notes/{slug}`) remains unprotected — this is intentional and documented (API auth tracked in issue #2) #### Open redirect on `next` parameter — LOW SEVERITY (non-blocking): `frontend.py:41,54`: ```python next_url = request.query_params.get("next", "/browse/") ... return RedirectResponse(url=next_url, status_code=302) ``` An attacker could craft `?next=https://evil.com` or `?next=//evil.com` to redirect a user to a malicious site after login. This is a classic open redirect. **Mitigation factors that make this non-blocking:** 1. The service runs behind Tailscale (no public internet exposure) 2. The only user is the platform owner (no public registration) 3. An attacker would need Tailscale access to even reach the login page **Suggested fix for a future PR:** Validate that `next_url` starts with `/` and does not start with `//`: ```python if not next_url.startswith("/") or next_url.startswith("//"): next_url = "/browse/" ``` --- ### 2. Bugs Found None. --- ### 3. Login Redirect UX — PASS - Private note → redirects to `/browse/login?message=Log+in+to+view+private+notes&next=/browse/notes/{slug}` ✓ - Login page renders the `message` query param in a styled info box ✓ - POST `/browse/login?next=...` reads `next` from query params and redirects after successful auth ✓ - The `login.html` form action preserves the query string: `action="/browse/login{{ '?' + request.query_string.decode() if request.query_string else '' }}"` ✓ --- ### 4. Linked Notes Filtering — PASS - `frontend.py:140-152`: Outgoing links (`NoteLink.source_id == note.id`) filtered by `Note.is_public` for unauthenticated ✓ - `frontend.py:146-152`: Incoming links (`NoteLink.target_id == note.id`) filtered by `Note.is_public` for unauthenticated ✓ - Authenticated users (`user is not None`) see all links without filtering ✓ - Three dedicated tests cover these scenarios (`test_unauthenticated_linked_notes_hides_private`, `test_authenticated_linked_notes_shows_all`, `test_unauthenticated_incoming_links_hides_private`) ✓ --- ### 5. Test Results — PASS All **33 tests passed** (19 auth + 11 CRUD + 2 health + 1 revision): ``` tests/test_auth.py: 19 passed tests/test_crud.py: 12 passed tests/test_health.py: 2 passed ``` Linting: `ruff check` and `ruff format --check` both clean. #### Test coverage assessment: The 19 auth tests cover: - Login flow (valid creds, bad creds, nonexistent user, logout) - `is_public` filtering on landing, tag pages, project pages - Private note redirect to login (with message + next) - Login with `next` param redirect - Linked notes filtering (outgoing + incoming, auth vs unauth) - Private badge rendering #### Minor gap (non-blocking): - No test for open redirect on `next` parameter (matches the finding above) - No test for `is_approved=False` user being rejected (the code handles it in `auth.py:22`, but no test exercises it) --- ### 6. Suggestions (non-blocking) 1. **Open redirect validation** on `next` parameter (described above) — track as a separate issue 2. **Test `is_approved=False` rejection** — small gap in auth test coverage 3. **`secret_key` default** — `config.py:8` has `secret_key: str = "change-me-in-production"`. This is fine for dev, but ensure the k8s deployment sets `PALDOCS_SECRET_KEY` via a secret. (Not in scope for this PR.) 4. **Deprecation warnings** — 15 warnings about `TemplateResponse(name, {"request": request})` being deprecated in favor of `TemplateResponse(request, name)`. Not blocking, but worth a cleanup PR. --- **Overall: APPROVED** — The two previously reported findings are correctly fixed. The open redirect is low-severity given Tailscale-only access and can be addressed in a follow-up.

forgejo_admin referenced this pull request from a commit 2026-02-26 02:11:05 +00:00

fix: address PR #27 review — open redirect, deprecation warnings, test gaps

forgejo_admin added 1 commit 2026-02-26 02:11:05 +00:00

fix: address PR #27 review — open redirect, deprecation warnings, test gaps ... 2303515579

- Validate `next` param on login to prevent open redirects (reject
  absolute URLs and protocol-relative paths like //evil.com)
- Migrate all TemplateResponse calls to modern Starlette API
  (request as first arg, context as keyword) to eliminate 15
  deprecation warnings
- Add test for is_approved=False user rejection
- Add PALDOCS_SECRET_KEY env var to k8s deployment referencing a
  k8s secret

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented 2026-02-26 02:14:33 +00:00

Author

Owner

Copy link

QA Review Pass 3 — APPROVED

Verdict: PASS

Security Checklist

#	Item	Status	Notes
1	Unauthenticated cannot see private notes on landing page	PASS	_apply_public_filter() applied to landing query; test confirms
2	Unauthenticated cannot see private notes on project pages	PASS	_apply_public_filter() applied in browse_project_notes; test confirms
3	Unauthenticated cannot see private notes on tag pages	PASS	_apply_public_filter() applied in browse_tag_notes; test confirms
4	Unauthenticated hitting /browse/notes/{private-slug} gets redirected to login	PASS	Explicit check at line 136 of frontend.py; test confirms with redirect location validation
5	After login with valid next param, redirects to private note	PASS	login_submit reads next from query params and redirects after auth; test confirms
6	Open redirect blocked (next=https://evil.com, next=//evil.com)	PASS	Guard at line 43-44; both test cases confirm fallback to /browse/. Also verified /%2F/evil.com edge case is safe (browser treats as same-origin path).
7	Linked notes on public notes don't leak private titles for unauth	PASS	Both outgoing_q and incoming_q filtered with Note.is_public.is_(True) when user is None; tests confirm
8	Password hashing uses bcrypt	PASS	auth.py uses bcrypt.hashpw with gensalt()
9	Session cookies: httponly, signed, https_only=True	PASS	SessionMiddleware with https_only=True. Starlette uses itsdangerous signing and sets httponly by default. Test client uses base_url="https://testserver" to match.
10	is_approved=False users cannot log in	PASS	Check at line 22 of auth.py; test test_login_unapproved_user_rejected confirms 401 response
11	PALDOCS_SECRET_KEY not hardcoded in production	PASS	k8s deployment.yaml references secretKeyRef: pal-e-docs-secrets / secret-key. Config default "change-me-in-production" is fine for dev.
12	No SQL injection vectors	PASS	All queries use SQLAlchemy ORM with parameterized filters. No raw SQL.
13	No XSS vectors introduced	PASS	Jinja2 autoescape verified enabled programmatically. {{ message }} in login.html is auto-escaped (confirmed <script> in query param renders as &lt;script&gt;). Only `
14	TemplateResponse deprecation warnings gone	PASS	All calls now use TemplateResponse(request, template, context=...) signature.

Test Results

• 37/37 tests passed (6.02s)
• Ruff check: all passed
• Ruff format: all files formatted

Test Coverage Assessment

23 auth-specific tests covering: login flow (valid, bad creds, nonexistent user, unapproved user), logout, public/private filtering on landing/tag/project pages, private note redirect to login with message, login redirect with next param, linked notes filtering (outgoing + incoming), open redirect prevention (absolute URL, protocol-relative), private badge rendering.

Code Quality

• Clean separation: auth.py for auth logic, _apply_public_filter helper for consistent filtering
• Consistent patterns across all route handlers
• Well-organized test helpers
• Migration chain valid: 54714bc57fbe -> a1b2c3d4e5f6 -> b2c3d4e5f6a7

Notes

• REST API (/notes, /notes/{slug}) remains unprotected by design — auth scope is browse frontend only
• No CSRF on login/logout forms — acceptable: login CSRF is low-risk, logout CSRF is minor. Future hardening pass if needed.
• seed_email/seed_password config is clean for deployment bootstrapping

No bugs found. No blocking concerns.

## QA Review Pass 3 — APPROVED **Verdict: PASS** ### Security Checklist | # | Item | Status | Notes | |---|------|--------|-------| | 1 | Unauthenticated cannot see private notes on landing page | PASS | `_apply_public_filter()` applied to landing query; test confirms | | 2 | Unauthenticated cannot see private notes on project pages | PASS | `_apply_public_filter()` applied in `browse_project_notes`; test confirms | | 3 | Unauthenticated cannot see private notes on tag pages | PASS | `_apply_public_filter()` applied in `browse_tag_notes`; test confirms | | 4 | Unauthenticated hitting `/browse/notes/{private-slug}` gets redirected to login | PASS | Explicit check at line 136 of `frontend.py`; test confirms with redirect location validation | | 5 | After login with valid `next` param, redirects to private note | PASS | `login_submit` reads `next` from query params and redirects after auth; test confirms | | 6 | Open redirect blocked (`next=https://evil.com`, `next=//evil.com`) | PASS | Guard at line 43-44; both test cases confirm fallback to `/browse/`. Also verified `/%2F/evil.com` edge case is safe (browser treats as same-origin path). | | 7 | Linked notes on public notes don't leak private titles for unauth | PASS | Both `outgoing_q` and `incoming_q` filtered with `Note.is_public.is_(True)` when user is None; tests confirm | | 8 | Password hashing uses bcrypt | PASS | `auth.py` uses `bcrypt.hashpw` with `gensalt()` | | 9 | Session cookies: httponly, signed, https_only=True | PASS | `SessionMiddleware` with `https_only=True`. Starlette uses `itsdangerous` signing and sets `httponly` by default. Test client uses `base_url="https://testserver"` to match. | | 10 | `is_approved=False` users cannot log in | PASS | Check at line 22 of `auth.py`; test `test_login_unapproved_user_rejected` confirms 401 response | | 11 | `PALDOCS_SECRET_KEY` not hardcoded in production | PASS | k8s `deployment.yaml` references `secretKeyRef: pal-e-docs-secrets / secret-key`. Config default `"change-me-in-production"` is fine for dev. | | 12 | No SQL injection vectors | PASS | All queries use SQLAlchemy ORM with parameterized filters. No raw SQL. | | 13 | No XSS vectors introduced | PASS | Jinja2 autoescape verified enabled programmatically. `{{ message }}` in login.html is auto-escaped (confirmed `<script>` in query param renders as `&lt;script&gt;`). Only `| safe` usage is pre-existing `note.html_content` for trusted MCP-authored content. | | 14 | TemplateResponse deprecation warnings gone | PASS | All calls now use `TemplateResponse(request, template, context=...)` signature. | ### Test Results - **37/37 tests passed** (6.02s) - **Ruff check**: all passed - **Ruff format**: all files formatted ### Test Coverage Assessment 23 auth-specific tests covering: login flow (valid, bad creds, nonexistent user, unapproved user), logout, public/private filtering on landing/tag/project pages, private note redirect to login with message, login redirect with `next` param, linked notes filtering (outgoing + incoming), open redirect prevention (absolute URL, protocol-relative), private badge rendering. ### Code Quality - Clean separation: `auth.py` for auth logic, `_apply_public_filter` helper for consistent filtering - Consistent patterns across all route handlers - Well-organized test helpers - Migration chain valid: `54714bc57fbe` -> `a1b2c3d4e5f6` -> `b2c3d4e5f6a7` ### Notes - REST API (`/notes`, `/notes/{slug}`) remains unprotected by design — auth scope is browse frontend only - No CSRF on login/logout forms — acceptable: login CSRF is low-risk, logout CSRF is minor. Future hardening pass if needed. - `seed_email`/`seed_password` config is clean for deployment bootstrapping **No bugs found. No blocking concerns.**

forgejo_admin merged commit 09b4f506f1 into main 2026-02-26 05:08:45 +00:00

forgejo_admin deleted branch 26-browse-auth 2026-02-26 05:08:45 +00:00

forgejo_admin referenced this pull request from a commit 2026-02-26 05:08:46 +00:00

feat: add browse frontend auth with is_public filtering (#27)

forgejo_admin referenced this pull request 2026-02-26 05:18:19 +00:00

feat: add seed credential env vars to k8s deployment #28

forgejo_admin referenced this pull request 2026-03-09 03:16:29 +00:00

Add pgvector extension + embedding schema migration #121

forgejo_admin referenced this pull request 2026-03-09 03:56:52 +00:00

6c: Async embedding pipeline + backfill #129

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/pal-e-api!27

No description provided.