Skip to content

test(bitbucket): add test coverage for defensive pagination and redirect guards - #1047

Merged
potiuk merged 3 commits into
apache:mainfrom
onlyarnav:test-bitbucket-defensive-guards
Aug 1, 2026
Merged

potiuk merged 3 commits into
apache:mainfrom
onlyarnav:test-bitbucket-defensive-guards

Conversation

@onlyarnav

Copy link
Copy Markdown
Member

Summary

Adds unit test cases in tools/bitbucket/tests/test_bitbucket.py for three defensive guards in the Bitbucket bridge to prevent regressions or accidental removal during future refactoring:

  1. Redirect Scheme Downgrade Guard:
    • test_same_host_redirect_handler_rejects_scheme_downgrade: Asserts SameHostRedirectHandler refuses https → http redirects on the same host and raises BitbucketError ("refusing to forward credentials").
  2. Data Center Non-advancing Cursor Guard:
    • test_datacenter_get_pull_request_commits_stops_on_non_advancing_next_page_start: Asserts datacenter.get_pull_request_commits terminates its pagination loop when nextPageStart is non-advancing (<= start), preventing infinite loops.
  3. Cloud Repeated Pagination URL Guard:
    • test_cloud_get_pull_request_commits_rejects_repeated_next_url: Asserts cloud.get_pull_request_commits rejects repeated next URLs and raises BitbucketError ("pagination returned a repeated URL").

Negative Control Verification

Each test was verified by temporarily disabling its guard in source code:

  • Disabling scheme validation in _origin() caused test_same_host_redirect_handler_rejects_scheme_downgrade to fail (DID NOT RAISE BitbucketError).
  • Removing if next_start <= start: break from datacenter.py caused test_datacenter_get_pull_request_commits_stops_on_non_advancing_next_page_start to fail (StopIteration).
  • Removing if next_url in seen_urls: from cloud.py caused test_cloud_get_pull_request_commits_rejects_repeated_next_url to fail (StopIteration).

Test Suite Status

All 101 unit tests in tools/bitbucket pass cleanly (uv run --directory tools/bitbucket pytest).

fixes: #1007

Generated using Claude Code

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM — three tests for three real guards, and they bite. Tests for defensive paths are easy to write in a way that passes whether or not the defence exists, so I checked by deleting each guard rather than by reading.

Scheme downgrade. _origin() returns (scheme, hostname, port), so an https → http redirect genuinely produces a different origin and trips SameHostRedirectHandler. The test pins the credential-forwarding refusal on the axis that matters most: a downgrade to plaintext is exactly when you least want the Authorization header to follow.

Non-advancing nextPageStart. Removing if next_start <= start: break from get_pull_request_commits makes your test fail — it blows up on StopIteration as the loop keeps asking for pages past the single mocked response, which is the infinite-loop behaviour the guard exists to prevent. assert len(opener.open.call_args_list) == 1 is what makes it precise: it pins one request, not merely a successful return.

Repeated next URL. Correctly targeted at the cloud paginator's own error.

Smaller observation — worth a follow-up, not a change here

While confirming the above I removed the same guard from get_repository_restrictions instead, and the entire 101-test suite still passed. So that sibling guard is completely unpinned.

datacenter.py repeats the next_start <= start pattern in six functions:

get_repository_restrictions   list_open_pull_requests    get_pull_request_commits
get_pull_request_status       get_pull_request_reviews   get_pull_request_discussion

This PR pins one of the six. cloud.py does not have that problem because its equivalent lives in a single _validated_next_url() helper, so one test covers every caller.

That contrast is the interesting part, and it points at a better follow-up than "write five more tests": hoisting the datacenter guard into a shared helper the way cloud already does would make one test cover all six, and would remove the possibility of the next paginator being added without it. Either way it is out of scope here — this PR strictly improves matters.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md § Opening a pull request.

@potiuk
potiuk merged commit 88d7345 into apache:main Aug 1, 2026
9 checks passed
@potiuk potiuk added contract:change-request Tool capability: proposed-change review + merge gate (PR / MR / Gerrit change) contract:tracker Tool capability: issue / board / label backend labels Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contract:change-request Tool capability: proposed-change review + merge gate (PR / MR / Gerrit change) contract:tracker Tool capability: issue / board / label backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tests for the Bitbucket pagination guards and the redirect scheme-downgrade case

2 participants