Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new email fetch path needs correct scope, safer JSON parsing, and more robust HTTP/error handling to avoid breaking authentication flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the OmniAuth::Strategies::Line strategy to include a user email in the returned OmniAuth info hash by verifying the id_token against LINE’s /oauth2/v2.1/verify endpoint and merging the resulting email into raw_info.
Changes:
- Add
emailto the OmniAuthinfohash. - Implement an
emailhelper that calls LINE’s token verification endpoint and parses the returned email. - Update
raw_infomemoization to inject the fetched email into the cached profile payload.
File summaries
| File | Description |
|---|---|
| lib/omniauth/strategies/line.rb | Adds email retrieval/merging into raw_info and exposes it via info[:email]. |
Review details
Suppressed comments (1)
lib/omniauth/strategies/line.rb:54
- Avoid
JSON.loadwhen parsing remote/untrusted JSON, since it can enable object creation viacreate_additionsin some configurations.JSON.parseis safer for HTTP responses.
@raw_info = JSON.load(access_token.get('v2/profile').body)
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| name: raw_info['displayName'], | ||
| image: raw_info['pictureUrl'], | ||
| description: raw_info['statusMessage'] | ||
| description: raw_info['statusMessage'], | ||
| email: raw_info["email"] | ||
| } |
| def email | ||
| params = { | ||
| id_token: @id_token, | ||
| client_id: client.id | ||
| } | ||
|
|
||
| response = Net::HTTP.post_form(URI("https://api.line.me/oauth2/v2.1/verify"), params) | ||
| JSON.load(response.body)["email"] | ||
| end |
There was a problem hiding this comment.
🟡 Changes recommended
The new email-fetching path can raise/break callbacks and introduces security/compatibility issues (e.g., JSON.load on remote data and present? without an ActiveSupport dependency).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
lib/omniauth/strategies/line.rb:40
emailcan raise and break the callback: it posts even when@id_tokenis nil/empty, doesn’t check for non-success HTTP responses, and usesJSON.loadon a remote response body. It also relies onNet::HTTP/URIwithout ensuring the stdlib is loaded. Consider making this method resilient and returning nil on failure.
response = Net::HTTP.post_form(URI("https://api.line.me/oauth2/v2.1/verify"), params)
JSON.load(response.body)["email"]
lib/omniauth/strategies/line.rb:56
present?is an ActiveSupport extension and isn’t provided by Ruby stdlib or this gem’s declared dependencies, so this can raiseNoMethodErrorin non-Rails consumers. Also,JSON.loadon remote data is riskier thanJSON.parse(can enable object creation viajson_class).
return @raw_info if @raw_info.present?
@raw_info = JSON.load(access_token.get('v2/profile').body)
@raw_info["email"] = email
@raw_info
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Lite
|
|
||
| extra do | ||
| hash = {} | ||
| hash[:id_token] = access_token['id_token'] if access_token['id_token'].present? |
| def build_access_token | ||
| verifier = request.params["code"] | ||
| get_token_params = {:redirect_uri => callback_url}.merge(token_params.to_hash(:symbolize_keys => true)) | ||
| result = client.auth_code.get_token(verifier, get_token_params, deep_symbolize(options.auth_token_params)) | ||
| @id_token = result.params["id_token"] | ||
|
|
||
| return result | ||
| end |
| info do | ||
| { | ||
| name: raw_info['displayName'], | ||
| image: raw_info['pictureUrl'], | ||
| description: raw_info['statusMessage'] | ||
| description: raw_info['statusMessage'], | ||
| email: raw_info["email"] | ||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
The current implementation introduces runtime-breaking present? calls (ActiveSupport dependency), plus brittle email fetching/error handling that can break the auth callback and lacks test coverage for the new email behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
lib/omniauth/strategies/line.rb:41
emailperforms a network call without requiringnet/http/uri, usesJSON.load, and has no guardrails for missingid_token, non-2xx responses, or JSON parse failures—any of which can raise and break the auth callback. It also always calls the verify endpoint even whenemailscope isn’t requested, adding latency for most installs.
def email
params = {
id_token: @id_token,
client_id: client.id
}
response = Net::HTTP.post_form(URI("https://api.line.me/oauth2/v2.1/verify"), params)
JSON.load(response.body)["email"]
end
lib/omniauth/strategies/line.rb:47
present?is an ActiveSupport extension and will raiseNoMethodErrorin non-Rails usage (this gem doesn’t depend on ActiveSupport). Use a plain Ruby emptiness check when deciding whether to expose:id_token.
hash = {}
hash[:id_token] = access_token['id_token'] if access_token['id_token'].present?
hash
lib/omniauth/strategies/line.rb:55
raw_infousespresent?(ActiveSupport-only) andJSON.load(less safe thanJSON.parse). This can crash in non-Rails apps and also enables JSON object deserialization if JSON additions are enabled.
def raw_info
return @raw_info if @raw_info.present?
@raw_info = JSON.load(access_token.get('v2/profile').body)
@raw_info["email"] = email
lib/omniauth/strategies/line.rb:68
build_access_tokenre-implementsomniauth-oauth2token exchange logic, increasing drift risk vs upstream. Prefer callingsuperand only extracting/storing theid_token.
def build_access_token
verifier = request.params["code"]
get_token_params = {:redirect_uri => callback_url}.merge(token_params.to_hash(:symbolize_keys => true))
result = client.auth_code.get_token(verifier, get_token_params, deep_symbolize(options.auth_token_params))
@id_token = result.params["id_token"]
return result
end
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| info do | ||
| { | ||
| name: raw_info['displayName'], | ||
| image: raw_info['pictureUrl'], | ||
| description: raw_info['statusMessage'] | ||
| description: raw_info['statusMessage'], | ||
| email: raw_info["email"] | ||
| } |
No description provided.