Skip to content

Require job access in coverage_report before redirecting - #5426

Open
herdiyana256 wants to merge 1 commit into
google:masterfrom
herdiyana256:fix-coverage-report-missing-access
Open

Require job access in coverage_report before redirecting#5426
herdiyana256 wants to merge 1 commit into
google:masterfrom
herdiyana256:fix-coverage-report-missing-access

Conversation

@herdiyana256

@herdiyana256 herdiyana256 commented Aug 14, 2026

Copy link
Copy Markdown

coverage_report resolves and redirects to the coverage report URL for a job taken from the request, decorated only with @handler.oauth:

def get_report_url(report_type, argument, date):
  ...
  job = argument
  return _get_project_report_url(job, date)   # no access check

Coverage reports are per-job data, but @handler.oauth authenticates the caller without authorizing access to any particular job (the same gap the other job-scoped handlers like fuzzer_stats already close). So any caller could resolve the coverage report location for an arbitrary job, and by extension its project.

from libs import access
...
if not access.has_access(job_type=job):
  raise helpers.AccessDeniedError()

The resolution is now gated on job access, matching the sibling job-scoped handlers. Adds a regression test for the access-denied path.

@herdiyana256
herdiyana256 requested a review from a team as a code owner August 14, 2026 18:33
GET /coverage-report resolved a job's coverage report URL and redirected
to it without an access check. The handler only uses @handler.oauth,
which runs even when the Authorization header is absent, so the route is
reachable without authentication, and get_report_url validated only the
job name and date formats.

Coverage reports are per-job data. The other job-scoped handlers already
gate on it (fuzzer_stats uses access.has_access(job_type=...)), so any
caller, including an unauthenticated one, could resolve the coverage
report location and job-to-project mapping for jobs they cannot access.

Add access.has_access(job_type=job) after the job name validation, plus a
test that a caller without access gets AccessDeniedError.
@herdiyana256
herdiyana256 force-pushed the fix-coverage-report-missing-access branch from 3c64c28 to b26d3ad Compare August 14, 2026 18:35
@herdiyana256

Copy link
Copy Markdown
Author

Friendly bump on this one, and a question about CI.

What the change does

coverage_report.py's get_report_url resolved a job's coverage report URL and redirected to it based on a caller-supplied job name, with no authorization check. The only decorator was @handler.oauth, which does not gate: the wrapper records _oauth_email when an Authorization header is present and calls the handler either way, so the route was reachable unauthenticated. The job value was validated for name format only.

The patch adds access.has_access(job_type=job) after the existing name validation and raises AccessDeniedError when it fails, plus a test_no_access case.

Why this shape is the right one

The parity argument is inside this repo. fuzzer_stats.py:456 gates the same category of per-job data with access.has_access(fuzzer_name=fuzzer, job_type=job), so coverage data is already treated as job-scoped elsewhere. This handler was the outlier.

On public OSS-Fuzz the coverage reports are public anyway, so the impact case is private deployments where jobs and projects are access controlled. There, an unauthenticated caller could enumerate job names and learn the coverage report location and project mapping for jobs they cannot otherwise see.

CI question

Two Cloud Build checks report failure here, All-Tests (clusterfuzz-testing-public) and Build-Base-Images-PR (clusterfuzz-images). Both report a duration of 0, which reads like they never actually executed rather than failing on the diff. Their logs live in the Cloud Build console, which I cannot see from outside, so I do not want to guess at the cause.

For what it is worth, the pattern across currently open PRs is mixed rather than uniformly fork related: #5424 and #5425 are also from forks and their All-Tests passes, while #5426 and #5429 fail. If these need a maintainer to kick off, please do, and if the logs show something genuinely caused by this diff I will fix it right away.

The change touches two files, coverage_report.py (+8) and coverage_report_test.py (+13), and the branch still merges cleanly.

One note for whoever picks this up

#5389, #5390 and #5391 are the same class of missing access check on other handlers, and all four have been sitting without a review since 14 August. If it is easier to look at them as a batch, or if you would rather I consolidate or close and resubmit them differently, just say which you prefer and I will do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant