fix(#59): clamp meta.last page to minimum of 1 for empty results#1340
fix(#59): clamp meta.last page to minimum of 1 for empty results#1340fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
When a paginated endpoint returns zero items, pagination_query.pages is 0, causing meta.last to point to page=0 which is an invalid URL that returns a 404. Fix by using max(1, pagination_query.pages) so meta.last always points to at least page=1, matching meta.first for empty collections. Add test_get_builds_empty to verify meta.last equals meta.first and contains page=1 when no builds exist. Note: kerberos-dependent test suites could not run in the sandbox (missing libkrb5-dev). The web/API tests covering the changed code passed. Full suite verification is required in CI. Closes #59
ReviewFindingsCritical
High
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| if: >- | ||
| github.event_name != 'issue_comment' | ||
| || github.event.comment.user.type != 'Bot' | ||
| uses: fullsend-ai/fullsend/.github/workflows/reusable-dispatch.yml@v0 |
There was a problem hiding this comment.
[critical] supply-chain risk
The dispatch job calls fullsend-ai/fullsend/.github/workflows/reusable-dispatch.yml@v0, pinned to a mutable Git tag. Combined with broad top-level permissions (actions:write, contents:write, id-token:write) and pull_request_target trigger, a compromised update to the reusable workflow can write arbitrary commits, mint OIDC tokens, and exfiltrate GCP secrets.
Suggested fix: Pin the reusable workflow to a full commit SHA. Scope permissions per-job rather than at workflow level.
| # branch in reusable-dispatch.yml — zero changes to this repo. | ||
| name: fullsend | ||
|
|
||
| permissions: |
There was a problem hiding this comment.
[high] excessive-permissions
Workflow-level permissions block grants contents:write, actions:write, and id-token:write to every job that does not override them. The dispatch job inherits all of these and hands them to an external, tag-pinned reusable workflow.
Suggested fix: Move permissions into individual jobs. Declare only the permissions that reusable-dispatch.yml actually requires.
| install_mode: per-repo | ||
| mint_url: ${{ vars.FULLSEND_MINT_URL }} | ||
| gcp_region: ${{ vars.FULLSEND_GCP_REGION }} | ||
| secrets: |
There was a problem hiding this comment.
[medium] secrets-exposure
Two GCP secrets (FULLSEND_GCP_WIF_PROVIDER, FULLSEND_GCP_PROJECT_ID) are passed to the external reusable workflow pinned to a mutable tag. The pull_request_target trigger means these secrets are available even for PRs from forks.
Suggested fix: Pin the reusable workflow to a commit SHA. Consider gating secret access behind a GitHub environment with required reviewers.
| base_dir=temp_dir, | ||
| from_index=from_index_resolved, | ||
| operators=operators, | ||
| operators=operators_in_db, |
There was a problem hiding this comment.
[low] api-contract
verify_operators_exists returns a set but opm_registry_rm_fbc type-hints its operators parameter as List[str]. Functionally correct but violates the type contract.
Suggested fix: Wrap with list(operators_in_db) or update the type hint to accept Iterable[str].
| github.event.comment.author_association == 'OWNER' | ||
| || github.event.comment.author_association == 'MEMBER' | ||
| || github.event.comment.author_association == 'COLLABORATOR' | ||
| || github.event.comment.author_association == 'CONTRIBUTOR' |
There was a problem hiding this comment.
[low] authorization-bypass
The stop-fix job allows CONTRIBUTOR association (any user who has had a PR merged) to disable the fix agent on any PR via /fs-fix-stop. This is a wider authorization surface than likely intended.
Suggested fix: Remove CONTRIBUTOR from the allowed associations, keeping only OWNER, MEMBER, COLLABORATOR, plus the PR author check.
When a paginated endpoint returns zero items, pagination_query.pages is 0, causing meta.last to point to page=0 which is an invalid URL that returns a 404. Fix by using max(1, pagination_query.pages) so meta.last always points to at least page=1, matching meta.first for empty collections.
Add test_get_builds_empty to verify meta.last equals meta.first and contains page=1 when no builds exist.
Note: kerberos-dependent test suites could not run in the sandbox (missing libkrb5-dev). The web/API tests covering the changed code passed. Full suite verification is required in CI.
Closes #59
Post-script verification
agent/59-fix-empty-pagination-last)6e0ac6cccda83166c240c4b0a4ebd4e8532c6f26..HEAD)