phase: 121_git_source_tokens
**Phase 121 final verification pass — all green** (all 4 tasks already in `complete/`; verified, no defects found, no changes needed) - Verified implementation vs phase design: migration `0021` (reversible, round-tripped via `alembic downgrade base` + `upgrade head` → head `0021`), `GitSource.token` column, `normalize_credential`/`clone_url_for`/`sanitize_url`, clone callers switched (`sync.py`, `import_docs.py`), masked token fields in add form + editor, `extra="forbid"` output shapes - Tests: `uv run pytest` → 2662 passed, 0 failed (exit 0); `uv run pytest --cov=app --cov-report=term-missing` → TOTAL **99%** (≥90% gate) - Lint/types: `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors, 0 warnings - E2E in isolation: `uv run pytest tests/e2e/test_git_source_tokens.py -v --no-cov` → **4 passed** Completion criteria: 1. Private repo (UI add or pasted embedded-token URL) clones with injected token; token absent from every API response, page text, title attr, and full HTML — **PASS** (integration raw-JSON assertions + E2E `_assert_token_nowhere`) 2. Legacy embedded-token rows still clone from stored URL; output sanitized — **PASS** (`test_sync_legacy_row_clones_with_original_stored_url`, `test_get_masks_legacy_embedded_token_row`, env-fallback masking) 3. Public/local sources byte-identical — **PASS** (verbatim-URL + no-userinfo-unchanged tests) 4. pytest / coverage / ruff / pyright — **PASS** (see above) 5. Commit + phase move — harness responsibility; task files already in `complete/`, changes left in working tree (no commit made, per protocol) Notable: no deviations; DB left at head, functional. Next pending phase: **122_image_documents** (then 123_chat_image_questions).
This commit is contained in:
+79
-18
@@ -49,11 +49,16 @@ embeddings) committed first, then the app-managed on-disk dir). The
|
||||
whole router sits behind :func:`app.core.auth.require_admin` —
|
||||
anonymous callers get 403 on every route.
|
||||
|
||||
No credential-echo path: git URLs may embed ``user:pass@`` (phase 32's
|
||||
masking discipline), so every git 409/422 detail is a fixed generic
|
||||
string that never repeats the submitted URL. Local paths are not
|
||||
secrets — the local 422/409 details name the (expanded) path so the
|
||||
owner sees exactly which directory failed.
|
||||
No credential-echo path (phase 32's masking discipline, extended by
|
||||
phase 121 — LOCKED A2): git URLs may embed ``user:pass@``, so (a)
|
||||
every git 409/422 detail is a fixed generic string that never repeats
|
||||
the submitted URL, and (b) every URL that LEAVES the API is masked
|
||||
through :func:`app.rag.git_sources.sanitize_url` before it enters a
|
||||
response (DB rows, env-fallback rows, POST 201, PATCH 200) — a legacy
|
||||
row whose credential is still embedded in the stored ``url`` clones
|
||||
fine (the stored value is untouched) but its API/UI output is
|
||||
bare. Local paths are not secrets — the local 422/409 details name
|
||||
the (expanded) path so the owner sees exactly which directory failed.
|
||||
|
||||
Scope boundary (phase locked decisions): the CRUD routes do NOT
|
||||
clone or import anything — the existing Sync button performs that, and
|
||||
@@ -110,6 +115,7 @@ from app.rag.archive_upload import (
|
||||
swap_in,
|
||||
unpack_archive,
|
||||
)
|
||||
from app.rag.git_sources import normalize_credential, sanitize_url
|
||||
from app.rag.importer import normalize_ignore_path
|
||||
from app.rag.llm import LLMClient
|
||||
from app.rag.overview import regenerate_overview
|
||||
@@ -220,6 +226,11 @@ def list_git_sources(
|
||||
git-only) with null ``id``/``added_at``, ``ignore_paths: []`` and
|
||||
``include_hidden: False`` (no DB row to store a list or a flag on),
|
||||
and ``from_env: true``.
|
||||
|
||||
Every URL is masked on the way out (phase 121, LOCKED A2 —
|
||||
:func:`sanitize_url`): an env value or a legacy stored URL may
|
||||
embed ``user:pass@`` — the env value and the DB value are
|
||||
untouched, only the response is bare.
|
||||
"""
|
||||
rows = db.scalars(
|
||||
select(GitSource).order_by(GitSource.added_at.asc(), GitSource.id.asc())
|
||||
@@ -232,7 +243,7 @@ def list_git_sources(
|
||||
GitSourceRow(
|
||||
id=row.id,
|
||||
kind=cast(Literal["git", "local"], row.kind),
|
||||
url=row.url,
|
||||
url=sanitize_url(row.url), # phase 121: never echo userinfo
|
||||
path=row.path,
|
||||
added_at=row.added_at,
|
||||
ignore_paths=row.ignore_paths or [],
|
||||
@@ -247,7 +258,7 @@ def list_git_sources(
|
||||
GitSourceRow(
|
||||
id=None,
|
||||
kind="git",
|
||||
url=url,
|
||||
url=sanitize_url(url), # phase 121: an env URL can embed a token
|
||||
path=None,
|
||||
added_at=None,
|
||||
ignore_paths=[],
|
||||
@@ -290,11 +301,20 @@ def create_git_source(
|
||||
``include_hidden`` (phase 105) — optional, both kinds: absent →
|
||||
stored ``False`` (A4), present → stored as sent; the stored flag is
|
||||
what is reported.
|
||||
|
||||
``token`` (phase 121, LOCKED A2) — the masked private-repo
|
||||
credential: write-only, stored in the dedicated column, never
|
||||
echoed (the response has no token field by contract). Git rows
|
||||
are normalized on the way in (``normalize_credential``): an
|
||||
old-style embedded ``user:pass@`` URL is stored bare with the
|
||||
credential in the token column, an explicit ``token`` wins over
|
||||
the embedded one (LOCKED A6), and the duplicate check runs on the
|
||||
bare URL.
|
||||
"""
|
||||
row = _create_git_row(payload, db) if payload.kind == "git" else _create_local_row(payload, db)
|
||||
return GitSourceOut(
|
||||
id=row.id,
|
||||
url=row.url,
|
||||
url=sanitize_url(row.url), # phase 121: the output mask, always
|
||||
added_at=row.added_at,
|
||||
ignore_paths=row.ignore_paths,
|
||||
include_hidden=row.include_hidden,
|
||||
@@ -346,6 +366,13 @@ def _create_git_row(payload: GitSourceIn, db: Session) -> GitSource:
|
||||
raise HTTPException(
|
||||
status_code=422, detail="not a valid git URL (expected https://, ssh:// or git@…)"
|
||||
)
|
||||
# Phase 121 (task 02, LOCKED A6): normalize the credential — an
|
||||
# old-style embedded ``user:pass@`` URL is stored BARE and the
|
||||
# embedded credential moves to the token column; an explicit
|
||||
# ``token`` field wins over the embedded one. The duplicate check
|
||||
# below runs on the BARE URL, so the same repo pasted with a
|
||||
# different credential is the same source (409, not a second row).
|
||||
url, effective_token = normalize_credential(url, payload.token)
|
||||
if db.scalar(select(GitSource).where(GitSource.url == url)) is not None:
|
||||
raise HTTPException(status_code=409, detail="a git source with this URL already exists")
|
||||
return _commit_new(
|
||||
@@ -354,6 +381,7 @@ def _create_git_row(payload: GitSourceIn, db: Session) -> GitSource:
|
||||
kind="git",
|
||||
ignore_paths=_validate_ignore_paths(payload.ignore_paths),
|
||||
include_hidden=bool(payload.include_hidden),
|
||||
token=effective_token or None,
|
||||
),
|
||||
"a git source with this URL already exists",
|
||||
db,
|
||||
@@ -380,7 +408,10 @@ def _create_local_row(payload: GitSourceIn, db: Session) -> GitSource:
|
||||
)
|
||||
# ``url`` is the table's NOT-NULL location column (phase 38: local
|
||||
# rows carry the expanded path there too — git URL shapes and absolute
|
||||
# paths cannot collide).
|
||||
# paths cannot collide). A ``token`` on a local row (phase 121) is
|
||||
# stored inert — local rows are walked, not cloned, so
|
||||
# ``clone_url_for`` never sees it — and, like on git rows, is never
|
||||
# echoed by any output shape.
|
||||
return _commit_new(
|
||||
GitSource(
|
||||
url=path,
|
||||
@@ -388,6 +419,7 @@ def _create_local_row(payload: GitSourceIn, db: Session) -> GitSource:
|
||||
path=path,
|
||||
ignore_paths=_validate_ignore_paths(payload.ignore_paths),
|
||||
include_hidden=bool(payload.include_hidden),
|
||||
token=payload.token or None,
|
||||
),
|
||||
f"a local source with this path already exists: {path}",
|
||||
db,
|
||||
@@ -400,14 +432,26 @@ def patch_git_source(
|
||||
payload: GitSourcePatchIn,
|
||||
db: Session = Depends(get_db), # noqa: B008
|
||||
) -> GitSourceOut:
|
||||
"""Edit one source's ignore list and/or hidden-folders flag.
|
||||
"""Edit one source's ignore list, hidden-folders flag, and/or
|
||||
private-repo token.
|
||||
|
||||
Phase 89 A5 (ignore list) + phase 105 (the flag): 404 unknown
|
||||
id; each PRESENT body field applies independently —
|
||||
``ignore_paths`` REPLACES the list (normalized + A4-validated,
|
||||
fixed 422 details); ``include_hidden`` sets the flag. Both
|
||||
absent → 200 no-op. Returns the updated row's public shape
|
||||
(id, url, added_at, ignore_paths, include_hidden).
|
||||
Phase 89 A5 (ignore list) + phase 105 (the flag) + phase 121
|
||||
(the token): 404 unknown id; each PRESENT body field applies
|
||||
independently — ``ignore_paths`` REPLACES the list (normalized +
|
||||
A4-validated, fixed 422 details); ``include_hidden`` sets the
|
||||
flag; ``token`` is TRI-STATE (LOCKED A2): absent/None = no change
|
||||
(the row's stored credential survives an edit that does not touch
|
||||
the masked field), non-empty = replace, empty string = clear
|
||||
(stored NULL). A PRESENT token also re-normalizes the (current
|
||||
url, new token) pair with the POST write-path rules — a legacy
|
||||
embedded-token URL gets its userinfo stripped (moved to the
|
||||
column) the first time an explicit credential is written; a clean
|
||||
URL comes back untouched. The 409 backstop: re-normalizing can
|
||||
make the stored URL collide with another row's bare URL (a
|
||||
legacy ``user:pass@`` row and a bare row for the same repo) — the
|
||||
unique index yields the generic 409, never a 500. Returns the
|
||||
updated row's public shape (id, url — masked, added_at,
|
||||
ignore_paths, include_hidden); the token is never echoed.
|
||||
"""
|
||||
row = db.get(GitSource, source_id)
|
||||
if row is None:
|
||||
@@ -416,11 +460,28 @@ def patch_git_source(
|
||||
row.ignore_paths = _validate_ignore_paths(payload.ignore_paths)
|
||||
if payload.include_hidden is not None:
|
||||
row.include_hidden = payload.include_hidden
|
||||
db.commit()
|
||||
if payload.token is not None:
|
||||
# Phase 121 (task 02): the tri-state applies — "" clears
|
||||
# (stored NULL), non-empty replaces. Re-normalize the pair
|
||||
# (see the docstring): a legacy embedded-token URL becomes
|
||||
# bare + column credential.
|
||||
row.url, effective = normalize_credential(row.url, payload.token)
|
||||
row.token = effective or None
|
||||
try:
|
||||
db.commit()
|
||||
except IntegrityError:
|
||||
# The re-normalized URL collided with another row's stored URL
|
||||
# (the legacy-embedded + bare sibling case) — the unique index
|
||||
# is the backstop: a generic 409, never a 500 (the phase-35
|
||||
# convention).
|
||||
db.rollback()
|
||||
raise HTTPException(
|
||||
status_code=409, detail="a git source with this URL already exists"
|
||||
) from None
|
||||
db.refresh(row)
|
||||
return GitSourceOut(
|
||||
id=row.id,
|
||||
url=row.url,
|
||||
url=sanitize_url(row.url), # phase 121: the output mask, always
|
||||
added_at=row.added_at,
|
||||
ignore_paths=row.ignore_paths,
|
||||
include_hidden=row.include_hidden,
|
||||
|
||||
Reference in New Issue
Block a user