assembling auth stuff for yours-ios

[isaacbowen]

https://github.com/lightward/yours-ios

[Copilot]

Pull Request Overview

This PR implements native iOS app authentication for the yours-ios application by adding a token-based authentication flow that works with ASWebAuthenticationSession and custom URL schemes.

Key Changes:

• Adds stateless auth token generation and verification using AES-256-GCM encryption and HMAC signatures
• Implements OAuth callback handling for native apps that redirects to lightward-yours:// URL scheme
• Adds session bootstrapping mechanism that exchanges one-time auth tokens for Rails sessions

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
app/models/resonance.rb	Adds generate_auth_token and find_by_auth_token methods for creating and validating stateless authentication tokens
app/controllers/google_sign_in/callbacks_controller.rb	New controller extending google_sign_in gem to handle native app OAuth flow with custom URL scheme redirects
app/controllers/application_controller.rb	Adds exchange_auth_token_for_session before_action to bootstrap sessions from auth tokens and turbo_native_app? detection
spec/models/resonance_native_auth_spec.rb	Tests for token generation, validation, security properties including signature verification and encryption
spec/requests/google_sign_in/native_callbacks_spec.rb	Tests for native app OAuth callback handling, error cases, and token generation in callback flow
spec/requests/native_auth_token_exchange_spec.rb	Tests for auth token to session exchange mechanism, cookie handling, and invalid token scenarios

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The expect with a custom error message uses old RSpec syntax. In modern RSpec, you should pass the message as the last argument to expect() or use a block with or. Change:

expect(token_parts.length).to eq(3), "Token format should be hash.encrypted.signature"

to:

expect(token_parts.length).to eq(3) # Token format should be hash.encrypted.signature

or use proper RSpec failure message syntax if needed. The comma-separated message format shown here is not valid RSpec syntax and will cause an error.

Suggested change

	expect(token_parts.length).to eq(3), "Token format should be hash.encrypted.signature"
	expect(token_parts.length).to eq(3) # Token format should be hash.encrypted.signature

[Copilot]

[nitpick] The comment says "We can derive the email from google_id if needed, or we could include it in the token" but this isn't accurate. You cannot derive an email address from a google_id - they are independent identifiers. If email is needed in the session, it should be included in the token during generation or stored separately. Consider clarifying this comment or removing it if the email isn't actually needed for native apps.

Suggested change

	# We can derive the email from google_id if needed, or we could include it in the token
	# For now, we'll skip the obfuscated email for native - it's display-only anyway
	# For now, we'll skip the obfuscated email for native - it's display-only anyway

[Copilot]

[nitpick] Using regex match /[+/=]/ to check for URL-unsafe characters, but the character class should be [+/= ] (with a space) based on line 97. However, line 36 only checks [+/=] without the space. This inconsistency means:

• Line 36: Checks for +, /, =
• Line 97: Checks for +, /, =, and space

Since Base64.urlsafe_encode64 with padding: false should never produce these characters anyway, both tests should pass. However, the inconsistency is confusing. Make both regex patterns consistent, or add a comment explaining why line 97 includes space but line 36 doesn't.

Suggested change

	# Should not contain characters that need escaping in URLs
	expect(token).not_to match(%r{[+/=]})
	# Should not contain characters that need escaping in URLs (including space)
	expect(token).not_to match(%r{[+/= ]})

[Copilot]

The decryption assumes fixed byte offsets (12 bytes IV, 16 bytes auth_tag) which are correct for AES-256-GCM, but this could fail silently if the encrypted data is shorter than expected. Consider adding a length check before slicing:

return nil if raw.bytesize < 28  # minimum: 12 (IV) + 16 (auth_tag)

This would make the error handling more explicit and prevent potential index out of bounds issues.

Suggested change

	raw = Base64.urlsafe_decode64(encrypted_google_id)
	raw = Base64.urlsafe_decode64(encrypted_google_id)
	return nil if raw.bytesize < 28 # minimum: 12 (IV) + 16 (auth_tag)

[Copilot]

[nitpick] Missing error handling for Base64.urlsafe_decode64. If the encrypted_google_id part is not valid base64, this will raise an ArgumentError that gets caught by the rescue clause, but it would be clearer to validate the format first or document this behavior. The rescue at line 94 does catch ArgumentError, so this works, but consider adding a comment explaining that invalid base64 is handled by the rescue block.

Suggested change

	# Decrypt google_id from token
	# Decrypt google_id from token
	# If encrypted_google_id is not valid base64, ArgumentError will be raised and handled by the rescue block below.

[Copilot]

The OAuth2::Error is caught and error.code is used directly in the redirect URL. However, OAuth2::Error might not always have a code method or it might return nil. Consider adding a safe fallback:

error_code = error.respond_to?(:code) ? error.code : error.message
redirect_to "lightward-yours://auth-error?message=#{CGI.escape(error_code || 'unknown_error')}", allow_other_host: true

This ensures the redirect always has a valid error message even if the error structure is unexpected.

Suggested change

	redirect_to "lightward-yours://auth-error?message=#{CGI.escape(error.code)}", allow_other_host: true
	error_code = error.respond_to?(:code) ? error.code : error.message
	redirect_to "lightward-yours://auth-error?message=#{CGI.escape(error_code || 'unknown_error')}", allow_other_host: true

[Copilot]

In the native app callback path, there's no state validation being performed. The valid_params? method only checks if state is present, but doesn't verify it matches any expected value. This could allow CSRF attacks where an attacker provides their own authorization code with an arbitrary state parameter.

For web flow, the code validates params[:state] == flash[:state] (line 71), but native apps skip this validation entirely. Consider storing the state in a secure cookie or session before redirecting to OAuth, and validating it in the callback even for native apps.

[Copilot]

The test comment says "only works once (token is deleted after use)" but the test itself doesn't actually verify one-time use. At line 40, the comment even acknowledges "Actually, the token itself is still valid" - which means tokens can be replayed.

The current implementation doesn't invalidate tokens after use; it only deletes the cookie. If an attacker intercepts the token, they could use it to create their own session. Consider implementing actual one-time use by:

1. Storing used token hashes in Redis/cache with TTL
2. Or adding an expiration timestamp in the token itself
3. Or generating a random nonce per token and storing active nonces

Suggested change

	# Second request with same token should not work (cookie was deleted)
	cookies[:_yours_auth_token] = auth_token
	get "/", headers: headers
	# Actually, the token itself is still valid, but the cookie gets deleted
	# Let's verify that the exchange happens and cookie is deleted
	# Second request with same token should not work (token should be invalid after first use)
	cookies[:_yours_auth_token] = auth_token
	get "/", headers: headers
	# The token should not be accepted a second time; session should not be established
	expect(session[:google_id]).to be_nil

[Copilot]

[nitpick] The comment states "Token is stateless, signed with secret, contains encrypted google_id" but this isn't entirely accurate. The token contains the google_id_hash (unencrypted) as the first part, which reveals information about the user. While the hash can't be reversed, it does allow:

1. Token linkability - Anyone can tell if two tokens belong to the same user
2. Correlation attacks - If the google_id is leaked elsewhere, it can be hashed and matched

Consider updating the comment to clarify that the first part is a hash (not encrypted), or reconsider whether exposing the hash is necessary for the design.

Suggested change

	# Token is stateless, signed with secret, contains encrypted google_id
	# Token format: {google_id_hash}.{encrypted_google_id}.{signature}
	# Token is stateless, signed with secret, contains a hash of the google_id (not encrypted) and the encrypted google_id.
	# Token format: {google_id_hash}.{encrypted_google_id}.{signature}
	# Note: The google_id_hash is deterministic and allows token linkability and correlation if the google_id is leaked elsewhere.

[Copilot]

This assignment to resonance is useless, since its value is never read.

Suggested change

	resonance = Resonance.find_or_create_by_google_id(google_id)
	Resonance.find_or_create_by_google_id(google_id)