Skip to content

Benchmark: sentry PR 67876 - #7

Open
celmis-codereviewer wants to merge 5 commits into
cr-base-67876from
cr-pr-67876
Open

Benchmark: sentry PR 67876#7
celmis-codereviewer wants to merge 5 commits into
cr-base-67876from
cr-pr-67876

Conversation

@celmis-codereviewer

Copy link
Copy Markdown

Benchmark reproduction of getsentry#67876

@celmis-codereviewer celmis-codereviewer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

💬 COMMENT — findings to consider

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

@celmis-codereviewer celmis-codereviewer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

💬 COMMENT — findings to consider

Full findings and scope are in the review summary — one persistent comment, updated in place on every run.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

@celmis-codereviewer celmis-codereviewer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

💬 COMMENT — findings to consider

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

# Check that the authenticated GitHub user is the same as who installed the app.
if (
pipeline.fetch_state("github_authenticated_user")
!= integration.metadata["sender"]["login"]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: integration.metadata can be missing the "sender" key on line 503; accessing integration.metadata["sender"]["login"] without a fallback check raises an unhandled KeyError or TypeError.

🟠 Unsafe dictionary lookup on integration.metadata can raise KeyError

If an Integration object has empty or legacy metadata that does not contain the "sender" dictionary (or where "sender" lacks the "login" key), accessing integration.metadata["sender"]["login"] directly will raise a KeyError or TypeError. This results in an unhandled 500 Server Error during pipeline execution instead of rendering the user-friendly error response.

Suggested change
!= integration.metadata["sender"]["login"]
sender_login = (integration.metadata or {}).get("sender", {}).get("login")
if (
pipeline.fetch_state("github_authenticated_user")
!= sender_login
):
return error(request, self.active_organization)

agent: defect · rule: defect.uncaught-exception · confidence: 0.95

"client_secret": github_client_secret,
}

# similar to OAuth2CallbackView.exchange_token

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: safe_urlopen on line 422 is placed outside the try block on line 424; if it raises a network or HTTP exception, the error is not caught and results in an unhandled 500 error.

🟠 safe_urlopen call outside try block can raise unhandled network exception

When exchanging the OAuth code for an access token, safe_urlopen is called on line 422 outside the try...except block that begins on line 424. If GitHub is unreachable, times out, or returns a network error, safe_urlopen will raise an uncaught exception (RequestException) causing a 500 Internal Server Error response instead of gracefully returning error(request, self.active_organization).

Suggested change
# similar to OAuth2CallbackView.exchange_token
try:
req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data)
body = safe_urlread(req).decode("utf-8")
payload = dict(parse_qsl(body))
except Exception:
payload = {}

agent: defect · rule: defect.uncaught-exception · confidence: 0.90

integration = Integration.objects.get(
external_id=installation_id, status=ObjectStatus.ACTIVE
)
except Integration.DoesNotExist:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 Bare except: catches everything including KeyboardInterrupt

Use except Exception: so SystemExit and KeyboardInterrupt still propagate as the user expects.

except Integration.DoesNotExist: ⏎             return error(request, self.active_organization)
Suggested change
except Integration.DoesNotExist:
except Exception:

agent: structural · rule: structural.py.bare-except · confidence: 1.00

@celmis-codereviewer

Copy link
Copy Markdown
Author

🤖 Code Review for PR #7

💬 COMMENT — findings to consider

Findings

  • 🟠 Error: 2
  • 🟡 Warning: 1

Scope

  • Files changed: 3
  • Lines: +247 / -50

Performance

  • Analysis time: 301.0s · agents: cve, structural, contract, security, defect · tokens: 35,462/34,808

Powered by Code Analyzer · context: tree-sitter graph + cve, structural, contract, security, defect

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.

2 participants