fix(security): address CodeQL clear-text logging and weak hashing alerts
- Convert f-string log interpolation to %s-style formatting in
app/api/pipelines.py and app/api/saved_searches.py to prevent
clear-text logging of request-derived data (CodeQL: clear-text
logging of sensitive information)
- Replace plain hashlib.sha256() with PBKDF2-HMAC-SHA256 via
hash_token() in app/auth.py for Bearer token verification,
consistent with how tokens are stored in api_tokens.py (CodeQL:
use of weak cryptographic hashing on sensitive data)
- Remove redundant {exc} from logger.exception() calls (the
traceback is already captured by logger.exception())
- Update test to verify PBKDF2 hash instead of plain SHA-256
Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>
This commit is contained in:
@@ -6,8 +6,8 @@ uploads, scripted integrations).
|
|||||||
|
|
||||||
Tokens use ``secrets.token_urlsafe`` from the Python standard library
|
Tokens use ``secrets.token_urlsafe`` from the Python standard library
|
||||||
(no extra dependencies) and are prefixed with ``de_`` for easy
|
(no extra dependencies) and are prefixed with ``de_`` for easy
|
||||||
identification. Only a SHA-256 hash is persisted; the plaintext is
|
identification. Only a PBKDF2-HMAC-SHA256 hash is persisted; the
|
||||||
returned exactly once at creation time.
|
plaintext is returned exactly once at creation time.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import hashlib
|
import hashlib
|
||||||
|
|||||||
@@ -291,15 +291,15 @@ def create_pipeline(request: Request, db: DbSession, body: PipelineCreate) -> di
|
|||||||
db.add(pipeline)
|
db.add(pipeline)
|
||||||
db.commit()
|
db.commit()
|
||||||
db.refresh(pipeline)
|
db.refresh(pipeline)
|
||||||
except Exception as exc:
|
except Exception:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
logger.exception(f"Failed to create pipeline user={user_id}: {exc}")
|
logger.exception("Failed to create pipeline user=%s", user_id)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||||
detail="Failed to create pipeline",
|
detail="Failed to create pipeline",
|
||||||
)
|
)
|
||||||
|
|
||||||
logger.info(f"Pipeline created: id={pipeline.id}, owner={user_id}, name={name!r}")
|
logger.info("Pipeline created: id=%s, owner=%s, name=%r", pipeline.id, user_id, name)
|
||||||
return _serialize_pipeline(pipeline)
|
return _serialize_pipeline(pipeline)
|
||||||
|
|
||||||
|
|
||||||
@@ -387,15 +387,15 @@ def update_pipeline(pipeline_id: int, request: Request, db: DbSession, body: Pip
|
|||||||
try:
|
try:
|
||||||
db.commit()
|
db.commit()
|
||||||
db.refresh(pipeline)
|
db.refresh(pipeline)
|
||||||
except Exception as exc:
|
except Exception:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
logger.exception(f"Failed to update pipeline id={pipeline_id}: {exc}")
|
logger.exception("Failed to update pipeline id=%s", pipeline_id)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||||
detail="Failed to update pipeline",
|
detail="Failed to update pipeline",
|
||||||
)
|
)
|
||||||
|
|
||||||
logger.info(f"Pipeline updated: id={pipeline_id}, user={user_id}")
|
logger.info("Pipeline updated: id=%s, user=%s", pipeline_id, user_id)
|
||||||
return _serialize_pipeline(pipeline, include_steps=True, db=db)
|
return _serialize_pipeline(pipeline, include_steps=True, db=db)
|
||||||
|
|
||||||
|
|
||||||
@@ -425,15 +425,15 @@ def delete_pipeline(pipeline_id: int, request: Request, db: DbSession) -> None:
|
|||||||
db.query(PipelineStep).filter(PipelineStep.pipeline_id == pipeline_id).delete()
|
db.query(PipelineStep).filter(PipelineStep.pipeline_id == pipeline_id).delete()
|
||||||
db.delete(pipeline)
|
db.delete(pipeline)
|
||||||
db.commit()
|
db.commit()
|
||||||
except Exception as exc:
|
except Exception:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
logger.exception(f"Failed to delete pipeline id={pipeline_id}: {exc}")
|
logger.exception("Failed to delete pipeline id=%s", pipeline_id)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||||
detail="Failed to delete pipeline",
|
detail="Failed to delete pipeline",
|
||||||
)
|
)
|
||||||
|
|
||||||
logger.info(f"Pipeline deleted: id={pipeline_id}, user={user_id}")
|
logger.info("Pipeline deleted: id=%s, user=%s", pipeline_id, user_id)
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
@@ -190,15 +190,15 @@ def create_saved_search(
|
|||||||
db.add(saved_search)
|
db.add(saved_search)
|
||||||
db.commit()
|
db.commit()
|
||||||
db.refresh(saved_search)
|
db.refresh(saved_search)
|
||||||
except Exception as exc:
|
except Exception:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
logger.exception(f"Failed to create saved search for user={user_id}: {exc}")
|
logger.exception("Failed to create saved search for user=%s", user_id)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||||
detail="Failed to save search",
|
detail="Failed to save search",
|
||||||
)
|
)
|
||||||
|
|
||||||
logger.info(f"Saved search created: user={user_id}, name={name!r}")
|
logger.info("Saved search created: user=%s, name=%r", user_id, name)
|
||||||
return _serialize_saved_search(saved_search)
|
return _serialize_saved_search(saved_search)
|
||||||
|
|
||||||
|
|
||||||
@@ -263,15 +263,15 @@ def update_saved_search(
|
|||||||
try:
|
try:
|
||||||
db.commit()
|
db.commit()
|
||||||
db.refresh(saved_search)
|
db.refresh(saved_search)
|
||||||
except Exception as exc:
|
except Exception:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
logger.exception(f"Failed to update saved search id={search_id}, user={user_id}: {exc}")
|
logger.exception("Failed to update saved search id=%s, user=%s", search_id, user_id)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||||
detail="Failed to update saved search",
|
detail="Failed to update saved search",
|
||||||
)
|
)
|
||||||
|
|
||||||
logger.info(f"Saved search updated: id={search_id}, user={user_id}")
|
logger.info("Saved search updated: id=%s, user=%s", search_id, user_id)
|
||||||
return _serialize_saved_search(saved_search)
|
return _serialize_saved_search(saved_search)
|
||||||
|
|
||||||
|
|
||||||
@@ -294,12 +294,12 @@ def delete_saved_search(search_id: int, request: Request, db: DbSession):
|
|||||||
try:
|
try:
|
||||||
db.delete(saved_search)
|
db.delete(saved_search)
|
||||||
db.commit()
|
db.commit()
|
||||||
except Exception as exc:
|
except Exception:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
logger.exception(f"Failed to delete saved search id={search_id}, user={user_id}: {exc}")
|
logger.exception("Failed to delete saved search id=%s, user=%s", search_id, user_id)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||||
detail="Failed to delete saved search",
|
detail="Failed to delete saved search",
|
||||||
)
|
)
|
||||||
|
|
||||||
logger.info(f"Saved search deleted: id={search_id}, user={user_id}")
|
logger.info("Saved search deleted: id=%s, user=%s", search_id, user_id)
|
||||||
|
|||||||
+2
-1
@@ -78,9 +78,10 @@ def _resolve_bearer_user(request: Request, db: Session) -> dict | None:
|
|||||||
if not raw_token or not isinstance(raw_token, str):
|
if not raw_token or not isinstance(raw_token, str):
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
from app.api.api_tokens import hash_token
|
||||||
from app.models import ApiToken
|
from app.models import ApiToken
|
||||||
|
|
||||||
token_hash = hashlib.sha256(raw_token.encode("utf-8")).hexdigest()
|
token_hash = hash_token(raw_token)
|
||||||
db_token = db.query(ApiToken).filter(ApiToken.token_hash == token_hash, ApiToken.is_active.is_(True)).first()
|
db_token = db.query(ApiToken).filter(ApiToken.token_hash == token_hash, ApiToken.is_active.is_(True)).first()
|
||||||
if db_token is None:
|
if db_token is None:
|
||||||
return None
|
return None
|
||||||
|
|||||||
@@ -1,6 +1,5 @@
|
|||||||
"""Tests for the personal API tokens feature (app/api/api_tokens.py + auth integration)."""
|
"""Tests for the personal API tokens feature (app/api/api_tokens.py + auth integration)."""
|
||||||
|
|
||||||
import hashlib
|
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
from fastapi.testclient import TestClient
|
from fastapi.testclient import TestClient
|
||||||
@@ -103,14 +102,15 @@ class TestTokenCreate:
|
|||||||
|
|
||||||
@pytest.mark.unit
|
@pytest.mark.unit
|
||||||
def test_create_token_stored_as_hash(self, tok_engine, tok_session):
|
def test_create_token_stored_as_hash(self, tok_engine, tok_session):
|
||||||
"""The database should only store a SHA-256 hash, never the plaintext."""
|
"""The database should only store a PBKDF2-HMAC-SHA256 hash, never the plaintext."""
|
||||||
|
from app.api.api_tokens import hash_token
|
||||||
from app.main import app
|
from app.main import app
|
||||||
|
|
||||||
client = _make_client(tok_engine)
|
client = _make_client(tok_engine)
|
||||||
try:
|
try:
|
||||||
resp = client.post("/api/api-tokens/", json={"name": "Hash Check"})
|
resp = client.post("/api/api-tokens/", json={"name": "Hash Check"})
|
||||||
token_plaintext = resp.json()["token"]
|
token_plaintext = resp.json()["token"]
|
||||||
expected_hash = hashlib.sha256(token_plaintext.encode()).hexdigest()
|
expected_hash = hash_token(token_plaintext)
|
||||||
|
|
||||||
db_token = tok_session.query(ApiToken).first()
|
db_token = tok_session.query(ApiToken).first()
|
||||||
assert db_token is not None
|
assert db_token is not None
|
||||||
|
|||||||
Reference in New Issue
Block a user