Require testcase access in issue_redirector before redirecting#5391
Open
herdiyana256 wants to merge 1 commit into
Open
Require testcase access in issue_redirector before redirecting#5391herdiyana256 wants to merge 1 commit into
herdiyana256 wants to merge 1 commit into
Conversation
GET /issue/<testcase_id> looked up the testcase and redirected straight to its issue tracker URL with no access check at all, not even the general access.has_access() used elsewhere -- this route has no auth decorator and helpers.get_testcase() is a plain datastore fetch with no ownership or security_flag check. testcase_id is a small sequential integer, so this let anyone, unauthenticated, enumerate testcases and learn the associated issue tracker URL for each one, including security-flagged/embargoed testcases that access.can_user_access_testcase() is meant to gate everywhere else in the app. Added a test confirming a caller without access gets a 403 instead of being redirected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GET /issue/<testcase_id> looked up the testcase and redirected straight to its issue tracker URL with no access check at all -- this route has no auth decorator, and helpers.get_testcase() is a plain datastore fetch with no ownership or security_flag check of its own.
testcase_id is a small sequential integer, so this let anyone, completely unauthenticated, enumerate testcases and learn the associated issue tracker URL for each one, including security-flagged/embargoed testcases that access.can_user_access_testcase() is meant to gate everywhere else in the app (crash_query, testcase_detail, download, etc. all check it).
Added the same check here: raise AccessDeniedError before building the redirect if the caller can't access the testcase.
Added a test confirming a caller without access gets a 403 instead of being redirected; the existing tests needed can_user_access_testcase mocked to True to keep passing, since they previously didn't exercise any access check at all.