Skip to content

Make configured options reach LINE and harden the outbound calls - #4

Merged
lipowen merged 8 commits into
masterfrom
fix/authorize-options-and-verify-hardening
Sep 8, 2026
Merged

lipowen merged 8 commits into
masterfrom
fix/authorize-options-and-verify-hardening

Conversation

@linyiru

@linyiru linyiru commented Sep 3, 2026

Copy link
Copy Markdown
Member

Summary

Four fixes, each with specs; suite stays at 100% line coverage (32 examples).

  • Forward LINE-specific authorize options. omniauth-oauth2 only forwards options named in authorize_options (default: scope/state), so prompt, bot_prompt, ui_locales, nonce, and every other LINE parameter set on the strategy was silently dropped before it ever reached the authorize URL. ⚠️ Downstream note: any app that has been setting these options will see them take effect for the first time when it picks up this change (e.g. a configured prompt: 'consent' starts showing the consent screen).
  • Honor an explicitly configured redirect_uri. Multi-host apps that compute their callback URL set options[:redirect_uri]; the previous callback_url override ignored it in favor of the request host.
  • Harden the outbound token and verify calls. The token exchange is pinned to auth_scheme: :request_body — LINE documents only body credentials; the oauth2 gem's basic-auth default happens to work but was verified to be undocumented LINE behavior. The id-token verify call gets explicit open/read timeouts (5s/10s, configurable via verify_options) and degrades email to nil with a logged warning on timeout, rejection, or a non-JSON body, instead of holding the login hostage or failing it. OAuth2::TimeoutError from the profile call (a Faraday subclass that omniauth-oauth2's callback_phase would let escape as a 500) is mapped to ::Timeout::Error so it becomes fail!(:timeout).
  • VCR cassettes recorded from the real LINE API. spec/cassettes/ locks the suite to LINE's actual error shapes for the token, verify, and profile endpoints. Recorded once with throwaway credentials (no secrets in the cassettes); CI only replays. Delete a cassette file to re-record.

Verification

  • bundle exec rake: 32 examples, 0 failures, 100% line coverage.
  • LINE's acceptance of both basic-auth and body credentials at the token endpoint was probed live against api.line.me (numeric client_id via Basic → invalid_grant, proving Basic is parsed; the pin to :request_body is about matching documented behavior, not fixing an outage).

https://claude.ai/code/session_01RBRHMr4tD5hZ5Js6WQSHr5

@linyiru
linyiru requested review from lipowen and a lite review from Copilot September 3, 2026 03:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR improves the OmniAuth::Strategies::Line OAuth2 strategy so configured LINE authorize options actually reach the authorize URL, callback/redirect handling respects explicit configuration, and outbound token/verify/profile calls are more robust (timeouts + best-effort email). It also adds VCR-backed specs/cassettes to lock behavior to real LINE API responses.

Changes:

  • Forward LINE-specific authorize parameters by expanding authorize_options, and honor options[:redirect_uri] in callback_url.
  • Harden outbound calls: pin token exchange to auth_scheme: :request_body, add verify timeouts + best-effort email degradation, and map OAuth2::TimeoutError to Timeout::Error.
  • Add VCR configuration + recorded cassettes/specs for real-world LINE API error shapes.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lib/omniauth/strategies/line.rb Adds authorize-option forwarding, redirect override, token auth scheme pinning, verify timeouts, and email degradation/logging.
spec/spec_helper.rb Introduces VCR configuration to support cassette replay/recording in specs.
spec/omniauth/strategies/line_spec.rb Expands specs for authorize params, redirect_uri behavior, timeout mapping, email degradation, and VCR-based API-shape assertions.
spec/cassettes/verify_invalid_id_token.yml Records LINE verify endpoint rejection behavior.
spec/cassettes/profile_invalid_access_token.yml Records LINE profile endpoint rejection behavior.
spec/cassettes/token_invalid_client.yml Records LINE token endpoint rejection behavior.
Gemfile Adds vcr to the test dependency group.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/omniauth/strategies/line.rb
Comment thread spec/cassettes/token_invalid_client.yml Outdated
Comment thread spec/spec_helper.rb

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

spec/cassettes/verify_invalid_id_token.yml:52

  • This cassette’s response body is marked application/json but the stored JSON contains a literal newline inside a quoted string, which makes it invalid JSON on replay. That forces the strategy down the JSON::ParserError path and prevents logging the actual error_description for this case.

Comment thread spec/spec_helper.rb

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

lib/omniauth/strategies/line.rb:78

  • email parses the verify response with JSON.parse(response.body) without guarding against a nil/invalid body. Net::HTTPResponse#body can be nil (or otherwise non-string), which raises TypeError and would break the intended “best-effort” email behavior by failing the callback instead of degrading to nil. Coerce to string and rescue TypeError alongside JSON::ParserError.

@lipowen

lipowen commented Sep 8, 2026

Copy link
Copy Markdown

Rebased this branch onto #6 (11e454a), which removes the scope guard in #email — LINE's token response never lists email in scope (LINE Login v2.1 reference), so the guard made info[:email] always nil. The rebase was conflict-free; the resulting #email keeps only return nil unless id_token plus this PR's timeout/degradation handling. One extra commit updates the recorded-behavior spec's token stub to LINE's real scope string.

Once #6 merges into master, this PR's diff shrinks back to its own commits. Local bundle exec rake: 31 examples, 0 failures, 100% line coverage.

@lipowen
lipowen force-pushed the fix/authorize-options-and-verify-hardening branch from 8b9545a to 87044d4 Compare September 8, 2026 02:37
linyiru and others added 8 commits September 8, 2026 10:47
omniauth-oauth2 only forwards options named in authorize_options (default
scope/state) to the authorize URL, so prompt, bot_prompt, ui_locales and
every other LINE parameter set on the strategy was silently dropped.

Claude-Session: https://claude.ai/code/session_01RBRHMr4tD5hZ5Js6WQSHr5
Apps that compute their callback URL (multi-host setups resolving the
tenant from the request) set options[:redirect_uri], which the previous
callback_url override ignored in favor of the request host.

Claude-Session: https://claude.ai/code/session_01RBRHMr4tD5hZ5Js6WQSHr5
- Pin the token exchange to client_secret_post: LINE documents only body
  credentials; the oauth2 gem's basic-auth default happens to work today
  but is undocumented LINE behavior.
- Give the id-token verify call explicit open/read timeouts and degrade
  email to nil (with a logged warning) on timeout, rejection, or a
  non-JSON body instead of failing the whole login.
- Map OAuth2::TimeoutError from the profile call to ::Timeout::Error so
  omniauth-oauth2's callback_phase turns it into fail!(:timeout) rather
  than a 500.

Claude-Session: https://claude.ai/code/session_01RBRHMr4tD5hZ5Js6WQSHr5
Cassettes under spec/cassettes were recorded once against the real LINE
API using throwaway credentials (no secrets involved) and are replayed
from then on, locking the suite to LINE's actual error shapes for the
token, verify, and profile endpoints. record: :once re-records only when
a cassette file is deleted; CI never hits the network.

Claude-Session: https://claude.ai/code/session_01RBRHMr4tD5hZ5Js6WQSHr5
filter_sensitive_data scrubs the channel secret from future recordings
(the committed cassette is scrubbed retroactively), and record: :none on
CI turns a missing cassette into a hard failure instead of a live call.

Claude-Session: https://claude.ai/code/session_01RBRHMr4tD5hZ5Js6WQSHr5
@lipowen
lipowen force-pushed the fix/authorize-options-and-verify-hardening branch from 87044d4 to 2133259 Compare September 8, 2026 02:48
@lipowen
lipowen merged commit f26459d into master Sep 8, 2026
4 checks passed
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.

3 participants