Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/actions/setup-python-env/action.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
name: "Set Up Python Environment"
description: "Set up a Python environment using uv"

inputs:
python-version:
Expand All @@ -13,10 +14,11 @@ runs:
with:
python-version: ${{ inputs.python-version }}
- name: Install uv
uses: astral-sh/setup-uv@v7.1.6
uses: astral-sh/setup-uv@v7.2.0
with:
python-version: ${{ inputs.python-version }}
version-file: "pyproject.toml"
enable-cache: true
enable-cache: "auto"
ignore-nothing-to-cache: true
- name: Install project and dependencies
run: uv sync --frozen --dev
Expand Down
20 changes: 16 additions & 4 deletions .github/workflows/checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -80,29 +80,41 @@ jobs:
run: |
uv run python -m pytest tests --verbose
style:
runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
python-version: ["3.11", "3.12"]
# TODO: also run on macos-latest pending docker/colima issue
os: [ubuntu-latest]
steps:
- name: Check out repository
uses: actions/checkout@v6
- name: Set up Python
uses: ./.github/actions/setup-python-env
with:
python-version: "3.11"
python-version: ${{ matrix.python-version }}
- name: Run formatter
run: |
uv run python -m ruff format --diff colandr
- name: Run linter
run: |
uv run python -m ruff check --exit-zero colandr
types:
runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
python-version: ["3.11", "3.12"]
# TODO: also run on macos-latest pending docker/colima issue
os: [ubuntu-latest]
steps:
- name: Check out repository
uses: actions/checkout@v6
- name: Set up Python
uses: ./.github/actions/setup-python-env
with:
python-version: "3.11"
python-version: ${{ matrix.python-version }}
- name: Check types with ty
run: |
uv run python -m ty check
2 changes: 1 addition & 1 deletion colandr/api/v1/authn.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# TODO: we should use redis for this
# import celery
# import redis.client
# _JWT_BLOCKLIST = celery.current_app.backend.client # type: ignore
# _JWT_BLOCKLIST = celery.current_app.backend.client
_JWT_BLOCKLIST = set()


Expand Down
3 changes: 2 additions & 1 deletion colandr/api/v1/authz.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def user_is_allowed_for_review(
True if user is allowed to access review; False otherwise
"""
review = db.session.get(models.Review, review_id)

review_user_assoc = (
sa.select(models.ReviewUserAssoc)
.where(models.ReviewUserAssoc.user_id == user.id)
Expand All @@ -44,7 +45,7 @@ def user_is_allowed_for_review(
or db.session.execute(review_user_assoc).scalar_one_or_none() is not None
)
# allowed is conditional on review being frozen or not
and (if_frozen is True or review.status != "frozen") # type: ignore
and (if_frozen is True or getattr(review, "status", None) != "frozen")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Authorization returns True for non-existent reviews

Medium Severity

The change from review.status to getattr(review, "status", None) alters authorization behavior when the review doesn't exist. When db.session.get() returns None (non-existent review), getattr(None, "status", None) returns None, and None != "frozen" evaluates to True. This causes user_is_allowed_for_review to return True for admin users accessing non-existent reviews, where previously it would raise an AttributeError. The function semantically indicates a user is "allowed" for a review that doesn't exist, which could lead to confusing downstream behavior or information leakage through 403/404 response differences.

🔬 Verification Test

Why verification test was not possible: This is a Flask application with database dependencies (SQLAlchemy, PostgreSQL). Testing would require setting up the full application context, database fixtures, and mocking the Flask extensions. The logic error is apparent from tracing the code: when review is None, getattr(None, "status", None) returns None, and None != "frozen" is True, so an admin user passes all authorization checks for a non-existent review.

Fix in Cursor Fix in Web

)


Expand Down
4 changes: 2 additions & 2 deletions colandr/api/v1/routes/citation_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def post(self, files_data, query_data):
if data_source is None:
data_source = models.DataSource(
source_type=source_type, source_name=source_name, source_url=source_url
) # type: ignore
)
db.session.add(data_source)
db.session.commit()

Expand Down Expand Up @@ -211,7 +211,7 @@ def post(self, files_data, query_data):
record_type="citation",
num_records=n_citations,
status=status,
) # type: ignore
)
db.session.add(citations_import)
db.session.commit()
current_app.logger.info(
Expand Down
2 changes: 1 addition & 1 deletion colandr/api/v1/routes/citation_screenings.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def post(self, id, json_data):
stage="citation",
status=json_data["status"],
exclude_reasons=json_data["exclude_reasons"],
) # type: ignore
)
study.screenings.add(screening)
db.session.commit()

Expand Down
4 changes: 2 additions & 2 deletions colandr/api/v1/routes/citations.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ def post(self, json_data, query_data):
if data_source is None:
data_source = models.DataSource(
source_type=source_type, source_name=source_name, source_url=source_url
) # type: ignore
)
db.session.add(data_source)
db.session.commit()
current_app.logger.info("%s inserted %s", current_user, data_source)
Expand All @@ -206,7 +206,7 @@ def post(self, json_data, query_data):
data_source_id=data_source.id,
# citation=citation,
citation=json_data,
) # type: ignore
)
if status is not None:
study.citation_status = status
db.session.add(study)
Expand Down
2 changes: 1 addition & 1 deletion colandr/api/v1/routes/data_extractions.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def put(self, id, json_data):
)

labels_map = {
item["label"]: (item["field_type"], set(item.get("allowed_values", []))) # type: ignore
item["label"]: (item["field_type"], set(item.get("allowed_values", [])))
for item in data_extraction_form[0]
}
# manually validate inputs, given data extraction form specification
Expand Down
4 changes: 2 additions & 2 deletions colandr/api/v1/routes/exports.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ def get(self, query_data):
row.citation_status: row.count
for row in db.session.execute(n_citations_by_status_stmt)
}
n_citations_screened = sum(n_citations_by_status.values()) # type: ignore
n_citations_screened = sum(n_citations_by_status.values())
n_citations_excluded = n_citations_by_status.get("excluded", 0)

n_fulltexts_by_status_stmt = (
Expand All @@ -383,7 +383,7 @@ def get(self, query_data):
row.fulltext_status: row.count
for row in db.session.execute(n_fulltexts_by_status_stmt)
}
n_fulltexts_screened = sum(n_fulltexts_by_status.values()) # type: ignore
n_fulltexts_screened = sum(n_fulltexts_by_status.values())
n_fulltexts_excluded = n_fulltexts_by_status.get("excluded", 0)

results = db.session.execute(
Expand Down
2 changes: 1 addition & 1 deletion colandr/api/v1/routes/fulltext_screenings.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ def post(self, id, json_data):
stage="fulltext",
status=json_data["status"],
exclude_reasons=json_data["exclude_reasons"],
) # type: ignore
)
study.screenings.add(screening)
db.session.commit()

Expand Down
2 changes: 1 addition & 1 deletion colandr/api/v1/routes/health.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class HealthAPI(MethodView):
)
@bp.output({"message": af.fields.String()})
def get(self):
redis_conn = celery.current_app.backend.client # type: ignore
redis_conn = celery.current_app.backend.client
assert isinstance(redis_conn, redis.client.Redis) # type guard
try:
_ = redis_conn.ping()
Expand Down
2 changes: 1 addition & 1 deletion colandr/api/v1/routes/reviews.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def post(self, json_data):
"pct": 100,
}
]
review = models.Review(name=name, **json_data) # type: ignore
review = models.Review(name=name, **json_data)
# TODO: do we want to allow admins to set other users as owners?
review.review_user_assoc.append(
models.ReviewUserAssoc(review, current_user, "owner")
Expand Down
2 changes: 1 addition & 1 deletion colandr/lib/fileio/ris.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def read(path_or_stream: t.BinaryIO | pathlib.Path) -> list[dict]:
def parse(data: str) -> list[dict]:
return rispy.loads(
data,
implementation=rispy.parser.RisParser, # type: ignore
implementation=rispy.parser.RisParser,
skip_unknown_tags=False,
)

Expand Down
4 changes: 2 additions & 2 deletions colandr/lib/models/deduper_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ def _estimate_prob_two_records_match(self) -> None:
splink.block_on("title", "author"),
]
self.model.training.estimate_probability_two_random_records_match(
deterministic_rules, # type: ignore
deterministic_rules,
recall=0.7,
)

Expand All @@ -341,7 +341,7 @@ def _estimate_u_parameters(
) -> None:
self.model.training.estimate_u_using_random_sampling(
max_pairs=max_pairs,
seed=seed, # type: ignore
seed=seed,
)

def _estimate_m_parameters(self) -> None:
Expand Down
6 changes: 3 additions & 3 deletions colandr/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ def num_citations_by_status(self, statuses: str | list[str]) -> dict[str, int]:
)
# ensure every status is in result, with default value (0)
result = {status: 0 for status in statuses}
result |= {row.citation_status: row.count for row in db.session.execute(stmt)} # type: ignore
result |= {row.citation_status: row.count for row in db.session.execute(stmt)}
return result

def num_fulltexts_by_status(self, statuses: str | list[str]) -> dict[str, int]:
Expand All @@ -273,7 +273,7 @@ def num_fulltexts_by_status(self, statuses: str | list[str]) -> dict[str, int]:
)
# ensure every status is in result, with default value (0)
result = {status: 0 for status in statuses}
result |= {row.fulltext_status: row.count for row in db.session.execute(stmt)} # type: ignore
result |= {row.fulltext_status: row.count for row in db.session.execute(stmt)}
return result


Expand Down Expand Up @@ -769,7 +769,7 @@ def __repr__(self):

@sa_event.listens_for(Review, "after_insert")
def insert_review_plan(mapper, connection, target):
review_plan = ReviewPlan(id=target.id) # type: ignore
review_plan = ReviewPlan(id=target.id)
connection.execute(sa.insert(ReviewPlan).values(id=target.id))
LOGGER.info("inserted %s and %s", target, review_plan)

Expand Down
4 changes: 2 additions & 2 deletions colandr/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def _get_redis_lock(lock_id: str, timeout: int = 120) -> redis.lock.Lock:


def _get_redis_conn() -> redis.client.Redis:
redis_conn = current_celery_app.backend.client # type: ignore
redis_conn = current_celery_app.backend.client
assert isinstance(redis_conn, redis.client.Redis) # type guard
return redis_conn

Expand Down Expand Up @@ -149,7 +149,7 @@ def deduplicate_citations(review_id: int):
LOGGER.info(
"<Review(id=%s)>: found %s duplicate clusters",
review_id,
len(clustered_dupes), # type: ignore
len(clustered_dupes),
)

# get *all* citation ids for this review, as well as included/excluded
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ dev = [
"pytest-postgresql~=7.0",
"SQLAlchemy-Utils~=0.42.0",
# TODO: update ty once officially out of beta
"ty~=0.0.7",
"ty~=0.0.11",
"ruff~=0.14.0",
]

Expand Down
Loading