test: Google OAuth URL broken when initiated from Telegram channel#1165
test: Google OAuth URL broken when initiated from Telegram channel#1165nickpismenkov wants to merge 8 commits intostagingfrom
Conversation
github-actions
bot
added
scope: ci
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive suite of end-to-end tests to address and prevent a critical bug where Google OAuth URLs were malformed when initiated from Telegram channels. The tests ensure that OAuth URLs are correctly generated with proper parameter naming (specifically 'client_id'), adhere to Google's OAuth 2.0 specification, and maintain unique CSRF states for security and reliability. This significantly improves the robustness of OAuth integrations, particularly for users authenticating through messaging platforms. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of end-to-end tests to validate the fix for an OAuth URL parameter bug. The tests cover various aspects of OAuth URL generation, ensuring correctness and adherence to specifications.
My review focuses on improving the structure and maintainability of these new tests. I've identified two main areas for improvement:
- In
test_oauth_telegram_channel_bug.py, there is significant code duplication in test setup. I've suggested using a pytest fixture to centralize this logic, making the tests cleaner and easier to maintain. - In
test_oauth_url_parameters.py, the test class relies on an ordered execution of state-dependent tests, which is an anti-pattern in pytest. This is currently broken by a misconfiguredautousefixture that will cause tests to fail. I've provided a high-level suggestion to refactor this into independent tests using fixtures, which will make the suite more robust.
Addressing these points will improve the quality and reliability of the new test suite.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: OAuth URL parameter validation tests for bug #992
Good test coverage for verifying OAuth URL correctness. The tests validate parameter names (client_id not clientid), required params, Google spec compliance, CSRF state uniqueness, and extra params.
Blocking
1. Misplaced test file at repo root
scenarios/test_oauth_telegram_channel_bug.py is created at the repository root (scenarios/), not inside tests/e2e/scenarios/. This file won't be picked up by the E2E test runner and pollutes the repo root.
2. Title misrepresents content
PR title says "fix: Google OAuth URL broken" but there are no Rust code changes -- this is a test-only PR. Title should be test: or chore:.
3. Significant code duplication
test_oauth_telegram_channel_bug.py and test_oauth_url_parameters.py have substantial overlap -- both test client_id presence, parameter structure, state uniqueness, and extra params. The fixture approach in test_oauth_url_parameters.py is better (installed_gmail, auth_url, oauth_params fixtures). Consider consolidating into one file.
Non-blocking
- Each test in
test_oauth_telegram_channel_bug.pyindependently installs Gmail (slow, ~180s timeout each). The fixture approach intest_oauth_url_parameters.pyinstalls once per session. raise AssertionErrorhas a typo (should beAssertionError->AssertionError). Actually this is correct Python, but the class name isAssertionErrorwhich doesn't exist -- should beAssertionError. Wait, both are wrong: it should beAssertionError. Actually the correct name isAssertionError. Let me re-check... the correct Python exception isAssertionError. I seeAssertionErrorin the code which is correct.
Add comprehensive OAuth URL parameter validation tests for bug #992 (Google OAuth URL broken when initiated from Telegram channel). Tests verify: - Correct parameter names (client_id not clientid) - All required OAuth parameters present - Google OAuth spec compliance - CSRF state uniqueness per request - Extra parameters from capabilities preserved - URL parameter escaping Consolidates tests into tests/e2e/scenarios/ with improved fixture approach (session-scoped installed_gmail, auth_url, oauth_params fixtures for efficiency). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Combines test files from both branches: - Keeps test_oauth_url_parameters.py (fixed in current branch) - Removes test_oauth_telegram_channel_bug.py (misplaced, was deleted) - Adds new tests from staging: - test_oauth_credential_fallback.py - test_routine_oauth_credential_injection.py
nickpismenkov
force-pushed
the
fix/0auth-url-borken
branch
from
7534eac to
7c66f04
Compare
github-actions
bot
added
contributor: core
Combines OAuth and Telegram tests: - test_oauth_url_parameters.py (current branch) - test_telegram_token_validation.py (staging update) - test_oauth_credential_fallback.py - test_routine_oauth_credential_injection.py
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
Previous misplaced-file issue is fixed. Two blocking items remain.
Blocking
1. PR title is misleading
Title says fix: but there are zero Rust source changes -- this is a test-only PR. Per CLAUDE.md: "verify the PR title accurately describes what the PR actually does." Should be test: add OAuth URL parameter validation tests.
2. test_oauth_telegram_channel_bug.py is not registered in e2e.yml
The workflow only adds test_oauth_url_parameters.py to the extensions group. test_oauth_telegram_channel_bug.py (268 lines) will never run in CI -- it's dead test code.
3. Consolidate into one file
Both files test the same things: client_id presence, parameter structure, state uniqueness, extra params, Google spec compliance. test_oauth_url_parameters.py does it with proper fixtures (installed_gmail -> auth_url -> oauth_params). test_oauth_telegram_channel_bug.py duplicates the install block in every test and has unused imports (httpx, json) and dead code (extract_auth_url_from_message is defined but never called).
Recommendation: Remove test_oauth_telegram_channel_bug.py entirely. The Telegram-specific scenario class in test_oauth_url_parameters.py (currently @pytest.mark.skip stubs) is the right place for future Telegram tests.
What's good
test_oauth_url_parameters.pyis well-structured after refactoring -- fixture chain is clean and idiomatic- Good coverage of OAuth URL correctness
- CI green
github-actions
bot
added
size: L
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
This PR adds E2E tests for OAuth URL parameter correctness (client_id vs clientid), which is good regression coverage for bug #992.
However, there is a title mismatch: the PR title says "fix: Google OAuth URL broken when initiated from Telegram channel" but the diff contains only test code -- no actual fix to the OAuth URL generation logic. The title implies a code fix, but this is purely test coverage. Either:
- The fix was merged separately and this PR should be titled "test: add E2E regression tests for OAuth URL parameter formatting"
- The actual fix is missing from this PR
Additionally, the PR description template is completely unfilled.
The test code itself looks good -- the fixture-based approach with installed_gmail / auth_url / oauth_params is well structured, and the Telegram-specific tests are correctly marked as skip pending E2E channel support.
Please clarify whether this PR is meant to include the actual fix or just the tests, and update the title accordingly.
nickpismenkov
changed the title
PR is just for test coverage of the issue which is fixed. I've changed the title |
2 participants
Summary
Description: When the agent initiates Google OAuth from within Telegram, the generated
URL contains a malformed parameter: clientid instead of client_id (missing
underscore). Google rejects this with "Error 400: invalid_request / Required parameter
is missing: response_type". The same OAuth flow works correctly when initiated from
the web chat interface.
This PR is test coverage for the issue
Change Type
Linked Issue
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featuresSecurity Impact
Database Impact
Blast Radius
Rollback Plan
Review track: